Re: [igt-dev] [PATCH i-g-t v5 07/13] igt: fb: Don't pass the stride when allocating a dumb, multi-planar buffer

Lyude Paul
2019-01-31T18:43:00Z

On Wed, 2019-01-30 at 16:38 +0100, Maxime Ripard wrote:
> Hi!
>
> On Tue, Jan 29, 2019 at 03:08:10PM -0500, Lyude Paul wrote:
> > On Fri, 2019-01-25 at 15:58 +0100, Maxime Ripard wrote:
> > > The dumb buffer allocation API only considers a single plane, and even
> > > though allocating multi-planar buffers through it is allowed, the stride
> > > it
> > > gives back is the the width times the bpp passed as an argument.
> > >
> > > That doesn't work in our case, since the bpp is going to be the one we
> > > give
> > > as an argument, but split over three planes so the stride doesn't match
> > > anymore.
> > >
> > > A proper fix for this would be to have a better dumb buffer allocation
> > > API,
> > > but for the time being, let's do it that way.
> > >
> > > Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > ---
> > > lib/igt_fb.c | 17 ++++++++++++++++-
> > > 1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > index 1c52aebb674e..048d274e5d36 100644
> > > --- a/lib/igt_fb.c
> > > +++ b/lib/igt_fb.c
> > > @@ -530,6 +530,7 @@ static int create_bo_for_fb(struct igt_fb *fb)
> > > {
> > > const struct format_desc_struct *fmt = lookup_drm_format(fb-
> > > > drm_format);
> > > unsigned int plane, bpp;
> > > + unsigned *strides = &fb->strides[0];
> > > int fd = fb->fd;
> > >
> > > if (fb->tiling || fb->size || fb->strides[0] || igt_format_is_yuv(fb-
> > > > drm_format)) {
> > > @@ -575,8 +576,22 @@ static int create_bo_for_fb(struct igt_fb *fb)
> > > plane ? fmt->hsub * fmt->vsub : 1);
> > >
> > > fb->is_dumb = true;
> > > +
> > > + /*
> > > + * We can't really pass the stride array here since the dumb
> > > + * buffer allocation is assuming that it operates on one
> > > + * plane, and therefore will calculate the stride as if each
> > > + * pixels were stored on a single plane.
> >
> > nitpick: s/each pixels were/each pixel was/
> >
> > > + *
> > > + * This might cause issues at some point on drivers that would
> > > + * change the stride of YUV buffers, but we haven't
> > > + * encountered any yet.
> > > + */
> > Is it possible to add an igt_assert to check for this? Either way, with
> > those
> > changes:
>
> I'm not sure we can test this actually. If we don't pass any pointer,
> then we don't get a stride back, and if we do then we will always get
> a stride that is wrong for what we're trying to do :/
Ah, makes sense
>
> Maxime
>
--
Cheers,
Lyude Paul