Re: [igt-dev] [PATCH i-g-t 2/4] lib/intel_blt: check if blt commands are supported by the platforms

[Karolina Stolarek <karolina.stolarek@intel.com>]

On 4.09.2023 14:39, Zbigniew Kempczyński wrote:
> On Mon, Sep 04, 2023 at 04:34:56PM +0530, sai.gowtham.ch@intel.com wrote:
>> From: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
>>
>> Check if mem-copy/mem-set commands are supported by fd device.
>>
>> Signed-off-by: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
>> ---
>>   lib/intel_blt.c | 32 ++++++++++++++++++++++++++++++++
>>   lib/intel_blt.h |  2 ++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/lib/intel_blt.c b/lib/intel_blt.c
>> index 429511920..b55fa9b52 100644
>> --- a/lib/intel_blt.c
>> +++ b/lib/intel_blt.c
>> @@ -289,6 +289,38 @@ bool blt_has_block_copy(int fd)
>>   	return blt_supports_command(cmds_info, XY_BLOCK_COPY);
>>   }
>>   
>> +/**
>> + * blt_has_mem_copy
>> + * @fd: drm fd
>> + *
>> + * Check if mem copy is supported by @fd device
>> + *
>> + * Returns:
>> + * true if it does, false otherwise.
>> + */
>> +bool blt_has_mem_copy(int fd)
>> +{
>> +	const struct intel_cmds_info *cmds_info = GET_CMDS_INFO(fd);
>> +
>> +	return blt_supports_command(cmds_info, MEM_COPY);
>> +}
>> +
>> +/**
>> + * blt_has_mem_set
>> + * @fd: drm fd
>> + *
>> + * Check if mem set is supported by @fd device
>> + *
>> + * Returns:
>> + * true if it does, false otherwise.
>> + */
>> +bool blt_has_mem_set(int fd)
>> +{
>> +	const struct intel_cmds_info *cmds_info = GET_CMDS_INFO(fd);
>> +
>> +	return blt_supports_command(cmds_info, MEM_SET);
>> +}
>> +
> 
> It is not true if you'll use those functions in i915 igts.
> mem-set and mem-copy commands you've introduced in 1/4 patch
> should be part of intel_blt.[ch] library.

Following this logic, we'd need to provide XY_SRC_COPY_BLT in intel_blt 
library as well. We don't -- mostly because intel_blt lacks support for 
relocations.

blt_has_<command> are lightweight checks that only say if you can issue 
a specific command to a platform, and they don't depend on a driver. 
They don't guarantee that a specific blitter function is implemented in 
intel_blt.

This confusion shows that we should've keep intel_cmds_info and 
intel_blt libraries separated from the beginning. Yes, they are closely 
related, but it's intel_blt quering intel_cmds_info about hardware 
capabilities, not implemented features.

Many thanks,
Karolina

> 
> BTW first patch 1/4 won't compile in step-by-step mode because
> it uses code from following patches.
> 
> --
> Zbigniew
> 
> 
>>   /**
>>    * blt_has_fast_copy
>>    * @fd: drm fd
>> diff --git a/lib/intel_blt.h b/lib/intel_blt.h
>> index b8b3d724d..d9c8883c7 100644
>> --- a/lib/intel_blt.h
>> +++ b/lib/intel_blt.h
>> @@ -175,6 +175,8 @@ bool blt_cmd_has_property(const struct intel_cmds_info *cmds_info,
>>   			  uint32_t prop);
>>   
>>   bool blt_has_block_copy(int fd);
>> +bool blt_has_mem_copy(int fd);
>> +bool blt_has_mem_set(int fd);
>>   bool blt_has_fast_copy(int fd);
>>   bool blt_has_xy_src_copy(int fd);
>>   bool blt_has_xy_color(int fd);
>> -- 
>> 2.39.1
>>