[Mesa-dev] [PATCH] i965: fix dma_buf import with non-zero offset.

[Pohjolainen, Topi]

On Tue, Mar 11, 2014 at 10:18:47AM +0200, Pohjolainen, Topi wrote:
> On Tue, Mar 11, 2014 at 08:59:46AM +0100, Gwenole Beauchesne wrote:
> > Hi,
> > 
> > 2014-03-11 7:37 GMT+01:00 Pohjolainen, Topi <topi.pohjolainen at intel.com>:
> > > On Mon, Mar 10, 2014 at 05:36:17PM +0100, Gwenole Beauchesne wrote:
> > >> Fix eglCreateImage() from a packed dma_buf surface with a non-zero offset
> > >> to pixels data. In particular, this fixes support for planar YUV surfaces
> > >> when they are individually mapped on a per-plane basis, i.e. when the
> > >> OES_EGL_image_external is not used and user application wants to use its
> > >> own shader code for composition, or processing on individual plane (OCL).
> > >>
> > >> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
> > >> ---
> > >>  src/mesa/drivers/dri/i965/intel_screen.c |    9 +++++++++
> > >>  1 file changed, 9 insertions(+)
> > >>
> > >> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> > >> index 05cf6b1..81a8330 100644
> > >> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > >> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > >> @@ -651,6 +651,7 @@ intel_create_image_from_fds(__DRIscreen *screen,
> > >>  {
> > >>     struct intel_screen *intelScreen = screen->driverPrivate;
> > >>     struct intel_image_format *f;
> > >> +   uint32_t mask_x, mask_y;
> > >>     __DRIimage *image;
> > >>     int i, index;
> > >>
> > >> @@ -684,6 +685,14 @@ intel_create_image_from_fds(__DRIscreen *screen,
> > >>        image->strides[index] = strides[index];
> > >>     }
> > >>
> > >> +   if (f->nplanes == 1) {
> > >> +      image->offset = image->offsets[0];
> > >
> > > I haven't thought through all the uses of '__DRIimage' but I'm a bit worried
> > > that there is now the offset possibly twice - in the base and again in the
> > > array describing the planes. I wonder if the real bug is that the offsets in
> > > the array are not used in the right place(s):
> > >
> > > In intel_image_target_texture_2d() the call:
> > >
> > >    intel_set_texture_image_region(ctx, texImage, image->region,
> > >                                   target, image->internal_format,
> > >                                   image->format, image->offset,
> > >                                   image->width,  image->height,
> > >                                   image->tile_x, image->tile_y);
> > >
> > > should probably take the plane offsets into account even now when only single
> > > plane images are accepted.
> > >
> > > Would this make sense?
> > >
> > 
> > Yes, or removing image->offset altogether and exclusively use image->offsets[]?
> 
> And now muddying the waters even more. Maybe we should just throw away the
> plane array until there is really some support for using it. I added it when I
> was working on the image external extension and planar YUV support. In the end
> there was no users for that, and the common opinion was not to introduce any
> planar support that the GPU couldn't handle natively.
> Hence I start to feel that you did the right thing - only in addition we should
> get rid of the planar support in '__DRIimage' as there is no backend for it
> anyway. And if and when any GPU native support is introduced in the backend
> then this could be re-introduced properly.

Just checked this a bit more carefully. The original user of planar support is
of course there (wayland and fromPlanar()), and the dma-buf extension needs to
co-exist along side with it. So the last idea is just noise, sorry for that.

> 
> Eric, Ian, do you have any take on this?
> 
> > 
> > >> +      intel_region_get_tile_masks(image->region, &mask_x, &mask_y, false);
> > >> +      if (image->offset & mask_x)
> > >> +         _mesa_warning(NULL,
> > >> +                       "intel_create_image_from_fds: offset not on tile boundary");
> > >> +   }
> > >> +
> > >>     intel_setup_image_from_dimensions(image);
> > >>
> > >>     return image;
> > >> --
> > >> 1.7.9.5
> > >>
> > >> _______________________________________________
> > >> mesa-dev mailing list
> > >> mesa-dev at lists.freedesktop.org
> > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev