Bug fixes all across the project

5 files changed, 92 insertions, 45 deletions

diff --git a/include/drm++/drm.hpp b/include/drm++/drm.hpp
index f128930..c75495b 100644
--- a/include/drm++/drm.hpp
+++ b/include/drm++/drm.hpp
@@ -50,7 +50,7 @@
 /// Sync objects are a relatively recent addition to DRM and are primarily used by the
 /// Vulkan WSI and Wayland for explicit synchronization.
 ///
-/// A sync object can hold one or more DRM fences, which are a kernel object that can be signaled
+/// A sync object can hold one or more DRM fences, which are kernel objects that can be signaled
 /// by the GPU when a certain operation has been completed. Fences inside sync objects can be
 /// exported to file descriptors called "sync files", which can be used by other APIs (such as
 /// Vulkan) to synchronize with the GPU.
@@ -177,8 +177,7 @@ public:
     }
 
     // Copy constructor/operator
-    Device(const Device& other) = delete;
-    Device& operator=(const Device& other) = delete;
+    NO_COPY(Device)
 
     // Destructor
     ~Device() noexcept {
diff --git a/include/drm++/gem.hpp b/include/drm++/gem.hpp
index 4a02e55..35829ee 100644
--- a/include/drm++/gem.hpp
+++ b/include/drm++/gem.hpp
@@ -12,24 +12,30 @@
 /// - Import a DMA-BUF via PRIME
 /// - Create a linear dumb buffer
 ///
-/// Upon importing a DMA-BUF, a new handle to the underlying GEM object is created and the
-/// reference count is increased. The file descriptor itself also holds a reference and must
-/// therefore be closed.
+/// Upon importing a DMA-BUF, the kernel first checks if a GEM object already exists for the
+/// given DMA-BUF and returns the existing handle (not incrementing any reference counts!).
+/// If no GEM object exists, a new one is created and the prime import is performed, incrementing
+/// the reference counts accordingly. When exporting a DMA-BUF, the reference count of the DMA-BUF
+/// is incremented.
 ///
-/// When creating a dumb buffer, there are no guarantees about the underlying memory. It may be
-/// in system memory or GPU memory and there are no guarantees about the performance of read/write
-/// operations. A dumb buffer can however always be mapped into CPU-accessible memory.
+/// Due to this, several GEM buffers may share the same handle. In order to provide a safe
+/// abstraction, this library independently reference counts these handles, ensuring no
+/// use-after-free or double-free occurs.
 ///
 /// * Dumb Buffers
 ///
-/// A dumb buffer is a primitive DRM-native driver independent GEM object, wrapping around a linear
+/// A dumb buffer is a primitive DRM-native driver-independent GEM object, wrapping around a linear
 /// memory allocation.
 ///
+/// When creating a dumb buffer, there are no guarantees about the underlying memory. It may be
+/// in system memory or GPU memory and there are no guarantees about the performance of read/write
+/// operations. A dumb buffer can however always be mapped into CPU-accessible memory.
+///
 /// The only creation parameters are width, height and bpp (bits per pixel / color mode). The
 /// bpp parameter also specifies the DRM formats this buffer can be used with. The table below
 /// can serve as a reference, however there are no guarantees that a format is compatible.
 ///
-/// Most drivers will support DRM_FORMAT_XRGB8888 with 32 bits per pixel.
+/// Most drivers will support DRM_FORMAT_XRGB8888 with 32 bits per pixel on primary planes.
 ///
 /// +-----+------------------------+------------------------+
 /// | BPP | Framebuffer format     | Compatible formats     |
@@ -45,7 +51,7 @@
 /// | 15  |  * DRM_FORMAT_XRGB1555 |  * DRM_FORMAT_BGRX1555 |
 /// |     |                        |  * DRM_FORMAT_RGBX1555 |
 /// |     |                        |  * DRM_FORMAT_XBGR1555 |
-/// ------+------------------------+------------------------+
+/// +-----+------------------------+------------------------+
 /// |  8  |  * DRM_FORMAT_C8       |  * DRM_FORMAT_D8       |
 /// |     |                        |  * DRM_FORMAT_R8       |
 /// +-----+------------------------+------------------------+
@@ -64,9 +70,40 @@
 
 #include "drm++/helper.hpp"
 
+#include <unordered_map>
+
 namespace drm::gem {
 
 ///
+/// GEM object manager.
+///
+/// This class is responsible for reference counting GEM handles as described above and must
+/// therefore outlive all GEM objects.
+///
+class ObjectManager {
+    friend class Object;
+    friend class DumbBuffer;
+public:
+    /// Create a new object manager.
+    ObjectManager(int fd) noexcept : m_fd(fd) {}
+
+    // Convenience method to create a new objects
+    template<typename T, typename... Args>
+    [[nodiscard]] T create(Args&&... args) {
+        return {*this, std::forward<Args>(args)...};
+    }
+
+    // Default operators and destructor
+    NO_COPY(ObjectManager)
+    NO_MOVE(ObjectManager)
+    ~ObjectManager() noexcept = default;
+private:
+    int m_fd;
+
+    std::unordered_map<u32, u32> m_refcounts; // GEM handle -> reference count
+};
+
+///
 /// GEM object wrapping a memory allocation
 ///
 /// @throws drm::ioctl::Exception on failure
@@ -80,7 +117,7 @@ public:
     /// An import can fail for various driver-specific reasons, especially for foreign DMA-BUFs.
     ///
     /// @param close Close fd after import (regardless of success)
-    Object(int fd, int dmabuf_fd, bool close = true);
+    Object(ObjectManager& manager, int dmabuf_fd, bool close = true);
 
     /// Flags for exporting fds
     enum class ExportFlags : u32 {
@@ -89,7 +126,7 @@ public:
         CloseOnExec = 1 << 1, //!< Close fd on execve()
     };
 
-    /// Obtain a DMA-BUF file descriptor for the GEM object, incrementing the reference count.
+    /// Export a DMA-BUF file descriptor from the GEM object.
     /// Requires DRM_PRIME_CAP_EXPORT.
     ///
     /// An export can fail for various driver-specific reasons, including lack of support on this
@@ -100,31 +137,30 @@ public:
 
     /// Change the handle of the GEM object.
     /// @param handle An unused GEM handle to change into.
+    /// @throws std::logic_error if the handle is still in use by another class instance.
     void changeHandle(u32 handle);
 
     // Private access
-    GETTER(fd)
     GETTER(handle)
 
     // Move constructor/operator
-    Object(Object&& other) noexcept : m_fd(other.m_fd), m_handle(other.m_handle) {
-        other.m_fd = -1; // invalidate other
+    Object(Object&& other) noexcept : m_manager(other.m_manager), m_handle(other.m_handle) {
+        other.m_manager = nullptr; // invalidate other
     }
 
     Object& operator=(Object&& other) noexcept {
         if (this != &other) {
             this->destruct();
             this->m_handle = other.m_handle;
-            this->m_fd = other.m_fd;
-            other.m_fd = -1; // invalidate other
+            this->m_manager = other.m_manager;
+            other.m_manager = nullptr; // invalidate other
         }
 
         return *this;
     }
 
     // Copy constructor/operator
-    Object(const Object& other) = delete;
-    Object& operator=(const Object& other) = delete;
+    NO_COPY(Object)
 
     // Destructor
     ~Object() noexcept {
@@ -132,10 +168,10 @@ public:
     }
 
 private:
-    int m_fd; // indicates object validity (>= 0)
+    ObjectManager* m_manager{nullptr}; // indicates object validity
     u32 m_handle;
 
-    Object(int fd) : m_fd(fd), m_handle(0) {}
+    Object(ObjectManager& manager) : m_manager(&manager), m_handle(0) {}
     void destruct() noexcept;
 };
 
@@ -148,7 +184,7 @@ private:
 class DumbBuffer : public Object {
 public:
     /// Create a new dumb buffer.
-    DumbBuffer(int fd, u32 width, u32 height, u32 bpp);
+    DumbBuffer(ObjectManager& manager, u32 width, u32 height, u32 bpp);
 
     /// Map the dumb buffer into userspace memory. May be called multiple times,
     /// will return the same pointer if already mapped.
@@ -188,8 +224,7 @@ public:
     }
 
     // Copy constructor/operator
-    DumbBuffer(const DumbBuffer& other) = delete;
-    DumbBuffer& operator=(const DumbBuffer& other) = delete;
+    NO_COPY(DumbBuffer)
 
     // Destructor
     ~DumbBuffer() noexcept {
diff --git a/include/drm++/helper.hpp b/include/drm++/helper.hpp
index 3b6e90d..7b927bf 100644
--- a/include/drm++/helper.hpp
+++ b/include/drm++/helper.hpp
@@ -31,5 +31,11 @@ using ref = std::reference_wrapper<T>;
     classname& operator=(const classname&) = default; \
     classname(classname&&) = default; \
     classname& operator=(classname&&) = default;
+#define NO_COPY(classname) \
+    classname(const classname&) = delete; \
+    classname& operator=(const classname&) = delete;
+#define NO_MOVE(classname) \
+    classname(classname&&) = delete; \
+    classname& operator=(classname&&) = delete;
 
 #pragma clang diagnostic pop
diff --git a/include/drm++/ioctl.hpp b/include/drm++/ioctl.hpp
index 29b08ae..49f8a1c 100644
--- a/include/drm++/ioctl.hpp
+++ b/include/drm++/ioctl.hpp
@@ -25,8 +25,10 @@ public:
     /// Construct an exception from errno.
     explicit Exception(int fd, unsigned long op);
 
-    /// Convert the exception into human-readable form.
-    std::string readable();
+    /// Get the error message.
+    [[nodiscard]] const char* what() const noexcept override {
+        return this->m_what.c_str();
+    }
 
     // Private access
     GETTER(fd)
@@ -42,6 +44,7 @@ private:
     unsigned long m_op;
     Error m_code;
     int m_syserrno;
+    std::string m_what;
 };
 
 /// Perform an ioctl() call.
diff --git a/include/drm++/syncobject.hpp b/include/drm++/syncobject.hpp
index 9ecbf17..beee332 100644
--- a/include/drm++/syncobject.hpp
+++ b/include/drm++/syncobject.hpp
@@ -12,27 +12,29 @@
 /// Vulkan, which will signal the fence when a certain operation has been completed.
 ///
 /// In core DRM, it is possible to emplace a trivially signaled fence into a sync object, or
-/// remove ("reset") a fence from a sync object. It is also possible to copy a fence from another
-/// sync object.
+/// remove ("reset") a fence from a sync object. It is also possible to copy ("transfer") a fence
+/// from another sync object.
 ///
 /// Fences can be imported and exported from a sync object via sync files, but it is also possible
-/// to export a reference to the sync object itself, which can be imported by another process.
+/// to export a reference to the sync object itself, which can be imported by another process. The
+/// file descriptor itself also holds a reference to the sync object and therefore must be
+/// closed after import (contrary to GEM objects, imports also increment the reference count).
 ///
-/// Finally, sync objects can be waited on, performing a CPU-side wait. It is also possible
-/// to merely wait for a sync object to be emplaced with a fence, without waiting for it to be
-/// signaled. During a wait, it is possible to set a deadline hint on the fence, described by
-/// the kernel as "to provide the fence signaler with an appropriate sense of urgency".
+/// Finally, sync objects can be waited on, performing a CPU-side wait (unless eventfd it used).
+/// It is also possible to merely wait for a sync object to be emplaced with a fence,
+/// without waiting for it to be signaled. During a wait, it is possible to set a deadline hint
+/// on the fence, described by the kernel as "to provide the fence signaler with an
+/// appropriate sense of urgency".
 ///
 /// * Timeline Sync Object
 ///
 /// A timeline sync object is a sync object which can hold multiple fences, each identified by a
-/// 64-bit unsigned integer "point". The point should be monotonically increasing, as this
-/// is what other APIs are designed with (e.g. Vulkan).
+/// 64-bit unsigned integer "point". The point should be monotonically increasing, else
+/// certain operations may return unexpected results.
 ///
 /// While the kernel does not differentiate between a (binary) sync object and a timeline
-/// sync object, there exists a clear distinction and mixing ioctls can lead to undefined
-/// behavior. This library protects against this by providing separate classes for each type
-/// of sync object.
+/// sync object, mixing ioctls can lead to unexpected behavior. This library protects against
+/// this by providing separate classes for each type of sync object.
 ///
 
 #include "drm++/helper.hpp"
@@ -62,7 +64,7 @@ public:
     /// @param close Close syncobj_fd after import (regardless of success)
     SyncObjectBase(int fd, int syncobj_fd, bool close = true);
 
-    /// Export a new reference to sync object, incrementing the reference count.
+    /// Export a new reference to sync object.
     [[nodiscard]] int exportFd() const;
 
     // Private access
@@ -89,7 +91,7 @@ public:
     SyncObjectBase(const SyncObjectBase& other)
         : SyncObjectBase(other.m_fd, other.exportFd(), true) {}
 
-        SyncObjectBase& operator=(const SyncObjectBase& other) {
+    SyncObjectBase& operator=(const SyncObjectBase& other) {
         if (this != &other) {
             const int fd{other.exportFd()};
             *this = SyncObjectBase(other.m_fd, fd, true);
@@ -132,11 +134,12 @@ public:
     /// @param close Close syncfile_fd after import (regardless of success)
     void importSyncFile(int syncfile_fd, bool close = true) const;
 
-    /// Export a sync file from the DRM fence within the sync object.
+    /// Export a sync file to the DRM fence within the sync object.
     /// Any subsequent modifications to the sync object are not applied to the exported sync file
     [[nodiscard]] int exportSyncFile() const;
 
-    /// Transfer a DRM fence into another binary sync object.
+    /// Transfer a DRM fence into another sync object.
+    /// Requires DRM_CAP_SYNCOBJ_TIMELINE.
     void transfer(const SyncObject& dest) const;
     void transfer(const TimelineSyncObject& dest, u64 destPoint) const;
 
@@ -156,6 +159,7 @@ public:
     ) const;
 
     /// Register an eventfd to the sync object.
+    /// Requires DRM_CAP_SYNCOBJ_TIMELINE.
     /// @param waitAvailable Trigger when a fence is emplaced, not when it is signaled
     void registerEventFd(int eventfd_fd, bool waitAvailable = false) const;
 };
@@ -175,7 +179,7 @@ public:
     /// @param close Close syncfile_fd after import (regardless of success)
     void importSyncFile(int syncfile_fd, u64 point, bool close = true) const;
 
-    /// Export a sync file from the DRM fence within the sync object.
+    /// Export a sync file to the DRM fence within the sync object.
     /// Any subsequent modifications to the sync object are not applied to the exported sync file.
     [[nodiscard]] int exportSyncFile(u64 point) const;