[igt-dev] [PATCH i-g-t v2 05/13] igt: fb: Move size computation to the common path

[Paul Kocialkowski]

Hi,

On Tue, 2019-01-08 at 16:19 +0100, Maxime Ripard wrote:
> In order to properly support the YUV buffer on non-i915 platforms, we need
> to run calc_fb_size for the dumb buffers allocation path too. Move it out
> of the special case and in the common code path.

Actually, I have one minor comment below.

> Signed-off-by: Maxime Ripard <maxime.ripard at bootlin.com>
> ---
>  lib/igt_fb.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d69c3fb2d38d..31e44d6188d9 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -528,16 +528,14 @@ static void clear_yuv_buffer(struct igt_fb *fb)
>  /* helpers to create nice-looking framebuffers */
>  static int create_bo_for_fb(struct igt_fb *fb)
>  {
> +	uint64_t size = calc_fb_size(fb);
>  	int fd = fb->fd;
>  
> +	/* respect the size requested by the caller */
> +	if (fb->size == 0)
> +		fb->size = size;
> +
>  	if (fb->tiling || fb->size || fb->strides[0] || igt_format_is_yuv(fb->drm_format)) {

Calling calc_fb_size when the function begins will automatically set
the strides if it was previously 0, so we can drop the "|| fb-
>strides[0]" here (will never happen).

Generally speaking, I think it would be much more simple to always go
with i915 gem create instead of dumb alloc as soon as i915 is detected
and drop all these conditions.

I can't see why dumb buffer would be preferable, but I might be missing
something here (i915 developers, feel free to point it out if that's
the case)!

Cheers,

Paul

> -		uint64_t size;
> -
> -		size = calc_fb_size(fb);
> -
> -		/* respect the size requested by the caller */
> -		if (fb->size == 0)
> -			fb->size = size;
>  
>  		fb->is_dumb = false;
>  
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com