Re: [PATCH 03/11] drm/i915: Stop pretending to mask/unmask LPE audio interrupts

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

On Tue, Apr 25, 2017 at 07:27:32PM -0500, Pierre-Louis Bossart wrote:
> On 4/25/17 3:27 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > vlv_display_irq_postinstall() enables the LPE audio interrupts
> > regardless of whether the LPE audio irq chip has masked/unmasked
> > them. Also the irqchip masking/unmasking doesn't consider the state
> > of the display power well or the device, and hence just leads to
> > dmesg spew when it tries to access the hardware while it's powered
> > down.
> >
> > If the current way works, then we don't need to do anything in the
> > mask/unmask hooks. If it doesn't work, well, then we'd need to properly
> > track whether the irqchip has masked/unmasked the interrupts when
> > we enable display interrupts. And the mask/unmask hooks would need
> > to check whether display interrupts are even enabled before frobbing
> > with he registers.
> >
> > So let's just assume the current way works and neuter the mask/unmask
> > hooks. Also clean up vlv_display_irq_postinstall() a bit and stop
> > it from trying to unmask/enable the LPE C interrupt on VLV since it
> > doesn't exist.
> 
> No objections, I assumed that we did want to update VLV_IMR and VLV_IIR 
> in the mask/unmask, that was the initial recommendation IIRC
> 
> There was also a comment where we removed all tests in 
> vlv_display_irq_postinstall:
> 
>  >> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
>  >> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
>  >
>  >I think both of these checks can be removed. We won't unmask the
>  >interrupts unless lpe is enabled, so the IIR bits will never be set in
>  >that case.

Hmm. Right, so I guess the idea originally was to just enable the
LPE interrupts in IER, but leave them masked in IMR until the
irqchip unmasks them. But the code wasn't actually doing that
because it was appending the bits to 'enable_mask' before it
assigned the value to irq_mask. Hence the interrupts were always
enabled and unmasked.

> 
> 
> 
> 
> >
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c        | 15 ++++++--------
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 36 ----------------------------------
> >  2 files changed, 6 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index fd97fe00cd0d..190f6aa5d15e 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2953,7 +2953,6 @@ static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
> >  	u32 pipestat_mask;
> >  	u32 enable_mask;
> >  	enum pipe pipe;
> > -	u32 val;
> >
> >  	pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
> >  			PIPE_CRC_DONE_INTERRUPT_STATUS;
> > @@ -2964,18 +2963,16 @@ static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
> >
> >  	enable_mask = I915_DISPLAY_PORT_INTERRUPT |
> >  		I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> > -		I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> > +		I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
> > +		I915_LPE_PIPE_A_INTERRUPT |
> > +		I915_LPE_PIPE_B_INTERRUPT;
> > +
> >  	if (IS_CHERRYVIEW(dev_priv))
> > -		enable_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
> > +		enable_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT |
> > +			I915_LPE_PIPE_C_INTERRUPT;
> >
> >  	WARN_ON(dev_priv->irq_mask != ~0);
> >
> > -	val = (I915_LPE_PIPE_A_INTERRUPT |
> > -		I915_LPE_PIPE_B_INTERRUPT |
> > -		I915_LPE_PIPE_C_INTERRUPT);
> > -
> > -	enable_mask |= val;
> > -
> >  	dev_priv->irq_mask = ~enable_mask;
> >
> >  	GEN5_IRQ_INIT(VLV_, dev_priv->irq_mask, enable_mask);
> > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > index 668f00480d97..292fedf30b00 100644
> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -149,44 +149,10 @@ static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
> >
> >  static void lpe_audio_irq_unmask(struct irq_data *d)
> >  {
> > -	struct drm_i915_private *dev_priv = d->chip_data;
> > -	unsigned long irqflags;
> > -	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > -		I915_LPE_PIPE_B_INTERRUPT);
> > -
> > -	if (IS_CHERRYVIEW(dev_priv))
> > -		val |= I915_LPE_PIPE_C_INTERRUPT;
> > -
> > -	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > -
> > -	dev_priv->irq_mask &= ~val;
> > -	I915_WRITE(VLV_IIR, val);
> > -	I915_WRITE(VLV_IIR, val);
> > -	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > -	POSTING_READ(VLV_IMR);
> > -
> > -	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  }
> >
> >  static void lpe_audio_irq_mask(struct irq_data *d)
> >  {
> > -	struct drm_i915_private *dev_priv = d->chip_data;
> > -	unsigned long irqflags;
> > -	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > -		I915_LPE_PIPE_B_INTERRUPT);
> > -
> > -	if (IS_CHERRYVIEW(dev_priv))
> > -		val |= I915_LPE_PIPE_C_INTERRUPT;
> > -
> > -	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > -
> > -	dev_priv->irq_mask |= val;
> > -	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > -	I915_WRITE(VLV_IIR, val);
> > -	I915_WRITE(VLV_IIR, val);
> > -	POSTING_READ(VLV_IIR);
> > -
> > -	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  }
> >
> >  static struct irq_chip lpe_audio_irqchip = {
> > @@ -330,8 +296,6 @@ void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
> >
> >  	desc = irq_to_desc(dev_priv->lpe_audio.irq);
> >
> > -	lpe_audio_irq_mask(&desc->irq_data);
> > -
> >  	lpe_audio_platdev_destroy(dev_priv);
> >
> >  	irq_free_desc(dev_priv->lpe_audio.irq);
> >

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