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-29T20:08:13Z

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:

Reviewed-by: Lyude Paul <lyude@redhat.com>

> + if (fb->num_planes > 1)
> + strides = NULL;
> +
> fb->gem_handle = kmstest_dumb_create(fd, fb->width, fb->height,
> - bpp, &fb->strides[0], &fb->size);
> + bpp, strides, &fb->size);
>
> return fb->gem_handle;
> }
--
Cheers,
Lyude Paul