[igt-dev] [PATCH i-g-t 1/2] lib/igt_fb: Use cairo conversion in igt_fb_convert_with_stride, v3.

[Paul Kocialkowski]

Hi,

It looks like I fell behind on reviewing earlier version here, sorry
about that.

On Fri, 2019-05-10 at 14:33 +0200, Maarten Lankhorst wrote:
> Ever since commit 3fa65f4b532bd9a5b ("fb: Add support for conversions
> through pixman") we can generate a valid cairo surface for any plane,
> use this to avoid having to implement our own conversion routine.
> 
> Instead of duplicating this functionality in igt_fb_convert_with_stride,
> we can simply convert this to a few cairo calls, because we now support
> cairo calls to any of the supported framebuffer formats.

I don't see how that is the case. Cairo definitely does not support our
tiled formats, so we can't just pipe them into it.

And we already use pixman for converting through the fb_convert call to
convert_pixman when possible. Also, my understanding is that cairo is
very limited format-wise, so we're much better off using pixman
directly (which is is what a non-accelerated cairo surface will do
anyway).

> This is required to make this function more generic, and convert from any
> format/modifier to any other format/modifier.

We've already designed it to be generic, we just need conversion
helpers from/to (currently only to) hardware-specific tiling modifiers.

We can't expect cairo or pixman to do that job and this change pretty
much kills support for the vc4 tiling modes we've added.

Unless I'm missing something, that's a NACK on my side.

Cheers,

Paul

> Changes since v1:
> - Return fb_id in the cairo case.
> Changes since v2:
> - Remove the manual conversion fallback.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Paul Kocialkowski <paul.kocialkowski at bootlin.com>
> ---
>  lib/igt_fb.c | 59 +++++++++++-----------------------------------------
>  1 file changed, 12 insertions(+), 47 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index d4929019971c..d7ccbc19b834 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -3102,58 +3102,23 @@ unsigned int igt_fb_convert_with_stride(struct igt_fb *dst, struct igt_fb *src,
>  					uint64_t dst_modifier,
>  					unsigned int dst_stride)
>  {
> -	struct fb_convert cvt = { };
> -	struct igt_fb linear;
> -	void *dst_ptr, *src_ptr;
> -	uint64_t base_modifier;
> +	/* Use the cairo api to convert */
> +	cairo_surface_t *surf = igt_get_cairo_surface(src->fd, src);
> +	cairo_t *cr;
>  	int fb_id;
>  
> -	if (is_vc4_device(src->fd))
> -		base_modifier = fourcc_mod_broadcom_mod(dst_modifier);
> -	else
> -		base_modifier = dst_modifier;
> -
> -	fb_id = igt_create_fb_with_bo_size(src->fd, src->width, src->height,
> -					   dst_fourcc,
> -					   LOCAL_DRM_FORMAT_MOD_NONE, &linear,
> -					   0, dst_stride);
> +	fb_id = igt_create_fb_with_bo_size(src->fd, src->width,
> +					   src->height, dst_fourcc,
> +					   dst_modifier, dst, 0,
> +					   dst_stride);
>  	igt_assert(fb_id > 0);
>  
> -	src_ptr = igt_fb_map_buffer(src->fd, src);
> -	igt_assert(src_ptr);
> -
> -	dst_ptr = igt_fb_map_buffer(linear.fd, &linear);
> -	igt_assert(dst_ptr);
> -
> -	cvt.dst.ptr = dst_ptr;
> -	cvt.dst.fb = &linear;
> -	cvt.src.ptr = src_ptr;
> -	cvt.src.fb = src;
> -	fb_convert(&cvt);
> -
> -	igt_fb_unmap_buffer(dst, dst_ptr);
> -	igt_fb_unmap_buffer(src, src_ptr);
> -
> -	switch (base_modifier) {
> -	case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
> -		fb_id = igt_vc4_fb_t_tiled_convert(dst, &linear);
> -		break;
> -	case DRM_FORMAT_MOD_BROADCOM_SAND32:
> -	case DRM_FORMAT_MOD_BROADCOM_SAND64:
> -	case DRM_FORMAT_MOD_BROADCOM_SAND128:
> -	case DRM_FORMAT_MOD_BROADCOM_SAND256:
> -		fb_id = vc4_fb_sand_tiled_convert(dst, &linear, dst_modifier);
> -		break;
> -	default:
> -		igt_assert(dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE);
> -	}
> -
> -	igt_assert(fb_id > 0);
> +	cr = igt_get_cairo_ctx(dst->fd, dst);
> +	cairo_set_source_surface(cr, surf, 0, 0);
> +	cairo_paint(cr);
> +	igt_put_cairo_ctx(dst->fd, dst, cr);
>  
> -	if (dst_modifier == LOCAL_DRM_FORMAT_MOD_NONE)
> -		*dst = linear;
> -	else
> -		igt_remove_fb(linear.fd, &linear);
> +	cairo_surface_destroy(surf);
>  
>  	return fb_id;
>  }
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com