[Intel-gfx] [RFC v2 2/2] drm/i915: Select engines via class and instance in execbuffer2

[Chris Wilson]

On Thu, Apr 27, 2017 at 11:09:35AM +0100, Tvrtko Ursulin wrote:
> 
> On 27/04/2017 10:25, Chris Wilson wrote:
> >On Thu, Apr 27, 2017 at 10:10:34AM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >>Building on top of the previous patch which exported the concept
> >>of engine classes and instances, we can also use this instead of
> >>the current awkward engine selection uAPI.
> >>
> >>This is primarily interesting for the VCS engine selection which
> >>is a) currently done via disjoint set of flags, and b) the
> >>current I915_EXEC_BSD flags has different semantics depending on
> >>the underlying hardware which is bad.
> >>
> >>Proposed idea here is to reserve 16-bits of flags, to pass in
> >>the engine class and instance (8 bits each), and a new flag
> >>named I915_EXEC_CLASS_INSTACE to tell the kernel this new engine
> >>selection API is in use.
> >>
> >>The new uAPI also removes access to the weak VCS engine
> >>balancing as currently existing in the driver.
> >>
> >>Example usage to send a command to VCS0:
> >>
> >>  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 0);
> >>
> >>Or to send a command to VCS1:
> >>
> >>  eb.flags = i915_execbuffer2_engine(DRM_I915_ENGINE_CLASS_VIDEO_DECODE, 1);
> >>
> >>v2:
> >> * Fix unknown flags mask.
> >> * Use I915_EXEC_RING_MASK for class. (Chris Wilson)
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>Cc: Ben Widawsky <ben at bwidawsk.net>
> >>Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>Cc: Daniel Vetter <daniel.vetter at intel.com>
> >>Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>Cc: Jon Bloomfield <jon.bloomfield at intel.com>
> >>Cc: Daniel Charles <daniel.charles at intel.com>
> >>Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin at intel.com>
> >>Cc: Oscar Mateo <oscar.mateo at intel.com>
> >>Cc: "Gong, Zhipeng" <zhipeng.gong at intel.com>
> >>Cc: intel-vaapi-media at lists.01.org
> >>Cc: mesa-dev at lists.freedesktop.org
> >>---
> >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 +++++++++++++++++++++++++++++
> >> include/uapi/drm/i915_drm.h                | 11 ++++++++++-
> >> 2 files changed, 39 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>index af1965774e7b..ecd1486642a7 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >>@@ -1492,6 +1492,32 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
> >> 	return file_priv->bsd_engine;
> >> }
> >>
> >>+extern u8 user_class_map[DRM_I915_ENGINE_CLASS_MAX];
> >>+
> >>+static struct intel_engine_cs *
> >>+eb_select_engine_class_instance(struct drm_i915_private *i915,
> >>+				struct drm_i915_gem_execbuffer2 *args)
> >>+{
> >>+	struct intel_engine_cs *engine;
> >>+	enum intel_engine_id id;
> >>+	u8 class, instance;
> >>+
> >>+	class = args->flags & I915_EXEC_RING_MASK;
> >>+	if (class >= DRM_I915_ENGINE_CLASS_MAX)
> >>+		return NULL;
> >>+	class = user_class_map[class];
> >>+
> >>+	instance = (args->flags >> I915_EXEC_INSTANCE_SHIFT) &&
> >>+		   I915_EXEC_INSTANCE_MASK;
> >>+
> >>+	for_each_engine(engine, i915, id) {
> >>+		if (engine->class == class && engine->instance == instance)
> >>+			return engine;
> >>+	}
> >
> >I am underwhelmed. No, i915->class_engine[class][instance] ?
> 
> Hey it's just an RFC for the uAPI proposal! Implementation
> efficiency only comes later! :)
> 
> >Still, at what point do we kill busy-ioctl per-engine reporting? Should
> 
> It's the one we already broke before without no one noticing, where
> it userspace only effectively cares about a boolean value?

Userspace does try to distinguish between RCS and !RCS. But it's a rough
heuristic that I'm not going to cry much over since it depends upon on
so many other factors outside of its control as to which placement is
better.
 
> If so you recommend we make it a real boolean?

Once we cross the u16 threshold, yup. Just then a busy read/write pair.
 
> >we update all tracepoints to use class:instance (I think that's a better
> >abi going forward).
> 
> I can't think of any big problems doing so. Could rename ring= to
> engine= there as well. engine=<class>.<instance> for example?

Works for me.

There are still a few other places where we want an index into an array,
so keeping a map to engine->uabi_id seems sensible. Or we always include
engine.instance as part of our uABI for extensible structs. E.g.

struct context_watchdog_param {
	u32 engine;
	u32 instance;
	u64 watchdog_ns;
};

Then set/get_context_param return an array of those rather than an array
of watchdog_ns.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre