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 8.5.2024 17.54, Kamil Konieczny wrote:
> Hi Juha-Pekka,
> On 2024-04-30 at 19:29:34 +0300, Juha-Pekka Heikkila wrote:
>> mc ccs need to go to vebox copy, blitter doesn't do mc ccs hence
> - ^^^^^^
> Could you decipher here (or even better as a comment in source and
> here) what does 'mc ccs' mean?

It hasn't been common practice to explain any framebuffer modifiers in 
detail at this level because they are comprehensively documented in 
include/drm-uapi/drm_fourcc.h which is copy from kernel sources.

Leaving comment here now could cause confusion if there come changes, 
it's better they're documented in same place where they are defined.

>> 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 (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);
>> +
>>   		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);
> 
> This is repeated code, what about a function like:
> 
> copy_with_engine_or_blit(blit, &linear->fb, fb);

That's good idea, this type restructuring will go into a new patch. Most 
of this set already is just about restructuring blitter path to be more 
readable. But scope of this patch is about fixing mc ccs and x-tile path 
on all platforms.

/Juha-Pekka

> 
>>   
>>   		linear->map = xe_bo_mmap_ext(fd, linear->fb.gem_handle,
>>   					     linear->fb.size, PROT_READ | PROT_WRITE);
>> -- 
>> 2.25.1
>>