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

[Chris Wilson]

On Thu, May 18, 2017 at 02:30:56PM +0100, Tvrtko Ursulin wrote:
> 
> On 17/05/2017 17:44, Chris Wilson wrote:
> >On Wed, May 17, 2017 at 04:40:57PM +0100, Tvrtko Ursulin wrote:
> >>+static struct intel_engine_cs *get_engine_class(struct drm_i915_private *i915,
> >>+						u8 class, u8 instance)
> >>+{
> >>+	if (class > MAX_ENGINE_CLASS || instance > MAX_ENGINE_INSTANCE)
> >>+		return NULL;
> >>+
> >>+	return i915->engine_class[class][instance];
> >>+}
> >
> >Be bold make this this an intel_engine_lookup(), I forsee some other
> >users appearing very shortly.
> 
> Still static or you want to export it straight away? Preference for
> where to put it? Here or intel_engine_cs.c?

Let's go with intel_engine_cs.c. We know we're going to move it
otherwise.

> >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>index ee144ec57935..a3b59043b991 100644
> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> >>@@ -92,6 +92,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >> #define VIDEO_ENHANCEMENT_CLASS	2
> >> #define COPY_ENGINE_CLASS	3
> >> #define OTHER_CLASS		4
> >>+#define MAX_ENGINE_CLASS	4
> >>+
> >>+#define MAX_ENGINE_INSTANCE	1
> >
> >We also need the names in the uapi.h as well. Do we want to mention
> >OTHER_CLASS before it is defined? I trust your crystal ball.
> 
> I have mentioned it just by the virtue of it existing in i915_reg.h
> Crystal ball is otherwise quiet.
> 
> >Amusingly I notice that you do claim that you have these names in the
> >changelog. :) We don't need DRM_I915 all the time. I915_ENGINE_CLASS is
> >going to be unique, at least for those users of i915_drm.h
> 
> They are all there courtesy of the previous patch.

Hey, definitely only one v3 patch in my inbox and no 2/2 to clue me in
:)

> I will drop the DRM_ prefix throughout.
> 
> >
> >> /* PCI config space */
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>index 7566cf48012f..c5ad51c43d23 100644
> >>--- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >>+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>@@ -225,7 +225,14 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
> >>
> >> 	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
> >>
> >>+	if (WARN_ON(info->class > MAX_ENGINE_CLASS ||
> >>+		    info->instance > MAX_ENGINE_INSTANCE ||
> >>+		    dev_priv->engine_class[info->class][info->instance]))
> >
> >Spread these out or add a message telling what was wrong.
> 
> Can do, but I considered these to be such a basic programming errors
> which should be caught during development. Only thing which
> prevented me from putting in under the GEM_BUG_ON is the fact
> someone could not have it enabled and that this function can already
> handle failures.

Spread them out and use GEM_WARN_ON?

> 
> >>+		return -EINVAL;
> >>+
> >>+	dev_priv->engine_class[info->class][info->instance] = engine;
> >> 	dev_priv->engine[id] = engine;
> >>+
> >> 	return 0;
> >> }
> >>
> >>diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> >>index 2ac6667e57ea..6a26bdf5e684 100644
> >>--- a/include/uapi/drm/i915_drm.h
> >>+++ b/include/uapi/drm/i915_drm.h
> >>@@ -906,7 +906,12 @@ struct drm_i915_gem_execbuffer2 {
> >>  */
> >> #define I915_EXEC_FENCE_OUT		(1<<17)
> >>
> >>-#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_OUT<<1))
> >>+#define I915_EXEC_CLASS_INSTANCE	(1<<18)
> >>+
> >>+#define I915_EXEC_INSTANCE_SHIFT	(19)
> >>+#define I915_EXEC_INSTANCE_MASK		(0xff << I915_EXEC_INSTANCE_SHIFT)
> >>+
> >>+#define __I915_EXEC_UNKNOWN_FLAGS (-((1 << 27) << 1))
> >>
> >> #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
> >> #define i915_execbuffer2_set_context_id(eb2, context) \
> >>@@ -914,6 +919,10 @@ struct drm_i915_gem_execbuffer2 {
> >> #define i915_execbuffer2_get_context_id(eb2) \
> >> 	((eb2).rsvd1 & I915_EXEC_CONTEXT_ID_MASK)
> >>
> >>+#define i915_execbuffer2_engine(class, instance) \
> >>+	(I915_EXEC_CLASS_INSTANCE | (class) | \
> >>+	((instance) << I915_EXEC_INSTANCE_SHIFT))
> >
> >(class |
> > (instance) << I915_EXEC_INSTANCE_SHIFT |
> > I915_EXEC_CLASS_INSTANCE)
> >
> >Just suggesting to spread it out over another line for better
> >readability.
> 
> Okay but I'd like for the flag to come first, followed by class and
> then instance. Okay with that?

Ok. I don't think we can stop it forming a triangle however we reorder
them.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre