[Intel-gfx] [PATCH 14/38] drm/i915: Introduce the i915_user_extension_method

[Tvrtko Ursulin]

On 04/03/2019 09:35, Tvrtko Ursulin wrote:
> 
> On 04/03/2019 09:04, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2019-03-04 08:54:10)
>>>
>>> On 01/03/2019 18:57, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-03-01 15:39:13)
>>>>>
>>>>> On 01/03/2019 14:03, Chris Wilson wrote:
>>>>>> +int i915_user_extensions(struct i915_user_extension __user *ext,
>>>>>> +                      const i915_user_extension_fn *tbl,
>>>>>> +                      unsigned long count,
>>>>>> +                      void *data)
>>>>>> +{
>>>>>> +     unsigned int stackdepth = 512;
>>>>>> +
>>>>>> +     while (ext) {
>>>>>> +             int err;
>>>>>> +             u64 x;
>>>>>> +
>>>>>> +             if (!stackdepth--) /* recursion vs useful 
>>>>>> flexibility */
>>>>>> +                     return -EINVAL;
>>>>>
>>>>> I don't get this. What stack? Did you mean "static unsigned int
>>>>> stackdepth" in case someone puts i915_user_extension into the 
>>>>> extension
>>>>> table? Or just a limit on number of chained extensions? But you are 
>>>>> not
>>>>> processing the recursively here.
>>>>
>>>> It's iterative recursion :)
>>>>
>>>> I still think of this loop in terms of its simple tail recursion.
>>>>
>>>> And if we need to individual levels for unwind, that is a manual stack.
>>> Okay..
>>>
>>> One thought I had - if unwind is too unwieldy, could we mandate each
>>> user extension user has an additional output field in the embedding
>>> struct which would hold the last processed extension id?
>>
>> id? I guess we could do depth, though I guess pointer is more practical.
>> I don't think you mean ext.name as that we expect to repeat a few times.
> 
> Pointer yes, I started with a pointer but then for some reason wrote id, 
> where I meant name.
> 
>>> This way the state of the object would be less undefined after failure.
>>> Userspace would be able to tell how far in the extension chain i915
>>> managed to get to.
>>
>> But I'm not convinced how practical that is. The answer is pretty much
>> always -EINVAL (anything else is much less ambiguous) and even then you
>> have no idea which field tripped EINVAL or why?
> 
> Which field tripped -EINVAL is why is always true.
> 
> I am not 100% convinced myself, just worry about usability. But I don't 
> have an usecase at the moment which would be practically helped with 
> knowing which extension failed. In all cases I can think of it's a 
> programming error ie. should be caught during development and not happen 
> in production.

To add - if we are adding a new extensible uAPI my gut feeling is we 
should provision for reporting which stage failed. I think it is easy 
now, while later it would be much harder.

Regards,

Tvrtko