Re: [Intel-gfx] [PATCH] drm/i915: Reduce register reads around GT interrupts

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> The GT subsystem is highly dependent on interrupts for driving the GPU;
> while the interrupt is being processed the GPU may idle leading to lower
> throughput and higher latency. Since the GT interrupts dominate, push
> the incidental display interrupt handling to later.
>
> gem_exec_nop/parallel on ivb [i7-3720QM]:
>
> Before:
>   average (individually): 2.278us
>   rcs0: 8337674 cycles, 2.399us
>   vcs0: 3286990 cycles, 6.085us
>   bcs0: 2047917 cycles, 9.766us
>
> After:
>   average (individually): 2.118us
>   rcs0: 10132462 cycles, 1.974us
>   bcs0: 3667130 cycles, 5.454us
>   vcs0: 3616618 cycles, 5.530us
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 58 +++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1fa67700d8f4..e156de88bdc6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2099,29 +2099,16 @@ static irqreturn_t ilk_irq_handler(int irq, void *arg)
>  {
>  	struct drm_i915_private *i915 = arg;
>  	void __iomem * const regs = i915->uncore.regs;
> -	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
> +	u32 de_iir, gt_iir, de_ier;
>  	irqreturn_t ret = IRQ_NONE;
>  
>  	if (unlikely(!intel_irqs_enabled(i915)))
>  		return IRQ_NONE;
>  
> -	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
> -	disable_rpm_wakeref_asserts(&i915->runtime_pm);
> -
>  	/* disable master interrupt before clearing iir  */
>  	de_ier = raw_reg_read(regs, DEIER);
>  	raw_reg_write(regs, DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>  
> -	/* Disable south interrupts. We'll only write to SDEIIR once, so further
> -	 * interrupts will will be stored on its back queue, and then we'll be
> -	 * able to process them after we restore SDEIER (as soon as we restore
> -	 * it, we'll get an interrupt if SDEIIR still has something to process
> -	 * due to its back queue). */
> -	if (!HAS_PCH_NOP(i915)) {
> -		sde_ier = raw_reg_read(regs, SDEIER);
> -		raw_reg_write(regs, SDEIER, 0);
> -	}
> -
>  	/* Find, clear, then process each source of interrupt */
>  
>  	gt_iir = raw_reg_read(regs, GTIIR);
> @@ -2134,32 +2121,47 @@ static irqreturn_t ilk_irq_handler(int irq, void *arg)
>  		ret = IRQ_HANDLED;
>  	}
>  
> +	if (INTEL_GEN(i915) >= 6) {
> +		u32 pm_iir = raw_reg_read(regs, GEN6_PMIIR);
> +		if (pm_iir) {
> +			raw_reg_write(regs, GEN6_PMIIR, pm_iir);
> +			gen6_rps_irq_handler(&i915->gt.rps, pm_iir);
> +			ret = IRQ_HANDLED;
> +		}
> +	}
> +
>  	de_iir = raw_reg_read(regs, DEIIR);
>  	if (de_iir) {
> +		u32 sde_ier = 0;
> +
> +		/*
> +		 * Disable south interrupts. We'll only write to SDEIIR once,
> +		 * so further interrupts will will be stored on its back queue,
> +		 * and then we'll be able to process them after we restore
> +		 * SDEIER (as soon as we restore it, we'll get an interrupt if
> +		 * SDEIIR still has something to process due to its back queue).
> +		 */
> +		if (!HAS_PCH_NOP(i915)) {
> +			sde_ier = raw_reg_read(regs, SDEIER);
> +			raw_reg_write(regs, SDEIER, 0);
> +		}
> +
> +		disable_rpm_wakeref_asserts(&i915->runtime_pm);
> +
>  		raw_reg_write(regs, DEIIR, de_iir);
>  		if (INTEL_GEN(i915) >= 7)
>  			ivb_display_irq_handler(i915, de_iir);
>  		else
>  			ilk_display_irq_handler(i915, de_iir);
> +
> +		enable_rpm_wakeref_asserts(&i915->runtime_pm);
>  		ret = IRQ_HANDLED;
> -	}
>  
> -	if (INTEL_GEN(i915) >= 6) {
> -		u32 pm_iir = raw_reg_read(regs, GEN6_PMIIR);
> -		if (pm_iir) {
> -			raw_reg_write(regs, GEN6_PMIIR, pm_iir);
> -			gen6_rps_irq_handler(&i915->gt.rps, pm_iir);
> -			ret = IRQ_HANDLED;
> -		}
> +		if (sde_ier)
> +			raw_reg_write(regs, SDEIER, sde_ier);
>  	}
>  
>  	raw_reg_write(regs, DEIER, de_ier);
> -	if (sde_ier)
> -		raw_reg_write(regs, SDEIER, sde_ier);
> -
> -	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
> -	enable_rpm_wakeref_asserts(&i915->runtime_pm);
> -
>  	return ret;
>  }
>  
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx