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

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

Hi,

On Tue, 2019-01-22 at 15:28 +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.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@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..a1368ec4703a 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;
>  
> -	if (fb->tiling || fb->size || fb->strides[0] || igt_format_is_yuv(fb->drm_format)) {
> -		uint64_t size;
> +	/* respect the size requested by the caller */
> +	if (fb->size == 0)
> +		fb->size = size;
>
> -		size = calc_fb_size(fb);
> -
> -		/* respect the size requested by the caller */
> -		if (fb->size == 0)
> -			fb->size = size;
> +	if (fb->tiling || fb->size || igt_format_is_yuv(fb->drm_format)) {

So in this case, fb->size would also never be zero (it's always set a
few lines before). Looking back at all this, we lose the ability to set
an arbitrary stride and size (unless it's Intel and tiled or YUV),
which is a bit problematic.

I think those things are definitely good to allow and keep, so I guess
we should hit the driver-specific GEM allocation when they occur (as
was the case until now).

In cases where we don't have any driver-specific implementation that
might allow a specific size/stride, I think it would be best to produce
an error rather than falling back on dumb alloc that won't honor what
was requested.

So that implies keeping stride = 0 and size = 0 intact when going
through this function.

I should probably have thought this up on the previous revision before
advising to remove the check on the stride though, sorry.

What do you think?

Cheers,

Paul

>  		fb->is_dumb = false;
>  
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev