| Message ID | b680ab2a-46c5-9eb1-15c1-75df3a26bac3@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -Chris
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
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
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
--- 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; + }; + }; }; Regards,