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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 22 Oct 2021 17:15:24 -0700, Dixit, Ashutosh wrote:
> 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@intel.com wrote:
> > >>>>> From: Vinay Belgaumkar <vinay.belgaumkar@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@intel.com>
> > >>>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > >>>>> Signed-off-by: Priyanka Dandamudi <priyanka.dandamudi@intel.com>
> > >>>>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > >>>>> Cc: Arjun Melkaveri <arjun.melkaveri@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.

Also by intel_ctx_0 I really mean any "legacy context" (any context created
with an all zero intel_ctx_cfg_t). At least one on which
I915_CONTEXT_PARAM_ENGINES has not been executed.

Also (I really should have asked this when reviewing Jason's patches) why
do we have this complication of a "legacy context" which only has "legacy
engines" (see legacy_ring_idx() in i915)? Why can't the default engines for
any context be all physical engines present on a platform (so that
I915_CONTEXT_PARAM_ENGINES is used only to restrict the number of engines
when needed, not to expand the number of engines as we do now). This would
have saved a lot of this intel_ctx_0 complication in IGT. Thanks.

> > 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@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@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