| Message ID | 1491490816-26965-5-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/04/2017 16:00, Oscar Mateo wrote: > There are some properties that logically belong to the engine class, and some > that belong to the engine instance. Make it explicit. > > v2: Commit message (Tvrtko) > > v3: > - Rebased > - Exec/uabi id should be per instance (Chris) > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 71 +++++++++++++++++++++------------- > 1 file changed, 44 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index c6a73d0..6eab22d 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -26,71 +26,84 @@ > #include "intel_ringbuffer.h" > #include "intel_lrc.h" > > -static const struct engine_info { > +struct engine_class_info { > const char *name; > + int (*init_legacy)(struct intel_engine_cs *engine); > + int (*init_execlists)(struct intel_engine_cs *engine); > +}; > + > +static const struct engine_class_info intel_engine_classes[] = { > + [RENDER_CLASS] = { > + .name = "rcs", > + .init_execlists = logical_render_ring_init, > + .init_legacy = intel_init_render_ring_buffer, > + }, > + [COPY_ENGINE_CLASS] = { > + .name = "bcs", > + .init_execlists = logical_xcs_ring_init, > + .init_legacy = intel_init_blt_ring_buffer, > + }, > + [VIDEO_DECODE_CLASS] = { > + .name = "vcs", > + .init_execlists = logical_xcs_ring_init, > + .init_legacy = intel_init_bsd_ring_buffer, > + }, > + [VIDEO_ENHANCEMENT_CLASS] = { > + .name = "vecs", > + .init_execlists = logical_xcs_ring_init, > + .init_legacy = intel_init_vebox_ring_buffer, > + }, > +}; > + > +struct engine_info { > unsigned int exec_id; > unsigned int hw_id; > enum intel_engine_class class; > u8 instance; > u32 mmio_base; > unsigned irq_shift; > - int (*init_legacy)(struct intel_engine_cs *engine); > - int (*init_execlists)(struct intel_engine_cs *engine); > -} intel_engines[] = { > +}; > + > +static const struct engine_info intel_engines[] = { > [RCS] = { > - .name = "rcs", > - .hw_id = RCS_HW, > .exec_id = I915_EXEC_RENDER, > + .hw_id = RCS_HW, One more bit of polish - avoid re-ordering the two fields here and below for a smaller diff. Or it is a deliberate choice to re-order? Regards, Tvrtko > .class = RENDER_CLASS, > .instance = 0, > .mmio_base = RENDER_RING_BASE, > .irq_shift = GEN8_RCS_IRQ_SHIFT, > - .init_execlists = logical_render_ring_init, > - .init_legacy = intel_init_render_ring_buffer, > }, > [BCS] = { > - .name = "bcs", > - .hw_id = BCS_HW, > .exec_id = I915_EXEC_BLT, > + .hw_id = BCS_HW, > .class = COPY_ENGINE_CLASS, > .instance = 0, > .mmio_base = BLT_RING_BASE, > .irq_shift = GEN8_BCS_IRQ_SHIFT, > - .init_execlists = logical_xcs_ring_init, > - .init_legacy = intel_init_blt_ring_buffer, > }, > [VCS] = { > - .name = "vcs", > - .hw_id = VCS_HW, > .exec_id = I915_EXEC_BSD, > + .hw_id = VCS_HW, > .class = VIDEO_DECODE_CLASS, > .instance = 0, > .mmio_base = GEN6_BSD_RING_BASE, > .irq_shift = GEN8_VCS1_IRQ_SHIFT, > - .init_execlists = logical_xcs_ring_init, > - .init_legacy = intel_init_bsd_ring_buffer, > }, > [VCS2] = { > - .name = "vcs", > - .hw_id = VCS2_HW, > .exec_id = I915_EXEC_BSD, > + .hw_id = VCS2_HW, > .class = VIDEO_DECODE_CLASS, > .instance = 1, > .mmio_base = GEN8_BSD2_RING_BASE, > .irq_shift = GEN8_VCS2_IRQ_SHIFT, > - .init_execlists = logical_xcs_ring_init, > - .init_legacy = intel_init_bsd_ring_buffer, > }, > [VECS] = { > - .name = "vecs", > - .hw_id = VECS_HW, > .exec_id = I915_EXEC_VEBOX, > + .hw_id = VECS_HW, > .class = VIDEO_ENHANCEMENT_CLASS, > .instance = 0, > .mmio_base = VEBOX_RING_BASE, > .irq_shift = GEN8_VECS_IRQ_SHIFT, > - .init_execlists = logical_xcs_ring_init, > - .init_legacy = intel_init_vebox_ring_buffer, > }, > }; > > @@ -99,6 +112,8 @@ > enum intel_engine_id id) > { > const struct engine_info *info = &intel_engines[id]; > + const struct engine_class_info *class_info = > + &intel_engine_classes[info->class]; > struct intel_engine_cs *engine; > > GEM_BUG_ON(dev_priv->engine[id]); > @@ -109,7 +124,7 @@ > engine->id = id; > engine->i915 = dev_priv; > snprintf(engine->name, sizeof(engine->name), "%s%u", > - info->name, info->instance); > + class_info->name, info->instance); > engine->exec_id = info->exec_id; > engine->hw_id = engine->guc_id = info->hw_id; > engine->mmio_base = info->mmio_base; > @@ -190,12 +205,14 @@ int intel_engines_init(struct drm_i915_private *dev_priv) > int err = 0; > > for_each_engine(engine, dev_priv, id) { > + const struct engine_class_info *class_info = > + &intel_engine_classes[engine->class]; > int (*init)(struct intel_engine_cs *engine); > > if (i915.enable_execlists) > - init = intel_engines[id].init_execlists; > + init = class_info->init_execlists; > else > - init = intel_engines[id].init_legacy; > + init = class_info->init_legacy; > if (!init) { > kfree(engine); > dev_priv->engine[id] = NULL; >
On Thu, Apr 06, 2017 at 08:00:15AM -0700, Oscar Mateo wrote: > There are some properties that logically belong to the engine class, and some > that belong to the engine instance. Make it explicit. > > v2: Commit message (Tvrtko) > > v3: > - Rebased > - Exec/uabi id should be per instance (Chris) > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 71 +++++++++++++++++++++------------- > 1 file changed, 44 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index c6a73d0..6eab22d 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -26,71 +26,84 @@ > #include "intel_ringbuffer.h" > #include "intel_lrc.h" > > -static const struct engine_info { > +struct engine_class_info { > const char *name; > + int (*init_legacy)(struct intel_engine_cs *engine); > + int (*init_execlists)(struct intel_engine_cs *engine); > +}; > + > +static const struct engine_class_info intel_engine_classes[] = { > + [RENDER_CLASS] = { > + .name = "rcs", > + .init_execlists = logical_render_ring_init, > + .init_legacy = intel_init_render_ring_buffer, > + }, > + [COPY_ENGINE_CLASS] = { > + .name = "bcs", > + .init_execlists = logical_xcs_ring_init, > + .init_legacy = intel_init_blt_ring_buffer, > + }, > + [VIDEO_DECODE_CLASS] = { > + .name = "vcs", > + .init_execlists = logical_xcs_ring_init, > + .init_legacy = intel_init_bsd_ring_buffer, > + }, > + [VIDEO_ENHANCEMENT_CLASS] = { > + .name = "vecs", > + .init_execlists = logical_xcs_ring_init, > + .init_legacy = intel_init_vebox_ring_buffer, > + }, > +}; > + > +struct engine_info { > unsigned int exec_id; > unsigned int hw_id; > enum intel_engine_class class; > u8 instance; > u32 mmio_base; > unsigned irq_shift; > - int (*init_legacy)(struct intel_engine_cs *engine); > - int (*init_execlists)(struct intel_engine_cs *engine); > -} intel_engines[] = { > +}; > + > +static const struct engine_info intel_engines[] = { > [RCS] = { > - .name = "rcs", > - .hw_id = RCS_HW, > .exec_id = I915_EXEC_RENDER, > + .hw_id = RCS_HW, > .class = RENDER_CLASS, > .instance = 0, > .mmio_base = RENDER_RING_BASE, > .irq_shift = GEN8_RCS_IRQ_SHIFT, > - .init_execlists = logical_render_ring_init, > - .init_legacy = intel_init_render_ring_buffer, > }, > [BCS] = { > - .name = "bcs", > - .hw_id = BCS_HW, > .exec_id = I915_EXEC_BLT, > + .hw_id = BCS_HW, > .class = COPY_ENGINE_CLASS, > .instance = 0, > .mmio_base = BLT_RING_BASE, > .irq_shift = GEN8_BCS_IRQ_SHIFT, > - .init_execlists = logical_xcs_ring_init, > - .init_legacy = intel_init_blt_ring_buffer, > }, > [VCS] = { > - .name = "vcs", > - .hw_id = VCS_HW, > .exec_id = I915_EXEC_BSD, > + .hw_id = VCS_HW, > .class = VIDEO_DECODE_CLASS, > .instance = 0, > .mmio_base = GEN6_BSD_RING_BASE, > .irq_shift = GEN8_VCS1_IRQ_SHIFT, > - .init_execlists = logical_xcs_ring_init, > - .init_legacy = intel_init_bsd_ring_buffer, > }, > [VCS2] = { > - .name = "vcs", > - .hw_id = VCS2_HW, > .exec_id = I915_EXEC_BSD, > + .hw_id = VCS2_HW, > .class = VIDEO_DECODE_CLASS, > .instance = 1, > .mmio_base = GEN8_BSD2_RING_BASE, > .irq_shift = GEN8_VCS2_IRQ_SHIFT, > - .init_execlists = logical_xcs_ring_init, > - .init_legacy = intel_init_bsd_ring_buffer, > }, > [VECS] = { > - .name = "vecs", > - .hw_id = VECS_HW, > .exec_id = I915_EXEC_VEBOX, > + .hw_id = VECS_HW, > .class = VIDEO_ENHANCEMENT_CLASS, > .instance = 0, > .mmio_base = VEBOX_RING_BASE, > .irq_shift = GEN8_VECS_IRQ_SHIFT, > - .init_execlists = logical_xcs_ring_init, > - .init_legacy = intel_init_vebox_ring_buffer, > }, > }; > > @@ -99,6 +112,8 @@ > enum intel_engine_id id) > { > const struct engine_info *info = &intel_engines[id]; > + const struct engine_class_info *class_info = > + &intel_engine_classes[info->class]; Hmm, maybe we should add some protection against out-of-bound access to the class info array ? We can start with: GEM_BUG_ON(info->class > ARRAY_SIZE(intel_engine_classes)); -Michal > struct intel_engine_cs *engine; > > GEM_BUG_ON(dev_priv->engine[id]); > @@ -109,7 +124,7 @@ > engine->id = id; > engine->i915 = dev_priv; > snprintf(engine->name, sizeof(engine->name), "%s%u", > - info->name, info->instance); > + class_info->name, info->instance); > engine->exec_id = info->exec_id; > engine->hw_id = engine->guc_id = info->hw_id; > engine->mmio_base = info->mmio_base; > @@ -190,12 +205,14 @@ int intel_engines_init(struct drm_i915_private *dev_priv) > int err = 0; > > for_each_engine(engine, dev_priv, id) { > + const struct engine_class_info *class_info = > + &intel_engine_classes[engine->class]; > int (*init)(struct intel_engine_cs *engine); > > if (i915.enable_execlists) > - init = intel_engines[id].init_execlists; > + init = class_info->init_execlists; > else > - init = intel_engines[id].init_legacy; > + init = class_info->init_legacy; > if (!init) { > kfree(engine); > dev_priv->engine[id] = NULL; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index c6a73d0..6eab22d 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -26,71 +26,84 @@ #include "intel_ringbuffer.h" #include "intel_lrc.h" -static const struct engine_info { +struct engine_class_info { const char *name; + int (*init_legacy)(struct intel_engine_cs *engine); + int (*init_execlists)(struct intel_engine_cs *engine); +}; + +static const struct engine_class_info intel_engine_classes[] = { + [RENDER_CLASS] = { + .name = "rcs", + .init_execlists = logical_render_ring_init, + .init_legacy = intel_init_render_ring_buffer, + }, + [COPY_ENGINE_CLASS] = { + .name = "bcs", + .init_execlists = logical_xcs_ring_init, + .init_legacy = intel_init_blt_ring_buffer, + }, + [VIDEO_DECODE_CLASS] = { + .name = "vcs", + .init_execlists = logical_xcs_ring_init, + .init_legacy = intel_init_bsd_ring_buffer, + }, + [VIDEO_ENHANCEMENT_CLASS] = { + .name = "vecs", + .init_execlists = logical_xcs_ring_init, + .init_legacy = intel_init_vebox_ring_buffer, + }, +}; + +struct engine_info { unsigned int exec_id; unsigned int hw_id; enum intel_engine_class class; u8 instance; u32 mmio_base; unsigned irq_shift; - int (*init_legacy)(struct intel_engine_cs *engine); - int (*init_execlists)(struct intel_engine_cs *engine); -} intel_engines[] = { +}; + +static const struct engine_info intel_engines[] = { [RCS] = { - .name = "rcs", - .hw_id = RCS_HW, .exec_id = I915_EXEC_RENDER, + .hw_id = RCS_HW, .class = RENDER_CLASS, .instance = 0, .mmio_base = RENDER_RING_BASE, .irq_shift = GEN8_RCS_IRQ_SHIFT, - .init_execlists = logical_render_ring_init, - .init_legacy = intel_init_render_ring_buffer, }, [BCS] = { - .name = "bcs", - .hw_id = BCS_HW, .exec_id = I915_EXEC_BLT, + .hw_id = BCS_HW, .class = COPY_ENGINE_CLASS, .instance = 0, .mmio_base = BLT_RING_BASE, .irq_shift = GEN8_BCS_IRQ_SHIFT, - .init_execlists = logical_xcs_ring_init, - .init_legacy = intel_init_blt_ring_buffer, }, [VCS] = { - .name = "vcs", - .hw_id = VCS_HW, .exec_id = I915_EXEC_BSD, + .hw_id = VCS_HW, .class = VIDEO_DECODE_CLASS, .instance = 0, .mmio_base = GEN6_BSD_RING_BASE, .irq_shift = GEN8_VCS1_IRQ_SHIFT, - .init_execlists = logical_xcs_ring_init, - .init_legacy = intel_init_bsd_ring_buffer, }, [VCS2] = { - .name = "vcs", - .hw_id = VCS2_HW, .exec_id = I915_EXEC_BSD, + .hw_id = VCS2_HW, .class = VIDEO_DECODE_CLASS, .instance = 1, .mmio_base = GEN8_BSD2_RING_BASE, .irq_shift = GEN8_VCS2_IRQ_SHIFT, - .init_execlists = logical_xcs_ring_init, - .init_legacy = intel_init_bsd_ring_buffer, }, [VECS] = { - .name = "vecs", - .hw_id = VECS_HW, .exec_id = I915_EXEC_VEBOX, + .hw_id = VECS_HW, .class = VIDEO_ENHANCEMENT_CLASS, .instance = 0, .mmio_base = VEBOX_RING_BASE, .irq_shift = GEN8_VECS_IRQ_SHIFT, - .init_execlists = logical_xcs_ring_init, - .init_legacy = intel_init_vebox_ring_buffer, }, }; @@ -99,6 +112,8 @@ enum intel_engine_id id) { const struct engine_info *info = &intel_engines[id]; + const struct engine_class_info *class_info = + &intel_engine_classes[info->class]; struct intel_engine_cs *engine; GEM_BUG_ON(dev_priv->engine[id]); @@ -109,7 +124,7 @@ engine->id = id; engine->i915 = dev_priv; snprintf(engine->name, sizeof(engine->name), "%s%u", - info->name, info->instance); + class_info->name, info->instance); engine->exec_id = info->exec_id; engine->hw_id = engine->guc_id = info->hw_id; engine->mmio_base = info->mmio_base; @@ -190,12 +205,14 @@ int intel_engines_init(struct drm_i915_private *dev_priv) int err = 0; for_each_engine(engine, dev_priv, id) { + const struct engine_class_info *class_info = + &intel_engine_classes[engine->class]; int (*init)(struct intel_engine_cs *engine); if (i915.enable_execlists) - init = intel_engines[id].init_execlists; + init = class_info->init_execlists; else - init = intel_engines[id].init_legacy; + init = class_info->init_legacy; if (!init) { kfree(engine); dev_priv->engine[id] = NULL;
There are some properties that logically belong to the engine class, and some that belong to the engine instance. Make it explicit. v2: Commit message (Tvrtko) v3: - Rebased - Exec/uabi id should be per instance (Chris) Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 71 +++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 27 deletions(-)