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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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