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 7.09.2023 15:36, Ch, Sai Gowtham wrote:
> 
> 
>> -----Original Message-----
>> From: Kempczynski, Zbigniew <zbigniew.kempczynski@intel.com>
>> Sent: Thursday, September 7, 2023 3:06 PM
>> To: Ch, Sai Gowtham <sai.gowtham.ch@intel.com>
>> Cc: igt-dev@lists.freedesktop.org; Stolarek, Karolina
>> <karolina.stolarek@intel.com>
>> Subject: Re: [igt-dev] [PATCH i-g-t 2/4] lib/intel_blt: check if blt commands are
>> supported by the platforms
>>
>> On Tue, Sep 05, 2023 at 06:11:54AM +0200, Ch, Sai Gowtham wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Kempczynski, Zbigniew <zbigniew.kempczynski@intel.com>
>>>> Sent: Monday, September 4, 2023 6:10 PM
>>>> To: Ch, Sai Gowtham <sai.gowtham.ch@intel.com>
>>>> Cc: igt-dev@lists.freedesktop.org; Stolarek, Karolina
>>>> <karolina.stolarek@intel.com>
>>>> Subject: Re: [igt-dev] [PATCH i-g-t 2/4] lib/intel_blt: check if blt
>>>> commands are supported by the platforms
>>>>
>>>> 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.
>>> Do you mean path for i915 needs to be enabled ?
>>
>> Yes, if I'm not wrong PVC will be supported on i915 as either.
>> So if blt_has_mem_set() will return true for it it should have the implementation
>> in the blitter library. Generally you got almost everything you need
>> (mem_set/copy code is already written so what you need is to extract this and
>> support two ways of entering it (for i915 and for xe).
> 
> Sure will make this work for both i915 and xe.
>>
>>>> mem-set and mem-copy commands you've introduced in 1/4 patch should
>>>> be part of intel_blt.[ch] library.
>>> Do you mean MEM_SET and MEM_COPY  in 3/4 patch ? Can you elaborate ?
>>
>> Related to above comment. You need to have batch commands + prepare
>> execution. Batch commands + execution for Xe you already have in the test. I
>> would migrate this to intel_blt.c and add i915 execution path.
>>
> do you mean something similar to "emit_blt_fast_copy" for fast_copy, mem_copy/set has to be migrated to
> Intel_blt.c ?

I'll jump on this.

That would be something similar, yes, but the interface would be 
different. You can't reuse blt_copy_data struct, and you'll need to 
define extra structs to describe source, destination and the command 
itself. Also, I'm not sure if we'd get much value from pipelined 
mem_copy/set (i.e. many copies issued in one BB), we could have just a 
single copy/set instead of emitting it.

That could stir a debate, but, in my opinion, it should be fine to first 
finish writing the test as it is, and then extract the implemented 
functions to intel_blt. Writing an additional path for i915 will take 
time. Sai, could you check if this is also a priority? If yes, then I'm 
all for implementing it as a part of the series. If not, well, I would 
reconsider doing it.

All the best,
Karolina

> 
> ----
> Ch Sai Gowtham
>> Then in the test just call the mem_set or mem_copy depending what you want
>> to test to.
>>
>> --
>> Zbigniew
>>
>>>>
>>>> BTW first patch 1/4 won't compile in step-by-step mode because it
>>>> uses code from following patches.
>>> I'll correct that in my next patch series.
>>> ----
>>> Ch Sai Gowtham
>>>>
>>>> --
>>>> 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
>>>>>