All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/10] ICL interrupt handling improvements
@ 2018-09-20 14:33 Mika Kuoppala
  2018-09-20 14:33 ` [PATCH 01/10] drm/i915/icl: No need to ack intr through master control Mika Kuoppala
                   ` (11 more replies)
  0 siblings, 12 replies; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx

Hi,

This series brings gen11 interrupt handling closer to what
we have with previous gens. Namely the early releasing of
master intr to reduce interrupt latency.

Also cleanups in guc interrupts and identity selector
handling are included.

Code size shrinks a little:
add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-156 (-156)
Function                                     old     new   delta
gen11_irq_postinstall                        651     623     -28
gen11_irq_reset                             1711    1681     -30
gen11_irq_handler                            880     836     -44
gen11_gt_engine_identity                     224     170     -54
Total: Before=1281048, After=1280892, chg -0.01%

Interrupt handling latency is improved generally as measured with
gem_exec_nop, the odd one in the bunch being basic-sequential which
suffers almost 11%. The oddity is that the sequential, which is same
test with longer timeout, showing 2% improvement.

Testname         diff-%
------------------------
basic-series     -26.051
basic-parallel   -24.589
basic-sequent-a  10.729
default          -24.738
signal-default   -11.749
render           -22.221
signal-render    -13.250
bsd              -16.422
signal-bsd       -9.931
blt              -22.721
signal-blt       -5.241
vebox            -22.526
signal-all       -4.839
series           -2.249
parallel         -2.747
sequential       -2.137
forked-sequent-i -24.196
forked-sequent-a  2.789
chained-sequen-i -2.351
chained-sequen-a  3.262       
context-sequen-i -5.904
context-sequen-a  2.128
preempt-default  -3.972
preempt-render    0.503
preempt-bsd      -2.355
preempt-blt      -1.822
preempt-vebox    -2.757
poll-default      0.384
poll-render      -0.753
poll-bsd          0.552 
poll-blt         -0.532
poll-vebox       -0.867
poll-sequential   2.865

-Mika

Mika Kuoppala (10):
  drm/i915/icl: No need to ack intr through master control
  drm/i915/icl: Disable master intr early
  drm/i915/icl: No need to early bailout on interrupt
  drm/i915/icl: Add helper to enable/disable master irq
  drm/i915/icl: Trim down posting reads on master intr control
  drm/i915/icl: Streamline guc irq handling
  drm/i915/icl: Make own function for display irq handler
  drm/i915/icl: Handle GT interrupts after enabling master
  drm/i915/icl: Handle display interrupts after enabling master
  drm/i915/icl: Only ack irq identities we did handle

 drivers/gpu/drm/i915/i915_irq.c | 134 ++++++++++++++++----------------
 1 file changed, 69 insertions(+), 65 deletions(-)

-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 01/10] drm/i915/icl: No need to ack intr through master control
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 14:33 ` [PATCH 02/10] drm/i915/icl: Disable master intr early Mika Kuoppala
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan

All other master control register bits, except the enable,
are read only and they are level indications of the second
level interrupt status. Only touch enable bit and rectify
the comment.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 10f28a2ee2e6..3d8c53bcbedb 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3156,8 +3156,8 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 
 	gen11_gu_misc_irq_ack(i915, master_ctl, &gu_misc_iir);
 
-	/* Acknowledge and enable interrupts. */
-	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ | master_ctl);
+	/* Enable interrupts. */
+	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
 
 	gen11_gu_misc_irq_handler(i915, master_ctl, gu_misc_iir);
 
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 02/10] drm/i915/icl: Disable master intr early
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
  2018-09-20 14:33 ` [PATCH 01/10] drm/i915/icl: No need to ack intr through master control Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 14:37   ` Chris Wilson
  2018-09-20 14:33 ` [PATCH 03/10] drm/i915/icl: No need to early bailout on interrupt Mika Kuoppala
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan

Disable master interrupt first before reading. This
guarantees that the sample we act upon is a frozen sample
of level indications and no interrupt was missed between
reading and disabling, possibly then ending up triggering
another interrupt later.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3d8c53bcbedb..1e05ffe16816 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3130,14 +3130,15 @@ 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)
-		return IRQ_NONE;
-
 	/* Disable interrupts. */
 	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, 0);
 
+	master_ctl = raw_reg_read(regs, GEN11_GFX_MSTR_IRQ) & ~GEN11_MASTER_IRQ;
+	if (!master_ctl) {
+		raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
+		return IRQ_NONE;
+	}
+
 	/* Find, clear, then process each source of interrupt. */
 	gen11_gt_irq_handler(i915, master_ctl);
 
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 03/10] drm/i915/icl: No need to early bailout on interrupt
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
  2018-09-20 14:33 ` [PATCH 01/10] drm/i915/icl: No need to ack intr through master control Mika Kuoppala
  2018-09-20 14:33 ` [PATCH 02/10] drm/i915/icl: Disable master intr early Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 14:55   ` Chris Wilson
  2018-09-20 16:11   ` Daniele Ceraolo Spurio
  2018-09-20 14:33 ` [PATCH 04/10] drm/i915/icl: Add helper to enable/disable master irq Mika Kuoppala
                   ` (8 subsequent siblings)
  11 siblings, 2 replies; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx

