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