From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id A1F5A6E4DD for ; Sat, 23 Oct 2021 00:15:25 +0000 (UTC) Date: Fri, 22 Oct 2021 17:15:24 -0700 Message-ID: <87lf2kk34z.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: References: <20211021043508.1158148-1-priyanka.dandamudi@intel.com> <20211021043508.1158148-4-priyanka.dandamudi@intel.com> <20211021073321.GC3640@zkempczy-mobl2> <87y26l9atx.wl-ashutosh.dixit@intel.com> <87wnm59a0x.wl-ashutosh.dixit@intel.com> <20211022044032.GA8290@zkempczy-mobl2> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-2 Content-Transfer-Encoding: quoted-printable Subject: Re: [igt-dev] [PATCH i-g-t 3/4] lib/igt_gt: Add compute engine List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Tvrtko Ursulin Cc: Zbigniew =?ISO-8859-2?Q?Kempczy=F1ski?= , priyanka.dandamudi@intel.com, igt-dev@lists.freedesktop.org, arjun.melkaveri@intel.com, John Harrison , Jason Ekstrand List-ID: On Fri, 22 Oct 2021 05:07:41 -0700, Tvrtko Ursulin wrote: > On 22/10/2021 05:40, Zbigniew Kempczy=F1ski 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=F1ski wrote: > >>>> > >>>> On Thu, Oct 21, 2021 at 10:05:07AM +0530, priyanka.dandamudi@intel.c= om wrote: > >>>>> From: Vinay Belgaumkar > >>>>> > >>>>> Add compute (CCS) engine. Add this to the IGT > >>>>> structure to allow gem tests to execute. > >>>>> > >>>>> Signed-off-by: Stuart Summers > >>>>> Signed-off-by: Vinay Belgaumkar > >>>>> Signed-off-by: Priyanka Dandamudi > >>>>> Cc: Ashutosh Dixit > >>>>> Cc: Arjun Melkaveri > >>>>> --- > >>>>> 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_exe= cution_engines2[] =3D { > >>>>> { "vcs1", I915_ENGINE_CLASS_VIDEO, 1, I915_EXEC_BSD | I915_EXEC_BS= D_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 le= gacy > >>> 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 engin= es 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 ad= ding > >> 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 I= GT. > > > > 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 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 > 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