[01/17] drm/i915: Clear pipestat consistently
diff mbox

Message ID 20170622115555.12176-2-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä June 22, 2017, 11:55 a.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have a lot of different ways of clearing the PIPESTAT registers.
Let's unify it all into one function. There's no magic in PIPESTAT
that would require any of the double clearing and whatnot that
some of the code tries to do. All we can really do is clear the status
bits and disable the enable bits. There is no way to mask anything
so as soon as another event happens the status bit will become set
again, and trying to clear them twice or something can't protect
against that.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 67 ++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 37 deletions(-)

Comments

Chris Wilson June 22, 2017, 12:39 p.m. UTC | #1
Quoting ville.syrjala@linux.intel.com (2017-06-22 12:55:39)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We have a lot of different ways of clearing the PIPESTAT registers.
> Let's unify it all into one function. There's no magic in PIPESTAT
> that would require any of the double clearing and whatnot that
> some of the code tries to do. All we can really do is clear the status
> bits and disable the enable bits. There is no way to mask anything
> so as soon as another event happens the status bit will become set
> again, and trying to clear them twice or something can't protect
> against that.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 67 ++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b1c7d1a04612..6daaf47482d4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1732,6 +1732,19 @@ static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
>         return ret;
>  }
>  
> +static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
> +{
> +       enum pipe pipe;
> +
> +       for_each_pipe(dev_priv, pipe) {
> +               I915_WRITE(PIPESTAT(pipe),
> +                          PIPESTAT_INT_STATUS_MASK |
> +                          PIPE_FIFO_UNDERRUN_STATUS);

Hmm, is this change for i915/i965 significant? Maybe explain it away in
the changelog?
-Chris
Ville Syrjälä June 30, 2017, 11:34 a.m. UTC | #2
On Thu, Jun 22, 2017 at 01:39:47PM +0100, Chris Wilson wrote:
> Quoting ville.syrjala@linux.intel.com (2017-06-22 12:55:39)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We have a lot of different ways of clearing the PIPESTAT registers.
> > Let's unify it all into one function. There's no magic in PIPESTAT
> > that would require any of the double clearing and whatnot that
> > some of the code tries to do. All we can really do is clear the status
> > bits and disable the enable bits. There is no way to mask anything
> > so as soon as another event happens the status bit will become set
> > again, and trying to clear them twice or something can't protect
> > against that.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 67 ++++++++++++++++++-----------------------
> >  1 file changed, 30 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index b1c7d1a04612..6daaf47482d4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1732,6 +1732,19 @@ static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
> >         return ret;
> >  }
> >  
> > +static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
> > +{
> > +       enum pipe pipe;
> > +
> > +       for_each_pipe(dev_priv, pipe) {
> > +               I915_WRITE(PIPESTAT(pipe),
> > +                          PIPESTAT_INT_STATUS_MASK |
> > +                          PIPE_FIFO_UNDERRUN_STATUS);
> 
> Hmm, is this change for i915/i965 significant? Maybe explain it away in
> the changelog?

Sorry missed your question. Which change are we concerned about here
specifically?

As the commit message tried to explain, the "first disable then clear"
thing at least is pointless, and we can just do it in one step.
But maybe that wasn't the part you're thinking about?
Chris Wilson June 30, 2017, 11:41 a.m. UTC | #3
Quoting Ville Syrjälä (2017-06-30 12:34:15)
> On Thu, Jun 22, 2017 at 01:39:47PM +0100, Chris Wilson wrote:
> > Quoting ville.syrjala@linux.intel.com (2017-06-22 12:55:39)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We have a lot of different ways of clearing the PIPESTAT registers.
> > > Let's unify it all into one function. There's no magic in PIPESTAT
> > > that would require any of the double clearing and whatnot that
> > > some of the code tries to do. All we can really do is clear the status
> > > bits and disable the enable bits. There is no way to mask anything
> > > so as soon as another event happens the status bit will become set
> > > again, and trying to clear them twice or something can't protect
> > > against that.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 67 ++++++++++++++++++-----------------------
> > >  1 file changed, 30 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index b1c7d1a04612..6daaf47482d4 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -1732,6 +1732,19 @@ static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
> > >         return ret;
> > >  }
> > >  
> > > +static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
> > > +{
> > > +       enum pipe pipe;
> > > +
> > > +       for_each_pipe(dev_priv, pipe) {
> > > +               I915_WRITE(PIPESTAT(pipe),
> > > +                          PIPESTAT_INT_STATUS_MASK |
> > > +                          PIPE_FIFO_UNDERRUN_STATUS);
> > 
> > Hmm, is this change for i915/i965 significant? Maybe explain it away in
> > the changelog?
> 
> Sorry missed your question. Which change are we concerned about here
> specifically?

We didn't set PIPESTAT_INT_STATUS_MASK | PIPE_FIFO_UNDERRUN_STATUS
previously afaics.
-Chris
Ville Syrjälä June 30, 2017, 11:59 a.m. UTC | #4
On Fri, Jun 30, 2017 at 12:41:53PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2017-06-30 12:34:15)
> > On Thu, Jun 22, 2017 at 01:39:47PM +0100, Chris Wilson wrote:
> > > Quoting ville.syrjala@linux.intel.com (2017-06-22 12:55:39)
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > We have a lot of different ways of clearing the PIPESTAT registers.
> > > > Let's unify it all into one function. There's no magic in PIPESTAT
> > > > that would require any of the double clearing and whatnot that
> > > > some of the code tries to do. All we can really do is clear the status
> > > > bits and disable the enable bits. There is no way to mask anything
> > > > so as soon as another event happens the status bit will become set
> > > > again, and trying to clear them twice or something can't protect
> > > > against that.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_irq.c | 67 ++++++++++++++++++-----------------------
> > > >  1 file changed, 30 insertions(+), 37 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > index b1c7d1a04612..6daaf47482d4 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -1732,6 +1732,19 @@ static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
> > > >         return ret;
> > > >  }
> > > >  
> > > > +static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
> > > > +{
> > > > +       enum pipe pipe;
> > > > +
> > > > +       for_each_pipe(dev_priv, pipe) {
> > > > +               I915_WRITE(PIPESTAT(pipe),
> > > > +                          PIPESTAT_INT_STATUS_MASK |
> > > > +                          PIPE_FIFO_UNDERRUN_STATUS);
> > > 
> > > Hmm, is this change for i915/i965 significant? Maybe explain it away in
> > > the changelog?
> > 
> > Sorry missed your question. Which change are we concerned about here
> > specifically?
> 
> We didn't set PIPESTAT_INT_STATUS_MASK | PIPE_FIFO_UNDERRUN_STATUS
> previously afaics.

Ah. Those are the sticky status bits, so the earlier
I915_WRITE(PIPESTAT, I915_READ(PIPESTAT)) did the same thing
effectively.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1c7d1a04612..6daaf47482d4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1732,6 +1732,19 @@  static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
+static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
+{
+	enum pipe pipe;
+
+	for_each_pipe(dev_priv, pipe) {
+		I915_WRITE(PIPESTAT(pipe),
+			   PIPESTAT_INT_STATUS_MASK |
+			   PIPE_FIFO_UNDERRUN_STATUS);
+
+		dev_priv->pipestat_irq_mask[pipe] = 0;
+	}
+}
+
 static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv,
 					u32 iir, u32 pipe_stats[I915_MAX_PIPES])
 {
@@ -2931,8 +2944,6 @@  static void gen5_gt_irq_reset(struct drm_i915_private *dev_priv)
 
 static void vlv_display_irq_reset(struct drm_i915_private *dev_priv)
 {
-	enum pipe pipe;
-
 	if (IS_CHERRYVIEW(dev_priv))
 		I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK_CHV);
 	else
@@ -2941,12 +2952,7 @@  static void vlv_display_irq_reset(struct drm_i915_private *dev_priv)
 	i915_hotplug_interrupt_update_locked(dev_priv, 0xffffffff, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
-	for_each_pipe(dev_priv, pipe) {
-		I915_WRITE(PIPESTAT(pipe),
-			   PIPE_FIFO_UNDERRUN_STATUS |
-			   PIPESTAT_INT_STATUS_MASK);
-		dev_priv->pipestat_irq_mask[pipe] = 0;
-	}
+	i9xx_pipestat_irq_reset(dev_priv);
 
 	GEN5_IRQ_RESET(VLV_);
 	dev_priv->irq_mask = ~0;
@@ -3604,10 +3610,9 @@  static void ironlake_irq_uninstall(struct drm_device *dev)
 static void i8xx_irq_preinstall(struct drm_device * dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int pipe;
 
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0);
+	i9xx_pipestat_irq_reset(dev_priv);
+
 	I915_WRITE16(IMR, 0xffff);
 	I915_WRITE16(IER, 0x0);
 	POSTING_READ16(IER);
@@ -3756,13 +3761,9 @@  static irqreturn_t i8xx_irq_handler(int irq, void *arg)
 static void i8xx_irq_uninstall(struct drm_device * dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int pipe;
 
-	for_each_pipe(dev_priv, pipe) {
-		/* Clear enable bits; then clear status bits */
-		I915_WRITE(PIPESTAT(pipe), 0);
-		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
-	}
+	i9xx_pipestat_irq_reset(dev_priv);
+
 	I915_WRITE16(IMR, 0xffff);
 	I915_WRITE16(IER, 0x0);
 	I915_WRITE16(IIR, I915_READ16(IIR));
@@ -3771,16 +3772,16 @@  static void i8xx_irq_uninstall(struct drm_device * dev)
 static void i915_irq_preinstall(struct drm_device * dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int pipe;
 
 	if (I915_HAS_HOTPLUG(dev_priv)) {
 		i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	}
 
+	i9xx_pipestat_irq_reset(dev_priv);
+
 	I915_WRITE16(HWSTAM, 0xeffe);
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0);
+
 	I915_WRITE(IMR, 0xffffffff);
 	I915_WRITE(IER, 0x0);
 	POSTING_READ(IER);
@@ -3973,36 +3974,32 @@  static irqreturn_t i915_irq_handler(int irq, void *arg)
 static void i915_irq_uninstall(struct drm_device * dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int pipe;
 
 	if (I915_HAS_HOTPLUG(dev_priv)) {
 		i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	}
 
+	i9xx_pipestat_irq_reset(dev_priv);
+
 	I915_WRITE16(HWSTAM, 0xffff);
-	for_each_pipe(dev_priv, pipe) {
-		/* Clear enable bits; then clear status bits */
-		I915_WRITE(PIPESTAT(pipe), 0);
-		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
-	}
+
 	I915_WRITE(IMR, 0xffffffff);
 	I915_WRITE(IER, 0x0);
-
 	I915_WRITE(IIR, I915_READ(IIR));
 }
 
 static void i965_irq_preinstall(struct drm_device * dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int pipe;
 
 	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
+	i9xx_pipestat_irq_reset(dev_priv);
+
 	I915_WRITE(HWSTAM, 0xeffe);
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0);
+
 	I915_WRITE(IMR, 0xffffffff);
 	I915_WRITE(IER, 0x0);
 	POSTING_READ(IER);
@@ -4204,7 +4201,6 @@  static irqreturn_t i965_irq_handler(int irq, void *arg)
 static void i965_irq_uninstall(struct drm_device * dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int pipe;
 
 	if (!dev_priv)
 		return;
@@ -4212,15 +4208,12 @@  static void i965_irq_uninstall(struct drm_device * dev)
 	i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
+	i9xx_pipestat_irq_reset(dev_priv);
+
 	I915_WRITE(HWSTAM, 0xffffffff);
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0);
+
 	I915_WRITE(IMR, 0xffffffff);
 	I915_WRITE(IER, 0x0);
-
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe),
-			   I915_READ(PIPESTAT(pipe)) & 0x8000ffff);
 	I915_WRITE(IIR, I915_READ(IIR));
 }