From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id 949D36EA85 for ; Wed, 30 Jun 2021 21:31:44 +0000 (UTC) Date: Wed, 30 Jun 2021 14:31:42 -0700 Message-ID: <87eecjavk1.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: References: <20210617191256.577244-1-jason@jlekstrand.net> <20210617191516.577394-22-jason@jlekstrand.net> <871r8l930c.wl-ashutosh.dixit@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Subject: Re: [igt-dev] [PATCH i-g-t 71/79] lib/i915: Rework engine API availability checks (v2) List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Jason Ekstrand Cc: IGT GPU Tools List-ID: On Wed, 30 Jun 2021 10:57:07 -0700, Jason Ekstrand wrote: > > On Mon, Jun 28, 2021 at 2:56 PM Dixit, Ashutosh > wrote: > > > > On Thu, 17 Jun 2021 12:15:08 -0700, Jason Ekstrand wrote: > > > > > > bool gem_has_engine_topology(int fd) > > > { > > > - struct drm_i915_gem_context_param param = { > > > - .param = I915_CONTEXT_PARAM_ENGINES, > > > - }; > > > - > > > - return !__gem_context_get_param(fd, ¶m); > > > + struct intel_engine_data ed; > > > + return !__query_engine_list(fd, &ed); > > > > Is this correct? Isn't this actually a check for the presence of > > I915_CONTEXT_PARAM_ENGINES (seems so looking from which this is called)? > > You're right that the check here is slightly different. The old check > queried the engine set on ctx0 but the new check queries the global > engine set. The three callers are gem_ctx_engines, gem_exec_balancer, > and gem_exec_schedule, all of which really want "is the engines API > available?" I don't think it matters which one we query from a kernel > support POV. Every kernel that has the global engines query should > have the CONTEXT_PARAM_ENGINES and vice versa. Can we add a comment to the function about this since it is not obvious. > > > So we could probably just create a new context and do a set_engines on it and > > return true if that succeeded? > > Yes, we could do that. It's just a lot more annoying to get right > because we first have to query the engine set to get a valid engine to > attempt to set. I'm not sure there's a point. > > Or do that in addition to > > __query_engine_list? Maybe intel_ctx_create_all_physical() will do both? > > Not sure what you mean by this. I was saying if we did a intel_ctx_create_all_physical() it would exercise and validate that all required uapi's are present in the kernel. However if you are saying that "Every kernel that has the global engines query should have the CONTEXT_PARAM_ENGINES and vice versa." then it's not needed and it seems there are no further changes to this needed (execpt maybe the comment I suggested above). Therefore this is: Reviewed-by: Ashutosh Dixit > > --Jason > > > Thanks. _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev