[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 at 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