dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] intel: Track known prime buffers for re-use
@ 2013-11-22 13:35 Keith Packard
  2013-11-25 15:39 ` [Mesa-dev] " Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Packard @ 2013-11-22 13:35 UTC (permalink / raw)
  To: mesa-dev; +Cc: dri-devel

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.

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

This one took a while to find -- multiple bo_gem structs pointing at
the same gem handle would either cause the object to be destroyed
before we were done using it, or we'd end up sending the same
gem_handle for multiple buffers.

This looks a lot like the named object handling stuff, as one would
expect.

 intel/intel_bufmgr_gem.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index df6fcec..2897bb2 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -112,6 +112,7 @@ typedef struct _drm_intel_bufmgr_gem {
 
 	drmMMListHead named;
 	drmMMListHead vma_cache;
+        drmMMListHead prime;
 	int vma_count, vma_open, vma_max;
 
 	uint64_t gtt_size;
@@ -2451,8 +2452,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->prime.next;
+	     list != &bufmgr_gem->prime;
+	     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 +2505,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->prime);
 
 	VG_CLEAR(get_tiling);
 	get_tiling.handle = bo_gem->gem_handle;
@@ -3301,5 +3319,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	DRMINITLISTHEAD(&bufmgr_gem->vma_cache);
 	bufmgr_gem->vma_max = -1; /* unlimited by default */
 
+	DRMINITLISTHEAD(&bufmgr_gem->prime);
+
 	return &bufmgr_gem->bufmgr;
 }
-- 
1.8.4.3

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Mesa-dev] [PATCH] intel: Track known prime buffers for re-use
  2013-11-22 13:35 [PATCH] intel: Track known prime buffers for re-use Keith Packard
@ 2013-11-25 15:39 ` Daniel Vetter
  2013-11-25 21:21   ` Keith Packard
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-11-25 15:39 UTC (permalink / raw)
  To: Keith Packard; +Cc: mesa-dev, dri-devel

On Fri, Nov 22, 2013 at 05:35:54AM -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.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
> 
> This one took a while to find -- multiple bo_gem structs pointing at
> the same gem handle would either cause the object to be destroyed
> before we were done using it, or we'd end up sending the same
> gem_handle for multiple buffers.
> 
> This looks a lot like the named object handling stuff, as one would
> expect.

Yeah, it unfortunately took a few rounds of kernel fixes and other
haggling to get the semantics right on this one. The kernel atm promises
to userspace (minus one big in a racy corner case no one should care
about, still need to fix that one) that it'll return the same gem handle
if userspace already has one for the underlying object.

We need that to make sure userspace doesn't submit the same bo in execbuf
multiple times and then upsets the kernel - we'll reject such batches as
userspace bugs.

I guess I should have reviewed userspace when doing the relevant kernel
fixes, shame on me. Two questions below.

> 
>  intel/intel_bufmgr_gem.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index df6fcec..2897bb2 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -112,6 +112,7 @@ typedef struct _drm_intel_bufmgr_gem {
>  
>  	drmMMListHead named;
>  	drmMMListHead vma_cache;
> +        drmMMListHead prime;
>  	int vma_count, vma_open, vma_max;
>  
>  	uint64_t gtt_size;
> @@ -2451,8 +2452,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->prime.next;
> +	     list != &bufmgr_gem->prime;
> +	     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 +2505,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->prime);

Won't this result in us having fun when a buffer is both imported from a
prime buffer and then also used with legacy flink? Or is this something
the X server won't support?

The second one is about exporting: With flink names we also add the name
to the lookup list in drm_intel_gem_bo_flink. I think we should do the
same for exported prime buffers just as a precaution - the kernel will
return the (existing) gem name also for a prime buffer that has been
exported by yourself. I guess that would imply insane userspace, but
better safe than sorry.

Cheers, Daniel

>  
>  	VG_CLEAR(get_tiling);
>  	get_tiling.handle = bo_gem->gem_handle;
> @@ -3301,5 +3319,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
>  	DRMINITLISTHEAD(&bufmgr_gem->vma_cache);
>  	bufmgr_gem->vma_max = -1; /* unlimited by default */
>  
> +	DRMINITLISTHEAD(&bufmgr_gem->prime);
> +
>  	return &bufmgr_gem->bufmgr;
>  }
> -- 
> 1.8.4.3
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Mesa-dev] [PATCH] intel: Track known prime buffers for re-use
  2013-11-25 15:39 ` [Mesa-dev] " Daniel Vetter
