diff mbox

drm/i915: Unbreak interrupts on pre-gen6

Message ID 1468340687-3596-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjala July 12, 2016, 4:24 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Prior to gen6 we didn't have per-ring IMR registers, which means that
since commit 61ff75ac20ff ("drm/i915: Simplify enabling
user-interrupts with L3-remapping") we're now masking off all interrupts
when init_render_ring() gets called. That's rather rude. Let's limit
the ring IMR frobbing to machines that actually have the per-ring IMR
registers.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 61ff75ac20ff ("drm/i915: Simplify enabling user-interrupts with L3-remapping")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Chris Wilson July 12, 2016, 4:47 p.m. UTC | #1 
On Tue, Jul 12, 2016 at 07:24:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Prior to gen6 we didn't have per-ring IMR registers, which means that
> since commit 61ff75ac20ff ("drm/i915: Simplify enabling
> user-interrupts with L3-remapping") we're now masking off all interrupts
> when init_render_ring() gets called.

That confused me, we're just writing to a non-existent register, so it
shouldn't have any effect.

> That's rather rude. Let's limit
> the ring IMR frobbing to machines that actually have the per-ring IMR
> registers.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: 61ff75ac20ff ("drm/i915: Simplify enabling user-interrupts with L3-remapping")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewd-by: Chris Wilson <chris@chris-wilson.co.uk>

Did you see anything to cause concern? I've run this patch on gen2-9, so
I wonder what I missed and how.
-Chris
Ville Syrjala July 12, 2016, 7:13 p.m. UTC | #2 
On Tue, Jul 12, 2016 at 05:47:02PM +0100, Chris Wilson wrote:
> On Tue, Jul 12, 2016 at 07:24:47PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Prior to gen6 we didn't have per-ring IMR registers, which means that
> > since commit 61ff75ac20ff ("drm/i915: Simplify enabling
> > user-interrupts with L3-remapping") we're now masking off all interrupts
> > when init_render_ring() gets called.
> 
> That confused me, we're just writing to a non-existent register, so it
> shouldn't have any effect.

RING_IMR(RCS) == 0x20a8 == IMR

> 
> > That's rather rude. Let's limit
> > the ring IMR frobbing to machines that actually have the per-ring IMR
> > registers.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Fixes: 61ff75ac20ff ("drm/i915: Simplify enabling user-interrupts with L3-remapping")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewd-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Did you see anything to cause concern? I've run this patch on gen2-9, so
> I wonder what I missed and how.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson July 12, 2016, 7:43 p.m. UTC | #3 
On Tue, Jul 12, 2016 at 10:13:48PM +0300, Ville Syrjälä wrote:
> On Tue, Jul 12, 2016 at 05:47:02PM +0100, Chris Wilson wrote:
> > On Tue, Jul 12, 2016 at 07:24:47PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Prior to gen6 we didn't have per-ring IMR registers, which means that
> > > since commit 61ff75ac20ff ("drm/i915: Simplify enabling
> > > user-interrupts with L3-remapping") we're now masking off all interrupts
> > > when init_render_ring() gets called.
> > 
> > That confused me, we're just writing to a non-existent register, so it
> > shouldn't have any effect.
> 
> RING_IMR(RCS) == 0x20a8 == IMR

Ah (I expected the global IIR et al not to be in the ring block). And
since we unmask everything, nothing is broken at first glance.
-Chris
Ville Syrjala July 13, 2016, 2:06 p.m. UTC | #4 
On Tue, Jul 12, 2016 at 05:47:02PM +0100, Chris Wilson wrote:
> On Tue, Jul 12, 2016 at 07:24:47PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Prior to gen6 we didn't have per-ring IMR registers, which means that
> > since commit 61ff75ac20ff ("drm/i915: Simplify enabling
> > user-interrupts with L3-remapping") we're now masking off all interrupts
> > when init_render_ring() gets called.
> 
> That confused me, we're just writing to a non-existent register, so it
> shouldn't have any effect.
> 
> > That's rather rude. Let's limit
> > the ring IMR frobbing to machines that actually have the per-ring IMR
> > registers.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Fixes: 61ff75ac20ff ("drm/i915: Simplify enabling user-interrupts with L3-remapping")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewd-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed to dinq. Thanks for the review.

> 
> Did you see anything to cause concern? I've run this patch on gen2-9, so
> I wonder what I missed and how.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 61e00bf9e87f..c8e77c082b21 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1305,7 +1305,8 @@  static int init_render_ring(struct intel_engine_cs *engine)
 	if (IS_GEN(dev_priv, 6, 7))
 		I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
 
-	I915_WRITE_IMR(engine, ~engine->irq_keep_mask);
+	if (INTEL_INFO(dev_priv)->gen >= 6)
+		I915_WRITE_IMR(engine, ~engine->irq_keep_mask);
 
 	return init_workarounds_ring(engine);
 }