Getting interrupt without any second level indications
is unlikely. So there is no real advantage to bailout early
as all the second level handlers can handle empty master
control status.

Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1e05ffe16816..27395a90bbef 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3134,10 +3134,6 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, 0);
 
 	master_ctl = raw_reg_read(regs, GEN11_GFX_MSTR_IRQ) & ~GEN11_MASTER_IRQ;
-	if (!master_ctl) {
-		raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
-		return IRQ_NONE;
-	}
 
 	/* Find, clear, then process each source of interrupt. */
 	gen11_gt_irq_handler(i915, master_ctl);
@@ -3162,7 +3158,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 
 	gen11_gu_misc_irq_handler(i915, master_ctl, gu_misc_iir);
 
-	return IRQ_HANDLED;
+	return master_ctl ? IRQ_HANDLED : IRQ_NONE;
 }
 
 static void i915_reset_device(struct drm_i915_private *dev_priv,
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 04/10] drm/i915/icl: Add helper to enable/disable master irq
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (2 preceding siblings ...)
  2018-09-20 14:33 ` [PATCH 03/10] drm/i915/icl: No need to early bailout on interrupt Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 14:57   ` Chris Wilson
  2018-09-20 14:33 ` [PATCH 05/10] drm/i915/icl: Trim down posting reads on master intr control Mika Kuoppala
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx

Add static inline helpers to master irq control. Related
comments become superfluous and can be removed.

Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 27395a90bbef..89d76e7f4d00 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3120,6 +3120,16 @@ gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv,
 		DRM_ERROR("Unexpected GU_MISC interrupt 0x%x\n", iir);
 }
 
+static inline void gen11_master_irq_enable(void __iomem * const regs)
+{
+	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
+}
+
+static inline void gen11_master_irq_disable(void __iomem * const regs)
+{
+	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, 0);
+}
+
 static irqreturn_t gen11_irq_handler(int irq, void *arg)
 {
 	struct drm_i915_private * const i915 = to_i915(arg);
@@ -3130,9 +3140,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(i915))
 		return IRQ_NONE;
 
-	/* Disable interrupts. */
-	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, 0);
-
+	gen11_master_irq_disable(regs);
 	master_ctl = raw_reg_read(regs, GEN11_GFX_MSTR_IRQ) & ~GEN11_MASTER_IRQ;
 
 	/* Find, clear, then process each source of interrupt. */
@@ -3153,8 +3161,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 
 	gen11_gu_misc_irq_ack(i915, master_ctl, &gu_misc_iir);
 
-	/* Enable interrupts. */
-	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
+	gen11_master_irq_enable(regs);
 
 	gen11_gu_misc_irq_handler(i915, master_ctl, gu_misc_iir);
 
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 05/10] drm/i915/icl: Trim down posting reads on master intr control
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (3 preceding siblings ...)
  2018-09-20 14:33 ` [PATCH 04/10] drm/i915/icl: Add helper to enable/disable master irq Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 14:58   ` Chris Wilson
  2018-09-20 14:33 ` [PATCH 06/10] drm/i915/icl: Streamline guc irq handling Mika Kuoppala
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx

If we return a master control value on disable, it will act
as a posting read and further streamline our interrupt handler.
We can then safely use the inlined helpers on irq reset
and on postinstall. Posting read on postinstall is
superfluous as nothing beneath it is depedent.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 89d76e7f4d00..c35576f9c3f5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3125,9 +3125,11 @@ static inline void gen11_master_irq_enable(void __iomem * const regs)
 	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
 }
 
-static inline void gen11_master_irq_disable(void __iomem * const regs)
+static inline u32 gen11_master_irq_disable(void __iomem * const regs)
 {
 	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, 0);
+
+	return raw_reg_read(regs, GEN11_GFX_MSTR_IRQ) & ~GEN11_MASTER_IRQ;
 }
 
 static irqreturn_t gen11_irq_handler(int irq, void *arg)
@@ -3140,8 +3142,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 	if (!intel_irqs_enabled(i915))
 		return IRQ_NONE;
 
-	gen11_master_irq_disable(regs);
-	master_ctl = raw_reg_read(regs, GEN11_GFX_MSTR_IRQ) & ~GEN11_MASTER_IRQ;
+	master_ctl = gen11_master_irq_disable(regs);
 
 	/* Find, clear, then process each source of interrupt. */
 	gen11_gt_irq_handler(i915, master_ctl);
@@ -3652,10 +3653,10 @@ static void gen11_gt_irq_reset(struct drm_i915_private *dev_priv)
 static void gen11_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	void __iomem * const regs = dev_priv->regs;
 	int pipe;
 
-	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
-	POSTING_READ(GEN11_GFX_MSTR_IRQ);
+	gen11_master_irq_disable(regs);
 
 	gen11_gt_irq_reset(dev_priv);
 
@@ -4308,6 +4309,7 @@ static void icp_irq_postinstall(struct drm_device *dev)
 static int gen11_irq_postinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	void __iomem * const regs = dev_priv->regs;
 	u32 gu_misc_masked = GEN11_GU_MISC_GSE;
 
 	if (HAS_PCH_ICP(dev_priv))
