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

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 9.5.2024 12.47, Zbigniew Kempczyński wrote:
> 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@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.

On i915 there was already vebox in use for tgl/dg1/dg2. What I get with 
this along with those other changes is mc ccs and will go to vebox on xe 
and x-tile will go to rendercopy instead of fastblit path on xe when 
have legacy blitter.

> 
>>   
>>   	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.

This is definitely something that would make things better. As is these 
blitter/engine copy selection should be further cleaned in any case. 
Here in igt_fb.c these rules are very messy in current state.

> 
> 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@intel.com>

Thanks! I'll merge this one patch because this is bug fix and Jani 
wanted to have this merged. Those other patches I'll fix, include that 
change Kamil did wanted to see and have another round.

> --
> 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
>>