[igt-dev] [PATCH i-g-t 3/4] lib/igt_gt: Add compute engine

[Tvrtko Ursulin]

On 22/10/2021 05:40, Zbigniew Kempczyński wrote:
> On Thu, Oct 21, 2021 at 05:29:50PM -0700, Dixit, Ashutosh wrote:
>> On Thu, 21 Oct 2021 17:12:26 -0700, Dixit, Ashutosh wrote:
>>>
>>> On Thu, 21 Oct 2021 00:33:21 -0700, Zbigniew Kempczyński wrote:
>>>>
>>>> On Thu, Oct 21, 2021 at 10:05:07AM +0530, priyanka.dandamudi at intel.com wrote:
>>>>> From: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>>>>>
>>>>> Add compute (CCS) engine. Add this to the IGT
>>>>> structure to allow gem tests to execute.
>>>>>
>>>>> Signed-off-by: Stuart Summers <stuart.summers at intel.com>
>>>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>>>>> Signed-off-by: Priyanka Dandamudi <priyanka.dandamudi at intel.com>
>>>>> Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
>>>>> Cc: Arjun Melkaveri <arjun.melkaveri at intel.com>
>>>>> ---
>>>>>   lib/igt_gt.c | 4 ++++
>>>>>   1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
>>>>> index a0ba04cc..80fb65ca 100644
>>>>> --- a/lib/igt_gt.c
>>>>> +++ b/lib/igt_gt.c
>>>>> @@ -604,6 +604,10 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
>>>>> 	{ "vcs1", I915_ENGINE_CLASS_VIDEO, 1, I915_EXEC_BSD | I915_EXEC_BSD_RING2 },
>>>>> 	{ "vcs2", I915_ENGINE_CLASS_VIDEO, 2, -1 },
>>>>> 	{ "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0, I915_EXEC_VEBOX },
>>>>> +	{ "ccs0", I915_ENGINE_CLASS_COMPUTE, 0 , -1},
>>>>> +	{ "ccs1", I915_ENGINE_CLASS_COMPUTE, 1 , -1},
>>>>> +	{ "ccs2", I915_ENGINE_CLASS_COMPUTE, 2 , -1},
>>>>> +	{ "ccs3", I915_ENGINE_CLASS_COMPUTE, 3 , -1},
>>>
>>> Is this correct? Isn't intel_execution_engines2 a list of just the legacy
>>> engines? I don't think compute engines (which may vary in number) can be
>>> added to the list of legacy engines. Isn't it true that compute engines can
>>> only be accessed by querying the present engines dynamically (using
>>> something like intel_ctx_create_all_physical())?
>>
>> I've copied Tvrtko (the boss :), but I am pretty sure what I said is
>> correct. Legacy engines are what context 0 is created with (without adding
>> any more engines onto context 0). So unless we can confirm that these
>> engines are present for context 0 in the kernel we can't add them in IGT.
> 
> I'm sorry, you're right. I wasn't aware legacy engines are "frozen", so
> context 0 won't get an access to ccs. My r-b is incorrect then and patch
> shouldn't land.  Another problem you've just realized me is with above
> patch I would still need to detect ccs somehow on context 0 depending
> on gen to allow iterator to go over existing engines.

Don't know any longer guys - in the past "static engines" iterator was 
supposed to be used to enumerate all possible engines _without_ querying 
the device (like when enumerating subtests it isn't allowed to open the 
device).

As such CCS would go to intel_execution_engines2 with "-1" for flags, as 
above patch had it. In other words in the past it was supposed to 
contain all engines i915 engine query could possibly return. The array 
would then also be used from elsewhere in the code to map class and 
instance to name and similar.

Whether or not all this changed in the intel_ctx_t rewrite I have no 
idea. I see the "legacy" comment was added in:

commit 9b32262bfadffffcde33c18ffb7c5292fbf4901e
Author: Jason Ekstrand <jason at jlekstrand.net>
Date:   Thu Apr 15 12:42:53 2021 -0500

     docs: Add gem_engine_topology.h to the docs

So perhaps ask Jason what was the plan there.

Regards,

Tvrtko