@@ -4320,8 +4322,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_irq_enable(regs);
 
 	return 0;
 }
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 06/10] drm/i915/icl: Streamline guc irq handling
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (4 preceding siblings ...)
  2018-09-20 14:33 ` [PATCH 05/10] drm/i915/icl: Trim down posting reads on master intr control Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 15:00   ` Chris Wilson
  2018-09-20 17:31   ` Daniele Ceraolo Spurio
  2018-09-20 14:33 ` [PATCH 07/10] drm/i915/icl: Make own function for display irq handler Mika Kuoppala
                   ` (5 subsequent siblings)
  11 siblings, 2 replies; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan

The returning of iir through function parameter is eyesore.
Make guc irq acking inline and return the iir directly, handling
the empty iir exception early. We can then omit passing the
master control to guc handler as the iir now contains everything
we need.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 38 ++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c35576f9c3f5..e9034d6d87b0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3088,36 +3088,34 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915,
 	spin_unlock(&i915->irq_lock);
 }
 
-static void
-gen11_gu_misc_irq_ack(struct drm_i915_private *dev_priv, const u32 master_ctl,
-		      u32 *iir)
+static inline u32
+gen11_gu_misc_irq_ack(void __iomem * const regs, const u32 master_ctl)
 {
-	void __iomem * const regs = dev_priv->regs;
+	u32 iir;
 
 	if (!(master_ctl & GEN11_GU_MISC_IRQ))
-		return;
+		return 0;
+
+	iir = raw_reg_read(regs, GEN11_GU_MISC_IIR);
+	if (likely(iir))
+		raw_reg_write(regs, GEN11_GU_MISC_IIR, iir);
+	else
+		DRM_ERROR("GU_MISC iir blank!\n");
 
-	*iir = raw_reg_read(regs, GEN11_GU_MISC_IIR);
-	if (likely(*iir))
-		raw_reg_write(regs, GEN11_GU_MISC_IIR, *iir);
+	return iir;
 }
 
 static void
 gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv,
-			  const u32 master_ctl, const u32 iir)
+			  const u32 iir)
 {
-	if (!(master_ctl & GEN11_GU_MISC_IRQ))
-		return;
-
-	if (unlikely(!iir)) {
-		DRM_ERROR("GU_MISC iir blank!\n");
+	if (!iir)
 		return;
-	}
 
 	if (iir & GEN11_GU_MISC_GSE)
-		intel_opregion_asle_intr(dev_priv);
-	else
-		DRM_ERROR("Unexpected GU_MISC interrupt 0x%x\n", iir);
+		return intel_opregion_asle_intr(dev_priv);
+
+	DRM_ERROR("Unexpected GU_MISC interrupt 0x%x\n", iir);
 }
 
 static inline void gen11_master_irq_enable(void __iomem * const regs)
@@ -3160,11 +3158,11 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 		enable_rpm_wakeref_asserts(i915);
 	}
 
-	gen11_gu_misc_irq_ack(i915, master_ctl, &gu_misc_iir);
+	gu_misc_iir = gen11_gu_misc_irq_ack(regs, master_ctl);
 
 	gen11_master_irq_enable(regs);
 
-	gen11_gu_misc_irq_handler(i915, master_ctl, gu_misc_iir);
+	gen11_gu_misc_irq_handler(i915, gu_misc_iir);
 
 	return master_ctl ? IRQ_HANDLED : IRQ_NONE;
 }
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 07/10] drm/i915/icl: Make own function for display irq handler
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (5 preceding siblings ...)
  2018-09-20 14:33 ` [PATCH 06/10] drm/i915/icl: Streamline guc irq handling Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 15:02   ` Chris Wilson
  2018-09-20 14:33 ` [PATCH 08/10] drm/i915/icl: Handle GT interrupts after enabling master Mika Kuoppala
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx

Move display interrupt handling outside of generic handler.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 36 ++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e9034d6d87b0..506cfd048dd6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3118,6 +3118,27 @@ gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv,
 	DRM_ERROR("Unexpected GU_MISC interrupt 0x%x\n", iir);
 }
 
+static void
+gen11_display_irq_handler(struct drm_i915_private * const i915,
+			  const u32 master_ctl)
+{
+	u32 disp_ctl;
+
+	if (!(master_ctl & GEN11_DISPLAY_IRQ))
+		return;
+
+	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
+	disp_ctl = raw_reg_read(i915->regs, GEN11_DISPLAY_INT_CTL);
+
+	disable_rpm_wakeref_asserts(i915);
+	/*
+	 * GEN11_DISPLAY_INT_CTL has same format as GEN8_MASTER_IRQ
+	 * for the display related bits.
+	 */
+	gen8_de_irq_handler(i915, disp_ctl);
+	enable_rpm_wakeref_asserts(i915);
+}
+
 static inline void gen11_master_irq_enable(void __iomem * const regs)
 {
 	raw_reg_write(regs, GEN11_GFX_MSTR_IRQ, GEN11_MASTER_IRQ);
@@ -3144,20 +3165,7 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 
 	/* Find, clear, then process each source of interrupt. */
 	gen11_gt_irq_handler(i915, master_ctl);
-
-	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
-	if (master_ctl & GEN11_DISPLAY_IRQ) {
-		const u32 disp_ctl = raw_reg_read(regs, GEN11_DISPLAY_INT_CTL);
-
-		disable_rpm_wakeref_asserts(i915);
-		/*
-		 * GEN11_DISPLAY_INT_CTL has same format as GEN8_MASTER_IRQ
-		 * for the display related bits.
-		 */
-		gen8_de_irq_handler(i915, disp_ctl);
-		enable_rpm_wakeref_asserts(i915);
-	}
-
+	gen11_display_irq_handler(i915, master_ctl);
 	gu_misc_iir = gen11_gu_misc_irq_ack(regs, master_ctl);
 
 	gen11_master_irq_enable(regs);
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 08/10] drm/i915/icl: Handle GT interrupts after enabling master
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (6 preceding siblings ...)
  2018-09-20 14:33 ` [PATCH 07/10] drm/i915/icl: Make own function for display irq handler Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 14:33 ` [PATCH 09/10] drm/i915/icl: Handle display " Mika Kuoppala
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx

