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?
.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)?
I am not saying it makes sense, just thinking out loud.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
