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