| Message ID | 1392224136-19620-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2014-02-12 at 17:55 +0100, Daniel Vetter wrote: > Just a bit of polish which I hope will help me with massaging some > internal patches to use Imre's reworked pipestat handling: > - Don't check for underrun reporting or enable pipestat interrupts > twice. > - Frob the comments a bit. > - Do the iir PIPE_EVENT to pipe mapping explicitly with a switch. We > only have one place which does this, so better to make it explicit. > > v2: Ville noticed that I've broken the logic a bit with trying to > avoid checking whether we're interested in a given pipe twice. push > the PIPESTAT read down after we've computed the mask of interesting > bits first to avoid that duplication properly. > > Cc: Imre Deak <imre.deak@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Looks simpler, so: Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 36 +++++++++++++++++++++++++----------- > drivers/gpu/drm/i915/i915_reg.h | 4 ---- > 2 files changed, 25 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 386a640b7c92..a45ed67407bd 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1559,25 +1559,39 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > spin_lock(&dev_priv->irq_lock); > for_each_pipe(pipe) { > int reg; > - u32 mask; > - > - if (!dev_priv->pipestat_irq_mask[pipe] && > - !__cpu_fifo_underrun_reporting_enabled(dev, pipe)) > - continue; > - > - reg = PIPESTAT(pipe); > - pipe_stats[pipe] = I915_READ(reg); > + u32 mask, iir_bit; > > /* > - * Clear the PIPE*STAT regs before the IIR > + * PIPESTAT bits get signalled even when the interrupt is > + * disabled with the mask bits, and some of the status bits do > + * not generate interrupts at all (like the underrun bit). Hence > + * we need to be careful that we only handle what we want to > + * handle. > */ > mask = PIPESTAT_INT_ENABLE_MASK; > if (__cpu_fifo_underrun_reporting_enabled(dev, pipe)) > mask |= PIPE_FIFO_UNDERRUN_STATUS; > - if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe)) > + > + switch (pipe) { > + case PIPE_A: > + iir_bit = I915_DISPLAY_PIPE_A_EVENT_INTERRUPT; > + break; > + case PIPE_B: > + iir_bit = I915_DISPLAY_PIPE_B_EVENT_INTERRUPT; > + break; > + } > + if (iir & iir_bit) > mask |= dev_priv->pipestat_irq_mask[pipe]; > - pipe_stats[pipe] &= mask; > > + if (!mask) > + continue; > + > + reg = PIPESTAT(pipe); > + pipe_stats[pipe] = I915_READ(reg) & mask; > + > + /* > + * Clear the PIPE*STAT regs before the IIR > + */ > if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS | > PIPESTAT_INT_STATUS_MASK)) > I915_WRITE(reg, pipe_stats[pipe]); > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 645221270c34..8344541bbb93 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -997,10 +997,6 @@ > #define I915_DISPLAY_PIPE_A_EVENT_INTERRUPT (1<<6) > #define I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT (1<<5) > #define I915_DISPLAY_PIPE_B_EVENT_INTERRUPT (1<<4) > -#define I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe) \ > - ((pipe) == PIPE_A ? I915_DISPLAY_PIPE_A_EVENT_INTERRUPT : \ > - I915_DISPLAY_PIPE_B_EVENT_INTERRUPT) > - > #define I915_DEBUG_INTERRUPT (1<<2) > #define I915_USER_INTERRUPT (1<<1) > #define I915_ASLE_INTERRUPT (1<<0)
On Wed, Feb 12, 2014 at 07:31:16PM +0200, Imre Deak wrote: > On Wed, 2014-02-12 at 17:55 +0100, Daniel Vetter wrote: > > Just a bit of polish which I hope will help me with massaging some > > internal patches to use Imre's reworked pipestat handling: > > - Don't check for underrun reporting or enable pipestat interrupts > > twice. > > - Frob the comments a bit. > > - Do the iir PIPE_EVENT to pipe mapping explicitly with a switch. We > > only have one place which does this, so better to make it explicit. > > > > v2: Ville noticed that I've broken the logic a bit with trying to > > avoid checking whether we're interested in a given pipe twice. push > > the PIPESTAT read down after we've computed the mask of interesting > > bits first to avoid that duplication properly. > > > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Looks simpler, so: > Reviewed-by: Imre Deak <imre.deak@intel.com> Queued for -next, thanks for the review. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 386a640b7c92..a45ed67407bd 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1559,25 +1559,39 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) spin_lock(&dev_priv->irq_lock); for_each_pipe(pipe) { int reg; - u32 mask; - - if (!dev_priv->pipestat_irq_mask[pipe] && - !__cpu_fifo_underrun_reporting_enabled(dev, pipe)) - continue; - - reg = PIPESTAT(pipe); - pipe_stats[pipe] = I915_READ(reg); + u32 mask, iir_bit; /* - * Clear the PIPE*STAT regs before the IIR + * PIPESTAT bits get signalled even when the interrupt is + * disabled with the mask bits, and some of the status bits do + * not generate interrupts at all (like the underrun bit). Hence + * we need to be careful that we only handle what we want to + * handle. */ mask = PIPESTAT_INT_ENABLE_MASK; if (__cpu_fifo_underrun_reporting_enabled(dev, pipe)) mask |= PIPE_FIFO_UNDERRUN_STATUS; - if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe)) + + switch (pipe) { + case PIPE_A: + iir_bit = I915_DISPLAY_PIPE_A_EVENT_INTERRUPT; + break; + case PIPE_B: + iir_bit = I915_DISPLAY_PIPE_B_EVENT_INTERRUPT; + break; + } + if (iir & iir_bit) mask |= dev_priv->pipestat_irq_mask[pipe]; - pipe_stats[pipe] &= mask; + if (!mask) + continue; + + reg = PIPESTAT(pipe); + pipe_stats[pipe] = I915_READ(reg) & mask; + + /* + * Clear the PIPE*STAT regs before the IIR + */ if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS | PIPESTAT_INT_STATUS_MASK)) I915_WRITE(reg, pipe_stats[pipe]); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 645221270c34..8344541bbb93 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -997,10 +997,6 @@ #define I915_DISPLAY_PIPE_A_EVENT_INTERRUPT (1<<6) #define I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT (1<<5) #define I915_DISPLAY_PIPE_B_EVENT_INTERRUPT (1<<4) -#define I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe) \ - ((pipe) == PIPE_A ? I915_DISPLAY_PIPE_A_EVENT_INTERRUPT : \ - I915_DISPLAY_PIPE_B_EVENT_INTERRUPT) - #define I915_DEBUG_INTERRUPT (1<<2) #define I915_USER_INTERRUPT (1<<1) #define I915_ASLE_INTERRUPT (1<<0)
Just a bit of polish which I hope will help me with massaging some internal patches to use Imre's reworked pipestat handling: - Don't check for underrun reporting or enable pipestat interrupts twice. - Frob the comments a bit. - Do the iir PIPE_EVENT to pipe mapping explicitly with a switch. We only have one place which does this, so better to make it explicit. v2: Ville noticed that I've broken the logic a bit with trying to avoid checking whether we're interested in a given pipe twice. push the PIPESTAT read down after we've computed the mask of interesting bits first to avoid that duplication properly. Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 36 +++++++++++++++++++++++++----------- drivers/gpu/drm/i915/i915_reg.h | 4 ---- 2 files changed, 25 insertions(+), 15 deletions(-)