Re: [PATCH 05/12] drm/i915: Clear VLV_IER around irq processing

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

Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:

> [ text/plain ]
> ville.syrjala@linux.intel.com writes:
>
>> [ text/plain ]
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> On VLV/CHV the master interrupt enable bit only affects GT/PM
>> interrupts. Display interrupts are not affected by the master
>> irq control.
>>
>> Also it seems that the CPU interrupt will only be generated when
>> the combined result of all GT/PM/display interrupts has a 0->1
>> edge. We already use the master interrupt enable bit to make sure
>> GT/PM interrupt can generate such an edge if we don't end up clearing
>> all IIR bits. We must do the same for display interrupts, and for
>> that we can simply clear out VLV_IER, and restore after we've acked
>> all the interrupts we are about to process.
>>
>> So with both master interrupt enable and VLV_IER cleared out, we will
>> guarantee that there will be a 0->1 edge if any IIR bits are still set
>> at the end, and thus another CPU interrupt will be generated.
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: 579de73b048a ("drm/i915: Exit cherryview_irq_handler() after
>> one pass")
>
> I have bisected the bsw ci/bat unstability (frequent gpu hangs) to the
> commit pointed by Fixes: And reverting seems to help. 
>
> I am now running the same drv_module_reload loop on top of this
> series. It looks good but its too early to draw conclusions.
> I will leave it running for the night.
>

Seems to work fine in my tests and as also reflected by CI/bat.

-Mika


> -Mika
>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 36 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 626775039919..46be03c616f4 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1778,7 +1778,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>>  	disable_rpm_wakeref_asserts(dev_priv);
>>  
>>  	while (true) {
>> -		/* Find, clear, then process each source of interrupt */
>> +		u32 ier = 0;
>>  
>>  		gt_iir = I915_READ(GTIIR);
>>  		pm_iir = I915_READ(GEN6_PMIIR);
>> @@ -1789,7 +1789,22 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>>  
>>  		ret = IRQ_HANDLED;
>>  
>> +		/*
>> +		 * Theory on interrupt generation, based on empirical evidence:
>> +		 *
>> +		 * x = ((VLV_IIR & VLV_IER) ||
>> +		 *      (((GT_IIR & GT_IER) || (GEN6_PMIIR & GEN6_PMIER)) &&
>> +		 *       (VLV_MASTER_IER & MASTER_INTERRUPT_ENABLE)));
>> +		 *
>> +		 * A CPU interrupt will only be raised when 'x' has a 0->1 edge.
>> +		 * Hence we clear MASTER_INTERRUPT_ENABLE and VLV_IER to
>> +		 * guarantee the CPU interrupt will be raised again even if we
>> +		 * don't end up clearing all the VLV_IIR, GT_IIR, GEN6_PMIIR
>> +		 * bits this time around.
>> +		 */
>>  		I915_WRITE(VLV_MASTER_IER, 0);
>> +		ier = I915_READ(VLV_IER);
>> +		I915_WRITE(VLV_IER, 0);
>>  
>>  		if (gt_iir)
>>  			I915_WRITE(GTIIR, gt_iir);
>> @@ -1815,6 +1830,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>>  		if (iir)
>>  			I915_WRITE(VLV_IIR, iir);
>>  
>> +		I915_WRITE(VLV_IER, ier);
>>  		I915_WRITE(VLV_MASTER_IER, MASTER_INTERRUPT_ENABLE);
>>  		POSTING_READ(VLV_MASTER_IER);
>>  	}
>> @@ -1839,6 +1855,8 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>>  	disable_rpm_wakeref_asserts(dev_priv);
>>  
>>  	do {
>> +		u32 ier = 0;
>> +
>>  		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
>>  		iir = I915_READ(VLV_IIR);
>>  
>> @@ -1847,7 +1865,22 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>>  
>>  		ret = IRQ_HANDLED;
>>  
>> +		/*
>> +		 * Theory on interrupt generation, based on empirical evidence:
>> +		 *
>> +		 * x = ((VLV_IIR & VLV_IER) ||
>> +		 *      ((GEN8_MASTER_IRQ & ~GEN8_MASTER_IRQ_CONTROL) &&
>> +		 *       (GEN8_MASTER_IRQ & GEN8_MASTER_IRQ_CONTROL)));
>> +		 *
>> +		 * A CPU interrupt will only be raised when 'x' has a 0->1 edge.
>> +		 * Hence we clear GEN8_MASTER_IRQ_CONTROL and VLV_IER to
>> +		 * guarantee the CPU interrupt will be raised again even if we
>> +		 * don't end up clearing all the VLV_IIR and GEN8_MASTER_IRQ_CONTROL
>> +		 * bits this time around.
>> +		 */
>>  		I915_WRITE(GEN8_MASTER_IRQ, 0);
>> +		ier = I915_READ(VLV_IER);
>> +		I915_WRITE(VLV_IER, 0);
>>  
>>  		gen8_gt_irq_handler(dev_priv, master_ctl);
>>  
>> @@ -1865,6 +1898,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>>  		if (iir)
>>  			I915_WRITE(VLV_IIR, iir);
>>  
>> +		I915_WRITE(VLV_IER, ier);
>>  		I915_WRITE(GEN8_MASTER_IRQ, GEN8_MASTER_IRQ_CONTROL);
>>  		POSTING_READ(GEN8_MASTER_IRQ);
>>  	} while (0);
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx