Re: [Mesa-dev] [PATCH v14 15/36] i965: Improve same-buffer restriction for imports

2017-06-06 Thread Jason Ekstrand 

On June 5, 2017 6:10:22 PM Varad Gautam  wrote:


Hi Daniel,

On Mon, 2017-06-05 at 14:53 +0100, Daniel Stone wrote:

Hi Varad,

On 30 May 2017 at 12:53, Varad Gautam  wrote:
>
> +   /* We only support all planes from the same bo.
> +* brw_bo_gem_create_from_prime() should return the same pointer for all
> +* fds received here */
> +   bo = brw_bo_gem_create_from_prime(screen->bufmgr, fds[0], size);
> +   for (i = 1; i < num_fds; i++) {
> +  if (bo != brw_bo_gem_create_from_prime(screen->bufmgr, fds[i], size))
> + return NULL;

This above takes a ref, which gets leaked.

   struct brw_bo *aux =
brw_bo_gem_create_from_prime(screen->bufmgr, fds[i], size);
   brw_bo_unreference(aux);
   if (aux != bo)


Thanks for spotting this. Can the unref(aux) happen before comparing 
against bo?


That should be fine as we have a reference to bo. If the two pointers 
compare equal they after the under the they were equal before the unref so 
they were the same before the unref.



Or should this be something like:
   for (...) {
      struct brw_bo *aux = brw_bo_gem_create_from_prime(screen->bufmgr, fds[i],
size);
      if (aux != bo) {
         brw_bo_unreference(aux);
         return NULL;
      }
      brw_bo_unreference(aux);
   }

  return NULL;

Thanks for the fix!

Cheers,
Daniel

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v14 15/36] i965: Improve same-buffer restriction for imports

2017-06-05 Thread Varad Gautam 
Hi Daniel,

On Mon, 2017-06-05 at 14:53 +0100, Daniel Stone wrote:
> Hi Varad,
> 
> On 30 May 2017 at 12:53, Varad Gautam  wrote:
> > 
> > +   /* We only support all planes from the same bo.
> > +* brw_bo_gem_create_from_prime() should return the same pointer for all
> > +* fds received here */
> > +   bo = brw_bo_gem_create_from_prime(screen->bufmgr, fds[0], size);
> > +   for (i = 1; i < num_fds; i++) {
> > +  if (bo != brw_bo_gem_create_from_prime(screen->bufmgr, fds[i], size))
> > + return NULL;
> 
> This above takes a ref, which gets leaked.
> 
>    struct brw_bo *aux =
> brw_bo_gem_create_from_prime(screen->bufmgr, fds[i], size);
>    brw_bo_unreference(aux);
>    if (aux != bo)

Thanks for spotting this. Can the unref(aux) happen before comparing against bo?
Or should this be something like:
   for (...) {
      struct brw_bo *aux = brw_bo_gem_create_from_prime(screen->bufmgr, fds[i],
size);
      if (aux != bo) {
         brw_bo_unreference(aux);
         return NULL;
      }
      brw_bo_unreference(aux);
   }
>   return NULL;
> 
> Thanks for the fix!
> 
> Cheers,
> Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v14 15/36] i965: Improve same-buffer restriction for imports

2017-06-05 Thread Daniel Stone 
Hi Varad,

On 5 June 2017 at 15:13, Varad Gautam  wrote:
> On Mon, 2017-06-05 at 14:53 +0100, Daniel Stone wrote:
>> This above takes a ref, which gets leaked.
>>
>>struct brw_bo *aux =
>> brw_bo_gem_create_from_prime(screen->bufmgr, fds[i], size);
>>brw_bo_unreference(aux);
>>if (aux != bo)
>
> Thanks for spotting this. Can the unref(aux) happen before comparing against 
> bo?

It's safe since it's not dereferenced: we only care about the absolute
value of the pointers, not the content of the memory they point to.
This was also leaking the fd[0] BO and doing the size calculation
twice: would you take https://hastebin.com/xofidugico as a fixup?

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v14 15/36] i965: Improve same-buffer restriction for imports

2017-06-05 Thread Daniel Stone 
Hi Varad,

On 30 May 2017 at 12:53, Varad Gautam  wrote:
> +   /* We only support all planes from the same bo.
> +* brw_bo_gem_create_from_prime() should return the same pointer for all
> +* fds received here */
> +   bo = brw_bo_gem_create_from_prime(screen->bufmgr, fds[0], size);
> +   for (i = 1; i < num_fds; i++) {
> +  if (bo != brw_bo_gem_create_from_prime(screen->bufmgr, fds[i], size))
> + return NULL;

This above takes a ref, which gets leaked.

   struct brw_bo *aux =
brw_bo_gem_create_from_prime(screen->bufmgr, fds[i], size);
   brw_bo_unreference(aux);
   if (aux != bo)
  return NULL;

Thanks for the fix!

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v14 15/36] i965: Improve same-buffer restriction for imports

2017-05-30 Thread Varad Gautam 
From: Daniel Stone 

Intel hardware requires that all planes of an image come from the same
buffer, which is currently implemented by testing that all FDs are
numerically the same.

However, when going through a winsys (e.g.) or anything which transits
FDs individually, the FDs may be different even if the underlying buffer
is the same.

Instead of checking the FDs for equality, we must check if they actually
point to the same buffer (Jason).

Signed-off-by: Daniel Stone 
Signed-off-by: Varad Gautam 
---
 src/mesa/drivers/dri/i965/intel_screen.c | 24 ++--
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
b/src/mesa/drivers/dri/i965/intel_screen.c
index 9842de6..ddaa8cf 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -815,20 +815,32 @@ intel_create_image_from_fds(__DRIscreen *dri_screen,
struct intel_screen *screen = dri_screen->driverPrivate;
struct intel_image_format *f;
__DRIimage *image;
-   int i, index;
+   struct brw_bo *bo;
+   int i, index, size = 0;
 
if (fds == NULL || num_fds < 1)
   return NULL;
 
-   /* We only support all planes from the same bo */
-   for (i = 0; i < num_fds; i++)
-  if (fds[0] != fds[i])
- return NULL;
-
f = intel_image_format_lookup(fourcc);
if (f == NULL)
   return NULL;
 
+   for (i = 0; i < f->nplanes; i++) {
+  index = f->planes[i].buffer_index;
+  const int plane_height = height >> f->planes[i].height_shift;
+  const int end = offsets[index] + plane_height * strides[index];
+  if (size < end)
+ size = end;
+   }
+   /* We only support all planes from the same bo.
+* brw_bo_gem_create_from_prime() should return the same pointer for all
+* fds received here */
+   bo = brw_bo_gem_create_from_prime(screen->bufmgr, fds[0], size);
+   for (i = 1; i < num_fds; i++) {
+  if (bo != brw_bo_gem_create_from_prime(screen->bufmgr, fds[i], size))
+ return NULL;
+   }
+
if (f->nplanes == 1)
   image = intel_allocate_image(screen, f->planes[0].dri_format,
loaderPrivate);
-- 
2.10.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev