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

[Dixit, Ashutosh]

On Fri, 22 Oct 2021 05:07:41 -0700, Tvrtko Ursulin wrote:
> 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.

Thanks Tvrtko, still looking at it but considering that future products may
have different number of engines of different classes there are two options
here (a) list every possible engine of every possible class of every
possible product here, or (b) list only a minimum set of engines which
every product is guaranteed to have (if some engines of this minimum set
are missing in a product this list may even have to be pruned).

I believe Jason's assumption during the i915 context mutation changes was
that context 0 is created with only this minimum (or legacy) set of
engines. But is that context 0 engine-set also what
intel_execution_engines2 is referring to? This is what my assumption is as
I was saying above. Hoping Jason responds with his thoughts.

> The array would then also be used from elsewhere in the code to map class
> and instance to name and similar.

Engine names are dynamically generated (from class name and engine
instance) in init_engine() since:

commit 6fa36c0233afb2cf98ed41c2b2255ab484e79add
Author: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Date:   Wed Jan 22 15:23:48 2020 +0000

    i915/gem_engine_topology: Generate engine names based on class

> 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