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

[Karolina Stolarek]

On 13.09.2023 09:39, Zbigniew Kempczyński wrote:
> On Mon, Sep 11, 2023 at 01:09:31PM +0200, Karolina Stolarek wrote:
>> On 4.09.2023 14:39, Zbigniew Kempczyński wrote:
>>> On Mon, Sep 04, 2023 at 04:34:56PM +0530, sai.gowtham.ch at intel.com wrote:
>>>> From: Sai Gowtham Ch <sai.gowtham.ch at intel.com>
>>>>
>>>> Check if mem-copy/mem-set commands are supported by fd device.
>>>>
>>>> Signed-off-by: Sai Gowtham Ch <sai.gowtham.ch at 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.
> 
> Yes, I hoped we will add this implementation. Adding relocations isn't
> problem here but I would stick to softpinning/vm_bind at the moment.

I was asked about adding support for relocations to intel_blt at least 
once, and I experienced it as a problem myself when working on 
gem_blits. So, it would be good to see them introduced there.

The tests that use blit commands are expected to work across 
generations. The blit library won't be widely adopted unless we provide 
support for relocations.

> Perfect would be if we claim that some command is supported we provide
> an implementation of such blit command for i915/xe.

OK, that's new to me, especially given the fact that we allow folks to 
query about supported commands even if they build them on their own. 
This library can exist on its own and doesn't have to be tied to 
intel_blt. I mean, it is now due to the review comments I got during the 
review.

But I agree, the ultimate goal would be to get everyone to use 
intel_blt, but it's not ready for it, yet.

All the best,
Karolina

> 
>>
>> 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.
> 
> Converging intel_blt to have implementation of supported hw blits should
> be our target - that's why I'm asking for mem_set/copy. Adding
> xy-src-copy and test it inside gem|xe_exercise_blt will close this gap.
> 
> --
> Zbigniew
> 
>>
>> 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
>>>>