[PATCH 1/3] drm/i915: Trim gen8_irq_handler

[PATCH 1/3] drm/i915: Trim gen8_irq_handler

[Chris Wilson]

The goal here is to trim an excess posting read and keep the predicates
tight (reusing the same predicate throughout for GT ack/handling).

add/remove: 0/0 grow/shrink: 2/1 up/down: 26/-30 (-4)
function                                     old     new   delta
gen8_gt_irq_handler                          282     301     +19
cherryview_irq_handler                       450     457      +7
gen8_irq_handler                            1653    1623     -30

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_irq.c | 54 +++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 91a2c5dbf2da..e12321cb7403 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1375,31 +1375,34 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
 }
 
 static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
-				u32 gt_iir[4])
+				u32 master_ctl, u32 gt_iir[4])
 {
-	if (gt_iir[0]) {
+	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
 		gen8_cs_irq_handler(dev_priv->engine[RCS],
 				    gt_iir[0], GEN8_RCS_IRQ_SHIFT);
 		gen8_cs_irq_handler(dev_priv->engine[BCS],
 				    gt_iir[0], GEN8_BCS_IRQ_SHIFT);
 	}
 
-	if (gt_iir[1]) {
+	if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
 		gen8_cs_irq_handler(dev_priv->engine[VCS],
 				    gt_iir[1], GEN8_VCS1_IRQ_SHIFT);
 		gen8_cs_irq_handler(dev_priv->engine[VCS2],
 				    gt_iir[1], GEN8_VCS2_IRQ_SHIFT);
 	}
 
-	if (gt_iir[3])
+	if (master_ctl & GEN8_GT_VECS_IRQ) {
 		gen8_cs_irq_handler(dev_priv->engine[VECS],
 				    gt_iir[3], GEN8_VECS_IRQ_SHIFT);
+	}
 
-	if (gt_iir[2] & dev_priv->pm_rps_events)
-		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
+	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
+		if (gt_iir[2] & dev_priv->pm_rps_events)
+			gen6_rps_irq_handler(dev_priv, gt_iir[2]);
 
-	if (gt_iir[2] & dev_priv->pm_guc_events)
-		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
+		if (gt_iir[2] & dev_priv->pm_guc_events)
+			gen9_guc_irq_handler(dev_priv, gt_iir[2]);
+	}
 }
 
 static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
@@ -1984,7 +1987,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
 		POSTING_READ(GEN8_MASTER_IRQ);
 
-		gen8_gt_irq_handler(dev_priv, gt_iir);
+		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
 
 		if (hotplug_status)
 			i9xx_hpd_irq_handler(dev_priv, hotplug_status);
@@ -2518,36 +2521,39 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 	return ret;
 }
 
+#define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
+			  GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ | \
+			  GEN8_GT_VECS_IRQ | GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)
+
 static irqreturn_t gen8_irq_handler(int irq, void *arg)
 {
-	struct drm_device *dev = arg;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	u32 master_ctl;
-	u32 gt_iir[4] = {};
-	irqreturn_t ret;
+	struct drm_i915_private *dev_priv = arg;
+	u32 master_ctl, gt_iir[4];
+	irqreturn_t ret = IRQ_NONE;
 
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
-	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
-	master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
+	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ) &  ~GEN8_MASTER_IRQ_CONTROL;
 	if (!master_ctl)
 		return IRQ_NONE;
 
 	I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
 
-	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
-	disable_rpm_wakeref_asserts(dev_priv);
-
 	/* Find, clear, then process each source of interrupt */
-	ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
-	gen8_gt_irq_handler(dev_priv, gt_iir);
-	ret |= gen8_de_irq_handler(dev_priv, master_ctl);
+	if (master_ctl & GEN8_GT_IRQ_BITS)
+		ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
+
+	if (master_ctl & ~GEN8_GT_IRQ_BITS) {
+		disable_rpm_wakeref_asserts(dev_priv);
+		ret |= gen8_de_irq_handler(dev_priv, master_ctl);
+		enable_rpm_wakeref_asserts(dev_priv);
+	}
 
 	I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
-	POSTING_READ_FW(GEN8_MASTER_IRQ);
 
-	enable_rpm_wakeref_asserts(dev_priv);
+	if (master_ctl & GEN8_GT_IRQ_BITS)
+		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
 
 	return ret;
 }
-- 
2.14.1

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

[PATCH 2/3] drm/i915: Remove the extraneous irqreturn_t from gen8 sub handlers

[Chris Wilson]

We know whether or not the irq was intended for us by simpling
inspecting the GEN8_MASTER_IRQ. If that wasn't signaled, the irq was
spurious and that is when, and only when, we should report to the core
that we didn't handle the irq.

add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-52 (-52)
function                                     old     new   delta
gen8_irq_handler                            1623    1619      -4
gen8_gt_irq_ack                              356     308     -48
Total: Before=1271276, After=1271224, chg -0.00%

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_irq.c | 54 ++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e12321cb7403..e1d503b7352e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1326,52 +1326,43 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
 		tasklet_hi_schedule(&engine->irq_tasklet);
 }
 
-static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
-				   u32 master_ctl,
-				   u32 gt_iir[4])
+static void gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
+			    u32 master_ctl, u32 gt_iir[4])
 {
-	irqreturn_t ret = IRQ_NONE;
-
 	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
 		gt_iir[0] = I915_READ_FW(GEN8_GT_IIR(0));
-		if (gt_iir[0]) {
+		if (likely(gt_iir[0]))
 			I915_WRITE_FW(GEN8_GT_IIR(0), gt_iir[0]);
-			ret = IRQ_HANDLED;
-		} else
+		else
 			DRM_ERROR("The master control interrupt lied (GT0)!\n");
 	}
 
 	if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
 		gt_iir[1] = I915_READ_FW(GEN8_GT_IIR(1));
-		if (gt_iir[1]) {
+		if (likely(gt_iir[1]))
 			I915_WRITE_FW(GEN8_GT_IIR(1), gt_iir[1]);
-			ret = IRQ_HANDLED;
-		} else
+		else
 			DRM_ERROR("The master control interrupt lied (GT1)!\n");
 	}
 
 	if (master_ctl & GEN8_GT_VECS_IRQ) {
 		gt_iir[3] = I915_READ_FW(GEN8_GT_IIR(3));
-		if (gt_iir[3]) {
+		if (likely(gt_iir[3]))
 			I915_WRITE_FW(GEN8_GT_IIR(3), gt_iir[3]);
-			ret = IRQ_HANDLED;
-		} else
+		else
 			DRM_ERROR("The master control interrupt lied (GT3)!\n");
 	}
 
 	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
 		gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
-		if (gt_iir[2] & (dev_priv->pm_rps_events |
-				 dev_priv->pm_guc_events)) {
+		if (likely(gt_iir[2] & (dev_priv->pm_rps_events |
+					dev_priv->pm_guc_events)))
 			I915_WRITE_FW(GEN8_GT_IIR(2),
 				      gt_iir[2] & (dev_priv->pm_rps_events |
 						   dev_priv->pm_guc_events));
-			ret = IRQ_HANDLED;
-		} else
+		else
 			DRM_ERROR("The master control interrupt lied (PM)!\n");
 	}
-
-	return ret;
 }
 
 static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
@@ -2387,10 +2378,9 @@ static void bxt_hpd_irq_handler(struct drm_i915_private *dev_priv,
 	intel_hpd_irq_handler(dev_priv, pin_mask, long_mask);
 }
 
-static irqreturn_t
+static void
 gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 {
-	irqreturn_t ret = IRQ_NONE;
 	u32 iir;
 	enum pipe pipe;
 
@@ -2398,13 +2388,11 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 		iir = I915_READ(GEN8_DE_MISC_IIR);
 		if (iir) {
 			I915_WRITE(GEN8_DE_MISC_IIR, iir);
-			ret = IRQ_HANDLED;
 			if (iir & GEN8_DE_MISC_GSE)
 				intel_opregion_asle_intr(dev_priv);
 			else
 				DRM_ERROR("Unexpected DE Misc interrupt\n");
-		}
-		else
+		} else
 			DRM_ERROR("The master control interrupt lied (DE MISC)!\n");
 	}
 
@@ -2415,7 +2403,6 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 			bool found = false;
 
 			I915_WRITE(GEN8_DE_PORT_IIR, iir);
-			ret = IRQ_HANDLED;
 
 			tmp_mask = GEN8_AUX_CHANNEL_A;
 			if (INTEL_GEN(dev_priv) >= 9)
@@ -2468,7 +2455,6 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 			continue;
 		}
 
-		ret = IRQ_HANDLED;
 		I915_WRITE(GEN8_DE_PIPE_IIR(pipe), iir);
 
 		if (iir & GEN8_PIPE_VBLANK)
@@ -2502,9 +2488,8 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 		iir = I915_READ(SDEIIR);
 		if (iir) {
 			I915_WRITE(SDEIIR, iir);
-			ret = IRQ_HANDLED;
-
-			if (HAS_PCH_SPT(dev_priv) || HAS_PCH_KBP(dev_priv) ||
+			if (HAS_PCH_SPT(dev_priv) ||
+			    HAS_PCH_KBP(dev_priv) ||
 			    HAS_PCH_CNP(dev_priv))
 				spt_irq_handler(dev_priv, iir);
 			else
@@ -2517,8 +2502,6 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 			DRM_DEBUG_DRIVER("The master control interrupt lied (SDE)!\n");
 		}
 	}
-
-	return ret;
 }
 
 #define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
@@ -2529,7 +2512,6 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 {
 	struct drm_i915_private *dev_priv = arg;
 	u32 master_ctl, gt_iir[4];
-	irqreturn_t ret = IRQ_NONE;
 
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
@@ -2542,11 +2524,11 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 	/* Find, clear, then process each source of interrupt */
 	if (master_ctl & GEN8_GT_IRQ_BITS)
-		ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
+		gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
 
 	if (master_ctl & ~GEN8_GT_IRQ_BITS) {
 		disable_rpm_wakeref_asserts(dev_priv);
-		ret |= gen8_de_irq_handler(dev_priv, master_ctl);
+		gen8_de_irq_handler(dev_priv, master_ctl);
 		enable_rpm_wakeref_asserts(dev_priv);
 	}
 
@@ -2555,7 +2537,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 	if (master_ctl & GEN8_GT_IRQ_BITS)
 		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
 
-	return ret;
+	return IRQ_HANDLED;
 }
 
 struct wedge_me {
-- 
2.14.1

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

[PATCH 3/3] drm/i915: Skip pipestats for GT operations in chv/vlv irq handler

[Chris Wilson]

When handling context-switch interrupts, we are very latency sensitive;
every unnecessary mmio (uncached) read contributes toward a large
decrease in request throughput. An example is gem_exec_whisper, which
ping-pongs between the engines, where avoiding the pipe underflow
checking reduces the runtime of the test from 29s to 22s. On the other
hand, we are now not checking for pipe underflows at 100KHz, more or
less reducing it to a check every pageflip (60Hz) or modeset at worst.

add/remove: 0/0 grow/shrink: 1/2 up/down: 24/-40 (-16)
function                                     old     new   delta
valleyview_pipestat_irq_ack                  259     283     +24
valleyview_irq_handler                       521     517      -4
cherryview_irq_handler                       457     421     -36

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 123 ++++++++++++++++++++++++----------------
 1 file changed, 73 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e1d503b7352e..345d73bd4039 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1703,16 +1703,17 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
 	}
 }
 
