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 <ch...@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuopp...@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Cc: Joonas Lahtinen <joonas.lahti...@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
