diff mbox

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

Message ID 20170913181846.18121-3-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Sept. 13, 2017, 6:18 p.m. UTC 
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(-)

Comments

Ville Syrjälä Sept. 14, 2017, 4:02 p.m. UTC | #1 
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
diff mbox

Patch

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;
 }