Re: [PATCH 10/10] drm/i915/icl: Only ack irq identities we did handle

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
To: Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 10/10] drm/i915/icl: Only ack irq identities we did handle
Date: Thu, 20 Sep 2018 10:14:13 -0700	[thread overview]
Message-ID: <3e4454bd-bd17-f779-cd77-494d0ba58abf@intel.com> (raw)
In-Reply-To: <20180920143350.29249-11-mika.kuoppala@linux.intel.com>

On 20/09/18 07:33, Mika Kuoppala wrote:
> If we ack the identities immediately after they have been
> handled, it should unblock next interrupt accumulation
> in the gathering register earlier. It also allows us to
> remove time based polling of valid bit. If we don't get a valid
> sample now, we will likely get a valid sample on next interrupt,
> which will be generated due to skipping the ack. The downside
> is that we will have as many ack writes as there were identities
> handled.
> 

I'm not sure this is going to work. It looks like if we skip the 
clearing of the identity register we need to skip the clearing of 
INTR_DW as well because the HW doesn't seem to set that again. If we 
clear it, when the interrupt gets re-triggered the driver won't be able 
to handle it and so it'll keep being re-triggered in a loop. I've just 
confirmed this behavior on the current code (i.e. without this series 
applied) by simply pretending the ident was not valid for the first 
interrupt:

[  728.765749] [drm:gen11_gt_engine_identity [i915]] *ERROR* 
INTR_IDENTITY_REG0:15 0x00000000 not valid!
[  728.765800] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!
[  728.765865] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!
[  728.766050] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!
[... more of the same ...]
[  782.395257] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!
[  782.395293] [drm:gen11_irq_handler [i915]] *ERROR* GT_INTR_DW0 blank!

I haven't checked in detail all the other patches in the series yet so 
not sure if any of them can mitigate this issue.

Daniele

> Leave small retry loop for safety and with debugs to see if we
> ever encounter read with valid not set.
> 
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++++---------------
>   1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 27116e3f21af..beb9fe4abf1f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2965,22 +2965,18 @@ gen11_gt_engine_identity(struct drm_i915_private * const i915,
>   			 const unsigned int bank, const unsigned int bit)
>   {
>   	void __iomem * const regs = i915->regs;
> -	u32 timeout_ts;
> -	u32 ident;
> +	u32 ident, retry = 0;
>   
>   	lockdep_assert_held(&i915->irq_lock);
>   
>   	raw_reg_write(regs, GEN11_IIR_REG_SELECTOR(bank), BIT(bit));
>   
> -	/*
> -	 * NB: Specs do not specify how long to spin wait,
> -	 * so we do ~100us as an educated guess.
> -	 */
> -	timeout_ts = (local_clock() >> 10) + 100;
>   	do {
>   		ident = raw_reg_read(regs, GEN11_INTR_IDENTITY_REG(bank));
> -	} while (!(ident & GEN11_INTR_DATA_VALID) &&
> -		 !time_after32(local_clock() >> 10, timeout_ts));
> +	} while (!(ident & GEN11_INTR_DATA_VALID) && ++retry <= 10);
> +
> +	if (unlikely(GEM_SHOW_DEBUG() && retry))
> +		WARN_ONCE(1, "INTR_IDENTITY took %u reads to settle\n", retry);
>   
>   	if (unlikely(!(ident & GEN11_INTR_DATA_VALID))) {
>   		DRM_ERROR("INTR_IDENTITY_REG%u:%u 0x%08x not valid!\n",
> @@ -3031,9 +3027,6 @@ gen11_gt_identity_handler(struct drm_i915_private * const i915,
>   	const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity);
>   	const u16 intr = GEN11_INTR_ENGINE_INTR(identity);
>   
> -	if (unlikely(!intr))
> -		return;
> -
>   	if (class <= COPY_ENGINE_CLASS)
>   		return gen11_engine_irq_handler(i915, class, instance, intr);
>   
> @@ -3065,11 +3058,14 @@ gen11_gt_bank_handler(struct drm_i915_private * const i915,
>   		const u32 ident = gen11_gt_engine_identity(i915,
>   							   bank, bit);
>   
> +		if (unlikely(!ident))
> +			continue;
> +
>   		gen11_gt_identity_handler(i915, ident);
> -	}
>   
> -	/* Clear must be after shared has been served for engine */
> -	raw_reg_write(regs, GEN11_GT_INTR_DW(bank), intr_dw);
> +		/* Clear must be after shared has been served for engine */
> +		raw_reg_write(regs, GEN11_GT_INTR_DW(bank), BIT(bit));
> +	}
>   }
>   
>   static void
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx