Re: [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/9] drm/i915: Drop posting reads to flush master interrupts
Date: Thu, 28 Jun 2018 16:06:40 +0300	[thread overview]
Message-ID: <20180628130640.GJ20518@intel.com> (raw)
In-Reply-To: <20180628123351.31240-1-chris@chris-wilson.co.uk>

On Thu, Jun 28, 2018 at 01:33:43PM +0100, Chris Wilson wrote:
> We do not need to do a posting read of our uncached mmio write to
> re-enable the master interrupt lines after handling an interrupt, so
> don't. This saves us a slow UC read before we can process the interrupt,
> most noticeable in execlists where any stalls imposes extra latency on
> GPU command execution.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7a7c4a2bd778..e83fcedcbf1d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2168,7 +2168,6 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  
>  		I915_WRITE(VLV_IER, ier);
>  		I915_WRITE(VLV_MASTER_IER, MASTER_INTERRUPT_ENABLE);
> -		POSTING_READ(VLV_MASTER_IER);
>  
>  		if (gt_iir)
>  			snb_gt_irq_handler(dev_priv, gt_iir);
> @@ -2253,7 +2252,6 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  
>  		I915_WRITE(VLV_IER, ier);
>  		I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
> -		POSTING_READ(GEN8_MASTER_IRQ);
>  
>  		gen8_gt_irq_handler(dev_priv, master_ctl, gt_iir);
>  
> @@ -2622,7 +2620,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	/* disable master interrupt before clearing iir  */
>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> -	POSTING_READ(DEIER);
>  
>  	/* 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
> @@ -2632,7 +2629,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	if (!HAS_PCH_NOP(dev_priv)) {
>  		sde_ier = I915_READ(SDEIER);
>  		I915_WRITE(SDEIER, 0);
> -		POSTING_READ(SDEIER);
>  	}
>  
>  	/* Find, clear, then process each source of interrupt */
> @@ -2667,11 +2663,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	}
>  
>  	I915_WRITE(DEIER, de_ier);
> -	POSTING_READ(DEIER);
> -	if (!HAS_PCH_NOP(dev_priv)) {
> +	if (!HAS_PCH_NOP(dev_priv))
>  		I915_WRITE(SDEIER, sde_ier);
> -		POSTING_READ(SDEIER);
> -	}

Not 100% sure about the SDEIER thing. I believe it goes over the link
to the PCH so I suppose it might be posted. No idea how it would get
ordered wrt. subsequent accesses to CPU side registers. Hmm. Oh we
actually restore master first, then SDEIER. But I suppose the order
doesn't actually matter. As long as SDEIER is 0 at some point we should
get a new edge in the end if SDEIIR still has something asserted.

Yeah, seems fine by me:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
>  	enable_rpm_wakeref_asserts(dev_priv);
> -- 
> 2.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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