Re: [PATCH 2/6] drm/i915: Allow clients to query own per-engine busyness

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

On 22/01/2018 12:32, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-01-22 11:45:04)
>>
>> On 22/01/2018 10:00, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-01-22 09:53:27)
>>>>
>>>> On 19/01/2018 21:08, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2018-01-19 13:45:24)
>>>>>> +       case I915_CONTEXT_GET_ENGINE_BUSY:
>>>>>> +               engine = intel_engine_lookup_user(i915, args->class,
>>>>>> +                                                 args->instance);
>>>>>> +               if (!engine) {
>>>>>> +                       ret = -EINVAL;
>>>>>> +                       break;
>>>>>> +               }
>>>>>> +
>>>>>> +               ce = &ctx->engine[engine->id];
>>>>>> +               if (!READ_ONCE(ce->stats.enabled)) {
>>>>>> +                       ret = i915_mutex_lock_interruptible(dev);
>>>>>> +                       if (!ret)
>>>>>> +                               break;
>>>>>> +
>>>>>> +                       if (!ce->stats.enabled) {
>>>>>> +                               ret = intel_enable_engine_stats(engine);
>>>>>
>>>>> * Blink.
>>>>>
>>>>> This caught me by surprise. (Other than struct_mutex) Not too offensive,
>>>>> but surprising. At the very least call out to a function to handle the
>>>>> request. Where did args->class, args->instance come from? You surely
>>>>> didn't extend the ioctl struct just for that?
>>>>
>>>> Haven't extended it no, just did this:
>>>>
>>>> --- a/include/uapi/drm/i915_drm.h
>>>> +++ b/include/uapi/drm/i915_drm.h
>>>> @@ -1468,7 +1468,16 @@ struct drm_i915_gem_context_param {
>>>>     #define   I915_CONTEXT_MAX_USER_PRIORITY      1023 /* inclusive */
>>>>     #define   I915_CONTEXT_DEFAULT_PRIORITY               0
>>>>     #define   I915_CONTEXT_MIN_USER_PRIORITY      -1023 /* inclusive */
>>>> -       __u64 value;
>>>> +#define I915_CONTEXT_GET_ENGINE_BUSY   0x7
>>>> +       union {
>>>> +               __u64 value;
>>>> +               struct {
>>>> +                       __u8 pad[6]; /* unused */
>>>> +
>>>> +                       __u8 class;
>>>> +                       __u8 instance;
>>>> +               };
>>>> +       };
>>>>     };
>>>
>>> Not entirely happy about mixing in/out parameters. It's already
>>> complicated by being either an out value or an out pointer.
>>>
>>> Closer to the original idea for context_getparam would be to return the
>>> array of engine values.
>>
>> It would have the advantage that multiple engines could be queried
>> (more) atomically. How about then:
>>
>> I915_CONTEXT_ENABLE_ENGINE_STATS:
>> value = &{
>>          __u32 num_engines;
>>          __u32 pad;
>>          
>>          struct {
>>                  __u8 class;
>>                  __u8 instance;
>>                  __u8 pad[6];
>>          } [num_engines];
>> };

We could also get away without explicit stats enable step if so is 
desired (like the posted RFC). Enable on first query, disable on context 
destroy.

>>
>> I915_CONTEXT_GET_ENGINE_STATS:
>> value = &{
>>          __u32 num_engines;
>>          __u32 pad;

Will need length here so we don't overwrite users memory.

>>
>>          struct {
>>                  __u8 class;
>>                  __u8 instance;
>>                  __u8 pad[6];
>>
>>                  __u64 busy;
>>          } [num_engines];
>> };
> 
> Yes, that's what I had in mind. How does that work in practice? Does it
> make userspace too clumsy?

I wouldn't expect it to be a problem. Gordon?

Regards,

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