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

Reply via email to