[Mesa-dev] intel: Track known prime buffers for re-use

Submitted by Keith Packard on Nov. 25, 2013, 9:22 p.m.

Details

Message ID 1385414561-29043-1-git-send-email-keithp@keithp.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Keith Packard Nov. 25, 2013, 9:22 p.m.
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(-)

Patch hide | download patch | download mbox

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

Comments

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
>
Daniel Vetter <daniel@ffwll.ch> writes:

> 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.

That seems pretty dangerous to me -- you'll end up with aliases to the
same buffer this way if user space isn't careful.

I bet you check duplicate buffer usage by pointer and not ID though,
which means user space will get errors when this happens. That's not
terrible, but it isn't great either as there's this nasty call to
exit(1) when the execbuffers fails...

> 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:

Not being able to test it is a bit sub-optimal; the duplicate handle
case for prime was well tested by the time I submitted that patch...

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

thanks.

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

Some kind of standard of conduct is clearly needed here - not letting
user space know they've got aliasing going on is pretty mean.