[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[]?

I think this would be inline with the conversion from planar image to
non-planar single (intel_from_planar()). There the '__DRIimage::offset' of the
parent image is igmored when '__DRIimage::planar_format' is set, and
'__DRIimage::planar_format::planes[]::offset' alone is used instead.

This indicates that the spec for '__DRIimage' reads along these lines:

The field "::offset" is valid only for non-planar images, i.e., for the kind
where "::planar_format" is NULL. Otherwise the offsets in the planar format are
to be used (::planar_format::planes[]::offset).

Now, having said all this, I'm not sure what the right thing to do is. At least
it seems that even your original solution wouldn't brake wayland as it doesn't
care about the base offset in case planar format is also set.

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