Don't keep master disabled while we handle the current
interrupts. This should help a little on latency of
generating the next interrupt.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 506cfd048dd6..b4992d397c5d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3163,13 +3163,12 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 
 	master_ctl = gen11_master_irq_disable(regs);
 
-	/* Find, clear, then process each source of interrupt. */
-	gen11_gt_irq_handler(i915, master_ctl);
 	gen11_display_irq_handler(i915, master_ctl);
 	gu_misc_iir = gen11_gu_misc_irq_ack(regs, master_ctl);
 
 	gen11_master_irq_enable(regs);
 
+	gen11_gt_irq_handler(i915, master_ctl);
 	gen11_gu_misc_irq_handler(i915, gu_misc_iir);
 
 	return master_ctl ? IRQ_HANDLED : IRQ_NONE;
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 09/10] drm/i915/icl: Handle display interrupts after enabling master
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (7 preceding siblings ...)
  2018-09-20 14:33 ` [PATCH 08/10] drm/i915/icl: Handle GT interrupts after enabling master Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 15:06   ` Chris Wilson
  2018-09-20 14:33 ` [PATCH 10/10] drm/i915/icl: Only ack irq identities we did handle Mika Kuoppala
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx

Don't keep master disabled while handling display interrupts.
This should help a little with latency of generating the
next interrupt.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b4992d397c5d..27116e3f21af 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3162,13 +3162,11 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
 		return IRQ_NONE;
 
 	master_ctl = gen11_master_irq_disable(regs);
-
-	gen11_display_irq_handler(i915, master_ctl);
 	gu_misc_iir = gen11_gu_misc_irq_ack(regs, master_ctl);
-
 	gen11_master_irq_enable(regs);
 
 	gen11_gt_irq_handler(i915, master_ctl);
+	gen11_display_irq_handler(i915, master_ctl);
 	gen11_gu_misc_irq_handler(i915, gu_misc_iir);
 
 	return master_ctl ? IRQ_HANDLED : IRQ_NONE;
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 10/10] drm/i915/icl: Only ack irq identities we did handle
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (8 preceding siblings ...)
  2018-09-20 14:33 ` [PATCH 09/10] drm/i915/icl: Handle display " Mika Kuoppala
@ 2018-09-20 14:33 ` Mika Kuoppala
  2018-09-20 17:14   ` Daniele Ceraolo Spurio
  2018-09-20 15:26 ` ✓ Fi.CI.BAT: success for ICL interrupt handling improvements Patchwork
  2018-09-20 20:52 ` ✓ Fi.CI.IGT: " Patchwork
  11 siblings, 1 reply; 23+ messages in thread
From: Mika Kuoppala @ 2018-09-20 14:33 UTC (permalink / raw)
  To: intel-gfx

If we ack the identities immediately after they have been
handled, it should unblock next interrupt accumulation
in the gathering register earlier. It also allows us to
remove time based polling of valid bit. If we don't get a valid
sample now, we will likely get a valid sample on next interrupt,
which will be generated due to skipping the ack. The downside
is that we will have as many ack writes as there were identities
handled.

Leave small retry loop for safety and with debugs to see if we
ever encounter read with valid not set.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 27116e3f21af..beb9fe4abf1f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2965,22 +2965,18 @@ gen11_gt_engine_identity(struct drm_i915_private * const i915,
 			 const unsigned int bank, const unsigned int bit)
 {
 	void __iomem * const regs = i915->regs;
-	u32 timeout_ts;
-	u32 ident;
+	u32 ident, retry = 0;
 
 	lockdep_assert_held(&i915->irq_lock);
 
 	raw_reg_write(regs, GEN11_IIR_REG_SELECTOR(bank), BIT(bit));
 
-	/*
-	 * NB: Specs do not specify how long to spin wait,
-	 * so we do ~100us as an educated guess.
-	 */
-	timeout_ts = (local_clock() >> 10) + 100;
 	do {
 		ident = raw_reg_read(regs, GEN11_INTR_IDENTITY_REG(bank));
-	} while (!(ident & GEN11_INTR_DATA_VALID) &&
-		 !time_after32(local_clock() >> 10, timeout_ts));
+	} while (!(ident & GEN11_INTR_DATA_VALID) && ++retry <= 10);
+
+	if (unlikely(GEM_SHOW_DEBUG() && retry))
+		WARN_ONCE(1, "INTR_IDENTITY took %u reads to settle\n", retry);
 
 	if (unlikely(!(ident & GEN11_INTR_DATA_VALID))) {
 		DRM_ERROR("INTR_IDENTITY_REG%u:%u 0x%08x not valid!\n",
@@ -3031,9 +3027,6 @@ gen11_gt_identity_handler(struct drm_i915_private * const i915,
 	const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity);
 	const u16 intr = GEN11_INTR_ENGINE_INTR(identity);
 
-	if (unlikely(!intr))
-		return;
-
 	if (class <= COPY_ENGINE_CLASS)
 		return gen11_engine_irq_handler(i915, class, instance, intr);
 
@@ -3065,11 +3058,14 @@ gen11_gt_bank_handler(struct drm_i915_private * const i915,
 		const u32 ident = gen11_gt_engine_identity(i915,
 							   bank, bit);
 
+		if (unlikely(!ident))
+			continue;
+
 		gen11_gt_identity_handler(i915, ident);
-	}
 
-	/* Clear must be after shared has been served for engine */
-	raw_reg_write(regs, GEN11_GT_INTR_DW(bank), intr_dw);
+		/* Clear must be after shared has been served for engine */
+		raw_reg_write(regs, GEN11_GT_INTR_DW(bank), BIT(bit));
+	}
 }
 
 static void
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 02/10] drm/i915/icl: Disable master intr early
  2018-09-20 14:33 ` [PATCH 02/10] drm/i915/icl: Disable master intr early Mika Kuoppala
@ 2018-09-20 14:37   ` Chris Wilson
  0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2018-09-20 14:37 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx; +Cc: Dhinakaran Pandiyan

Quoting Mika Kuoppala (2018-09-20 15:33:42)
> Disable master interrupt first before reading. This
> guarantees that the sample we act upon is a frozen sample
> of level indications and no interrupt was missed between
> reading and disabling, possibly then ending up triggering
> another interrupt later.

Your argument is convincing. Care to spin a series changing the other
and cc Ville, so that we have both coverage of machines and experience.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 03/10] drm/i915/icl: No need to early bailout on interrupt
  2018-09-20 14:33 ` [PATCH 03/10] drm/i915/icl: No need to early bailout on interrupt Mika Kuoppala
@ 2018-09-20 14:55   ` Chris Wilson
  2018-09-20 16:11   ` Daniele Ceraolo Spurio
  1 sibling, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2018-09-20 14:55 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2018-09-20 15:33:43)
> Getting interrupt without any second level indications
> is unlikely. So there is no real advantage to bailout early
> as all the second level handlers can handle empty master
> control status.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Looks reasonable,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 04/10] drm/i915/icl: Add helper to enable/disable master irq
  2018-09-20 14:33 ` [PATCH 04/10] drm/i915/icl: Add helper to enable/disable master irq Mika Kuoppala
@ 2018-09-20 14:57   ` Chris Wilson
  0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2018-09-20 14:57 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2018-09-20 15:33:44)
> Add static inline helpers to master irq control. Related
> comments become superfluous and can be removed.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Hmm, need a little more convincing as it breaks the pattern. Does it
make sense to apply this back to gen6 (excluding the atoms) where we
have the same master ctl?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 05/10] drm/i915/icl: Trim down posting reads on master intr control
  2018-09-20 14:33 ` [PATCH 05/10] drm/i915/icl: Trim down posting reads on master intr control Mika Kuoppala
@ 2018-09-20 14:58   ` Chris Wilson
  0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2018-09-20 14:58 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2018-09-20 15:33:45)
> If we return a master control value on disable, it will act
> as a posting read and further streamline our interrupt handler.
> We can then safely use the inlined helpers on irq reset
> and on postinstall. Posting read on postinstall is
> superfluous as nothing beneath it is depedent.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Ok, this adds more weight to patch 4. The question about extending the
pattern back as far as it goes still applies :)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 06/10] drm/i915/icl: Streamline guc irq handling
  2018-09-20 14:33 ` [PATCH 06/10] drm/i915/icl: Streamline guc irq handling Mika Kuoppala
@ 2018-09-20 15:00   ` Chris Wilson
  2018-09-20 17:31   ` Daniele Ceraolo Spurio
  1 sibling, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2018-09-20 15:00 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx; +Cc: Dhinakaran Pandiyan

Quoting Mika Kuoppala (2018-09-20 15:33:46)
> The returning of iir through function parameter is eyesore.
> Make guc irq acking inline and return the iir directly, handling
> the empty iir exception early. We can then omit passing the
> master control to guc handler as the iir now contains everything
> we need.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 38 ++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c35576f9c3f5..e9034d6d87b0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3088,36 +3088,34 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915,
>         spin_unlock(&i915->irq_lock);
>  }
>  
> -static void
> -gen11_gu_misc_irq_ack(struct drm_i915_private *dev_priv, const u32 master_ctl,
> -                     u32 *iir)
> +static inline u32
> +gen11_gu_misc_irq_ack(void __iomem * const regs, const u32 master_ctl)
>  {
> -       void __iomem * const regs = dev_priv->regs;
> +       u32 iir;
>  
>         if (!(master_ctl & GEN11_GU_MISC_IRQ))
> -               return;
> +               return 0;
> +
> +       iir = raw_reg_read(regs, GEN11_GU_MISC_IIR);
> +       if (likely(iir))
> +               raw_reg_write(regs, GEN11_GU_MISC_IIR, iir);
> +       else
> +               DRM_ERROR("GU_MISC iir blank!\n");
>  
> -       *iir = raw_reg_read(regs, GEN11_GU_MISC_IIR);
> -       if (likely(*iir))
> -               raw_reg_write(regs, GEN11_GU_MISC_IIR, *iir);
> +       return iir;
>  }
>  
>  static void
>  gen11_gu_misc_irq_handler(struct drm_i915_private *dev_priv,
> -                         const u32 master_ctl, const u32 iir)
> +                         const u32 iir)
>  {
> -       if (!(master_ctl & GEN11_GU_MISC_IRQ))
> -               return;

However, the argument is that by using master_ctl as our guard for all
functions, should encourage the compiler to keep it around in a
register. That's the thinking at least.

Care to test that theory if it makes any significant difference to code
layout and register usage?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 07/10] drm/i915/icl: Make own function for display irq handler
  2018-09-20 14:33 ` [PATCH 07/10] drm/i915/icl: Make own function for display irq handler Mika Kuoppala
@ 2018-09-20 15:02   ` Chris Wilson
  0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2018-09-20 15:02 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2018-09-20 15:33:47)
> Move display interrupt handling outside of generic handler.

Nah, this needs to be split into the ack/handle as per Ville's
suggestion.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 09/10] drm/i915/icl: Handle display interrupts after enabling master
  2018-09-20 14:33 ` [PATCH 09/10] drm/i915/icl: Handle display " Mika Kuoppala
@ 2018-09-20 15:06   ` Chris Wilson
  0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2018-09-20 15:06 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2018-09-20 15:33:49)
> Don't keep master disabled while handling display interrupts.
> This should help a little with latency of generating the
> next interrupt.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b4992d397c5d..27116e3f21af 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3162,13 +3162,11 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg)
>                 return IRQ_NONE;
>  
>         master_ctl = gen11_master_irq_disable(regs);
> -
> -       gen11_display_irq_handler(i915, master_ctl);
>         gu_misc_iir = gen11_gu_misc_irq_ack(regs, master_ctl);
> -
>         gen11_master_irq_enable(regs);
>  
>         gen11_gt_irq_handler(i915, master_ctl);
> +       gen11_display_irq_handler(i915, master_ctl);
>         gen11_gu_misc_irq_handler(i915, gu_misc_iir);

Hmm. So we no longer do ack within the interrupts off section. Is there
even a point to disabling master-ctl in that scenario. The danger is
simply we raise more master interrupts for sub-level interrupts that we
proceed to handle. Doesn't seem like a huge deal... But there's usually
some interesting rules on edge level interrupt that bite.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* ✓ Fi.CI.BAT: success for ICL interrupt handling improvements
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (9 preceding siblings ...)
  2018-09-20 14:33 ` [PATCH 10/10] drm/i915/icl: Only ack irq identities we did handle Mika Kuoppala
@ 2018-09-20 15:26 ` Patchwork
  2018-09-20 20:52 ` ✓ Fi.CI.IGT: " Patchwork
  11 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2018-09-20 15:26 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

== Series Details ==

Series: ICL interrupt handling improvements
URL   : https://patchwork.freedesktop.org/series/49971/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4851 -> Patchwork_10239 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49971/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10239 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-modeset:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-bdw-samus:       NOTRUN -> INCOMPLETE (fdo#107773)

    igt@kms_psr@primary_page_flip:
      fi-kbl-r:           PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS
      fi-bdw-samus:       INCOMPLETE (fdo#107773) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773


== Participating hosts (51 -> 47) ==

  Additional (2): fi-hsw-4770r fi-icl-u 
  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-skl-caroline 


== Build changes ==

    * Linux: CI_DRM_4851 -> Patchwork_10239

  CI_DRM_4851: f9b15cfe6b2059cec1465980d556d30be0fb7f75 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4647: ae8187922d8de2bc739519da3bd40cf5f03f5e4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10239: 55cf33be427cac92f05fac6793a6ef4d1a0b9f2d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

55cf33be427c drm/i915/icl: Only ack irq identities we did handle
8dd8e3ae6d78 drm/i915/icl: Handle display interrupts after enabling master
5de1efadeb46 drm/i915/icl: Handle GT interrupts after enabling master
78eeaa6937d6 drm/i915/icl: Make own function for display irq handler
d9e3a88f8697 drm/i915/icl: Streamline guc irq handling
fa484b8ca969 drm/i915/icl: Trim down posting reads on master intr control
42c13d4cda92 drm/i915/icl: Add helper to enable/disable master irq
832790b9e43f drm/i915/icl: No need to early bailout on interrupt
e0de72a3f1e9 drm/i915/icl: Disable master intr early
1cccecc51f8d drm/i915/icl: No need to ack intr through master control

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10239/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 03/10] drm/i915/icl: No need to early bailout on interrupt
  2018-09-20 14:33 ` [PATCH 03/10] drm/i915/icl: No need to early bailout on interrupt Mika Kuoppala
  2018-09-20 14:55   ` Chris Wilson
@ 2018-09-20 16:11   ` Daniele Ceraolo Spurio
  1 sibling, 0 replies; 23+ messages in thread
