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

[Chris Wilson]

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];
> };
> 
> I915_CONTEXT_GET_ENGINE_STATS:
> value = &{
>         __u32 num_engines;
>         __u32 pad;
> 
>         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?
-Chris