Re: [PATCH 14/38] drm/i915: Introduce the i915_user_extension_method

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

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.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx