Re: [PATCH] drm/i915: Remove spurious DRM_ERROR for cancelled interrupts

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

On Fri, Feb 02, 2018 at 03:34:48PM +0000, Chris Wilson wrote:
> As we ourselves cancel interrupts during reset by clearing the GTIIR, it
> is possible for the master IIR to indicate a pending IRQ for which we
> have already cleared from the GTIIR. In this case, the DRM_ERROR are
> intended and should not be flagged as an error.

I guess an alternative would be to not clear IIR and use
sychronize_irq() instead as needed. But I suppose that doesn't
really provide any real benefits.

One concern is that we will now claim that all interrupts are handled,
and thus couldn't detect spurious interrupts. But one might hope that
MSI and whanot has made those a thing of the past.

I never checked what the kernel's threshold was for disabling the
interrupt line on spurious interrupts. I have occasionally thought about
it though as I too have realized that the IIR clearing could result in
the interrupt handler having nothing to do.

So yeah, not quite sure which is best, always claiming IRQ_HANDLED or
only when we had some IIR bits to clear. Maybe just return IRQ_HANDLED
always for all MSI capable platforms?

Ramblings aside, I think this should be safe enough:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 35 +++++++++--------------------------
>  1 file changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 252feff2892d..b886bd459acc 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1413,37 +1413,25 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift)
>  		tasklet_hi_schedule(&execlists->tasklet);
>  }
>  
> -static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
> -				   u32 master_ctl,
> -				   u32 gt_iir[4])
> +static void gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
> +			    u32 master_ctl, u32 gt_iir[4])
>  {
> -	irqreturn_t ret = IRQ_NONE;
> -
>  	if (master_ctl & (GEN8_GT_RCS_IRQ | GEN8_GT_BCS_IRQ)) {
>  		gt_iir[0] = I915_READ_FW(GEN8_GT_IIR(0));
> -		if (gt_iir[0]) {
> +		if (gt_iir[0])
>  			I915_WRITE_FW(GEN8_GT_IIR(0), gt_iir[0]);
> -			ret = IRQ_HANDLED;
> -		} else
> -			DRM_ERROR("The master control interrupt lied (GT0)!\n");
>  	}
>  
>  	if (master_ctl & (GEN8_GT_VCS1_IRQ | GEN8_GT_VCS2_IRQ)) {
>  		gt_iir[1] = I915_READ_FW(GEN8_GT_IIR(1));
> -		if (gt_iir[1]) {
> +		if (gt_iir[1])
>  			I915_WRITE_FW(GEN8_GT_IIR(1), gt_iir[1]);
> -			ret = IRQ_HANDLED;
> -		} else
> -			DRM_ERROR("The master control interrupt lied (GT1)!\n");
>  	}
>  
>  	if (master_ctl & GEN8_GT_VECS_IRQ) {
>  		gt_iir[3] = I915_READ_FW(GEN8_GT_IIR(3));
> -		if (gt_iir[3]) {
> +		if (gt_iir[3])
>  			I915_WRITE_FW(GEN8_GT_IIR(3), gt_iir[3]);
> -			ret = IRQ_HANDLED;
> -		} else
> -			DRM_ERROR("The master control interrupt lied (GT3)!\n");
>  	}
>  
>  	if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
> @@ -1453,12 +1441,8 @@ static irqreturn_t gen8_gt_irq_ack(struct drm_i915_private *dev_priv,
>  			I915_WRITE_FW(GEN8_GT_IIR(2),
>  				      gt_iir[2] & (dev_priv->pm_rps_events |
>  						   dev_priv->pm_guc_events));
> -			ret = IRQ_HANDLED;
> -		} else
> -			DRM_ERROR("The master control interrupt lied (PM)!\n");
> +		}
>  	}
> -
> -	return ret;
>  }
>  
>  static void gen8_gt_irq_handler(struct drm_i915_private *dev_priv,
> @@ -2695,7 +2679,6 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	u32 master_ctl;
>  	u32 gt_iir[4] = {};
> -	irqreturn_t ret;
>  
>  	if (!intel_irqs_enabled(dev_priv))
>  		return IRQ_NONE;
> @@ -2711,16 +2694,16 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  	disable_rpm_wakeref_asserts(dev_priv);
>  
>  	/* Find, clear, then process each source of interrupt */
> -	ret = gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
> +	gen8_gt_irq_ack(dev_priv, master_ctl, gt_iir);
>  	gen8_gt_irq_handler(dev_priv, gt_iir);
> -	ret |= gen8_de_irq_handler(dev_priv, master_ctl);
> +	gen8_de_irq_handler(dev_priv, master_ctl);
>  
>  	I915_WRITE_FW(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
>  	POSTING_READ_FW(GEN8_MASTER_IRQ);
>  
>  	enable_rpm_wakeref_asserts(dev_priv);
>  
> -	return ret;
> +	return IRQ_HANDLED;
>  }
>  
>  struct wedge_me {
> -- 
> 2.15.1

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