omap: add refcnting and handle tracking

There can be scenarios, especially when re-importing an existing buffer,
where you end up with multiple 'struct omap_bo's wrapping a single GEM
object handle.  Which causes badness when the first of the evil-clones
is omap_bo_del()'d.

To do this, introduce reference counting and a hashtable to track the
handles per fd.

First, to avoid bo's slipping through the crack if multiple 'struct
omap_device's are created for one drm fd, a hashtable mapping drm
fd to omap_device, and the omap_device itself is reference counted.
Per omap_device, we keep a handle_table mapping GEM handle to omap_bo.
When buffers are imported from flink name or dmabuf fd, the handle
table is consulted, and if an omap_bo already exists, it's refcnt is
incremented and it is returned.  For good measure, to avoid the
handle_table being deleted before the omap_bo is freed, the omap_bo
holds a reference to the omap_device.

TODO: check the overhead of the hashtable.  If too much we could maybe
get away with only tracking exported and imported bo's in the table.

TODO: all the import/export flink/dmabuf operations are generic DRM
ioctls.  Really all this functionality could be handled by a generic
drm_bo and drm_device "base class" that could be extended by omap,
exynos, etc.  That would also allow more common userspace code by
avoiding artificial libdrm_omap dependencies.

Signed-off-by: Rob Clark <rob@ti.com>

diff --git a/configure.ac b/configure.ac
index a1c8c6985da9e9633d40d73fc658cb273df26e40..09fed53ef1842288b76a4bfb2f7319e6f58f4f18 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -214,7 +214,7 @@ if test "x$HAVE_LIBUDEV" = xyes; then
 fi
 AM_CONDITIONAL(HAVE_LIBUDEV, [test "x$HAVE_LIBUDEV" = xyes])
 
-if test "x$INTEL" != "xno" -o "x$RADEON" != "xno" -o "x$NOUVEAU" != "xno"; then
+if test "x$INTEL" != "xno" -o "x$RADEON" != "xno" -o "x$NOUVEAU" != "xno" -o "x$OMAP" != "xno"; then
     # Check for atomic intrinsics
     AC_CACHE_CHECK([for native atomic primitives], drm_cv_atomic_primitives,
     [

diff --git a/omap/omap_drm.c b/omap/omap_drm.c
index 1d37e451d903c1c8176de6693400edc2bcfe4ba1..cd8e8bc47d0e51befbece7993f3994a8de34add2 100644 (file)
--- a/omap/omap_drm.c
+++ b/omap/omap_drm.c
@@ -32,12 +32,15 @@
 
 #include <stdlib.h>
 #include <linux/stddef.h>
+#include <linux/types.h>
 #include <errno.h>
 #include <sys/mman.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <pthread.h>
 
 #include <xf86drm.h>
+#include <xf86atomic.h>
 
 #include "omap_drm.h"
 #include "omap_drmif.h"
@@ -46,8 +49,23 @@
 #define round_up(x, y) ((((x)-1) | __round_mask(x, y))+1)
 #define PAGE_SIZE 4096
 
+static pthread_mutex_t table_lock = PTHREAD_MUTEX_INITIALIZER;
+static void * dev_table;
+
 struct omap_device {
        int fd;
+       atomic_t refcnt;
+
+       /* The handle_table is used to track GEM bo handles associated w/
+        * this fd.  This is needed, in particular, when importing
+        * dmabuf's because we don't want multiple 'struct omap_bo's
+        * floating around with the same handle.  Otherwise, when the
+        * first one is omap_bo_del()'d the handle becomes no longer
+        * valid, and the remaining 'struct omap_bo's are left pointing
+        * to an invalid handle (and possible a GEM bo that is already
+        * free'd).
+        */
+       void *handle_table;
 };
 
 /* a GEM buffer object allocated from the DRM device */
@@ -59,19 +77,57 @@ struct omap_bo {
        uint32_t        name;           /* flink global handle (DRI2 name) */
        uint64_t        offset;         /* offset to mmap() */
        int             fd;             /* dmabuf handle */
+       atomic_t        refcnt;
 };
 
-struct omap_device * omap_device_new(int fd)
+static struct omap_device * omap_device_new_impl(int fd)
 {
        struct omap_device *dev = calloc(sizeof(*dev), 1);
        if (!dev)
                return NULL;
        dev->fd = fd;
+       atomic_set(&dev->refcnt, 1);
+       dev->handle_table = drmHashCreate();
+       return dev;
+}
+
+struct omap_device * omap_device_new(int fd)
+{
+       struct omap_device *dev = NULL;
+
+       pthread_mutex_lock(&table_lock);
+
+       if (!dev_table)
+               dev_table = drmHashCreate();
+
+       if (drmHashLookup(dev_table, fd, (void **)&dev)) {
+               /* not found, create new device */
+               dev = omap_device_new_impl(fd);
+               drmHashInsert(dev_table, fd, dev);
+       } else {
+               /* found, just incr refcnt */
+               dev = omap_device_ref(dev);
+       }
+
+       pthread_mutex_unlock(&table_lock);
+
+       return dev;
+}
+
+struct omap_device * omap_device_ref(struct omap_device *dev)
+{
+       atomic_inc(&dev->refcnt);
        return dev;
 }
 
 void omap_device_del(struct omap_device *dev)
 {
+       if (!atomic_dec_and_test(&dev->refcnt))
+               return;
+       pthread_mutex_lock(&table_lock);
+       drmHashDestroy(dev->handle_table);
+       drmHashDelete(dev_table, dev->fd);
+       pthread_mutex_unlock(&table_lock);
        free(dev);
 }
 
@@ -101,6 +157,38 @@ int omap_set_param(struct omap_device *dev, uint64_t param, uint64_t value)
        return drmCommandWrite(dev->fd, DRM_OMAP_SET_PARAM, &req, sizeof(req));
 }
 
+/* lookup a buffer from it's handle, call w/ table_lock held: */
+static struct omap_bo * lookup_bo(struct omap_device *dev,
+               uint32_t handle)
+{
+       struct omap_bo *bo = NULL;
+       if (!drmHashLookup(dev->handle_table, handle, (void **)&bo)) {
+               /* found, incr refcnt and return: */
+               bo = omap_bo_ref(bo);
+       }
+       return bo;
+}
+
+/* allocate a new buffer object, call w/ table_lock held */
+static struct omap_bo * bo_from_handle(struct omap_device *dev,
+               uint32_t handle)
+{
+       struct omap_bo *bo = calloc(sizeof(*bo), 1);
+       if (!bo) {
+               struct drm_gem_close req = {
+                               .handle = handle,
+               };
+               drmIoctl(dev->fd, DRM_IOCTL_GEM_CLOSE, &req);
+               return NULL;
+       }
+       bo->dev = omap_device_ref(dev);
+       bo->handle = handle;
+       atomic_set(&bo->refcnt, 1);
+       /* add ourselves to the handle table: */
+       drmHashInsert(dev->handle_table, handle, bo);
+       return bo;
+}
+
 /* allocate a new buffer object */
 static struct omap_bo * omap_bo_new_impl(struct omap_device *dev,
                union omap_gem_size size, uint32_t flags)
@@ -115,12 +203,13 @@ static struct omap_bo * omap_bo_new_impl(struct omap_device *dev,
                goto fail;
        }
 
-       bo = calloc(sizeof(*bo), 1);
-       if (!bo) {
+       if (drmCommandWriteRead(dev->fd, DRM_OMAP_GEM_NEW, &req, sizeof(req))) {
                goto fail;
        }
 
-       bo->dev = dev;
+       pthread_mutex_lock(&table_lock);
+       bo = bo_from_handle(dev, req.handle);
+       pthread_mutex_unlock(&table_lock);
 
        if (flags & OMAP_BO_TILED) {
                bo->size = round_up(size.tiled.width, PAGE_SIZE) * size.tiled.height;
@@ -128,12 +217,6 @@ static struct omap_bo * omap_bo_new_impl(struct omap_device *dev,
                bo->size = size.bytes;
        }
 
-       if (drmCommandWriteRead(dev->fd, DRM_OMAP_GEM_NEW, &req, sizeof(req))) {
-               goto fail;
-       }
-
-       bo->handle = req.handle;
-
        return bo;
 
 fail:
@@ -171,6 +254,12 @@ struct omap_bo * omap_bo_new_tiled(struct omap_device *dev,
        return omap_bo_new_impl(dev, gsize, flags);
 }
 
+struct omap_bo * omap_bo_ref(struct omap_bo *bo)
+{
+       atomic_inc(&bo->refcnt);
+       return bo;
+}
+
 /* get buffer info */
 static int get_buffer_info(struct omap_bo *bo)
 {
@@ -193,23 +282,24 @@ static int get_buffer_info(struct omap_bo *bo)
 /* import a buffer object from DRI2 name */
 struct omap_bo * omap_bo_from_name(struct omap_device *dev, uint32_t name)
 {
-       struct omap_bo *bo;
+       struct omap_bo *bo = NULL;
        struct drm_gem_open req = {
                        .name = name,
        };
 
-       bo = calloc(sizeof(*bo), 1);
-       if (!bo) {
-               goto fail;
-       }
+       pthread_mutex_lock(&table_lock);
 
        if (drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req)) {
                goto fail;
        }
 
-       bo->dev = dev;
-       bo->name = name;
-       bo->handle = req.handle;
+       bo = lookup_bo(dev, req.handle);
+       if (!bo) {
+               bo = bo_from_handle(dev, req.handle);
+               bo->name = name;
+       }
+
+       pthread_mutex_unlock(&table_lock);
 
        return bo;
 
@@ -224,24 +314,25 @@ fail:
  */
 struct omap_bo * omap_bo_from_dmabuf(struct omap_device *dev, int fd)
 {
-       struct omap_bo *bo;
+       struct omap_bo *bo = NULL;
        struct drm_prime_handle req = {
                        .fd = fd,
        };
        int ret;
 
-       bo = calloc(sizeof(*bo), 1);
-       if (!bo) {
-               goto fail;
-       }
+       pthread_mutex_lock(&table_lock);
 
        ret = drmIoctl(dev->fd, DRM_IOCTL_PRIME_FD_TO_HANDLE, &req);
        if (ret) {
                goto fail;
        }
 
-       bo->dev = dev;
-       bo->handle = req.handle;
+       bo = lookup_bo(dev, req.handle);
+       if (!bo) {
+               bo = bo_from_handle(dev, req.handle);
+       }
+
+       pthread_mutex_unlock(&table_lock);
 
        return bo;
 
@@ -257,6 +348,9 @@ void omap_bo_del(struct omap_bo *bo)
                return;
        }
 
+       if (!atomic_dec_and_test(&bo->refcnt))
+               return;
+
        if (bo->map) {
                munmap(bo->map, bo->size);
        }
@@ -269,10 +363,14 @@ void omap_bo_del(struct omap_bo *bo)
                struct drm_gem_close req = {
                                .handle = bo->handle,
                };
-
+               pthread_mutex_lock(&table_lock);
+               drmHashDelete(bo->dev->handle_table, bo->handle);
                drmIoctl(bo->dev->fd, DRM_IOCTL_GEM_CLOSE, &req);
+               pthread_mutex_unlock(&table_lock);
        }
 
+       omap_device_del(bo->dev);
+
        free(bo);
 }
 

diff --git a/omap/omap_drmif.h b/omap/omap_drmif.h
index 284b9cccc467d75661bf55cec2718a5df6b0c3d8..e62d127820de5a83e8382e9972e58e67da49eb92 100644 (file)
--- a/omap/omap_drmif.h
+++ b/omap/omap_drmif.h
@@ -38,6 +38,7 @@ struct omap_device;
  */
 
 struct omap_device * omap_device_new(int fd);
+struct omap_device * omap_device_ref(struct omap_device *dev);
 void omap_device_del(struct omap_device *dev);
 int omap_get_param(struct omap_device *dev, uint64_t param, uint64_t *value);
 int omap_set_param(struct omap_device *dev, uint64_t param, uint64_t value);
@@ -49,6 +50,7 @@ struct omap_bo * omap_bo_new(struct omap_device *dev,
                uint32_t size, uint32_t flags);
 struct omap_bo * omap_bo_new_tiled(struct omap_device *dev,
                uint32_t width, uint32_t height, uint32_t flags);
+struct omap_bo * omap_bo_ref(struct omap_bo *bo);
 struct omap_bo * omap_bo_from_name(struct omap_device *dev, uint32_t name);
 struct omap_bo * omap_bo_from_dmabuf(struct omap_device *dev, int fd);
 void omap_bo_del(struct omap_bo *bo);