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

From: Imre Deak <imre.deak@intel.com>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 17/19] drm/i915: vlv: factor out valleyview_display_irq_install
Date: Mon, 24 Feb 2014 15:34:39 +0200	[thread overview]
Message-ID: <1393248879.13131.93.camel@intelbox> (raw)
In-Reply-To: <20140220115616.023016a5@jbarnes-desktop>

[-- Attachment #1.1: Type: text/plain, Size: 8907 bytes --]

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@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.

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@virtuousgeek.org>
> 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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