-static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
+static bool valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
 					u32 iir, u32 pipe_stats[I915_MAX_PIPES])
 {
+	bool handled = false;
 	int pipe;
 
 	spin_lock(&dev_priv->irq_lock);
 
 	if (!dev_priv->display_irqs_enabled) {
 		spin_unlock(&dev_priv->irq_lock);
-		return;
+		return false;
 	}
 
 	for_each_pipe(dev_priv, pipe) {
@@ -1744,8 +1745,10 @@ static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
 		if (iir & iir_bit)
 			mask |= dev_priv->pipestat_irq_mask[pipe];
 
-		if (!mask)
+		if (!mask) {
+			pipe_stats[pipe] = 0;
 			continue;
+		}
 
 		reg = PIPESTAT(pipe);
 		mask |= PIPESTAT_INT_ENABLE_MASK;
@@ -1757,8 +1760,12 @@ static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
 		if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
 					PIPESTAT_INT_STATUS_MASK))
 			I915_WRITE(reg, pipe_stats[pipe]);
+
+		handled = true;
 	}
 	spin_unlock(&dev_priv->irq_lock);
+
+	return handled;
 }
 
 static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv,
@@ -1836,8 +1843,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 
 	do {
 		u32 iir, gt_iir, pm_iir;
-		u32 pipe_stats[I915_MAX_PIPES] = {};
+		u32 pipe_stats[I915_MAX_PIPES];
 		u32 hotplug_status = 0;
+		bool has_pipe_stats;
 		u32 ier = 0;
 
 		gt_iir = I915_READ(GTIIR);
@@ -1876,7 +1884,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 
 		/* Call regardless, as some status bits might not be
 		 * signalled in iir */
-		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
+		has_pipe_stats =
+			valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
 
 		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
 			   I915_LPE_PIPE_B_INTERRUPT))
@@ -1901,7 +1910,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		if (hotplug_status)
 			i9xx_hpd_irq_handler(dev_priv, hotplug_status);
 
-		valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
+		if (has_pipe_stats)
+			valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
 	} while (0);
 
 	enable_rpm_wakeref_asserts(dev_priv);
@@ -1914,27 +1924,14 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 	struct drm_device *dev = arg;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	irqreturn_t ret = IRQ_NONE;
+	u32 master_ctl;
 
 	if (!intel_irqs_enabled(dev_priv))
 		return IRQ_NONE;
 
-	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
-	disable_rpm_wakeref_asserts(dev_priv);
-
+	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
 	do {
-		u32 master_ctl, iir;
-		u32 gt_iir[4] = {};
-		u32 pipe_stats[I915_MAX_PIPES] = {};
-		u32 hotplug_status = 0;
-		u32 ier = 0;
-
-		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
-		iir = I915_READ(VLV_IIR);
-
-		if (master_ctl == 0 && iir == 0)
-			break;
-
-		ret = IRQ_HANDLED;
+		u32 iir;
 
 		/*
 		 * Theory on interrupt generation, based on empirical evidence:
@@ -1949,44 +1946,70 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		 * don't end up clearing all the VLV_IIR and GEN8_MASTER_IRQ_CONTROL
 		 * bits this time around.
 		 */
-		I915_WRITE(GEN8_MASTER_IRQ, 0);
-		ier = I915_READ(VLV_IER);
-		I915_WRITE(VLV_IER, 0);
+		if (master_ctl & ~GEN8_MASTER_IRQ_CONTROL) {
+			u32 gt_iir[4];
 
-		gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
+			I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
 
-		if (iir & I915_DISPLAY_PORT_INTERRUPT)
-			hotplug_status = i9xx_hpd_irq_ack(dev_priv);
+			gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
 
-		/* Call regardless, as some status bits might not be
-		 * signalled in iir */
-		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
+			I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
+			ret = IRQ_HANDLED;
 
-		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
-			   I915_LPE_PIPE_B_INTERRUPT |
-			   I915_LPE_PIPE_C_INTERRUPT))
-			intel_lpe_audio_irq_handler(dev_priv);
+			gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
+			master_ctl = 0;
+		}
 
-		/*
-		 * VLV_IIR is single buffered, and reflects the level
-		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
-		 */
-		if (iir)
-			I915_WRITE(VLV_IIR, iir);
+		iir = I915_READ_FW(VLV_IIR);
+		if (iir) {
+			u32 pipe_stats[I915_MAX_PIPES];
+			u32 hotplug_status = 0;
+			bool has_pipe_stats = false;
+			u32 ier;
 
-		I915_WRITE(VLV_IER, ier);
-		I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
-		POSTING_READ(GEN8_MASTER_IRQ);
+			/*
+			 * IRQs are synced during runtime_suspend,
+			 * we don't require a wakeref
+			 */
+			disable_rpm_wakeref_asserts(dev_priv);
 
-		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
 
-		if (hotplug_status)
-			i9xx_hpd_irq_handler(dev_priv, hotplug_status);
+			ier = I915_READ_FW(VLV_IER);
+			I915_WRITE_FW(VLV_IER, 0);
 
-		valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
-	} while (0);
+			if (iir & I915_DISPLAY_PORT_INTERRUPT)
+				hotplug_status = i9xx_hpd_irq_ack(dev_priv);
 
-	enable_rpm_wakeref_asserts(dev_priv);
+			if (iir & (I915_LPE_PIPE_A_INTERRUPT |
+				   I915_LPE_PIPE_B_INTERRUPT |
+				   I915_LPE_PIPE_C_INTERRUPT))
+				intel_lpe_audio_irq_handler(dev_priv);
+
+			/*
+			 * Call regardless, as some status bits might not be
+			 * signalled in iir.
+			 */
+			has_pipe_stats =
+				valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
+
+			/*
+			 * VLV_IIR is single buffered, and reflects the level
+			 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
+			 */
+			I915_WRITE_FW(VLV_IIR, iir);
+			I915_WRITE_FW(VLV_IER, ier);
+			ret = IRQ_HANDLED;
+
+			if (hotplug_status)
+				i9xx_hpd_irq_handler(dev_priv, hotplug_status);
+
+			if (has_pipe_stats)
+				valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
+
+			enable_rpm_wakeref_asserts(dev_priv);
+			master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
+		}
+	} while (master_ctl & ~GEN8_MASTER_IRQ_CONTROL);
 
 	return ret;
 }
-- 
2.14.1

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

✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Trim gen8_irq_handler

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Trim gen8_irq_handler
URL   : https://patchwork.freedesktop.org/series/30311/
State : warning

== Summary ==

Series 30311v1 series starting with [1/3] drm/i915: Trim gen8_irq_handler
https://patchwork.freedesktop.org/api/1.0/series/30311/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                pass       -> FAIL       (fi-kbl-7500u) fdo#102514
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (fi-cfl-s) fdo#102294 +1
Test kms_frontbuffer_tracking:
        Subgroup basic:
                dmesg-warn -> PASS       (fi-bdw-5557u) fdo#102473
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-1:
                pass       -> DMESG-WARN (fi-cfl-s)
        Subgroup bad-nb-words-3:
                pass       -> DMESG-WARN (fi-cfl-s)
        Subgroup bad-pipe:
                pass       -> DMESG-WARN (fi-cfl-s)
        Subgroup bad-source:
                pass       -> DMESG-WARN (fi-cfl-s)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-n2820) fdo#101705
                pass       -> FAIL       (fi-skl-6700k) fdo#100367

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fdo#102473 https://bugs.freedesktop.org/show_bug.cgi?id=102473
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
fdo#100367 https://bugs.freedesktop.org/show_bug.cgi?id=100367

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:446s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:376s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:530s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:268s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:503s
fi-byt-n2820     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:500s
fi-cfl-s         total:289  pass:219  dwarn:38  dfail:0   fail:0   skip:32  time:555s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:455s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:594s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:430s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:412s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:437s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:487s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:462s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:489s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:576s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:585s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:551s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:456s
fi-skl-6700k     total:289  pass:264  dwarn:0   dfail:0   fail:1   skip:24  time:511s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:501s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:463s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:471s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:568s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:423s

fd4b78812edddb5118f97329ba04f36483e16e2a drm-tip: 2017y-09m-13d-17h-15m-44s UTC integration manifest
7dadef59de65 drm/i915: Skip pipestats for GT operations in chv/vlv irq handler
4908579a5dea drm/i915: Remove the extraneous irqreturn_t from gen8 sub handlers
2be56fc2436f drm/i915: Trim gen8_irq_handler

== Logs ==

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

Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler

[Pandiyan, Dhinakaran]

On Wed, 2017-09-13 at 19:18 +0100, Chris Wilson wrote:
> The goal here is to trim an excess posting read and keep the predicates

Curious why we do the posting reads, is that a hardware requirement?