From: Daniele Ceraolo Spurio @ 2018-09-20 16:11 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx



On 20/09/18 07:33, Mika Kuoppala wrote:
> Getting interrupt without any second level indications
> is unlikely.

Do you have any numbers for this? I remember seeing empty interrupts 
quite often (i.e. up to 5-10%) in early testing on workloads submitting 
lots of no-ops due to the double buffering, when both events ended up 
being serviced by the first interrupt. The code was different at the 
time though, so those might just have been due to less optimal SW 
handing, but I'd be curious to see the numbers if you have any.

Thanks,
Daniele

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 10/10] drm/i915/icl: Only ack irq identities we did handle
  2018-09-20 14:33 ` [PATCH 10/10] drm/i915/icl: Only ack irq identities we did handle Mika Kuoppala
@ 2018-09-20 17:14   ` Daniele Ceraolo Spurio
  0 siblings, 0 replies; 23+ messages in thread
From: Daniele Ceraolo Spurio @ 2018-09-20 17:14 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx



On 20/09/18 07:33, Mika Kuoppala wrote:
> If we ack the identities immediately after they have been
> handled, it should unblock next interrupt accumulation
> in the gathering register earlier. It also allows us to
> remove time based polling of valid bit. If we don't get a valid
> sample now, we will likely get a valid sample on next interrupt,
> which will be generated due to skipping the ack. The downside
> is that we will have as many ack writes as there were identities
> handled.
> 

I'm not sure this is going to work. It looks like if we skip the 
clearing of the identity register we need to skip the clearing of 
INTR_DW as well because the HW doesn't seem to set that again. If we 
clear it, when the interrupt gets re-triggered the driver won't be able 
to handle it and so it'll keep being re-triggered in a loop. I've just 
confirmed this behavior on the current code (i.e. without this series 
applied) by simply pretending the ident was not valid for the first 
interrupt:

[  728.765749] [drm:gen11_gt_engine_identity [i915]] *ERROR* 
INTR_IDENTITY_REG0:15 0x00000000 not valid!
[  728.765800] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!
[  728.765865] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!
[  728.766050] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!
[... more of the same ...]
[  782.395257] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!
[  782.395293] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!

I haven't checked in detail all the other patches in the series yet so 
not sure if any of them can mitigate this issue.

Daniele

> Leave small retry loop for safety and with debugs to see if we
> ever encounter read with valid not set.
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++++---------------
>   1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 27116e3f21af..beb9fe4abf1f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2965,22 +2965,18 @@ gen11_gt_engine_identity(struct drm_i915_private * const i915,
>   			 const unsigned int bank, const unsigned int bit)
>   {
>   	void __iomem * const regs = i915->regs;
> -	u32 timeout_ts;
> -	u32 ident;
> +	u32 ident, retry = 0;
>   
>   	lockdep_assert_held(&i915->irq_lock);
>   
>   	raw_reg_write(regs, GEN11_IIR_REG_SELECTOR(bank), BIT(bit));
>   
> -	/*
> -	 * NB: Specs do not specify how long to spin wait,
> -	 * so we do ~100us as an educated guess.
> -	 */
> -	timeout_ts = (local_clock() >> 10) + 100;
>   	do {
>   		ident = raw_reg_read(regs, GEN11_INTR_IDENTITY_REG(bank));
> -	} while (!(ident & GEN11_INTR_DATA_VALID) &&
> -		 !time_after32(local_clock() >> 10, timeout_ts));
> +	} while (!(ident & GEN11_INTR_DATA_VALID) && ++retry <= 10);
> +
> +	if (unlikely(GEM_SHOW_DEBUG() && retry))
> +		WARN_ONCE(1, "INTR_IDENTITY took %u reads to settle\n", retry);
>   
>   	if (unlikely(!(ident & GEN11_INTR_DATA_VALID))) {
>   		DRM_ERROR("INTR_IDENTITY_REG%u:%u 0x%08x not valid!\n",
> @@ -3031,9 +3027,6 @@ gen11_gt_identity_handler(struct drm_i915_private * const i915,
>   	const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity);
>   	const u16 intr = GEN11_INTR_ENGINE_INTR(identity);
>   
> -	if (unlikely(!intr))
> -		return;
> -
>   	if (class <= COPY_ENGINE_CLASS)
>   		return gen11_engine_irq_handler(i915, class, instance, intr);
>   
> @@ -3065,11 +3058,14 @@ gen11_gt_bank_handler(struct drm_i915_private * const i915,
>   		const u32 ident = gen11_gt_engine_identity(i915,
>   							   bank, bit);
>   
> +		if (unlikely(!ident))
> +			continue;
> +
>   		gen11_gt_identity_handler(i915, ident);
> -	}
>   
> -	/* Clear must be after shared has been served for engine */
> -	raw_reg_write(regs, GEN11_GT_INTR_DW(bank), intr_dw);
> +		/* Clear must be after shared has been served for engine */
> +		raw_reg_write(regs, GEN11_GT_INTR_DW(bank), BIT(bit));
> +	}
>   }
>   
>   static void
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 06/10] drm/i915/icl: Streamline guc irq handling
  2018-09-20 14:33 ` [PATCH 06/10] drm/i915/icl: Streamline guc irq handling Mika Kuoppala
  2018-09-20 15:00   ` Chris Wilson
@ 2018-09-20 17:31   ` Daniele Ceraolo Spurio
  1 sibling, 0 replies; 23+ messages in thread
From: Daniele Ceraolo Spurio @ 2018-09-20 17:31 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx; +Cc: Dhinakaran Pandiyan



On 20/09/18 07:33, Mika Kuoppala wrote:
> The returning of iir through function parameter is eyesore.
> Make guc irq acking inline and return the iir directly, 

being pedantic, this is not the guc irq but gu_misc irq (in the commit 
title as well). Sounds similar but it's a different interrupt :P
GuC is under INTR_DW0, bit 25.

Daniele

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

* ✓ Fi.CI.IGT: success for ICL interrupt handling improvements
  2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
                   ` (10 preceding siblings ...)
  2018-09-20 15:26 ` ✓ Fi.CI.BAT: success for ICL interrupt handling improvements Patchwork
@ 2018-09-20 20:52 ` Patchwork
  11 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2018-09-20 20:52 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

== Series Details ==

Series: ICL interrupt handling improvements
URL   : https://patchwork.freedesktop.org/series/49971/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4851_full -> Patchwork_10239_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10239_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_big:
      shard-hsw:          PASS -> TIMEOUT (fdo#107937)

    igt@kms_busy@extended-modeset-hang-newfb-render-b:
      shard-snb:          NOTRUN -> DMESG-WARN (fdo#107956) +1

    igt@kms_busy@extended-modeset-hang-newfb-render-c:
      shard-kbl:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-apl:          PASS -> FAIL (fdo#103232, fdo#103191)

    igt@kms_flip_tiling@flip-to-yf-tiled:
      shard-apl:          PASS -> DMESG-WARN (fdo#105602, fdo#103558) +3

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-snb:          NOTRUN -> FAIL (fdo#103166)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@gem_eio@in-flight-1us:
      shard-glk:          FAIL (fdo#105957) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105957 https://bugs.freedesktop.org/show_bug.cgi?id=105957
  fdo#107937 https://bugs.freedesktop.org/show_bug.cgi?id=107937
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4851 -> Patchwork_10239

  CI_DRM_4851: f9b15cfe6b2059cec1465980d556d30be0fb7f75 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4647: ae8187922d8de2bc739519da3bd40cf5f03f5e4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10239: 55cf33be427cac92f05fac6793a6ef4d1a0b9f2d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10239/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2018-09-20 20:52 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-20 14:33 [PATCH 00/10] ICL interrupt handling improvements Mika Kuoppala
2018-09-20 14:33 ` [PATCH 01/10] drm/i915/icl: No need to ack intr through master control Mika Kuoppala
2018-09-20 14:33 ` [PATCH 02/10] drm/i915/icl: Disable master intr early Mika Kuoppala
2018-09-20 14:37   ` Chris Wilson
2018-09-20 14:33 ` [PATCH 03/10] drm/i915/icl: No need to early bailout on interrupt Mika Kuoppala
2018-09-20 14:55   ` Chris Wilson
2018-09-20 16:11   ` Daniele Ceraolo Spurio
2018-09-20 14:33 ` [PATCH 04/10] drm/i915/icl: Add helper to enable/disable master irq Mika Kuoppala
2018-09-20 14:57   ` Chris Wilson
2018-09-20 14:33 ` [PATCH 05/10] drm/i915/icl: Trim down posting reads on master intr control Mika Kuoppala
2018-09-20 14:58   ` Chris Wilson
2018-09-20 14:33 ` [PATCH 06/10] drm/i915/icl: Streamline guc irq handling Mika Kuoppala
2018-09-20 15:00   ` Chris Wilson
2018-09-20 17:31   ` Daniele Ceraolo Spurio
2018-09-20 14:33 ` [PATCH 07/10] drm/i915/icl: Make own function for display irq handler Mika Kuoppala
2018-09-20 15:02   ` Chris Wilson
2018-09-20 14:33 ` [PATCH 08/10] drm/i915/icl: Handle GT interrupts after enabling master Mika Kuoppala
2018-09-20 14:33 ` [PATCH 09/10] drm/i915/icl: Handle display " Mika Kuoppala
2018-09-20 15:06   ` Chris Wilson
2018-09-20 14:33 ` [PATCH 10/10] drm/i915/icl: Only ack irq identities we did handle Mika Kuoppala
2018-09-20 17:14   ` Daniele Ceraolo Spurio
2018-09-20 15:26 ` ✓ Fi.CI.BAT: success for ICL interrupt handling improvements Patchwork
2018-09-20 20:52 ` ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.