Re: [PATCH 17/19] drm/i915: vlv: factor out valleyview_display_irq_install

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 18 Feb 2014 00:02:18 +0200
Imre Deak <imre.deak@intel.com> wrote:

> We'll need to disable/re-enable the display-side IRQs when turning
> off/on the VLV display power well. Factor out the helper functions
> for this. For now keep the display IRQs enabled by default, so the
> functionality doesn't change. This will be changed to enable/disable
> the IRQs on-demand when adding support for VLV power wells in an
> upcoming patch.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |   1 +
>  drivers/gpu/drm/i915/i915_drv.h |   5 ++
>  drivers/gpu/drm/i915/i915_irq.c | 122 ++++++++++++++++++++++++++++++++--------
>  3 files changed, 103 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f8f7a59..dca4dc3 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1668,6 +1668,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		goto out_mtrrfree;
>  	}
>  
> +	dev_priv->display_irqs_enabled = true;
>  	intel_irq_init(dev);
>  	intel_uncore_sanitize(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 632f9d8..227c349 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1454,6 +1454,8 @@ typedef struct drm_i915_private {
>  	/* protects the irq masks */
>  	spinlock_t irq_lock;
>  
> +	bool display_irqs_enabled;
> +
>  	/* To control wakeup latency, e.g. for irq-driven dp aux transfers. */
>  	struct pm_qos_request pm_qos;
>  
> @@ -2052,6 +2054,9 @@ void
>  i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
>  		      u32 status_mask);
>  
> +void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv);
> +void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv);
> +
>  /* i915_gem.c */
>  int i915_gem_init_ioctl(struct drm_device *dev, void *data,
>  			struct drm_file *file_priv);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 75dd0a8..6078d06 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3016,41 +3016,109 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  	return 0;
>  }
>  
> +static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
> +{
> +	unsigned long irqflags;
> +	u32 pipestat_mask;
> +	u32 iir_mask;
> +
> +	pipestat_mask = PIPESTAT_INT_STATUS_MASK |
> +			PIPE_FIFO_UNDERRUN_STATUS;
> +	I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
> +	I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
> +	POSTING_READ(PIPESTAT(PIPE_A));
> +
> +	pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
> +			PIPE_CRC_DONE_INTERRUPT_STATUS;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	i915_enable_pipestat(dev_priv, PIPE_A, pipestat_mask |
> +					       PIPE_GMBUS_INTERRUPT_STATUS);
> +	i915_enable_pipestat(dev_priv, PIPE_B, pipestat_mask);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	iir_mask = I915_DISPLAY_PORT_INTERRUPT |
> +		   I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> +		   I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> +	dev_priv->irq_mask &= ~iir_mask;
> +
> +	I915_WRITE(VLV_IIR, iir_mask);
> +	I915_WRITE(VLV_IIR, iir_mask);
> +	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +	I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> +	POSTING_READ(VLV_IER);
> +}
> +
> +static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> +{
> +	unsigned long irqflags;
> +	u32 pipestat_mask;
> +	u32 iir_mask;
> +
> +	iir_mask = I915_DISPLAY_PORT_INTERRUPT |
> +		   I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> +		   I915_DISPLAY_PIPE_A_EVENT_INTERRUPT;
> +	dev_priv->irq_mask |= iir_mask;
> +	I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> +	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +	I915_WRITE(VLV_IIR, iir_mask);
> +	I915_WRITE(VLV_IIR, iir_mask);
> +	POSTING_READ(VLV_IIR);
> +
> +	pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
> +			PIPE_CRC_DONE_INTERRUPT_STATUS;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	i915_disable_pipestat(dev_priv, PIPE_A, pipestat_mask |
> +					        PIPE_GMBUS_INTERRUPT_STATUS);
> +	i915_disable_pipestat(dev_priv, PIPE_B, pipestat_mask);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +	pipestat_mask = PIPESTAT_INT_STATUS_MASK |
> +			PIPE_FIFO_UNDERRUN_STATUS;
> +	I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
> +	I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
> +	POSTING_READ(PIPESTAT(PIPE_A));
> +}
> +
> +void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv)
> +{
> +	if (dev_priv->display_irqs_enabled)
> +		return;
> +
> +	dev_priv->display_irqs_enabled = true;
> +
> +	if (dev_priv->dev->irq_enabled)
> +		valleyview_display_irqs_install(dev_priv);
> +}
> +
> +void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv)
> +{
> +	if (!dev_priv->display_irqs_enabled)
> +		return;
> +
> +	dev_priv->display_irqs_enabled = false;
> +
> +	if (dev_priv->dev->irq_enabled)
> +		valleyview_display_irqs_uninstall(dev_priv);
> +}
> +
>  static int valleyview_irq_postinstall(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -	u32 enable_mask;
> -	u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV |
> -		PIPE_CRC_DONE_INTERRUPT_STATUS;
> -	unsigned long irqflags;
>  
> -	enable_mask = I915_DISPLAY_PORT_INTERRUPT;
> -	enable_mask |= I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> -		I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> -
> -	/*
> -	 *Leave vblank interrupts masked initially.  enable/disable will
> -	 * toggle them based on usage.
> -	 */
> -	dev_priv->irq_mask = ~enable_mask;
> +	dev_priv->irq_mask = ~0;
>  
>  	I915_WRITE(PORT_HOTPLUG_EN, 0);
>  	POSTING_READ(PORT_HOTPLUG_EN);
>  
>  	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> -	I915_WRITE(VLV_IER, enable_mask);
> +	I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
>  	I915_WRITE(VLV_IIR, 0xffffffff);
> -	I915_WRITE(PIPESTAT(0), 0xffff);
> -	I915_WRITE(PIPESTAT(1), 0xffff);
>  	POSTING_READ(VLV_IER);
>  
> -	/* Interrupt setup is already guaranteed to be single-threaded, this is
> -	 * just to make the assert_spin_locked check happy. */
> -	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	i915_enable_pipestat(dev_priv, PIPE_A, pipestat_enable);
> -	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> -	i915_enable_pipestat(dev_priv, PIPE_B, pipestat_enable);
> -	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +	if (dev_priv->display_irqs_enabled)
> +		valleyview_display_irqs_install(dev_priv);
>  
>  	I915_WRITE(VLV_IIR, 0xffffffff);
>  	I915_WRITE(VLV_IIR, 0xffffffff);
> @@ -3193,8 +3261,12 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>  	I915_WRITE(HWSTAM, 0xffffffff);
>  	I915_WRITE(PORT_HOTPLUG_EN, 0);
>  	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> -	for_each_pipe(pipe)
> -		I915_WRITE(PIPESTAT(pipe), 0xffff);
> +
> +	if (dev_priv->display_irqs_enabled)
> +		valleyview_display_irqs_uninstall(dev_priv);
> +
> +	dev_priv->irq_mask = 0;
> +
>  	I915_WRITE(VLV_IIR, 0xffffffff);
>  	I915_WRITE(VLV_IMR, 0xffffffff);
>  	I915_WRITE(VLV_IER, 0x0);

IRQ enable state and reg state may be another good thing to cross
check.  And on disable, it might be a good thing to just write all of
the pipestat irq status bits to avoid stuck interrupts (I think the
order is ok though).  But you've probably tested it already so I assume
it works ok.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center