Shareable bufmgr objects

Shareable bufmgr objects

[Lionel Landwerlin]

Hi there,

Here is a small modification I had to make to get buffers shared
between Mesa and LibVA on Chrome OS. This is required to have
refcounting properly between the 2 API otherwise, Mesa might end up
calling exit() when the kernel tells it that one of the buffer object
used in a batch buffer is invalid :

http://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/drivers/dri/i965/intel_batchbuffer.c#n282

Thanks,

-
Lionel

[PATCH] intel: make bufmgr_gem shareable from different API

[Lionel Landwerlin]

When using Mesa and LibVA in the same process, one would like to be
able bind buffers from the output of the decoder to a GL texture
through an EGLImage.

LibVA can reuse buffers allocated by Gbm through a file descriptor. It
will then wrap it into a drm_intel_bo with
drm_intel_bo_gem_create_from_prime().

Given both libraries are using libdrm to allocate and use buffer
objects, there is a need to have the buffer objects properly
refcounted. That is possible if both API use the same drm_intel_bo
objects, but that also requires that both API use the same
drm_intel_bufmgr object.

This patch modifies drm_intel_bufmgr_gem_init() so given a file
descriptor, it will look for an already existing drm_intel_bufmgr
using the same file descriptor and return that object.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 intel/intel_bufmgr_gem.c | 100 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 88 insertions(+), 12 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 0e1cb0d..125c81c 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -94,6 +94,8 @@ struct drm_intel_gem_bo_bucket {
 typedef struct _drm_intel_bufmgr_gem {
 	drm_intel_bufmgr bufmgr;
 
+	atomic_t refcount;
+
 	int fd;
 
 	int max_relocs;
@@ -3186,6 +3188,85 @@ drm_intel_bufmgr_gem_set_aub_annotations(drm_intel_bo *bo,
 	bo_gem->aub_annotation_count = count;
 }
 
+static pthread_mutex_t bufmgr_list_mutex = PTHREAD_MUTEX_INITIALIZER;
+static drm_intel_bufmgr_gem **bufmgr_list = NULL;
+static unsigned bufmgr_list_size = 0, bufmgr_list_nb;
+
+static drm_intel_bufmgr_gem *
+drm_intel_bufmgr_gem_find_or_create_for_fd(int fd, int *found)
+{
+        drm_intel_bufmgr_gem *bufmgr_gem;
+
+        assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
+
+        if (bufmgr_list == NULL) {
+                bufmgr_list_size = 2;
+                bufmgr_list = calloc(bufmgr_list_size, sizeof(drm_intel_bufmgr_gem *));
+        } else {
+                unsigned i;
+                for (i = 0; i < bufmgr_list_nb; i++) {
+                        bufmgr_gem = bufmgr_list[i];
+                        if (bufmgr_gem->fd == fd) {
+                                atomic_inc(&bufmgr_gem->refcount);
+                                *found = 1;
+                                goto exit;
+                        }
+                }
+        }
+
+        bufmgr_gem = calloc(1, sizeof(*bufmgr_gem));
+        if (bufmgr_gem == NULL)
+                goto exit;
+
+        bufmgr_gem->fd = fd;
+        atomic_set(&bufmgr_gem->refcount, 1);
+
+	assert(pthread_mutex_init(&bufmgr_gem->lock, NULL) == 0);
+
+        if (bufmgr_list_nb >= bufmgr_list_size) {
+                bufmgr_list_size *= 2;
+                bufmgr_list = realloc(bufmgr_list, bufmgr_list_size);
+                assert(bufmgr_list != NULL);
+        }
+        bufmgr_list[bufmgr_list_nb] = bufmgr_gem;
+        bufmgr_list_nb++;
+
+        pthread_mutex_lock(&bufmgr_gem->lock);
+
+        *found = 0;
+
+exit:
+        pthread_mutex_unlock(&bufmgr_list_mutex);
+
+        return bufmgr_gem;
+}
+
+static void
+drm_intel_bufmgr_gem_unref (drm_intel_bufmgr *bufmgr)
+{
+        drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
+
+        if (atomic_dec_and_test(&bufmgr_gem->refcount)) {
+                unsigned i, compact_start = bufmgr_list_nb;
+
+                assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
+
+                for (i = 0; i < bufmgr_list_nb; i++) {
+                        if (bufmgr_list[i] == bufmgr_gem) {
+                                compact_start = i;
+                                bufmgr_list_nb--;
+                                break;
+                        }
+                }
+                for (i = compact_start; i < bufmgr_list_nb; i++)
+                        bufmgr_list[i] = bufmgr_list[i + 1];
+
+                pthread_mutex_unlock(&bufmgr_list_mutex);
+
+                drm_intel_bufmgr_gem_destroy(bufmgr);
+        }
+}
+
 /**
  * Initializes the GEM buffer manager, which uses the kernel to allocate, map,
  * and manage map buffer objections.
@@ -3201,16 +3282,9 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	int ret, tmp;
 	bool exec2 = false;
 
-	bufmgr_gem = calloc(1, sizeof(*bufmgr_gem));
-	if (bufmgr_gem == NULL)
-		return NULL;
-
-	bufmgr_gem->fd = fd;
-
-	if (pthread_mutex_init(&bufmgr_gem->lock, NULL) != 0) {
-		free(bufmgr_gem);
-		return NULL;
-	}
+        bufmgr_gem = drm_intel_bufmgr_gem_find_or_create_for_fd(fd, &ret);
+	if (bufmgr_gem && ret)
+		return &bufmgr_gem->bufmgr;
 
 	ret = drmIoctl(bufmgr_gem->fd,
 		       DRM_IOCTL_I915_GEM_GET_APERTURE,
@@ -3245,7 +3319,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	else if (IS_GEN8(bufmgr_gem->pci_device))
 		bufmgr_gem->gen = 8;
 	else {
-		free(bufmgr_gem);
+		drm_intel_bufmgr_gem_unref(&bufmgr_gem->bufmgr);
 		return NULL;
 	}
 
@@ -3357,7 +3431,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 		bufmgr_gem->bufmgr.bo_exec = drm_intel_gem_bo_exec;
 	bufmgr_gem->bufmgr.bo_busy = drm_intel_gem_bo_busy;
 	bufmgr_gem->bufmgr.bo_madvise = drm_intel_gem_bo_madvise;
-	bufmgr_gem->bufmgr.destroy = drm_intel_bufmgr_gem_destroy;
+	bufmgr_gem->bufmgr.destroy = drm_intel_bufmgr_gem_unref;
 	bufmgr_gem->bufmgr.debug = 0;
 	bufmgr_gem->bufmgr.check_aperture_space =
 	    drm_intel_gem_check_aperture_space;
@@ -3373,5 +3447,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	DRMINITLISTHEAD(&bufmgr_gem->vma_cache);
 	bufmgr_gem->vma_max = -1; /* unlimited by default */
 
+        pthread_mutex_unlock(&bufmgr_gem->lock);
+
 	return &bufmgr_gem->bufmgr;
 }
-- 
2.0.1

Re: [PATCH] intel: make bufmgr_gem shareable from different API

[Chris Wilson]

On Thu, Sep 11, 2014 at 12:33:41PM +0100, Lionel Landwerlin wrote:
> When using Mesa and LibVA in the same process, one would like to be
> able bind buffers from the output of the decoder to a GL texture
> through an EGLImage.
> 
> LibVA can reuse buffers allocated by Gbm through a file descriptor. It
> will then wrap it into a drm_intel_bo with
> drm_intel_bo_gem_create_from_prime().
> 
> Given both libraries are using libdrm to allocate and use buffer
> objects, there is a need to have the buffer objects properly
> refcounted. That is possible if both API use the same drm_intel_bo
> objects, but that also requires that both API use the same
> drm_intel_bufmgr object.

The description is wrong though. Reusing buffers export and import
through a dmabuf, should work and be correctly refcounted already.

This patch adds the ability to use the same /dev/dri/card0 device fd
between two libraries. This implies that they share the same context and
address space, which is probably not what you want, but nevertheless
seems sensible if they are sharing the device fd in the first place.

I suspect this may break unwary users such as igt, which would fork
after creating a bufmgr, close the fds, but then open their own device
fd with the same fd as before. Not a huge issue, just something to check
in case it causes some fun fallout.
 
> This patch modifies drm_intel_bufmgr_gem_init() so given a file
> descriptor, it will look for an already existing drm_intel_bufmgr
> using the same file descriptor and return that object.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  intel/intel_bufmgr_gem.c | 100 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 88 insertions(+), 12 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 0e1cb0d..125c81c 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -94,6 +94,8 @@ struct drm_intel_gem_bo_bucket {
>  typedef struct _drm_intel_bufmgr_gem {
>  	drm_intel_bufmgr bufmgr;
>  
> +	atomic_t refcount;
> +
>  	int fd;
>  
>  	int max_relocs;
> @@ -3186,6 +3188,85 @@ drm_intel_bufmgr_gem_set_aub_annotations(drm_intel_bo *bo,
>  	bo_gem->aub_annotation_count = count;
>  }
>  
> +static pthread_mutex_t bufmgr_list_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static drm_intel_bufmgr_gem **bufmgr_list = NULL;
> +static unsigned bufmgr_list_size = 0, bufmgr_list_nb;
> +
> +static drm_intel_bufmgr_gem *
> +drm_intel_bufmgr_gem_find_or_create_for_fd(int fd, int *found)
> +{
> +        drm_intel_bufmgr_gem *bufmgr_gem;
> +
> +        assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
> +
> +        if (bufmgr_list == NULL) {

Just use an embedded list rather than array, that would greatly simplify
the search, cration and deletion.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] intel: make bufmgr_gem shareable from different API

[Daniel Stone]

[-- Attachment #1.1: Type: text/plain, Size: 1328 bytes --]

Hi,

On 11 September 2014 12:52, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, Sep 11, 2014 at 12:33:41PM +0100, Lionel Landwerlin wrote:
> > When using Mesa and LibVA in the same process, one would like to be
> > able bind buffers from the output of the decoder to a GL texture
> > through an EGLImage.
> >
> > LibVA can reuse buffers allocated by Gbm through a file descriptor. It
> > will then wrap it into a drm_intel_bo with
> > drm_intel_bo_gem_create_from_prime().
> >
> > Given both libraries are using libdrm to allocate and use buffer
> > objects, there is a need to have the buffer objects properly
> > refcounted. That is possible if both API use the same drm_intel_bo
> > objects, but that also requires that both API use the same
> > drm_intel_bufmgr object.
>
> The description is wrong though. Reusing buffers export and import
> through a dmabuf, should work and be correctly refcounted already.
>

Indeed.

I've been using the attached patch to deal with the case where we have two
EGLDisplays/DRIscreens that can share DRIimage objects (long story, and a
much more ugly patch), and it works perfectly.

The cover letter's description is right though, in that you get a cryptic
message thanks to relocation having been totally skipped when you submit
objects from a foreign bufmgr.

Cheers,
Daniel

[-- Attachment #1.2: Type: text/html, Size: 1846 bytes --]

[-- Attachment #2: mesa-intel-foreign-bufmgr.patch --]
[-- Type: text/x-patch, Size: 2783 bytes --]

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 23aa528..54df6a8 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1262,7 +1262,8 @@ dri2_create_image_wayland_wl_buffer(_EGLDisplay *disp, _EGLContext *ctx,
       return NULL;
    }
 
-   dri_image = dri2_dpy->image->fromPlanar(buffer->driver_buffer, plane, NULL);
+   dri_image = dri2_dpy->image->fromPlanar(buffer->driver_buffer, plane,
+                                           dri2_dpy->dri_screen);
 
    if (dri_image == NULL) {
       _eglError(EGL_BAD_PARAMETER, "dri2_create_image_wayland_wl_buffer");
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
index 9bd4fda..667620b 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -739,6 +739,9 @@ intel_from_planar(__DRIimage *parent, int plane, void *loaderPrivate)
     struct intel_image_format *f;
     uint32_t mask_x, mask_y;
     __DRIimage *image;
+    const __DRIscreen *dri_screen = loaderPrivate;
+    const struct intel_screen *const intel_screen =
+        (struct intel_screen *) dri_screen->driverPrivate;
 
     if (parent == NULL || parent->planar_format == NULL)
         return NULL;
@@ -771,13 +774,42 @@ intel_from_planar(__DRIimage *parent, int plane, void *loaderPrivate)
        return NULL;
     }
 
+    if (parent->region->bo->bufmgr == intel_screen->bufmgr) {
+ 
+
+        image->region->bo = parent->region->bo;
+        drm_intel_bo_reference(image->region->bo);
+    } else {
+        int err, fd;
+
+  
+
+        err = drm_intel_bo_gem_export_to_prime(parent->region->bo, &fd);
+        if (err != 0) {
+            _mesa_warning(NULL, "intel_create_sub_image: prime export failed: %d\n",
+                          -err);
+            free(image->region);
+            free(image);
+            return NULL;
+        }
+
+        image->region->bo =
+            drm_intel_bo_gem_create_from_prime(intel_screen->bufmgr, fd,
+                                               parent->region->bo->size);
+        close(fd);
+        if (!image->region->bo) {
+           _mesa_warning(NULL, "intel_create_sub_image: prime import failed\n");
+           free(image->region);
+           free(image);
+           return NULL;
+       }
+    }
+
     image->region->cpp = _mesa_get_format_bytes(image->format);
     image->region->width = width;
     image->region->height = height;
     image->region->pitch = stride;
     image->region->refcount = 1;
-    image->region->bo = parent->region->bo;
-    drm_intel_bo_reference(image->region->bo);
     image->region->tiling = parent->region->tiling;
     image->offset = offset;
     intel_setup_image_from_dimensions(image);

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] intel: make bufmgr_gem shareable from different API

[Lionel Landwerlin]

On 11/09/14 12:52, Chris Wilson wrote:
> On Thu, Sep 11, 2014 at 12:33:41PM +0100, Lionel Landwerlin wrote:
>> When using Mesa and LibVA in the same process, one would like to be
>> able bind buffers from the output of the decoder to a GL texture
>> through an EGLImage.
>>
>> LibVA can reuse buffers allocated by Gbm through a file descriptor. It
>> will then wrap it into a drm_intel_bo with
>> drm_intel_bo_gem_create_from_prime().
>>
>> Given both libraries are using libdrm to allocate and use buffer
>> objects, there is a need to have the buffer objects properly
>> refcounted. That is possible if both API use the same drm_intel_bo
>> objects, but that also requires that both API use the same
>> drm_intel_bufmgr object.
> The description is wrong though. Reusing buffers export and import
> through a dmabuf, should work and be correctly refcounted already.
>
> This patch adds the ability to use the same /dev/dri/card0 device fd
> between two libraries. This implies that they share the same context and
> address space, which is probably not what you want, but nevertheless
> seems sensible if they are sharing the device fd in the first place.

That's what I meant, sorry if it was unclear.

>
> I suspect this may break unwary users such as igt, which would fork
> after creating a bufmgr, close the fds, but then open their own device
> fd with the same fd as before. Not a huge issue, just something to check
> in case it causes some fun fallout.
Will have a look, thanks.

>   
>> This patch modifies drm_intel_bufmgr_gem_init() so given a file
>> descriptor, it will look for an already existing drm_intel_bufmgr
>> using the same file descriptor and return that object.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   intel/intel_bufmgr_gem.c | 100 +++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 88 insertions(+), 12 deletions(-)
>>
>> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
>> index 0e1cb0d..125c81c 100644
>> --- a/intel/intel_bufmgr_gem.c
>> +++ b/intel/intel_bufmgr_gem.c
>> @@ -94,6 +94,8 @@ struct drm_intel_gem_bo_bucket {
>>   typedef struct _drm_intel_bufmgr_gem {
>>   	drm_intel_bufmgr bufmgr;
>>   
>> +	atomic_t refcount;
>> +
>>   	int fd;
>>   
>>   	int max_relocs;
>> @@ -3186,6 +3188,85 @@ drm_intel_bufmgr_gem_set_aub_annotations(drm_intel_bo *bo,
>>   	bo_gem->aub_annotation_count = count;
>>   }
>>   
>> +static pthread_mutex_t bufmgr_list_mutex = PTHREAD_MUTEX_INITIALIZER;
>> +static drm_intel_bufmgr_gem **bufmgr_list = NULL;
>> +static unsigned bufmgr_list_size = 0, bufmgr_list_nb;
>> +
>> +static drm_intel_bufmgr_gem *
>> +drm_intel_bufmgr_gem_find_or_create_for_fd(int fd, int *found)
>> +{
>> +        drm_intel_bufmgr_gem *bufmgr_gem;
>> +
>> +        assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
>> +
>> +        if (bufmgr_list == NULL) {
> Just use an embedded list rather than array, that would greatly simplify
> the search, cration and deletion.
> -Chris
>

I tried to use the embedded list, but from my understanding I need the 
embedded structure at the top of the bufmgr struct. Is that possible? 
Sounds like an ABI break.

Thanks,

-
Lionel

Re: [PATCH] intel: make bufmgr_gem shareable from different API

[Chris Wilson]

On Thu, Sep 11, 2014 at 01:21:13PM +0100, Lionel Landwerlin wrote:
> On 11/09/14 12:52, Chris Wilson wrote:
> >Just use an embedded list rather than array, that would greatly simplify
> >the search, cration and deletion.
> 
> I tried to use the embedded list, but from my understanding I need
> the embedded structure at the top of the bufmgr struct. Is that
> possible? Sounds like an ABI break.

The drmMMListHead allows embedding anywhere within the parent, and
drm_intel_bufmgr_gem is opaque so can be freely extended.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Shareable bufmgr objects v2

[Lionel Landwerlin]

Following Chris' review, here is an updated patch using drmMMListHead.

I did a quick read of the benchmarks/tests files in igt, as far as I
can see, drm_intel_bufmgr_destroy() is always called before the drm
file descriptor is closed. So it seems this change shouldn't break
anything.

Cheers,

-
Lionel

[PATCH] intel: make bufmgr_gem shareable from different API

[Lionel Landwerlin]

When using Mesa and LibVA in the same process, one would like to be
able bind buffers from the output of the decoder to a GL texture
through an EGLImage.

LibVA can reuse buffers allocated by Gbm through a file descriptor. It
will then wrap it into a drm_intel_bo with
drm_intel_bo_gem_create_from_prime().

The problem at the moment is that both library get a different
drm_intel_bufmgr object when they call drm_intel_bufmgr_gem_init()
even though they're using the same drm file descriptor. As a result,
instead of manipulating the same buffer object for a given file
descriptor, they get 2 different drm_intel_bo objects and 2 different
refcounts, leading one of the library to get errors from the kernel on
invalid BO when one of the 2 library is done with a shared buffer.

This patch modifies drm_intel_bufmgr_gem_init() so, given a file
descriptor, it will look for an already existing drm_intel_bufmgr
using the same file descriptor and return that object.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 intel/intel_bufmgr_gem.c | 82 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 70 insertions(+), 12 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 0e1cb0d..ce43bc6 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -94,6 +94,8 @@ struct drm_intel_gem_bo_bucket {
 typedef struct _drm_intel_bufmgr_gem {
 	drm_intel_bufmgr bufmgr;
 
+	atomic_t refcount;
+
 	int fd;
 
 	int max_relocs;
@@ -111,6 +113,8 @@ typedef struct _drm_intel_bufmgr_gem {
 	int num_buckets;
 	time_t time;
 
+	drmMMListHead managers;
+
 	drmMMListHead named;
 	drmMMListHead vma_cache;
 	int vma_count, vma_open, vma_max;
@@ -3186,6 +3190,65 @@ drm_intel_bufmgr_gem_set_aub_annotations(drm_intel_bo *bo,
 	bo_gem->aub_annotation_count = count;
 }
 
+static pthread_mutex_t bufmgr_list_mutex = PTHREAD_MUTEX_INITIALIZER;
+static drmMMListHead bufmgr_list = { NULL, NULL };
+
+static drm_intel_bufmgr_gem *
+drm_intel_bufmgr_gem_find_or_create_for_fd(int fd, int *found)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem;
+
+	assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
+
+	if (bufmgr_list.next == NULL) {
+		DRMINITLISTHEAD(&bufmgr_list);
+	} else {
+		DRMLISTFOREACHENTRY(bufmgr_gem, &bufmgr_list, managers) {
+			if (bufmgr_gem->fd == fd) {
+				atomic_inc(&bufmgr_gem->refcount);
+				*found = 1;
+				goto exit;
+			}
+		}
+	}
+
+	bufmgr_gem = calloc(1, sizeof(*bufmgr_gem));
+	if (bufmgr_gem == NULL)
+		goto exit;
+
+	bufmgr_gem->fd = fd;
+	atomic_set(&bufmgr_gem->refcount, 1);
+
+	DRMLISTADD(&bufmgr_gem->managers, &bufmgr_list);
+
+	assert(pthread_mutex_init(&bufmgr_gem->lock, NULL) == 0);
+
+	pthread_mutex_lock(&bufmgr_gem->lock);
+
+	*found = 0;
+
+exit:
+	pthread_mutex_unlock(&bufmgr_list_mutex);
+
+	return bufmgr_gem;
+}
+
+static void
+drm_intel_bufmgr_gem_unref (drm_intel_bufmgr *bufmgr)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
+
+	if (atomic_dec_and_test(&bufmgr_gem->refcount)) {
+		assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
+
+		DRMLISTDEL(&bufmgr_gem->managers);
+
+		pthread_mutex_unlock(&bufmgr_list_mutex);
+
+		drm_intel_bufmgr_gem_destroy(bufmgr);
+	}
+}
+
 /**
  * Initializes the GEM buffer manager, which uses the kernel to allocate, map,
  * and manage map buffer objections.
@@ -3201,16 +3264,9 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	int ret, tmp;
 	bool exec2 = false;
 
-	bufmgr_gem = calloc(1, sizeof(*bufmgr_gem));
-	if (bufmgr_gem == NULL)
-		return NULL;
-
-	bufmgr_gem->fd = fd;
-
-	if (pthread_mutex_init(&bufmgr_gem->lock, NULL) != 0) {
-		free(bufmgr_gem);
-		return NULL;
-	}
+	bufmgr_gem = drm_intel_bufmgr_gem_find_or_create_for_fd(fd, &ret);
+	if (bufmgr_gem && ret)
+		return &bufmgr_gem->bufmgr;
 
 	ret = drmIoctl(bufmgr_gem->fd,
 		       DRM_IOCTL_I915_GEM_GET_APERTURE,
@@ -3245,7 +3301,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	else if (IS_GEN8(bufmgr_gem->pci_device))
 		bufmgr_gem->gen = 8;
 	else {
-		free(bufmgr_gem);
+		drm_intel_bufmgr_gem_unref(&bufmgr_gem->bufmgr);
 		return NULL;
 	}
 
@@ -3357,7 +3413,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 		bufmgr_gem->bufmgr.bo_exec = drm_intel_gem_bo_exec;
 	bufmgr_gem->bufmgr.bo_busy = drm_intel_gem_bo_busy;
 	bufmgr_gem->bufmgr.bo_madvise = drm_intel_gem_bo_madvise;
-	bufmgr_gem->bufmgr.destroy = drm_intel_bufmgr_gem_destroy;
+	bufmgr_gem->bufmgr.destroy = drm_intel_bufmgr_gem_unref;
 	bufmgr_gem->bufmgr.debug = 0;
 	bufmgr_gem->bufmgr.check_aperture_space =
 	    drm_intel_gem_check_aperture_space;
@@ -3373,5 +3429,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	DRMINITLISTHEAD(&bufmgr_gem->vma_cache);
 	bufmgr_gem->vma_max = -1; /* unlimited by default */
 
+	pthread_mutex_unlock(&bufmgr_gem->lock);
+
 	return &bufmgr_gem->bufmgr;
 }
-- 
2.0.1

Re: [PATCH] intel: make bufmgr_gem shareable from different API

[Chris Wilson]

On Thu, Sep 11, 2014 at 04:36:20PM +0100, Lionel Landwerlin wrote:
> When using Mesa and LibVA in the same process, one would like to be
> able bind buffers from the output of the decoder to a GL texture
> through an EGLImage.
> 
> LibVA can reuse buffers allocated by Gbm through a file descriptor. It
> will then wrap it into a drm_intel_bo with
> drm_intel_bo_gem_create_from_prime().
> 
> The problem at the moment is that both library get a different
> drm_intel_bufmgr object when they call drm_intel_bufmgr_gem_init()
> even though they're using the same drm file descriptor. As a result,
> instead of manipulating the same buffer object for a given file
> descriptor, they get 2 different drm_intel_bo objects and 2 different
> refcounts, leading one of the library to get errors from the kernel on
> invalid BO when one of the 2 library is done with a shared buffer.
> 
> This patch modifies drm_intel_bufmgr_gem_init() so, given a file
> descriptor, it will look for an already existing drm_intel_bufmgr
> using the same file descriptor and return that object.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  intel/intel_bufmgr_gem.c | 82 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 70 insertions(+), 12 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 0e1cb0d..ce43bc6 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -94,6 +94,8 @@ struct drm_intel_gem_bo_bucket {
>  typedef struct _drm_intel_bufmgr_gem {
>  	drm_intel_bufmgr bufmgr;
>  
> +	atomic_t refcount;
> +
>  	int fd;
>  
>  	int max_relocs;
> @@ -111,6 +113,8 @@ typedef struct _drm_intel_bufmgr_gem {
>  	int num_buckets;
>  	time_t time;
>  
> +	drmMMListHead managers;
> +
>  	drmMMListHead named;
>  	drmMMListHead vma_cache;
>  	int vma_count, vma_open, vma_max;
> @@ -3186,6 +3190,65 @@ drm_intel_bufmgr_gem_set_aub_annotations(drm_intel_bo *bo,
>  	bo_gem->aub_annotation_count = count;
>  }
>  
> +static pthread_mutex_t bufmgr_list_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static drmMMListHead bufmgr_list = { NULL, NULL };

We don't have a static initialializer? Oh well,
static drmMMListHead bufmgr_list = { &bufmgr_list, &bufmgr_list };

> +static drm_intel_bufmgr_gem *
> +drm_intel_bufmgr_gem_find_or_create_for_fd(int fd, int *found)
> +{
> +	drm_intel_bufmgr_gem *bufmgr_gem;
> +
> +	assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
> +
> +	if (bufmgr_list.next == NULL) {
> +		DRMINITLISTHEAD(&bufmgr_list);

Not needed with the static initializer above.

> +	} else {
> +		DRMLISTFOREACHENTRY(bufmgr_gem, &bufmgr_list, managers) {
> +			if (bufmgr_gem->fd == fd) {
> +				atomic_inc(&bufmgr_gem->refcount);
> +				*found = 1;
> +				goto exit;
> +			}
> +		}
> +	}
> +
> +	bufmgr_gem = calloc(1, sizeof(*bufmgr_gem));
> +	if (bufmgr_gem == NULL)
> +		goto exit;
> +
> +	bufmgr_gem->fd = fd;
> +	atomic_set(&bufmgr_gem->refcount, 1);
> +
> +	DRMLISTADD(&bufmgr_gem->managers, &bufmgr_list);
> +
> +	assert(pthread_mutex_init(&bufmgr_gem->lock, NULL) == 0);
> +
> +	pthread_mutex_lock(&bufmgr_gem->lock);

There is an issue with dropping the lock here. A second thread may try
to use the uninitialised bufmgr and crash. We need to hold the lock
until we have finished initialising the bufmgr. So this function can
just be reduced to a list search called with the lock held.

> +
> +	*found = 0;
> +
> +exit:
> +	pthread_mutex_unlock(&bufmgr_list_mutex);
> +
> +	return bufmgr_gem;
> +}
> +
> +static void
> +drm_intel_bufmgr_gem_unref (drm_intel_bufmgr *bufmgr)
> +{
> +	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
> +
> +	if (atomic_dec_and_test(&bufmgr_gem->refcount)) {
> +		assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);

You need to recheck the reference count after grabbing the lock.

> +
> +		DRMLISTDEL(&bufmgr_gem->managers);
> +
> +		pthread_mutex_unlock(&bufmgr_list_mutex);
> +
> +		drm_intel_bufmgr_gem_destroy(bufmgr);
> +	}
> +}

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] intel: make bufmgr_gem shareable from different API

[Lionel Landwerlin]

When using Mesa and LibVA in the same process, one would like to be
able bind buffers from the output of the decoder to a GL texture
through an EGLImage.

LibVA can reuse buffers allocated by Gbm through a file descriptor. It
will then wrap it into a drm_intel_bo with
drm_intel_bo_gem_create_from_prime().

The problem at the moment is that both library get a different
drm_intel_bufmgr object when they call drm_intel_bufmgr_gem_init()
even though they're using the same drm file descriptor. As a result,
instead of manipulating the same buffer object for a given file
descriptor, they get 2 different drm_intel_bo objects and 2 different
refcounts, leading one of the library to get errors from the kernel on
invalid BO when one of the 2 library is done with a shared buffer.

This patch modifies drm_intel_bufmgr_gem_init() so, given a file
descriptor, it will look for an already existing drm_intel_bufmgr
using the same file descriptor and return that object.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 intel/intel_bufmgr_gem.c | 60 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 55 insertions(+), 5 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 0e1cb0d..0a2a62b 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -94,6 +94,8 @@ struct drm_intel_gem_bo_bucket {
 typedef struct _drm_intel_bufmgr_gem {
 	drm_intel_bufmgr bufmgr;
 
+	atomic_t refcount;
+
 	int fd;
 
 	int max_relocs;
@@ -111,6 +113,8 @@ typedef struct _drm_intel_bufmgr_gem {
 	int num_buckets;
 	time_t time;
 
+	drmMMListHead managers;
+
 	drmMMListHead named;
 	drmMMListHead vma_cache;
 	int vma_count, vma_open, vma_max;
@@ -3186,6 +3190,40 @@ drm_intel_bufmgr_gem_set_aub_annotations(drm_intel_bo *bo,
 	bo_gem->aub_annotation_count = count;
 }
 
+static pthread_mutex_t bufmgr_list_mutex = PTHREAD_MUTEX_INITIALIZER;
+static drmMMListHead bufmgr_list = { &bufmgr_list, &bufmgr_list };
+
+static drm_intel_bufmgr_gem *
+drm_intel_bufmgr_gem_find(int fd)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem;
+
+	DRMLISTFOREACHENTRY(bufmgr_gem, &bufmgr_list, managers) {
+		if (bufmgr_gem->fd == fd) {
+			atomic_inc(&bufmgr_gem->refcount);
+			return bufmgr_gem;
+		}
+	}
+
+	return NULL;
+}
+
+static void
+drm_intel_bufmgr_gem_unref(drm_intel_bufmgr *bufmgr)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
+
+	if (atomic_dec_and_test(&bufmgr_gem->refcount)) {
+		assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
+
+		DRMLISTDEL(&bufmgr_gem->managers);
+
+		pthread_mutex_unlock(&bufmgr_list_mutex);
+
+		drm_intel_bufmgr_gem_destroy(bufmgr);
+	}
+}
+
 /**
  * Initializes the GEM buffer manager, which uses the kernel to allocate, map,
  * and manage map buffer objections.
@@ -3201,15 +3239,21 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	int ret, tmp;
 	bool exec2 = false;
 
+	bufmgr_gem = drm_intel_bufmgr_gem_find(fd);
+	if (bufmgr_gem)
+		goto exit;
+
 	bufmgr_gem = calloc(1, sizeof(*bufmgr_gem));
 	if (bufmgr_gem == NULL)
-		return NULL;
+		goto exit;
 
 	bufmgr_gem->fd = fd;
+	atomic_set(&bufmgr_gem->refcount, 1);
 
 	if (pthread_mutex_init(&bufmgr_gem->lock, NULL) != 0) {
 		free(bufmgr_gem);
-		return NULL;
+		bufmgr_gem = NULL;
+		goto exit;
 	}
 
 	ret = drmIoctl(bufmgr_gem->fd,
@@ -3246,7 +3290,8 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 		bufmgr_gem->gen = 8;
 	else {
 		free(bufmgr_gem);
-		return NULL;
+		bufmgr_gem = NULL;
+		goto exit;
 	}
 
 	if (IS_GEN3(bufmgr_gem->pci_device) &&
@@ -3357,7 +3402,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 		bufmgr_gem->bufmgr.bo_exec = drm_intel_gem_bo_exec;
 	bufmgr_gem->bufmgr.bo_busy = drm_intel_gem_bo_busy;
 	bufmgr_gem->bufmgr.bo_madvise = drm_intel_gem_bo_madvise;
-	bufmgr_gem->bufmgr.destroy = drm_intel_bufmgr_gem_destroy;
+	bufmgr_gem->bufmgr.destroy = drm_intel_bufmgr_gem_unref;
 	bufmgr_gem->bufmgr.debug = 0;
 	bufmgr_gem->bufmgr.check_aperture_space =
 	    drm_intel_gem_check_aperture_space;
@@ -3373,5 +3418,10 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	DRMINITLISTHEAD(&bufmgr_gem->vma_cache);
 	bufmgr_gem->vma_max = -1; /* unlimited by default */
 
-	return &bufmgr_gem->bufmgr;
+	DRMLISTADD(&bufmgr_gem->managers, &bufmgr_list);
+
+exit:
+	pthread_mutex_unlock(&bufmgr_list_mutex);
+
+	return bufmgr_gem != NULL ? &bufmgr_gem->bufmgr : NULL;
 }
-- 
2.0.1

Re: [PATCH] intel: make bufmgr_gem shareable from different API

[Chris Wilson]

On Thu, Sep 11, 2014 at 05:59:40PM +0100, Lionel Landwerlin wrote:
> When using Mesa and LibVA in the same process, one would like to be
> able bind buffers from the output of the decoder to a GL texture
> through an EGLImage.
> 
> LibVA can reuse buffers allocated by Gbm through a file descriptor. It
> will then wrap it into a drm_intel_bo with
> drm_intel_bo_gem_create_from_prime().
> 
> The problem at the moment is that both library get a different
> drm_intel_bufmgr object when they call drm_intel_bufmgr_gem_init()
> even though they're using the same drm file descriptor. As a result,
> instead of manipulating the same buffer object for a given file
> descriptor, they get 2 different drm_intel_bo objects and 2 different
> refcounts, leading one of the library to get errors from the kernel on
> invalid BO when one of the 2 library is done with a shared buffer.
> 
> This patch modifies drm_intel_bufmgr_gem_init() so, given a file
> descriptor, it will look for an already existing drm_intel_bufmgr
> using the same file descriptor and return that object.

Almost there!

> +static void
> +drm_intel_bufmgr_gem_unref(drm_intel_bufmgr *bufmgr)
> +{
> +	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
> +
> +	if (atomic_dec_and_test(&bufmgr_gem->refcount)) {
> +		assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);

Consider thread A destroying its bufmgr on fd, whilst thread B is
creating his.

Thread A drops the last reference and so takes the mutex.

But just before it does, thread B leaps in grabs the mutex, searches
through the cache, finds its fd, bumps the refcount and leaves with the
old bufmgr (not before dropping the lock!).

Thread A resumes with the lock and frees the bufmgr now owned by thread
B.

The usual idiom is

static inline void
drm_gem_object_unreference_unlocked(struct drm_gem_object *obj)
{
        if (obj && !atomic_add_unless(&obj->refcount.refcount, -1, 1)) {
                struct drm_device *dev = obj->dev;

                mutex_lock(&dev->struct_mutex);
                if (likely(atomic_dec_and_test(&obj->refcount.refcount)))
                        drm_gem_object_free(&obj->refcount);
                mutex_unlock(&dev->struct_mutex);
        }
}

-- 
Chris Wilson, Intel Open Source Technology Centre

Shareable bufmgr objects v3

[Lionel Landwerlin]

Hopefully I got this right this time :)

-
Lionel

[PATCH] intel: make bufmgr_gem shareable from different API

[Lionel Landwerlin]

When using Mesa and LibVA in the same process, one would like to be
able bind buffers from the output of the decoder to a GL texture
through an EGLImage.

LibVA can reuse buffers allocated by Gbm through a file descriptor. It
will then wrap it into a drm_intel_bo with
drm_intel_bo_gem_create_from_prime().

The problem at the moment is that both library get a different
drm_intel_bufmgr object when they call drm_intel_bufmgr_gem_init()
even though they're using the same drm file descriptor. As a result,
instead of manipulating the same buffer object for a given file
descriptor, they get 2 different drm_intel_bo objects and 2 different
refcounts, leading one of the library to get errors from the kernel on
invalid BO when one of the 2 library is done with a shared buffer.

This patch modifies drm_intel_bufmgr_gem_init() so, given a file
descriptor, it will look for an already existing drm_intel_bufmgr
using the same file descriptor and return that object.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 intel/intel_bufmgr_gem.c | 63 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 0e1cb0d..73f46a1 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -94,6 +94,8 @@ struct drm_intel_gem_bo_bucket {
 typedef struct _drm_intel_bufmgr_gem {
 	drm_intel_bufmgr bufmgr;
 
+	atomic_t refcount;
+
 	int fd;
 
 	int max_relocs;
@@ -111,6 +113,8 @@ typedef struct _drm_intel_bufmgr_gem {
 	int num_buckets;
 	time_t time;
 
+	drmMMListHead managers;
+
 	drmMMListHead named;
 	drmMMListHead vma_cache;
 	int vma_count, vma_open, vma_max;
@@ -3186,6 +3190,41 @@ drm_intel_bufmgr_gem_set_aub_annotations(drm_intel_bo *bo,
 	bo_gem->aub_annotation_count = count;
 }
 
+static pthread_mutex_t bufmgr_list_mutex = PTHREAD_MUTEX_INITIALIZER;
+static drmMMListHead bufmgr_list = { &bufmgr_list, &bufmgr_list };
+
+static drm_intel_bufmgr_gem *
+drm_intel_bufmgr_gem_find(int fd)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem;
+
+	DRMLISTFOREACHENTRY(bufmgr_gem, &bufmgr_list, managers) {
+		if (bufmgr_gem->fd == fd) {
+			atomic_inc(&bufmgr_gem->refcount);
+			return bufmgr_gem;
+		}
+	}
+
+	return NULL;
+}
+
+static void
+drm_intel_bufmgr_gem_unref(drm_intel_bufmgr *bufmgr)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
+
+	if (atomic_dec_and_test(&bufmgr_gem->refcount)) {
+		assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);
+
+		if (atomic_read(&bufmgr_gem->refcount)) {
+			DRMLISTDEL(&bufmgr_gem->managers);
+			drm_intel_bufmgr_gem_destroy(bufmgr);
+		}
+
+		pthread_mutex_unlock(&bufmgr_list_mutex);
+	}
+}
+
 /**
  * Initializes the GEM buffer manager, which uses the kernel to allocate, map,
  * and manage map buffer objections.
@@ -3201,15 +3240,23 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	int ret, tmp;
 	bool exec2 = false;
 
+	pthread_mutex_lock(&bufmgr_list_mutex);
+
+	bufmgr_gem = drm_intel_bufmgr_gem_find(fd);
+	if (bufmgr_gem)
+		goto exit;
+
 	bufmgr_gem = calloc(1, sizeof(*bufmgr_gem));
 	if (bufmgr_gem == NULL)
-		return NULL;
+		goto exit;
 
 	bufmgr_gem->fd = fd;
+	atomic_set(&bufmgr_gem->refcount, 1);
 
 	if (pthread_mutex_init(&bufmgr_gem->lock, NULL) != 0) {
 		free(bufmgr_gem);
-		return NULL;
+		bufmgr_gem = NULL;
+		goto exit;
 	}
 
 	ret = drmIoctl(bufmgr_gem->fd,
@@ -3246,7 +3293,8 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 		bufmgr_gem->gen = 8;
 	else {
 		free(bufmgr_gem);
-		return NULL;
+		bufmgr_gem = NULL;
+		goto exit;
 	}
 
 	if (IS_GEN3(bufmgr_gem->pci_device) &&
@@ -3357,7 +3405,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 		bufmgr_gem->bufmgr.bo_exec = drm_intel_gem_bo_exec;
 	bufmgr_gem->bufmgr.bo_busy = drm_intel_gem_bo_busy;
 	bufmgr_gem->bufmgr.bo_madvise = drm_intel_gem_bo_madvise;
-	bufmgr_gem->bufmgr.destroy = drm_intel_bufmgr_gem_destroy;
+	bufmgr_gem->bufmgr.destroy = drm_intel_bufmgr_gem_unref;
 	bufmgr_gem->bufmgr.debug = 0;
 	bufmgr_gem->bufmgr.check_aperture_space =
 	    drm_intel_gem_check_aperture_space;
@@ -3373,5 +3421,10 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	DRMINITLISTHEAD(&bufmgr_gem->vma_cache);
 	bufmgr_gem->vma_max = -1; /* unlimited by default */
 
-	return &bufmgr_gem->bufmgr;
+	DRMLISTADD(&bufmgr_gem->managers, &bufmgr_list);
+
+exit:
+	pthread_mutex_unlock(&bufmgr_list_mutex);
+
+	return bufmgr_gem != NULL ? &bufmgr_gem->bufmgr : NULL;
 }
-- 
2.0.1

Re: [PATCH] intel: make bufmgr_gem shareable from different API

[Chris Wilson]

On Fri, Sep 12, 2014 at 11:27:07AM +0100, Lionel Landwerlin wrote:
> When using Mesa and LibVA in the same process, one would like to be
> able bind buffers from the output of the decoder to a GL texture
> through an EGLImage.
> 
> LibVA can reuse buffers allocated by Gbm through a file descriptor. It
> will then wrap it into a drm_intel_bo with
> drm_intel_bo_gem_create_from_prime().
> 
> The problem at the moment is that both library get a different
> drm_intel_bufmgr object when they call drm_intel_bufmgr_gem_init()
> even though they're using the same drm file descriptor. As a result,
> instead of manipulating the same buffer object for a given file
> descriptor, they get 2 different drm_intel_bo objects and 2 different
> refcounts, leading one of the library to get errors from the kernel on
> invalid BO when one of the 2 library is done with a shared buffer.
> 
> This patch modifies drm_intel_bufmgr_gem_init() so, given a file
> descriptor, it will look for an already existing drm_intel_bufmgr
> using the same file descriptor and return that object.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

So close.

> +static void
> +drm_intel_bufmgr_gem_unref(drm_intel_bufmgr *bufmgr)
> +{
> +	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;
> +
> +	if (atomic_dec_and_test(&bufmgr_gem->refcount)) {
> +		assert(pthread_mutex_lock(&bufmgr_list_mutex) == 0);

(Oh, don't use assert for expressions with side-effects)

> +
> +		if (atomic_read(&bufmgr_gem->refcount)) {

As a thought exercise, now consider what happens if thread B grabs a
reference to this bufmgr and frees it all before this thread wakes up?

Double free, kaboom.

This does have to be an atomic_add_unless, which does not yet exist in
libdrm:

static inline int atomic_add_unless(atomic_t *v, int add, int unless)
{
        int c, old;
        c = atomic_read(v);
        while (c != unless && (old = atomic_cmpxchg(v, c, c + add)) != c)
                c = old;
        return c == unless;
}

static void
drm_intel_bufmgr_gem_unref(drm_intel_bufmgr *bufmgr)
{
	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bufmgr;

	if (atomic_add_unless(&bufmgr_gem->refcount, -1, 1)) {
		pthread_mutex_lock(&bufmgr_list_mutex);
		if (atomic_dec_and_test(&bufmgr_gem->refcount)) {
			free stuff;
		}
		pthread_mutex_unlock(&bufmgr_list_mutex);
         }
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre