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);
  

• }

+}

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; }