> tight (reusing the same predicate throughout for GT ack/handling).
> 
> add/remove: 0/0 grow/shrink: 2/1 up/down: 26/-30 (-4)
> function                                     old     new   delta
> gen8_gt_irq_handler                          282     301     +19
> cherryview_irq_handler                       450     457      +7
> gen8_irq_handler                            1653    1623     -30
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 54 +++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 91a2c5dbf2da..e12321cb7403 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1375,31 +1375,34 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
>  }
>  
>  static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
> -				u32 gt_iir[4])
> +				u32 master_ctl, u32 gt_iir[4])
>  {
> -	if (gt_iir[0]) {
> +	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
>  		gen8_cs_irq_handler(dev_priv->engine[RCS],
>  				    gt_iir[0], GEN8_RCS_IRQ_SHIFT);
>  		gen8_cs_irq_handler(dev_priv->engine[BCS],
>  				    gt_iir[0], GEN8_BCS_IRQ_SHIFT);
>  	}
>  
> -	if (gt_iir[1]) {
> +	if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
>  		gen8_cs_irq_handler(dev_priv->engine[VCS],
>  				    gt_iir[1], GEN8_VCS1_IRQ_SHIFT);
>  		gen8_cs_irq_handler(dev_priv->engine[VCS2],
>  				    gt_iir[1], GEN8_VCS2_IRQ_SHIFT);
>  	}
>  
> -	if (gt_iir[3])
> +	if (master_ctl & GEN8_GT_VECS_IRQ) {
>  		gen8_cs_irq_handler(dev_priv->engine[VECS],
>  				    gt_iir[3], GEN8_VECS_IRQ_SHIFT);
> +	}
>  
> -	if (gt_iir[2] & dev_priv->pm_rps_events)
> -		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> +	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
> +		if (gt_iir[2] & dev_priv->pm_rps_events)
> +			gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>  
> -	if (gt_iir[2] & dev_priv->pm_guc_events)
> -		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> +		if (gt_iir[2] & dev_priv->pm_guc_events)
> +			gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> +	}
>  }
>  
>  static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> @@ -1984,7 +1987,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  		I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
>  		POSTING_READ(GEN8_MASTER_IRQ);
>  
> -		gen8_gt_irq_handler(dev_priv, gt_iir);
> +		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>  
>  		if (hotplug_status)
>  			i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> @@ -2518,36 +2521,39 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>  	return ret;
>  }
>  
> +#define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
> +			  GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ | \
> +			  GEN8_GT_VECS_IRQ | GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)
> +
>  static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  {
> -	struct drm_device *dev = arg;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	u32 master_ctl;
> -	u32 gt_iir[4] = {};
> -	irqreturn_t ret;
> +	struct drm_i915_private *dev_priv = arg;
> +	u32 master_ctl, gt_iir[4];
> +	irqreturn_t ret = IRQ_NONE;
>  
>  	if (!intel_irqs_enabled(dev_priv))
>  		return IRQ_NONE;
>  
> -	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> -	master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
> +	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ) &  ~GEN8_MASTER_IRQ_CONTROL;
>  	if (!master_ctl)
>  		return IRQ_NONE;
>  
>  	I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
>  
> -	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
> -	disable_rpm_wakeref_asserts(dev_priv);
> -
>  	/* Find, clear, then process each source of interrupt */
> -	ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> -	gen8_gt_irq_handler(dev_priv, gt_iir);
> -	ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> +	if (master_ctl & GEN8_GT_IRQ_BITS)
> +		ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> +
> +	if (master_ctl & ~GEN8_GT_IRQ_BITS) {
> +		disable_rpm_wakeref_asserts(dev_priv);
> +		ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> +		enable_rpm_wakeref_asserts(dev_priv);
> +	}
>  
>  	I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> -	POSTING_READ_FW(GEN8_MASTER_IRQ);
>  
> -	enable_rpm_wakeref_asserts(dev_priv);
> +	if (master_ctl & GEN8_GT_IRQ_BITS)
> +		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>  
>  	return ret;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler

[Chris Wilson]

Quoting Pandiyan, Dhinakaran (2017-09-14 08:20:21)
> 
> On Wed, 2017-09-13 at 19:18 +0100, Chris Wilson wrote:
> > The goal here is to trim an excess posting read and keep the predicates
> 
> Curious why we do the posting reads, is that a hardware requirement?

In most cases, no. In very few cases, we do need a delay in order to be
sure the hw has processed the write before we continue (e.g. when
enabling interrupts). But really it all started as a bad idea to try and
move the regs over to WC with manual flushing, and cargo-culted since
then. The challenge is that now those delays are in place, removing them
requires care.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler

[Ville Syrjälä]

On Wed, Sep 13, 2017 at 07:18:44PM +0100, Chris Wilson wrote:
> The goal here is to trim an excess posting read and keep the predicates
> tight (reusing the same predicate throughout for GT ack/handling).
> 
> add/remove: 0/0 grow/shrink: 2/1 up/down: 26/-30 (-4)
> function                                     old     new   delta
> gen8_gt_irq_handler                          282     301     +19
> cherryview_irq_handler                       450     457      +7
> gen8_irq_handler                            1653    1623     -30
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 54 +++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 91a2c5dbf2da..e12321cb7403 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1375,31 +1375,34 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
>  }
>  
>  static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
> -				u32 gt_iir[4])
> +				u32 master_ctl, u32 gt_iir[4])
>  {
> -	if (gt_iir[0]) {
> +	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
>  		gen8_cs_irq_handler(dev_priv->engine[RCS],
>  				    gt_iir[0], GEN8_RCS_IRQ_SHIFT);
>  		gen8_cs_irq_handler(dev_priv->engine[BCS],
>  				    gt_iir[0], GEN8_BCS_IRQ_SHIFT);
>  	}
>  
> -	if (gt_iir[1]) {
> +	if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
>  		gen8_cs_irq_handler(dev_priv->engine[VCS],
>  				    gt_iir[1], GEN8_VCS1_IRQ_SHIFT);
>  		gen8_cs_irq_handler(dev_priv->engine[VCS2],
>  				    gt_iir[1], GEN8_VCS2_IRQ_SHIFT);
>  	}
>  
> -	if (gt_iir[3])
> +	if (master_ctl & GEN8_GT_VECS_IRQ) {
>  		gen8_cs_irq_handler(dev_priv->engine[VECS],
>  				    gt_iir[3], GEN8_VECS_IRQ_SHIFT);
> +	}
>  
> -	if (gt_iir[2] & dev_priv->pm_rps_events)
> -		gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> +	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
> +		if (gt_iir[2] & dev_priv->pm_rps_events)
> +			gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>  
> -	if (gt_iir[2] & dev_priv->pm_guc_events)
> -		gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> +		if (gt_iir[2] & dev_priv->pm_guc_events)
> +			gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> +	}
>  }
>  
>  static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> @@ -1984,7 +1987,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  		I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
>  		POSTING_READ(GEN8_MASTER_IRQ);
>  
> -		gen8_gt_irq_handler(dev_priv, gt_iir);
> +		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>  
>  		if (hotplug_status)
>  			i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> @@ -2518,36 +2521,39 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>  	return ret;
>  }
>  
> +#define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
> +			  GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ | \
> +			  GEN8_GT_VECS_IRQ | GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)
> +
>  static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  {
> -	struct drm_device *dev = arg;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	u32 master_ctl;
> -	u32 gt_iir[4] = {};
> -	irqreturn_t ret;
> +	struct drm_i915_private *dev_priv = arg;
> +	u32 master_ctl, gt_iir[4];
> +	irqreturn_t ret = IRQ_NONE;
>  
>  	if (!intel_irqs_enabled(dev_priv))
>  		return IRQ_NONE;
>  
> -	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> -	master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
> +	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ) &  ~GEN8_MASTER_IRQ_CONTROL;
>  	if (!master_ctl)
>  		return IRQ_NONE;
>  
>  	I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
>  
> -	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
> -	disable_rpm_wakeref_asserts(dev_priv);
> -
>  	/* Find, clear, then process each source of interrupt */
> -	ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> -	gen8_gt_irq_handler(dev_priv, gt_iir);
> -	ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> +	if (master_ctl & GEN8_GT_IRQ_BITS)
> +		ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> +
> +	if (master_ctl & ~GEN8_GT_IRQ_BITS) {
> +		disable_rpm_wakeref_asserts(dev_priv);

Hmm. Why is this needed for DE interrupts but not GT interrupts? Just
the _FW() vs. not in the codepaths? If I'm reading things right we still
have some non _FW() accesses in the RPS handler at least.

BDW+ doesn't suffer from the "hang when accessing the same cacheline from
multiple cpus" issue anymore?

> +		ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> +		enable_rpm_wakeref_asserts(dev_priv);
> +	}

