diff mbox

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

Message ID 1460571598-24452-6-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä April 13, 2016, 6:19 p.m. UTC 
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")
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(-)

Comments

Chris Wilson April 13, 2016, 8:53 p.m. UTC | #1 
On Wed, Apr 13, 2016 at 09:19:51PM +0300, ville.syrjala@linux.intel.com wrote:
> 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")
> 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.

Following this logic, we want to enable MASTER_IER before VLV_IER such
that we get an immediate irq if there is a residual VLV_IIR.

> +		 */
>  		I915_WRITE(VLV_MASTER_IER, 0);
> +		ier = I915_READ(VLV_IER);

+wishlist: dev_priv->irq_enabled to save adding another mmio read.

> +		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);
>  	}
>
Ville Syrjälä April 14, 2016, 8:22 a.m. UTC | #2 
On Wed, Apr 13, 2016 at 09:53:38PM +0100, Chris Wilson wrote:
> On Wed, Apr 13, 2016 at 09:19:51PM +0300, ville.syrjala@linux.intel.com wrote:
> > 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")
> > 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.
> 
> Following this logic, we want to enable MASTER_IER before VLV_IER such
> that we get an immediate irq if there is a residual VLV_IIR.

The order between master irq enable and VLV_IIR shouldn't matter. They
are totally independent of each other. Master irq enable is for GT,
VLV_IER is for display. We just have to make sure both of them are
going to be zero simultaneously, which will guarantee that the CPU
interrupt generation logic will see x==0 at that point.

> 
> > +		 */
> >  		I915_WRITE(VLV_MASTER_IER, 0);
> > +		ier = I915_READ(VLV_IER);
> 
> +wishlist: dev_priv->irq_enabled to save adding another mmio read.

I suppose such a thing could be added.

> 
> > +		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);
> >  	}
> > 
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson April 14, 2016, 8:32 a.m. UTC | #3 
On Thu, Apr 14, 2016 at 11:22:48AM +0300, Ville Syrjälä wrote:
> On Wed, Apr 13, 2016 at 09:53:38PM +0100, Chris Wilson wrote:
> > On Wed, Apr 13, 2016 at 09:19:51PM +0300, ville.syrjala@linux.intel.com wrote:
> > > +		/*
> > > +		 * 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.
> > 
> > Following this logic, we want to enable MASTER_IER before VLV_IER such
> > that we get an immediate irq if there is a residual VLV_IIR.
> 
> The order between master irq enable and VLV_IIR shouldn't matter. They
> are totally independent of each other. Master irq enable is for GT,
> VLV_IER is for display. We just have to make sure both of them are
> going to be zero simultaneously, which will guarantee that the CPU
> interrupt generation logic will see x==0 at that point.

There is no flow from VLV_IER to VLV_MASTER_IER ?

Hmm, I guess it only matters if the interrupt is raised on the leading
edge versus the level. I presumed we had only edge triggered interrupts,
that is the signal is sent fom VLV_IIR & VLV_IER once and will not
result in an interrupt unless VLV_MASTER_IER is enabled.
-Chris
Ville Syrjälä April 14, 2016, 8:49 a.m. UTC | #4 
On Thu, Apr 14, 2016 at 09:32:30AM +0100, Chris Wilson wrote:
> On Thu, Apr 14, 2016 at 11:22:48AM +0300, Ville Syrjälä wrote:
> > On Wed, Apr 13, 2016 at 09:53:38PM +0100, Chris Wilson wrote:
> > > On Wed, Apr 13, 2016 at 09:19:51PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > +		/*
> > > > +		 * 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.
> > > 
> > > Following this logic, we want to enable MASTER_IER before VLV_IER such
> > > that we get an immediate irq if there is a residual VLV_IIR.
> > 
> > The order between master irq enable and VLV_IIR shouldn't matter. They
> > are totally independent of each other. Master irq enable is for GT,
> > VLV_IER is for display. We just have to make sure both of them are
> > going to be zero simultaneously, which will guarantee that the CPU
> > interrupt generation logic will see x==0 at that point.
> 
> There is no flow from VLV_IER to VLV_MASTER_IER ?

Nope. The master only handles GT interrupts.

> 
> Hmm, I guess it only matters if the interrupt is raised on the leading
> edge versus the level. I presumed we had only edge triggered interrupts,
> that is the signal is sent fom VLV_IIR & VLV_IER once and will not
> result in an interrupt unless VLV_MASTER_IER is enabled.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson April 14, 2016, 9:36 a.m. UTC | #5 
On Wed, Apr 13, 2016 at 09:19:51PM +0300, ville.syrjala@linux.intel.com wrote:
> 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")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Ville is very persuasive,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Other than wishing the extra POSTING_READ and reading known registers
away, the rest of the series is also
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjälä April 14, 2016, 12:30 p.m. UTC | #6 
On Thu, Apr 14, 2016 at 10:36:00AM +0100, Chris Wilson wrote:
> On Wed, Apr 13, 2016 at 09:19:51PM +0300, ville.syrjala@linux.intel.com wrote:
> > 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")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Ville is very persuasive,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Other than wishing the extra POSTING_READ and reading known registers
> away, the rest of the series is also
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Entire series pushed to dinq. Thanks for the review.

Let's keep our fingers crossed that this cures all the
current chv ailments.
Mika Kuoppala April 14, 2016, 12:47 p.m. UTC | #7 
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.

-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
Mika Kuoppala April 15, 2016, 7:32 a.m. UTC | #8 
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
diff mbox

Patch

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);