[3/3] drm/i915/icl: Disable master intr before reading

Commit Message

Mika Kuoppala Oct. 15, 2018, 2:14 p.m. UTC

Disable master interrupt before reading level indications.
This will close a race where we get a level indication between
reading and disabling, generating an extra interrupt where we
could have avoided one.

Further, as the reading acts also as a post, replace the
write/post on the irq reset with the helper. On enabling side,
posting doesn't serve any purpose so it can also be replaced
with helper.

Cc: Chris Wilson <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Daniele Ceraolo Spurio <[e-mail]>
Signed-off-by: Mika Kuoppala <[e-mail]>
Acked-by: Chris Wilson <[e-mail]>
---
 drivers/gpu/drm/i915/i915_irq.c | 37 ++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 12 deletions(-)

Comments

Daniele Ceraolo Spurio Oct. 15, 2018, 5:41 p.m. UTC | #1

On 15/10/18 07:14, Mika Kuoppala wrote:
> Disable master interrupt before reading level indications.
> This will close a race where we get a level indication between
> reading and disabling, generating an extra interrupt where we
> could have avoided one.
> 
> Further, as the reading acts also as a post, replace the
> write/post on the irq reset with the helper. On enabling side,
> posting doesn't serve any purpose so it can also be replaced
> with helper.
> 
> Cc: Chris Wilson <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Daniele Ceraolo Spurio <[e-mail]>
> Signed-off-by: Mika Kuoppala <[e-mail]>
> Acked-by: Chris Wilson <[e-mail]>
> ---

Reviewed-by: Daniele Ceraolo Spurio <[e-mail]>

Daniele

Mika Kuoppala Oct. 16, 2018, 10:39 a.m. UTC | #2

Daniele Ceraolo Spurio <[e-mail]> writes:

> On 15/10/18 07:14, Mika Kuoppala wrote:
>> Disable master interrupt before reading level indications.
>> This will close a race where we get a level indication between
>> reading and disabling, generating an extra interrupt where we
>> could have avoided one.
>> 
>> Further, as the reading acts also as a post, replace the
>> write/post on the irq reset with the helper. On enabling side,
>> posting doesn't serve any purpose so it can also be replaced
>> with helper.
>> 
>> Cc: Chris Wilson <[e-mail]>
>> Cc: Ville Syrjälä <[e-mail]>
>> Cc: Daniele Ceraolo Spurio <[e-mail]>
>> Signed-off-by: Mika Kuoppala <[e-mail]>
>> Acked-by: Chris Wilson <[e-mail]>
>> ---
>
> Reviewed-by: Daniele Ceraolo Spurio <[e-mail]>

All 3 patches pushed. Thank you all for review.
-Mika

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e0310ebd9c8d..5d1f53723388 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3129,6 +3129,24 @@  gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv, const u32 iir)
 		intel_opregion_asle_intr(dev_priv);
 }
 
+static inline u32 gen11_master_intr_disable(void __iomem * const regs)
+{
+	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, 0);
+
+	/*
+	 * Now with master disabled, get a sample of level indications
+	 * for this interrupt. Indications will be cleared on related acks.
+	 * New indications can and will light up during processing,
+	 * and will generate new interrupt after enabling master.
+	 */
+	return raw_reg_read(regs, GEN11_GFX_MSTR_IRQ);
+}
+
+static inline void gen11_master_intr_enable(void __iomem * const regs)
+{
+	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
+}
+
 static irqreturn_t gen11_irq_handler(int irq, void *arg)
 {
 	struct drm_i915_private * const i915 = to_i915(arg);
@@ -3139,13 +3157,11 @@  static irqreturn_t gen11_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(i915))
 		return IRQ_NONE;
 
-	master_ctl = raw_reg_read(regs, GEN11_GFX_MSTR_IRQ);
-	master_ctl &= ~GEN11_MASTER_IRQ;
-	if (!master_ctl)
+	master_ctl = gen11_master_intr_disable(regs);
+	if (!master_ctl) {
+		gen11_master_intr_enable(regs);
 		return IRQ_NONE;
-
-	/* Disable interrupts. */
-	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, 0);
+	}
 
 	/* Find, clear, then process each source of interrupt. */
 	gen11_gt_irq_handler(i915, master_ctl);
@@ -3165,8 +3181,7 @@  static irqreturn_t gen11_irq_handler(int irq, void *arg)
 
 	gu_misc_iir = gen11_gu_misc_irq_ack(i915, master_ctl);
 
-	/* Enable interrupts. */
-	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
+	gen11_master_intr_enable(regs);
 
 	gen11_gu_misc_irq_handler(i915, gu_misc_iir);
 
@@ -3658,8 +3673,7 @@  static void gen11_irq_reset(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe;
 
-	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
-	POSTING_READ(GEN11_GFX_MSTR_IRQ);
+	gen11_master_intr_disable(dev_priv->regs);
 
 	gen11_gt_irq_reset(dev_priv);
 
@@ -4323,8 +4337,7 @@  static int gen11_irq_postinstall(struct drm_device *dev)
 
 	I915_WRITE(GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE);
 
-	I915_WRITE(GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
-	POSTING_READ(GEN11_GFX_MSTR_IRQ);
+	gen11_master_intr_enable(dev_priv->regs);
 
 	return 0;
 }