Bug fixes all across the project

8 files changed, 159 insertions, 92 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;
 
diff --git a/src/gem.cpp b/src/gem.cpp
index 0572c3b..b6b3d38 100644
--- a/src/gem.cpp
+++ b/src/gem.cpp
@@ -4,8 +4,11 @@
 #include "drm++/helper.hpp"
 #include "drm++/ioctl.hpp"
 
+#include <cassert>
 #include <cerrno>
+#include <stdexcept>
 #include <system_error>
+#include <utility>
 
 #include <drm.h>
 #include <drm_mode.h>
@@ -18,14 +21,15 @@ using namespace drm::gem;
 
 /* GEM object class */
 
-Object::Object(int fd, int dmabuf_fd, bool close) : m_fd(fd) {
+Object::Object(ObjectManager& manager, int dmabuf_fd, bool close) : m_manager(&manager) {
     try {
         drm_prime_handle args{
             .fd = dmabuf_fd
         };
-        ioctl::perform(this->m_fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, args);
+        ioctl::perform(this->m_manager->m_fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, args);
 
         this->m_handle = args.handle;
+        this->m_manager->m_refcounts[this->m_handle]++;
     } catch (...) {
         if (close) {
             ::close(dmabuf_fd);
@@ -44,22 +48,36 @@ int Object::exportFd(ExportFlags flags) const {
         .handle = this->m_handle,
         .flags = static_cast<u32>(flags)
     };
-    ioctl::perform(this->m_fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, args);
+    ioctl::perform(this->m_manager->m_fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, args);
 
     return args.fd;
 }
 
 void Object::changeHandle(u32 handle) {
-    drm_gem_close args{
-        .handle = this->m_handle
+    auto& refcounts{this->m_manager->m_refcounts};
+    if (refcounts[handle] > 0 || refcounts[this->m_handle] > 1) {
+        throw std::logic_error("GEM handle is still in use");
+    }
+
+    drm_gem_change_handle args{
+        .handle = this->m_handle,
+        .new_handle = handle
     };
-    ioctl::perform(this->m_fd, DRM_IOCTL_GEM_CLOSE, args);
+    ioctl::perform(this->m_manager->m_fd, DRM_IOCTL_GEM_CHANGE_HANDLE, args);
 
+    std::exchange(refcounts[handle], refcounts[this->m_handle]);
     this->m_handle = handle;
 }
 
 void Object::destruct() noexcept {
-    if (this->m_fd < 0) {
+    if (this->m_manager == nullptr) {
+        return;
+    }
+
+    auto& refcounts{this->m_manager->m_refcounts};
+    if (refcounts[this->m_handle] > 1) {
+        refcounts[this->m_handle]--;
+        this->m_manager = nullptr;
         return;
     }
 
@@ -67,26 +85,28 @@ void Object::destruct() noexcept {
         .handle = this->m_handle
     };
     try {
-        ioctl::perform(this->m_fd, DRM_IOCTL_GEM_CLOSE, args);
+        ioctl::perform(this->m_manager->m_fd, DRM_IOCTL_GEM_CLOSE, args);
     } catch(...) {
-        (void) 0; // not much we can do about the leak
+        assert(false && "GEM close failed, memory leak likely");
     }
 
-    this->m_fd = -1;
+    this->m_manager = nullptr;
 }
 
 /* Dumb buffer class */
 
-DumbBuffer::DumbBuffer(int fd, u32 width, u32 height, u32 bpp)
-        : Object(fd), m_width(width), m_height(height), m_bpp(bpp) {
+DumbBuffer::DumbBuffer(ObjectManager& manager, u32 width, u32 height, u32 bpp)
+        : Object(manager), m_width(width), m_height(height), m_bpp(bpp) {
     drm_mode_create_dumb args{
         .height = height,
         .width = width,
         .bpp = bpp
     };
-    ioctl::perform(this->m_fd, DRM_IOCTL_MODE_CREATE_DUMB, args);
+    ioctl::perform(this->m_manager->m_fd, DRM_IOCTL_MODE_CREATE_DUMB, args);
 
     this->m_handle = args.handle;
+    this->m_manager->m_refcounts[this->m_handle]++;
+
     this->m_pitch = args.pitch;
     this->m_size = args.size;
 }
@@ -99,10 +119,10 @@ void* DumbBuffer::map() {
     drm_mode_map_dumb args{
         .handle = this->m_handle
     };
-    ioctl::perform(this->m_fd, DRM_IOCTL_MODE_MAP_DUMB, args);
+    ioctl::perform(this->m_manager->m_fd, DRM_IOCTL_MODE_MAP_DUMB, args);
 
     void* map{::mmap(nullptr, this->m_size, PROT_READ | PROT_WRITE, MAP_SHARED,
-        this->m_fd, static_cast<off_t>(args.offset))};
+        this->m_manager->m_fd, static_cast<off_t>(args.offset))};
     if (map == MAP_FAILED) {
         throw std::system_error(errno, std::generic_category(), "mmap failed");
     }
@@ -112,7 +132,14 @@ void* DumbBuffer::map() {
 }
 
 void DumbBuffer::destruct() noexcept {
-    if (this->m_fd < 0) {
+    if (this->m_manager == nullptr) {
+        return;
+    }
+
+    auto& refcounts{this->m_manager->m_refcounts};
+    if (refcounts[this->m_handle] > 1) {
+        refcounts[this->m_handle]--;
+        this->m_manager = nullptr;
         return;
     }
 
@@ -125,10 +152,10 @@ void DumbBuffer::destruct() noexcept {
         .handle = this->m_handle
     };
     try {
-        ioctl::perform(this->m_fd, DRM_IOCTL_MODE_DESTROY_DUMB, args);
+        ioctl::perform(this->m_manager->m_fd, DRM_IOCTL_MODE_DESTROY_DUMB, args);
     } catch(...) {
-        (void) 0; // not much we can do about the leak
+        assert(false && "Dumb buffer destroy failed, memory leak likely");
     }
 
-    this->m_fd = -1;
+    this->m_manager = nullptr;
 }
diff --git a/src/ioctl.cpp b/src/ioctl.cpp
index 982e1e9..8b4962f 100644
--- a/src/ioctl.cpp
+++ b/src/ioctl.cpp
@@ -13,43 +13,35 @@ Exception::Exception(int fd, unsigned long op)
         : m_fd(fd), m_op(op), m_syserrno(errno) {
     switch (errno) {
         case EBADF:
-            m_code = Error::BadFileDescriptor;
+            this->m_code = Error::BadFileDescriptor;
+            this->m_what = std::format(
+                "ioctl({}, 0x{:x}) failed: Invalid file descriptor",
+                this->m_fd, this->m_op
+            );
             break;
         case EINVAL:
         case ENOTTY:
-            m_code = Error::InvalidArgument;
-            break;
-        case ENOTSUP:
-            m_code = Error::NotSupported;
-            break;
-        default:
-            m_code = Error::Other;
-    }
-}
-
-std::string Exception::readable() {
-    switch (this->m_code) {
-        case Error::BadFileDescriptor:
-            return std::format(
-                "ioctl({}, {}) failed: Invalid file descriptor",
-                this->m_fd, this->m_op
-            );
-        case Error::InvalidArgument:
-            return std::format(
-                "ioctl({}, {}) failed: Invalid argument (errno {})",
+            this->m_code = Error::InvalidArgument;
+            this->m_what = std::format(
+                "ioctl({}, 0x{:x}) failed: Invalid argument (errno {})",
                 this->m_fd, this->m_op, this->m_syserrno
             );
-        case Error::NotSupported:
-            return std::format(
-                "ioctl({}, {}) failed: Operation not supported",
+            break;
+        case ENOTSUP:
+            this->m_code = Error::NotSupported;
+            this->m_what = std::format(
+                "ioctl({}, 0x{:x}) failed: Operation not supported",
                 this->m_fd, this->m_op
             );
-        case Error::Other:
-            return std::format(
-                "ioctl({}, {}) failed: Unknown error (errno {})",
+            break;
+        default:
+            this->m_code = Error::Other;
+            this->m_what = std::format(
+                "ioctl({}, 0x{:x}) failed: Unknown error (errno {})",
                 this->m_fd, this->m_op, this->m_syserrno
             );
     }
 }
 
+
 Exception::~Exception() = default;
diff --git a/src/syncobject.cpp b/src/syncobject.cpp
index f2d3bbc..4ec1a9c 100644
--- a/src/syncobject.cpp
+++ b/src/syncobject.cpp
@@ -4,6 +4,7 @@
 #include "drm++/helper.hpp"
 #include "drm++/ioctl.hpp"
 
+#include <cassert>
 #include <cstddef>
 #include <optional>
 #include <span>
@@ -68,7 +69,7 @@ void SyncObjectBase::destruct() noexcept {
     try {
         ioctl::perform(this->m_fd, DRM_IOCTL_SYNCOBJ_DESTROY, args);
     } catch(...) {
-        (void) 0; // not much we can do about the leak
+        assert(false && "Sync object destroy failed, memory leak likely");
     }
 
     this->m_fd = -1;