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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