@ 2013-11-25 21:21   ` Keith Packard
  2013-11-25 21:22     ` Keith Packard
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Packard @ 2013-11-25 21:21 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: mesa-dev, dri-devel


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

Daniel Vetter <daniel@ffwll.ch> writes:

> Yeah, it unfortunately took a few rounds of kernel fixes and other
> haggling to get the semantics right on this one. The kernel atm promises
> to userspace (minus one big in a racy corner case no one should care
> about, still need to fix that one) that it'll return the same gem handle
> if userspace already has one for the underlying object.

That's definitely something we want it to do -- returning different
handles to the same object would result in madness. We just need to deal with the
userspace consequences.

> We need that to make sure userspace doesn't submit the same bo in execbuf
> multiple times and then upsets the kernel - we'll reject such batches as
> userspace bugs.

Oh, I'm well aware of that; you can imagine the adventures I had trying
to debug this...

>> -	DRMINITLISTHEAD(&bo_gem->name_list);
>>  	DRMINITLISTHEAD(&bo_gem->vma_list);
>> +	DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->prime);
>
> Won't this result in us having fun when a buffer is both imported from a
> prime buffer and then also used with legacy flink? Or is this something
> the X server won't support?

Well, the whole point of prime-based FD buffer passing is to *not* use
flink, of course. However, you could use both DRI2 and DRI3 on
the same pixmap (presumably through different APIs).

Ok, I just tried to create a completely separate prime list for this,
and I think that's wrong. If the question is whether the kernel might
return the same object from two calls, then we'd best actually keep a
single list and look things up for both APIs there. *and*, I think we
need to do the flink->gem handle conversion and then look in the list
again to see if that gem handle was already returned from another flink
or prime fd.

> The second one is about exporting: With flink names we also add the name
> to the lookup list in drm_intel_gem_bo_flink. I think we should do the
> same for exported prime buffers just as a precaution - the kernel will
> return the (existing) gem name also for a prime buffer that has been
> exported by yourself. I guess that would imply insane userspace, but
> better safe than sorry.

yeah, that would seem like crazy user-space behaviour, but user space
often seems insane.

Thanks for your review; replacement patch to follow shortly.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

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

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH]  intel: Track known prime buffers for re-use
  2013-11-25 21:21   ` Keith Packard
@ 2013-11-25 21:22     ` Keith Packard
  2013-11-26  9:42       ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Packard @ 2013-11-25 21:22 UTC (permalink / raw)
  To: mesa-dev; +Cc: dri-devel

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] intel: Track known prime buffers for re-use
  2013-11-25 21:22     ` Keith Packard
@ 2013-11-26  9:42       ` Daniel Vetter
  2013-11-26 11:40         ` Keith Packard
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-11-26  9:42 UTC (permalink / raw)
  To: Keith Packard; +Cc: mesa-dev, dri-devel

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
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]  intel: Track known prime buffers for re-use
  2013-11-26  9:42       ` Daniel Vetter
@ 2013-11-26 11:40         ` Keith Packard
  0 siblings, 0 replies; 6+ messages in thread
From: Keith Packard @ 2013-11-26 11:40 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: mesa-dev, dri-devel


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

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.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

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

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-11-26 11:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-22 13:35 [PATCH] intel: Track known prime buffers for re-use Keith Packard
2013-11-25 15:39 ` [Mesa-dev] " Daniel Vetter
2013-11-25 21:21   ` Keith Packard
2013-11-25 21:22     ` Keith Packard
2013-11-26  9:42       ` Daniel Vetter
2013-11-26 11:40         ` Keith Packard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox