[igt-dev] [PATCH i-g-t 71/79] lib/i915: Rework engine API availability checks (v2)

[Dixit, Ashutosh]

On Wed, 30 Jun 2021 10:57:07 -0700, Jason Ekstrand wrote:
>
> On Mon, Jun 28, 2021 at 2:56 PM Dixit, Ashutosh
> <ashutosh.dixit at intel.com> 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, &param);
> > > +     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 <ashutosh.dixit at intel.com>


>
> --Jason
>
> > Thanks.