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

[Chris Wilson]

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 ([])
	.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.
-Chris