On Fri, Apr 29, 2016 at 10:04:35AM +0100, Tvrtko Ursulin wrote:
>
> On 28/04/16 18:35, Chris Wilson wrote:
> >Move all of the constant assignments up front and into a common
> >function. This is primarily to ensure the backpointers are set as early
> >as possible for later use during initialisation.
> >
> >v2: Use a constant struct so that all the similar values are set
> >together.
> >
> >Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> >Cc: Dave Gordon <david.s.gordon@xxxxxxxxx>
> >---
> > drivers/gpu/drm/i915/intel_lrc.c | 176 +++++++++++++++++++++------------------
> > 1 file changed, 93 insertions(+), 83 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 874c2515f9d4..2e0eaa9fa240 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> > }
> >
> > static void
> >-logical_ring_default_vfuncs(struct drm_device *dev,
> >- struct intel_engine_cs *engine)
> >+logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> > {
> > /* Default vfuncs which can be overriden by each engine. */
> > engine->init_hw = gen8_init_common_ring;
> >@@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
> > engine->emit_bb_start = gen8_emit_bb_start;
> > engine->get_seqno = gen8_get_seqno;
> > engine->set_seqno = gen8_set_seqno;
> >- if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> >+ if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
> > engine->irq_seqno_barrier = bxt_a_seqno_barrier;
> > engine->set_seqno = bxt_a_set_seqno;
> > }
> >@@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
> > {
> > engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> > engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> >+ init_waitqueue_head(&engine->irq_queue);
> > }
> >
> > static int
> >@@ -1964,31 +1964,68 @@ lrc_setup_hws(struct intel_engine_cs *engine,
> > return 0;
> > }
> >
> >-static int
> >-logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> >+static const struct logical_ring_info {
> >+ const char *name;
> >+ unsigned exec_id;
> >+ unsigned guc_id;
> >+ u32 mmio_base;
> >+ unsigned irq_shift;
> >+} logical_rings[] = {
> >+ [RCS] = {
> >+ .name = "render ring",
> >+ .exec_id = I915_EXEC_RENDER,
> >+ .guc_id = GUC_RENDER_ENGINE,
> >+ .mmio_base = RENDER_RING_BASE,
> >+ .irq_shift = GEN8_RCS_IRQ_SHIFT,
> >+ },
> >+ [BCS] = {
> >+ .name = "blitter ring",
> >+ .exec_id = I915_EXEC_BLT,
> >+ .guc_id = GUC_BLITTER_ENGINE,
> >+ .mmio_base = BLT_RING_BASE,
> >+ .irq_shift = GEN8_BCS_IRQ_SHIFT,
> >+ },
> >+ [VCS] = {
> >+ .name = "bsd ring",
> >+ .exec_id = I915_EXEC_BSD,
> >+ .guc_id = GUC_VIDEO_ENGINE,
> >+ .mmio_base = GEN6_BSD_RING_BASE,
> >+ .irq_shift = GEN8_VCS1_IRQ_SHIFT,
> >+ },
> >+ [VCS2] = {
> >+ .name = "bsd2 ring",
> >+ .exec_id = I915_EXEC_BSD,
> >+ .guc_id = GUC_VIDEO_ENGINE2,
> >+ .mmio_base = GEN8_BSD2_RING_BASE,
> >+ .irq_shift = GEN8_VCS2_IRQ_SHIFT,
> >+ },
> >+ [VECS] = {
> >+ .name = "video enhancement ring",
> >+ .exec_id = I915_EXEC_VEBOX,
> >+ .guc_id = GUC_VIDEOENHANCE_ENGINE,
> >+ .mmio_base = VEBOX_RING_BASE,
> >+ .irq_shift = GEN8_VECS_IRQ_SHIFT,
> >+ },
> >+};
> >+
> >+static struct intel_engine_cs *
> >+logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> > {
>
> Would dev_priv be better? Just to gradually move towards the correct
> state of things.
I have a patch queued up to do engine->i915 (1 KiB in object code
reduction) next.
> >+ const struct logical_ring_info *info = &logical_rings[id];
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >- struct intel_context *dctx = dev_priv->kernel_context;
> >+ struct intel_engine_cs *engine = &dev_priv->engine[id];
> > enum forcewake_domains fw_domains;
> >- int ret;
> >-
> >- /* Intentionally left blank. */
> >- engine->buffer = NULL;
> >
> > engine->dev = dev;
>
> Looking at usages of intel_engine_initialized... one potential
> danger scenario would be interrupt noise during driver load end in
> notify ring and explodes. Sounds very unlikely but theoretically it
> is a regression compared to where engine->dev initialization was
> before.
>
> We should really move away from engine->dev for this and just add an
> explicit flag.
Hmm. not that but I think we really should be sanitizing the irq here
and enabling them last.
Like:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2e0eaa9fa240..2c94072ab085 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
struct intel_engine_cs *engine = &dev_priv->engine[id];
enum forcewake_domains fw_domains;
- engine->dev = dev;
-
engine->id = id;
engine->name = info->name;
engine->exec_id = info->exec_id;
engine->guc_id = info->guc_id;
engine->mmio_base = info->mmio_base;
+ /* disable interrupts to this engine before we install ourselves*/
+ I915_WRITE_IMR(engine, ~0);
+
+ engine->dev = dev;
+
/* Intentionally left blank. */
engine->buffer = NULL;
Make sense?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
