Re: [PATCH] intel: Track known prime buffers for re-use

On Mon, Nov 25, 2013 at 01:22:41PM -0800, Keith Packard wrote:

If the application sends us a file descriptor pointing at a prime buffer that we've already got, we have to re-use the same bo_gem structure or chaos will result.

Track the set of all known prime objects and look to see if the kernel has returned one of those for a new file descriptor.

Also checks for prime buffers in the flink case.

Signed-off-by: Keith Packard keithp@keithp.com

intel/intel_bufmgr_gem.c | 50 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index df6fcec..2b7fe07 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -149,6 +149,8 @@ struct _drm_intel_bo_gem {

/** * Kenel-assigned global name for this object

•     *
  

•     * List contains both flink named and prime fd'd objects
  

  */ unsigned int global_name; drmMMListHead name_list;

@@ -862,10 +864,6 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr, } }

• bo_gem = calloc(1, sizeof(*bo_gem));
• if (!bo_gem)

• return NULL;
  

• VG_CLEAR(open_arg); open_arg.name = handle; ret = drmIoctl(bufmgr_gem->fd,

@@ -874,9 +872,26 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr, if (ret != 0) { DBG("Couldn't reference %s handle 0x%08x: %s\n", name, handle, strerror(errno));

• free(bo_gem);
  

  return NULL; }

•    /* Now see if someone has used a prime handle to get this
  

•     * object from the kernel before by looking through the list
  

•     * again for a matching gem_handle
  

•     */
  

• for (list = bufmgr_gem->named.next;

•     list != &bufmgr_gem->named;
  

•     list = list->next) {
  

• bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
  

• if (bo_gem->gem_handle == open_arg.handle) {
  

• 	drm_intel_gem_bo_reference(&bo_gem->bo);
  

• 	return &bo_gem->bo;
  

• }
  

• }

The kernel actually doesn't bother with this, i.e. an open on an flink name will always create a new handle. Given that it was a major pita to get the prime reimporting going (due to a pile of funny lifetime issues around reference loops and some assorted locking fun) I'm not volunteering to fix this ;-) And I also think that a piece of userspace which both flink-opens and prime imports on the same buffer gets both pieces.

Otoh this can't hurt either, so if you want to stick with this hunk maybe add a small comment saying that the kernel lies. Or just remove it. Either way:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

Aside: I think drm is the only subsystem that goes out of it's way to ensure a unique relationship between dmabuf and other handles and underlying objects. If you throw v4l into the mix (e.g. by building a gstreamer pipe that feeds into an egl image or so) I expect some fun to happen. Otoh no open-source v4l driver for intel socs, so lalala ;-)

• bo_gem = calloc(1, sizeof(*bo_gem));
• if (!bo_gem)

• return NULL;
  

• bo_gem->bo.size = open_arg.size; bo_gem->bo.offset = 0; bo_gem->bo.virtual = NULL;

@@ -2451,8 +2466,25 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s uint32_t handle; drm_intel_bo_gem *bo_gem; struct drm_i915_gem_get_tiling get_tiling;

• drmMMListHead *list;

  ret = drmPrimeFDToHandle(bufmgr_gem->fd, prime_fd, &handle);

• /*

• * See if the kernel has already returned this buffer to us. Just as
  

• * for named buffers, we must not create two bo's pointing at the same
  

• * kernel object
  

• */
  

• for (list = bufmgr_gem->named.next;

•     list != &bufmgr_gem->named;
  

•     list = list->next) {
  

• bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
  

• if (bo_gem->gem_handle == handle) {
  

• 	drm_intel_gem_bo_reference(&bo_gem->bo);
  

• 	return &bo_gem->bo;
  

• }
  

• }

• if (ret) { fprintf(stderr,"ret is %d %d\n", ret, errno); return NULL;

@@ -2487,8 +2519,8 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s bo_gem->has_error = false; bo_gem->reusable = false;

• DRMINITLISTHEAD(&bo_gem->name_list); DRMINITLISTHEAD(&bo_gem->vma_list);

• DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);

  VG_CLEAR(get_tiling); get_tiling.handle = bo_gem->gem_handle;

@@ -2513,6 +2545,9 @@ drm_intel_bo_gem_export_to_prime(drm_intel_bo *bo, int *prime_fd) drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr; drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;

•    if (DRMLISTEMPTY(&bo_gem->name_list))
  

•            DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
  

• if (drmPrimeHandleToFD(bufmgr_gem->fd, bo_gem->gem_handle, DRM_CLOEXEC, prime_fd) != 0) return -errno;

@@ -2542,7 +2577,8 @@ drm_intel_gem_bo_flink(drm_intel_bo *bo, uint32_t * name) bo_gem->global_name = flink.name; bo_gem->reusable = false;

• DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
  

•            if (DRMLISTEMPTY(&bo_gem->name_list))
  

•                    DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
  

  }

  *name = bo_gem->global_name;

-- 1.8.4.4