[PATCH i-g-t v3 1/6] lib/igt_fb: fix intel modifiers for fb copying on xe driver

[Zbigniew Kempczyński]

On Tue, Apr 30, 2024 at 07:29:34PM +0300, Juha-Pekka Heikkila wrote:
> mc ccs need to go to vebox copy, blitter doesn't do mc ccs hence
> on all platforms with ccs modifiers use engine copy for those.
> Use render engine for x-tile on legacy platforms where x-tile
> would otherwise endup on fastblit patch which is known to have
> limitations.
> 
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> ---
>  lib/igt_fb.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index cc70cb91c..5df5cb08f 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2508,9 +2508,12 @@ static bool blitter_ok(const struct igt_fb *fb)
>  	if (!is_intel_device(fb->fd))
>  		return false;
>  
> -	if ((is_ccs_modifier(fb->modifier) &&
> -	     !HAS_FLATCCS(intel_get_drm_devid(fb->fd))) ||
> -	     is_gen12_mc_ccs_modifier(fb->modifier))
> +	if ((!HAS_FLATCCS(intel_get_drm_devid(fb->fd)) &&
> +	    is_ccs_modifier(fb->modifier)) ||
> +	    is_gen12_mc_ccs_modifier(fb->modifier) ||
> +	    (!blt_uses_extended_block_copy(fb->fd) &&
> +	    fb->modifier == I915_FORMAT_MOD_X_TILED &&
> +	    is_xe_device(fb->fd)))
>  		return false;

If I'm not wrong this will switch to vebox copy on TGL and DG1
are other platforms also touched? TGL works both on i915/xe
whereas if I'm not wrong DG1 is i915 only.

>  
>  	if (is_xe_device(fb->fd))
> @@ -2551,6 +2554,7 @@ static bool use_enginecopy(const struct igt_fb *fb)
>  		return false;
>  
>  	return fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
> +	       fb->modifier == I915_FORMAT_MOD_X_TILED ||
>  	       (!HAS_FLATCCS(intel_get_drm_devid(fb->fd)) && is_ccs_modifier(fb->modifier)) ||
>  	       is_gen12_mc_ccs_modifier(fb->modifier);
>  }
> @@ -3062,7 +3066,12 @@ static void free_linear_mapping(struct fb_blit_upload *blit)
>  		igt_nouveau_delete_bo(&linear->fb);
>  	} else if (is_xe_device(fd)) {
>  		gem_munmap(linear->map, linear->fb.size);
> -		blitcopy(fb, &linear->fb);
> +
> +		if (blit->ibb)
> +			copy_with_engine(blit, fb, &linear->fb);
> +		else
> +			blitcopy(fb, &linear->fb);

I've taken a look at the code and blitcopy() which does:

	if (is_xe)
		do_block_copy(src_fb, dst_fb, mem_region, i, ahnd,
			      bb, bb_size, xe_ctx, NULL);

uses implicit assumptions regarding mc ccs established in blitter_ok()
and use_enginecopy(). Do you see an option to migrate some
tiling/formats/etc selection logic to intel_cmds_info? I mean I would
like to use some centralized place to keep all the knowledge about
that instead hardcoding conditions around the code.

Anyway, I see no objections to merge as it is, as such refactoring
is time consuming and not necessary at the moment. But if you would
consider to do that in the future it would be great.

>From me at the moment:

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
--
Zbigniew
> +
>  		gem_close(fd, linear->fb.gem_handle);
>  	} else {
>  		gem_munmap(linear->map, linear->fb.size);
> @@ -3142,7 +3151,10 @@ static void setup_linear_mapping(struct fb_blit_upload *blit)
>  
>  		linear->map = igt_nouveau_mmap_bo(&linear->fb, PROT_READ | PROT_WRITE);
>  	} else if (is_xe_device(fd)) {
> -		blitcopy(&linear->fb, fb);
> +		if (blit->ibb)
> +			copy_with_engine(blit, &linear->fb, fb);
> +		else
> +			blitcopy(&linear->fb, fb);
>  
>  		linear->map = xe_bo_mmap_ext(fd, linear->fb.gem_handle,
>  					     linear->fb.size, PROT_READ | PROT_WRITE);
> -- 
> 2.25.1
>