Re: [igt-dev] [PATCH i-g-t v5 05/13] igt: fb: Refactor dumb buffer allocation path

Deepak Singh Rawat
2019-01-25T16:16:28Z

On Fri, 2019-01-25 at 15:58 +0100, Maxime Ripard wrote:
> The else condition is not needed, since all the other conditions
> return
> when they are done.
>
> Move the KMS dumb buffer allocation outside of the outer else
> condition,
> this will also allow to ease later changes.
>
> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
> lib/igt_fb.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d69c3fb2d38d..270db8d6dc90 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -557,15 +557,24 @@ static int create_bo_for_fb(struct igt_fb *fb)
> igt_require(driver_has_gem_api);
> return -EINVAL;
> }
> - } else {
> - fb->is_dumb = true;
> -
> - fb->gem_handle = kmstest_dumb_create(fd, fb->width, fb-
> >height,
> - fb->plane_bpp[0],
> - &fb->strides[0],
> &fb->size);
> -
> - return fb->gem_handle;
> }
> +
> + /*
> + * The current dumb buffer allocation API doesn't really allow
> to
> + * specify a custom size or stride. Yet the caller is free to
> specify
> + * them, so we need to make sure to error out in this case.
> + */
> + igt_assert(fb->size == 0);
> + igt_assert(fb->strides[0] == 0);

Hi Maxime, thanks for doing this. I am not an expert but do you think
this should be igt_skip instead? Tests will simply fail where they
should really be skipping if driver don't support the feature.

> +
> + fb->size = calc_fb_size(fb);
> +
> + fb->is_dumb = true;
> + fb->gem_handle = kmstest_dumb_create(fd, fb->width, fb->height,
> + fb->plane_bpp[0],
> + &fb->strides[0], &fb-
> >size);
> +
> + return fb->gem_handle;
> }
>
> /**