[Intel-gfx] [PATCH 08/13] drm/i915: Allow a context to define its set of engines

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-11 16:16:27)
> 
> On 11/03/2019 14:45, Chris Wilson wrote:
> > Quoting Chris Wilson (2019-03-11 09:45:17)
> >> Quoting Tvrtko Ursulin (2019-03-11 09:23:44)
> >>>
> >>> On 08/03/2019 16:47, Chris Wilson wrote:
> >>>> Quoting Tvrtko Ursulin (2019-03-08 16:27:22)
> >>>>>
> >>>>> On 08/03/2019 14:12, Chris Wilson wrote:
> >>>>>> +static int
> >>>>>> +set_engines(struct i915_gem_context *ctx,
> >>>>>> +         const struct drm_i915_gem_context_param *args)
> >>>>>> +{
> >>>>>> +     struct i915_context_param_engines __user *user;
> >>>>>> +     struct set_engines set = { .ctx = ctx };
> >>>>>> +     u64 size, extensions;
> >>>>>> +     unsigned int n;
> >>>>>> +     int err;
> >>>>>> +
> >>>>>> +     user = u64_to_user_ptr(args->value);
> >>>>>> +     size = args->size;
> >>>>>> +     if (!size)
> >>>>>> +             goto out;
> >>>>>
> >>>>> This prevents a hypothetical extension with empty map data.
> >>>>
> >>>> No... This is required for resetting and I think that's covered in what
> >>>> little docs there are. It's the set.nengine==0 test later
> >>>> that you mean to object to. But we can't do that as that's how we
> >>>> differentiate between modes at the moment.
> >>>>
> >>>> We could use ctx->nengine = 0 and ctx->engines = ZERO_PTR.
> >>>
> >>> size == sizeof(struct i915_context_param_engines) could mean reset -
> >>> meaning no map array provided.
> >>
> >> Nah, size=sizeof() => 0 [], size=0 => default map.
> >>   
> >>> Meaning one could reset the map and still pass in extensions.
> >>
> >> I missed that you were pointing out we didn't follow the extensions on
> >> resetting.
> >>
> >> I'm not sure if that makes sense tbh. The extensions are written around
> >> the concept of applying to the new engines[], and if the use has
> >> explicitly removed the engines[] (distinct from defining a zero array),
> >> what extensions can apply? One hopes they end up -EINVAL. As they should
> >> -EINVAL, I guess it is no harm done to apply them.
> > 
> > Ok, the problem with the size=0 case is that quite obviously there is no
> > extension chain to follow. (That was silly of me.)
> > 
> > I think
> >       .size = 0 => reset to default
> > and
> >       .size = sizeof(arg) => 0 engines ([])
> 
> What is the difference between these two?

One uses the legacy ring mask, and the other is a context with no
engines.

> >       .size = sizeof(arg) + N*sizeof(*class_instance) => N engines ([N])
> > make the most logical sense, which does imply that if you want to apply
> > extension options to ctx->engines[] you need to specify them.
> > 
> > And that also implies that if we have an extension that may make sense
> > to the default setup, then either we say creating the engine[] map is
> > compulsory, or we don't use a set-engines extension for that.
> 
> Hm.. load_balance is extension of set_engines. If we wanted to go crazy 
> we could also support it directly from set_param(param=LOAD_BALANCE)?

My thinking was that it only made sense as a constructor property. (Back
before we hooked set-parameter for constructor properties). I still like
how set-engines + extensions has turned out. (But haven't coded up the
alternatives, so who knows.)
-Chris