| Message ID | 20190329161900.29774-1-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: fix i9xx irq enable/disable | expand |
On 3/29/19 9:19 AM, Daniele Ceraolo Spurio wrote: > Those functions are used on gen4 as well and gen4 does have a non-RCS > engine, so remove the BUG_ON and flip back the logic to what it was > before the ENGINE_READ/WRITE update > > Fixes: baba6e572b38 ("drm/i915: take a reference to uncore in the engine and use it") I'm not sure why this hasn't failed on our gen4 machines in CI. Are we now so proactive in polling the HWSP that we just never enable the irqs? Daniele > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 48ba4d61a4ae..586e75c9edf3 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -976,20 +976,16 @@ gen5_irq_disable(struct intel_engine_cs *engine) > static void > i9xx_irq_enable(struct intel_engine_cs *engine) > { > - GEM_BUG_ON(engine->id != RCS0); > - > engine->i915->irq_mask &= ~engine->irq_enable_mask; > - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); > - ENGINE_POSTING_READ(engine, RING_IMR); > + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); > + ENGINE_POSTING_READ_FW(engine, RING_IMR); > } > > static void > i9xx_irq_disable(struct intel_engine_cs *engine) > { > - GEM_BUG_ON(engine->id != RCS0); > - > engine->i915->irq_mask |= engine->irq_enable_mask; > - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); > + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); > } > > static void > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index e58d6f04177b..f34459ffaeb8 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -52,6 +52,7 @@ struct drm_printer; > #define ENGINE_READ(...) __ENGINE_READ_OP(read, __VA_ARGS__) > #define ENGINE_READ_FW(...) __ENGINE_READ_OP(read_fw, __VA_ARGS__) > #define ENGINE_POSTING_READ(...) __ENGINE_READ_OP(posting_read, __VA_ARGS__) > +#define ENGINE_POSTING_READ_FW(...) __ENGINE_READ_OP(posting_read_fw, __VA_ARGS__) > > #define ENGINE_READ64(engine__, lower_reg__, upper_reg__) \ > __ENGINE_REG_OP(read64_2x32, (engine__), \ >
Quoting Daniele Ceraolo Spurio (2019-03-29 16:19:00) > Those functions are used on gen4 as well and gen4 does have a non-RCS > engine, so remove the BUG_ON and flip back the logic to what it was > before the ENGINE_READ/WRITE update Oh, silly me, gm45. > Fixes: baba6e572b38 ("drm/i915: take a reference to uncore in the engine and use it") > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 48ba4d61a4ae..586e75c9edf3 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -976,20 +976,16 @@ gen5_irq_disable(struct intel_engine_cs *engine) > static void > i9xx_irq_enable(struct intel_engine_cs *engine) > { > - GEM_BUG_ON(engine->id != RCS0); > - > engine->i915->irq_mask &= ~engine->irq_enable_mask; > - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); > - ENGINE_POSTING_READ(engine, RING_IMR); > + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); > + ENGINE_POSTING_READ_FW(engine, RING_IMR); But now we are posting something else on vcs0. Make it an intel_uncore_posting_read_fw(engine->uncore, IMR) to match. -Chris
Quoting Daniele Ceraolo Spurio (2019-03-29 16:20:48) > > > On 3/29/19 9:19 AM, Daniele Ceraolo Spurio wrote: > > Those functions are used on gen4 as well and gen4 does have a non-RCS > > engine, so remove the BUG_ON and flip back the logic to what it was > > before the ENGINE_READ/WRITE update > > > > Fixes: baba6e572b38 ("drm/i915: take a reference to uncore in the engine and use it") > > I'm not sure why this hasn't failed on our gen4 machines in CI. Are we > now so proactive in polling the HWSP that we just never enable the irqs? It died on boot and so dropped out of CI. Our mechanism for catching broken boots let us down here. Lessons learnt. -Chris
On Fri, Mar 29, 2019 at 09:19:00AM -0700, Daniele Ceraolo Spurio wrote: > Those functions are used on gen4 as well and gen4 does have a non-RCS > engine, so remove the BUG_ON and flip back the logic to what it was > before the ENGINE_READ/WRITE update > > Fixes: baba6e572b38 ("drm/i915: take a reference to uncore in the engine and use it") > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 48ba4d61a4ae..586e75c9edf3 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -976,20 +976,16 @@ gen5_irq_disable(struct intel_engine_cs *engine) > static void > i9xx_irq_enable(struct intel_engine_cs *engine) > { > - GEM_BUG_ON(engine->id != RCS0); > - > engine->i915->irq_mask &= ~engine->irq_enable_mask; > - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); > - ENGINE_POSTING_READ(engine, RING_IMR); > + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); > + ENGINE_POSTING_READ_FW(engine, RING_IMR); RING_IMR seems wrong still. > } > > static void > i9xx_irq_disable(struct intel_engine_cs *engine) > { > - GEM_BUG_ON(engine->id != RCS0); > - > engine->i915->irq_mask |= engine->irq_enable_mask; > - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); > + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); > } > > static void > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index e58d6f04177b..f34459ffaeb8 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -52,6 +52,7 @@ struct drm_printer; > #define ENGINE_READ(...) __ENGINE_READ_OP(read, __VA_ARGS__) > #define ENGINE_READ_FW(...) __ENGINE_READ_OP(read_fw, __VA_ARGS__) > #define ENGINE_POSTING_READ(...) __ENGINE_READ_OP(posting_read, __VA_ARGS__) > +#define ENGINE_POSTING_READ_FW(...) __ENGINE_READ_OP(posting_read_fw, __VA_ARGS__) > > #define ENGINE_READ64(engine__, lower_reg__, upper_reg__) \ > __ENGINE_REG_OP(read64_2x32, (engine__), \ > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 3/29/19 9:32 AM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2019-03-29 16:19:00) >> Those functions are used on gen4 as well and gen4 does have a non-RCS >> engine, so remove the BUG_ON and flip back the logic to what it was >> before the ENGINE_READ/WRITE update > > Oh, silly me, gm45. > >> Fixes: baba6e572b38 ("drm/i915: take a reference to uncore in the engine and use it") >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++------- >> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + >> 2 files changed, 4 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index 48ba4d61a4ae..586e75c9edf3 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -976,20 +976,16 @@ gen5_irq_disable(struct intel_engine_cs *engine) >> static void >> i9xx_irq_enable(struct intel_engine_cs *engine) >> { >> - GEM_BUG_ON(engine->id != RCS0); >> - >> engine->i915->irq_mask &= ~engine->irq_enable_mask; >> - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); >> - ENGINE_POSTING_READ(engine, RING_IMR); >> + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); >> + ENGINE_POSTING_READ_FW(engine, RING_IMR); > > But now we are posting something else on vcs0. Make it an > intel_uncore_posting_read_fw(engine->uncore, IMR) to match. > -Chris > I thought so as well, but the pre-rework code did: POSTING_READ_FW(RING_IMR(engine->mmio_base)); So I matched that. Daniele
Quoting Daniele Ceraolo Spurio (2019-03-29 16:41:55) > > > On 3/29/19 9:32 AM, Chris Wilson wrote: > > Quoting Daniele Ceraolo Spurio (2019-03-29 16:19:00) > >> Those functions are used on gen4 as well and gen4 does have a non-RCS > >> engine, so remove the BUG_ON and flip back the logic to what it was > >> before the ENGINE_READ/WRITE update > > > > Oh, silly me, gm45. > > > >> Fixes: baba6e572b38 ("drm/i915: take a reference to uncore in the engine and use it") > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++------- > >> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > >> 2 files changed, 4 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > >> index 48ba4d61a4ae..586e75c9edf3 100644 > >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >> @@ -976,20 +976,16 @@ gen5_irq_disable(struct intel_engine_cs *engine) > >> static void > >> i9xx_irq_enable(struct intel_engine_cs *engine) > >> { > >> - GEM_BUG_ON(engine->id != RCS0); > >> - > >> engine->i915->irq_mask &= ~engine->irq_enable_mask; > >> - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); > >> - ENGINE_POSTING_READ(engine, RING_IMR); > >> + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); > >> + ENGINE_POSTING_READ_FW(engine, RING_IMR); > > > > But now we are posting something else on vcs0. Make it an > > intel_uncore_posting_read_fw(engine->uncore, IMR) to match. > > -Chris > > > > I thought so as well, but the pre-rework code did: > > POSTING_READ_FW(RING_IMR(engine->mmio_base)); > > So I matched that. Go for that second fix with one patch. What could possibly go wrong? -Chris
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 48ba4d61a4ae..586e75c9edf3 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -976,20 +976,16 @@ gen5_irq_disable(struct intel_engine_cs *engine) static void i9xx_irq_enable(struct intel_engine_cs *engine) { - GEM_BUG_ON(engine->id != RCS0); - engine->i915->irq_mask &= ~engine->irq_enable_mask; - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); - ENGINE_POSTING_READ(engine, RING_IMR); + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); + ENGINE_POSTING_READ_FW(engine, RING_IMR); } static void i9xx_irq_disable(struct intel_engine_cs *engine) { - GEM_BUG_ON(engine->id != RCS0); - engine->i915->irq_mask |= engine->irq_enable_mask; - ENGINE_WRITE(engine, RING_IMR, engine->i915->irq_mask); + intel_uncore_write(engine->uncore, IMR, engine->i915->irq_mask); } static void diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index e58d6f04177b..f34459ffaeb8 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -52,6 +52,7 @@ struct drm_printer; #define ENGINE_READ(...) __ENGINE_READ_OP(read, __VA_ARGS__) #define ENGINE_READ_FW(...) __ENGINE_READ_OP(read_fw, __VA_ARGS__) #define ENGINE_POSTING_READ(...) __ENGINE_READ_OP(posting_read, __VA_ARGS__) +#define ENGINE_POSTING_READ_FW(...) __ENGINE_READ_OP(posting_read_fw, __VA_ARGS__) #define ENGINE_READ64(engine__, lower_reg__, upper_reg__) \ __ENGINE_REG_OP(read64_2x32, (engine__), \
Those functions are used on gen4 as well and gen4 does have a non-RCS engine, so remove the BUG_ON and flip back the logic to what it was before the ENGINE_READ/WRITE update Fixes: baba6e572b38 ("drm/i915: take a reference to uncore in the engine and use it") Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++------- drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + 2 files changed, 4 insertions(+), 7 deletions(-)