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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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