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

[Tvrtko Ursulin]

On 11/03/2019 16:22, Chris Wilson wrote:
> 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.

The latter is hypothetical, no? Current patches don't have this ability 
AFAIR. Why would we want this?

>>>        .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.)

Everyone if fine with it so lets not change it much. :)

Regards,

Tvrtko