[Intel-gfx] [PATCH 07/27] drm/i915/icl: Interrupt handling

[Tvrtko Ursulin]

On 10/01/2018 10:16, Joonas Lahtinen wrote:
> On Tue, 2018-01-09 at 21:23 -0200, Paulo Zanoni wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> v2: Rebase.
>>
>> v3:
>>    * Remove DPF, it has been removed from SKL+.
>>    * Fix -internal rebase wrt. execlists interrupt handling.
>>
>> v4: Rebase.
>>
>> v5:
>>    * Updated for POR changes. (Daniele Ceraolo Spurio)
>>    * Merged with irq handling fixes by Daniele Ceraolo Spurio:
>>        * Simplify the code by using gen8_cs_irq_handler.
>>        * Fix interrupt handling for the upstream kernel.
>>
>> v6:
>>    * Remove early bringup debug messages (Tvrtko)
>>    * Add NB about arbitrary spin wait timeout (Tvrtko)
>>
>> v7 (from Paulo):
>>    * Don't try to write RO bits to registers.
>>    * Don't check for PCH types that don't exist. PCH interrupts are not
>>      here yet.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> <SNIP>
> 
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -415,6 +415,9 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
>>   	if (READ_ONCE(rps->interrupts_enabled))
>>   		return;
>>   
>> +	if (WARN_ON_ONCE(IS_GEN11(dev_priv)))
>> +		return;
>> +
>>   	spin_lock_irq(&dev_priv->irq_lock);
>>   	WARN_ON_ONCE(rps->pm_iir);
>>   	WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events);
>> @@ -431,6 +434,9 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>>   	if (!READ_ONCE(rps->interrupts_enabled))
>>   		return;
>>   
>> +	if (WARN_ON_ONCE(IS_GEN11(dev_priv)))
>> +		return;
>> +
> 
> These should be GEM_BUG_ON at most and should be the first thing in
> function.

That would be a bit unfriendly when you are bringing up the codebase and 
not all pices have been implemented yet. It gets removed later in the 
series, or in a following series, whatever.

>>   	spin_lock_irq(&dev_priv->irq_lock);
>>   	rps->interrupts_enabled = false;
>>   
>> @@ -2751,6 +2757,131 @@ static void __fini_wedge(struct wedge_me *w)
>>   	     (W)->i915;							\
>>   	     __fini_wedge((W)))
>>   
>> +static __always_inline void
>> +gen11_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
>> +{
>> +	gen8_cs_irq_handler(engine, iir, 0);
>> +}
>> +
>> +static irqreturn_t
>> +gen11_gt_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> 
> This function could use some readability :)

Name or body?

>> +{
>> +	irqreturn_t ret = IRQ_NONE;
>> +	u16 irq[2][32];
>> +	u32 dw, ident;
>> +	unsigned long tmp;
>> +	unsigned int bank, bit, engine;
>> +	unsigned long wait_start, wait_end;
>> +
>> +	memset(irq, 0, sizeof(irq));
>> +
>> +	for (bank = 0; bank < 2; bank++) {
>> +		if (master_ctl & GEN11_GT_DW_IRQ(bank)) {
> 
> Invert condition and use continue;
> 
>> +			dw = I915_READ_FW(GEN11_GT_INTR_DW(bank));
>> +			if (!dw)
>> +				DRM_ERROR("GT_INTR_DW%u blank!\n", bank);
> 
> Probably needs a more appropriate action, GEM_BUG_ON if we've never
> seen this on HW.

To early to know. TBD.

>> +			tmp = dw;
>> +			for_each_set_bit(bit, &tmp, 32) {
>> +				I915_WRITE_FW(GEN11_IIR_REG_SELECTOR(bank), 1 << bit);
> 
> BIT(bit)
> 
> + newline here

Yes sir!

> 
>> +				wait_start = local_clock() >> 10;
>> +				/* NB: Specs do not specify how long to spin wait.
>> +				 * Taking 100us as an educated guess */
>> +				wait_end = wait_start + 100;
>> +				do {
>> +					ident = I915_READ_FW(GEN11_INTR_IDENTITY_REG(bank));
>> +				} while (!(ident & GEN11_INTR_DATA_VALID) &&
>> +					 !time_after((unsigned long)local_clock() >> 10, wait_end));
> 
> Uh oh, not a really nice thing to do in IRQ handler :( We should
> probably look at some actual delay numbers and not do this.

Too early to know. TBD. Spec wants us to busy poll and doesn't specify 
the timeout. Chris also complained and we chatted about this on IRC, 
guess you missed this particular stream of conversation.

>> +
>> +				if (!(ident & GEN11_INTR_DATA_VALID))
>> +					DRM_ERROR("INTR_IDENTITY_REG%u:%u timed out!\n",
>> +						  bank, bit);
>> +
>> +				irq[bank][bit] = ident & GEN11_INTR_ENGINE_MASK;
>> +				if (!irq[bank][bit])
>> +					DRM_ERROR("INTR_IDENTITY_REG%u:%u blank!\n",
>> +						  bank, bit);
> 
> DRM_ERROR again seems like bit of an understatement for interrupt.

Precedent in "master control interrupt lied" messages. Can be revisited 
when it is known how the real silicon behaves.

> 
> + newline here
> 
>> +				I915_WRITE_FW(GEN11_INTR_IDENTITY_REG(bank), ident);
>> +			}
> 
> + newline here
> 
>> +			I915_WRITE_FW(GEN11_GT_INTR_DW(bank), dw);
>> +		}
>> +	}
>> +
>> +	if (irq[0][GEN11_RCS0]) {
>> +		gen11_cs_irq_handler(dev_priv->engine[RCS],
>> +				     irq[0][GEN11_RCS0]);
>> +		ret = IRQ_HANDLED;
>> +	}
>> +
>> +	if (irq[0][GEN11_BCS]) {
>> +		gen11_cs_irq_handler(dev_priv->engine[BCS],
>> +				     irq[0][GEN11_BCS]);
>> +		ret = IRQ_HANDLED;
>> +	}
>> +
>> +	for (engine = 0; engine < 4; engine++) {
>> +		if (irq[1][GEN11_VCS(engine)]) {
>> +			gen11_cs_irq_handler(dev_priv->engine[_VCS(engine)],
>> +					     irq[1][GEN11_VCS(engine)]);
>> +			ret = IRQ_HANDLED;
>> +		}
>> +	}
>> +
>> +	for (engine = 0; engine < 2; engine++) {
>> +		if (irq[1][GEN11_VECS(engine)]) {
>> +			gen11_cs_irq_handler(dev_priv->engine[_VECS(engine)],
>> +					     irq[1][GEN11_VECS(engine)]);
>> +			ret = IRQ_HANDLED;
>> +		}
>> +	}
> 
> Doesn't the above code look like a table to be iterated over?

Chris also had some ideas on how to modify the looping, maybe the same 
as yours. Best we chat about it when we are all present.

> 
> <SNIP>
> 
>> +static void gen11_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>> +{
>> +	const u32 irqs = GT_RENDER_USER_INTERRUPT | GT_CONTEXT_SWITCH_INTERRUPT;
>> +
>> +	BUILD_BUG_ON(irqs & 0xffff0000);
> 
> #define for the mask.
> 
>> +
>> +	/* Enable RCS, BCS, VCS and VECS class interrupts. */
>> +	I915_WRITE(GEN11_RENDER_COPY_INTR_ENABLE, irqs << 16 | irqs);
>> +	I915_WRITE(GEN11_VCS_VECS_INTR_ENABLE,	  irqs << 16 | irqs);
>> +
>> +	/* Unmask irqs on RCS, BCS, VCS and VECS engines. */
>> +	I915_WRITE(GEN11_RCS0_RSVD_INTR_MASK,	~(irqs << 16));
>> +	I915_WRITE(GEN11_BCS_RSVD_INTR_MASK,	~(irqs << 16));
>> +	I915_WRITE(GEN11_VCS0_VCS1_INTR_MASK,	~(irqs | irqs << 16));
>> +	I915_WRITE(GEN11_VCS2_VCS3_INTR_MASK,	~(irqs | irqs << 16));
>> +	I915_WRITE(GEN11_VECS0_VECS1_INTR_MASK,	~(irqs | irqs << 16));
> 
> We should also have an actual shift and mask #defines for these, now
> it's all hardcoded here.
> 
>> +
>> +	dev_priv->pm_imr = 0xffffffff; /* TODO */
> 
> Some code missing it seems.

Not sure on the states of patches which bring up these bits. Maybe it 
can be re-arranged somehow but not sure.

> 
>> +}
> 
> <SNIP>
> 
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -7957,7 +7957,10 @@ void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv)
>>   	dev_priv->gt_pm.rc6.enabled = true; /* force RC6 disabling */
>>   	intel_disable_gt_powersave(dev_priv);
>>   
>> -	gen6_reset_rps_interrupts(dev_priv);
>> +	if (INTEL_GEN(dev_priv) < 11)
>> +		gen6_reset_rps_interrupts(dev_priv);
>> +	else
>> +		WARN_ON_ONCE(1);
> 
> Some code is obviously missing here.

Yes, as above.

> 
>>   }
>>   
>>   static inline void intel_disable_llc_pstate(struct drm_i915_private *i915)
>> @@ -8070,6 +8073,8 @@ static void intel_enable_rps(struct drm_i915_private *dev_priv)
>>   		cherryview_enable_rps(dev_priv);
>>   	} else if (IS_VALLEYVIEW(dev_priv)) {
>>   		valleyview_enable_rps(dev_priv);
>> +	} else if (WARN_ON_ONCE(INTEL_GEN(dev_priv) >= 11)) {
>> +		/* TODO */
> 
> Ditto.
> 
> If these are in a later patch, should be squashed here.

It might be possible in some cases, or it might be quite challenging in 
others. Need to look into it but no promises. We might have to live with 
having place holders like this in the code which get removed by later 
patches/series. It's quite complex logistically to organise multiple 
series, written by multiple authors, at different times, and make it 
look 100% pretty. (And not just squash and butcher everything up at 
merge time.)

Regards,

Tvrtko