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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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.




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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx