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

[Tvrtko Ursulin]

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];
};

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

	struct {
		__u8 class;
		__u8 instance;
		__u8 pad[6];

		__u64 busy;
	} [num_engines];
};

Regards,

Tvrtko