This thing reminds me that I'd still like to split the DE stuff into
ack/handle stuff as well.

>  
>  	I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> -	POSTING_READ_FW(GEN8_MASTER_IRQ);
>  
> -	enable_rpm_wakeref_asserts(dev_priv);
> +	if (master_ctl & GEN8_GT_IRQ_BITS)
> +		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>  
>  	return ret;
>  }
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler

[Chris Wilson]

Quoting Ville Syrjälä (2017-09-14 16:46:10)
> On Wed, Sep 13, 2017 at 07:18:44PM +0100, Chris Wilson wrote:
> > The goal here is to trim an excess posting read and keep the predicates
> > tight (reusing the same predicate throughout for GT ack/handling).
> > 
> > add/remove: 0/0 grow/shrink: 2/1 up/down: 26/-30 (-4)
> > function                                     old     new   delta
> > gen8_gt_irq_handler                          282     301     +19
> > cherryview_irq_handler                       450     457      +7
> > gen8_irq_handler                            1653    1623     -30
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 54 +++++++++++++++++++++++------------------
> >  1 file changed, 30 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 91a2c5dbf2da..e12321cb7403 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1375,31 +1375,34 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
> > -                             u32 gt_iir[4])
> > +                             u32 master_ctl, u32 gt_iir[4])
> >  {
> > -     if (gt_iir[0]) {
> > +     if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
> >               gen8_cs_irq_handler(dev_priv->engine[RCS],
> >                                   gt_iir[0], GEN8_RCS_IRQ_SHIFT);
> >               gen8_cs_irq_handler(dev_priv->engine[BCS],
> >                                   gt_iir[0], GEN8_BCS_IRQ_SHIFT);
> >       }
> >  
> > -     if (gt_iir[1]) {
> > +     if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
> >               gen8_cs_irq_handler(dev_priv->engine[VCS],
> >                                   gt_iir[1], GEN8_VCS1_IRQ_SHIFT);
> >               gen8_cs_irq_handler(dev_priv->engine[VCS2],
> >                                   gt_iir[1], GEN8_VCS2_IRQ_SHIFT);
> >       }
> >  
> > -     if (gt_iir[3])
> > +     if (master_ctl & GEN8_GT_VECS_IRQ) {
> >               gen8_cs_irq_handler(dev_priv->engine[VECS],
> >                                   gt_iir[3], GEN8_VECS_IRQ_SHIFT);
> > +     }
> >  
> > -     if (gt_iir[2] & dev_priv->pm_rps_events)
> > -             gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> > +     if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
> > +             if (gt_iir[2] & dev_priv->pm_rps_events)
> > +                     gen6_rps_irq_handler(dev_priv, gt_iir[2]);
> >  
> > -     if (gt_iir[2] & dev_priv->pm_guc_events)
> > -             gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> > +             if (gt_iir[2] & dev_priv->pm_guc_events)
> > +                     gen9_guc_irq_handler(dev_priv, gt_iir[2]);
> > +     }
> >  }
> >  
> >  static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> > @@ -1984,7 +1987,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> >               I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> >               POSTING_READ(GEN8_MASTER_IRQ);
> >  
> > -             gen8_gt_irq_handler(dev_priv, gt_iir);
> > +             gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
> >  
> >               if (hotplug_status)
> >                       i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> > @@ -2518,36 +2521,39 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> >       return ret;
> >  }
> >  
> > +#define GEN8_GT_IRQ_BITS (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ | \
> > +                       GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ | \
> > +                       GEN8_GT_VECS_IRQ | GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)
> > +
> >  static irqreturn_t gen8_irq_handler(int irq, void *arg)
> >  {
> > -     struct drm_device *dev = arg;
> > -     struct drm_i915_private *dev_priv = to_i915(dev);
> > -     u32 master_ctl;
> > -     u32 gt_iir[4] = {};
> > -     irqreturn_t ret;
> > +     struct drm_i915_private *dev_priv = arg;
> > +     u32 master_ctl, gt_iir[4];
> > +     irqreturn_t ret = IRQ_NONE;
> >  
> >       if (!intel_irqs_enabled(dev_priv))
> >               return IRQ_NONE;
> >  
> > -     master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> > -     master_ctl &= ~GEN8_MASTER_IRQ_CONTROL;
> > +     master_ctl = I915_READ_FW(GEN8_MASTER_IRQ) &  ~GEN8_MASTER_IRQ_CONTROL;
> >       if (!master_ctl)
> >               return IRQ_NONE;
> >  
> >       I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
> >  
> > -     /* IRQs are synced during runtime_suspend, we don't require a wakeref */
> > -     disable_rpm_wakeref_asserts(dev_priv);
> > -
> >       /* Find, clear, then process each source of interrupt */
> > -     ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> > -     gen8_gt_irq_handler(dev_priv, gt_iir);
> > -     ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> > +     if (master_ctl & GEN8_GT_IRQ_BITS)
> > +             ret |= gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> > +
> > +     if (master_ctl & ~GEN8_GT_IRQ_BITS) {
> > +             disable_rpm_wakeref_asserts(dev_priv);
> 
> Hmm. Why is this needed for DE interrupts but not GT interrupts? Just
> the _FW() vs. not in the codepaths? If I'm reading things right we still
> have some non _FW() accesses in the RPS handler at least.

The GT interrupts are covered by a wakeref. I left it for DE as I
haven't verified that all should be ok. The atomic in there isn't as
expensive as the mmio, but it certainly isn't free either.

> BDW+ doesn't suffer from the "hang when accessing the same cacheline from
> multiple cpus" issue anymore?

No, that was a gen7 bug.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Skip pipestats for GT operations in chv/vlv irq handler

[Ville Syrjälä]

On Wed, Sep 13, 2017 at 07:18:46PM +0100, Chris Wilson wrote:
> When handling context-switch interrupts, we are very latency sensitive;
> every unnecessary mmio (uncached) read contributes toward a large
> decrease in request throughput. An example is gem_exec_whisper, which
> ping-pongs between the engines, where avoiding the pipe underflow
> checking reduces the runtime of the test from 29s to 22s. On the other
> hand, we are now not checking for pipe underflows at 100KHz, more or
> less reducing it to a check every pageflip (60Hz) or modeset at worst.
> 
> add/remove: 0/0 grow/shrink: 1/2 up/down: 24/-40 (-16)
> function                                     old     new   delta
> valleyview_pipestat_irq_ack                  259     283     +24
> valleyview_irq_handler                       521     517      -4
> cherryview_irq_handler                       457     421     -36
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 123 ++++++++++++++++++++++++----------------
>  1 file changed, 73 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e1d503b7352e..345d73bd4039 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1703,16 +1703,17 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
>  	}
>  }
>  
> -static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> +static bool valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
>  					u32 iir, u32 pipe_stats[I915_MAX_PIPES])
>  {
> +	bool handled = false;
>  	int pipe;
>  
>  	spin_lock(&dev_priv->irq_lock);
>  
>  	if (!dev_priv->display_irqs_enabled) {
>  		spin_unlock(&dev_priv->irq_lock);
> -		return;
> +		return false;
>  	}
>  
>  	for_each_pipe(dev_priv, pipe) {
> @@ -1744,8 +1745,10 @@ static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
>  		if (iir & iir_bit)
>  			mask |= dev_priv->pipestat_irq_mask[pipe];
>  
> -		if (!mask)
> +		if (!mask) {
> +			pipe_stats[pipe] = 0;
>  			continue;
> +		}
>  
>  		reg = PIPESTAT(pipe);
>  		mask |= PIPESTAT_INT_ENABLE_MASK;
> @@ -1757,8 +1760,12 @@ static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
>  		if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
>  					PIPESTAT_INT_STATUS_MASK))
>  			I915_WRITE(reg, pipe_stats[pipe]);
> +
> +		handled = true;
>  	}
>  	spin_unlock(&dev_priv->irq_lock);
> +
> +	return handled;
>  }
>  
>  static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv,
> @@ -1836,8 +1843,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  
>  	do {
>  		u32 iir, gt_iir, pm_iir;
> -		u32 pipe_stats[I915_MAX_PIPES] = {};
> +		u32 pipe_stats[I915_MAX_PIPES];
>  		u32 hotplug_status = 0;
> +		bool has_pipe_stats;
>  		u32 ier = 0;
>  
>  		gt_iir = I915_READ(GTIIR);
> @@ -1876,7 +1884,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  
>  		/* Call regardless, as some status bits might not be
>  		 * signalled in iir */
> -		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> +		has_pipe_stats =
> +			valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>  
>  		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
>  			   I915_LPE_PIPE_B_INTERRUPT))
> @@ -1901,7 +1910,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		if (hotplug_status)
>  			i9xx_hpd_irq_handler(dev_priv, hotplug_status);
>  
> -		valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> +		if (has_pipe_stats)
> +			valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
>  	} while (0);
>  
>  	enable_rpm_wakeref_asserts(dev_priv);
> @@ -1914,27 +1924,14 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  	struct drm_device *dev = arg;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	irqreturn_t ret = IRQ_NONE;
> +	u32 master_ctl;
>  
>  	if (!intel_irqs_enabled(dev_priv))
>  		return IRQ_NONE;
>  
> -	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
> -	disable_rpm_wakeref_asserts(dev_priv);
> -
> +	master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
>  	do {
> -		u32 master_ctl, iir;
> -		u32 gt_iir[4] = {};
> -		u32 pipe_stats[I915_MAX_PIPES] = {};
> -		u32 hotplug_status = 0;
> -		u32 ier = 0;
> -
> -		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
> -		iir = I915_READ(VLV_IIR);
> -
> -		if (master_ctl == 0 && iir == 0)
> -			break;
> -
> -		ret = IRQ_HANDLED;
> +		u32 iir;
>  
>  		/*
>  		 * Theory on interrupt generation, based on empirical evidence:
> @@ -1949,44 +1946,70 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  		 * don't end up clearing all the VLV_IIR and GEN8_MASTER_IRQ_CONTROL
>  		 * bits this time around.
>  		 */
> -		I915_WRITE(GEN8_MASTER_IRQ, 0);
> -		ier = I915_READ(VLV_IER);
> -		I915_WRITE(VLV_IER, 0);
> +		if (master_ctl & ~GEN8_MASTER_IRQ_CONTROL) {
> +			u32 gt_iir[4];
>  
> -		gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> +			I915_WRITE_FW(GEN8_MASTER_IRQ, 0);
>  
> -		if (iir & I915_DISPLAY_PORT_INTERRUPT)
> -			hotplug_status = i9xx_hpd_irq_ack(dev_priv);
> +			gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
>  
> -		/* Call regardless, as some status bits might not be
> -		 * signalled in iir */
> -		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> +			I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> +			ret = IRQ_HANDLED;

I suspect this won't work correctly. If I'm correct on how the edge
triggering works we really need to have both MASTER_IRQ_CONTROL and IER
disabled at the same time. Otherwise one can prevent the other from
generating an edge to the CPU interrupt logic.

>  
> -		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> -			   I915_LPE_PIPE_B_INTERRUPT |
> -			   I915_LPE_PIPE_C_INTERRUPT))
> -			intel_lpe_audio_irq_handler(dev_priv);
> +			gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
> +			master_ctl = 0;
> +		}
>  
> -		/*
> -		 * VLV_IIR is single buffered, and reflects the level
> -		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> -		 */
> -		if (iir)
> -			I915_WRITE(VLV_IIR, iir);
> +		iir = I915_READ_FW(VLV_IIR);
> +		if (iir) {
> +			u32 pipe_stats[I915_MAX_PIPES];
> +			u32 hotplug_status = 0;
> +			bool has_pipe_stats = false;
> +			u32 ier;
>  
> -		I915_WRITE(VLV_IER, ier);
> -		I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> -		POSTING_READ(GEN8_MASTER_IRQ);
> +			/*
> +			 * IRQs are synced during runtime_suspend,
> +			 * we don't require a wakeref
> +			 */
> +			disable_rpm_wakeref_asserts(dev_priv);
>  
> -		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>  
> -		if (hotplug_status)
> -			i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> +			ier = I915_READ_FW(VLV_IER);
> +			I915_WRITE_FW(VLV_IER, 0);
>  
> -		valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> -	} while (0);
> +			if (iir & I915_DISPLAY_PORT_INTERRUPT)
> +				hotplug_status = i9xx_hpd_irq_ack(dev_priv);
>  
> -	enable_rpm_wakeref_asserts(dev_priv);
> +			if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> +				   I915_LPE_PIPE_B_INTERRUPT |
> +				   I915_LPE_PIPE_C_INTERRUPT))
> +				intel_lpe_audio_irq_handler(dev_priv);
> +
> +			/*
> +			 * Call regardless, as some status bits might not be
> +			 * signalled in iir.
> +			 */
> +			has_pipe_stats =
> +				valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> +
> +			/*
> +			 * VLV_IIR is single buffered, and reflects the level
> +			 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> +			 */
> +			I915_WRITE_FW(VLV_IIR, iir);
> +			I915_WRITE_FW(VLV_IER, ier);
> +			ret = IRQ_HANDLED;
> +
> +			if (hotplug_status)
> +				i9xx_hpd_irq_handler(dev_priv, hotplug_status);
> +
> +			if (has_pipe_stats)
> +				valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> +
> +			enable_rpm_wakeref_asserts(dev_priv);
> +			master_ctl = I915_READ_FW(GEN8_MASTER_IRQ);
> +		}
> +	} while (master_ctl & ~GEN8_MASTER_IRQ_CONTROL);
>  
>  	return ret;
>  }
> -- 
> 2.14.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Trim gen8_irq_handler

[Pandiyan, Dhinakaran]

On Thu, 2017-09-14 at 09:37 +0100, Chris Wilson wrote:
> Quoting Pandiyan, Dhinakaran (2017-09-14 08:20:21)
> > 
> > On Wed, 2017-09-13 at 19:18 +0100, Chris Wilson wrote:
> > > The goal here is to trim an excess posting read and keep the predicates
> > 
> > Curious why we do the posting reads, is that a hardware requirement?
> 
> In most cases, no. In very few cases, we do need a delay in order to be
> sure the hw has processed the write before we continue (e.g. when
> enabling interrupts). But really it all started as a bad idea to try and
> move the regs over to WC with manual flushing, and cargo-culted since
> then.

That explains the writes followed by a read in this file! Thanks for the
interesting piece of background.

>  The challenge is that now those delays are in place, removing them
> requires care.
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx