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

Jesse Barnes
2014-02-20T19:56:03Z

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