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

[Pohjolainen, Topi]

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.

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