On Thu, 2014-02-20 at 11:56 -0800, Jesse Barnes wrote:
> On Tue, 18 Feb 2014 00:02:18 +0200
> Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
A good idea, I can add that check as a follow-up.
> 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).
I added
+ pipestat_mask = PIPESTAT_INT_STATUS_MASK |
+ PIPE_FIFO_UNDERRUN_STATUS;
+ I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
+ I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
which does that.
> But you've probably tested it already so I assume it works ok.
kms_flip passed except the modset_vs_hang subtests which have an open
bugzilla ticket and I think are unrelated to this change.
One more thing I noticed meanwhile, is that the locking in
valleyview_display_irqs_install/uninstall() should be around all
register accesses, since we can call these functions with interrupts
already enabled.
--Imre
>
> Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
>
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
