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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
