On Wed, 2014-02-05 at 17:11 +0200, Ville Syrjälä wrote:
> On Tue, Feb 04, 2014 at 09:35:51PM +0200, Imre Deak wrote:
> > Atm we call the handlers for pending pipestat interrupt events even if
> > they aren't explicitly enabled by i915_enable_pipestat(). This isn't an
> > issue for events other than the vblank start event, since those are
> > always enabled anyways. Otoh, we enable the vblank start event
> > on-demand, so we'll end up calling the vblank handler at times when they
> > are disabled.
> >
> > I haven't checked if this causes any real problem, but for consistency
> > and to remove some overhead we should still fix this by clearing /
> > handling only the enabled interrupt events. Also this is a dependency
> > for the upcoming VLV power domain patchset where we need to disable all
> > the pipestat interrupts whenever the display power well is off.
> >
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_irq.c | 33 ++++++++++++++++++++++++++++++---
> > drivers/gpu/drm/i915/i915_reg.h | 4 ++++
> > 3 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 43f37ca..faca5b4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1427,6 +1427,7 @@ typedef struct drm_i915_private {
> > };
> > u32 gt_irq_mask;
> > u32 pm_irq_mask;
> > + u32 pipestat_irq_mask[I915_MAX_PIPES];
> >
> > struct work_struct hotplug_work;
> > bool enable_hotplug_processing;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index eea5398..2cac477 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -419,6 +419,16 @@ done:
> > return ret;
> > }
> >
> > +static bool __cpu_fifo_underrun_reporting_enabled(struct drm_device *dev,
> > + enum pipe pipe)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > + return !intel_crtc->cpu_fifo_underrun_disabled;
> > +}
> > +
> > /**
> > * intel_set_pch_fifo_underrun_reporting - enable/disable FIFO underrun messages
> > * @dev: drm device
> > @@ -520,6 +530,8 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> > if ((pipestat & enable_mask) == enable_mask)
> > return;
> >
> > + dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> > +
> > /* Enable the interrupt, clear any pending status */
> > pipestat |= enable_mask | status_mask;
> > I915_WRITE(reg, pipestat);
> > @@ -550,6 +562,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> > if ((pipestat & enable_mask) == 0)
> > return;
> >
> > + dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> > +
> > pipestat &= ~enable_mask;
> > I915_WRITE(reg, pipestat);
> > POSTING_READ(reg);
> > @@ -1530,18 +1544,31 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> > static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - u32 pipe_stats[I915_MAX_PIPES];
> > + u32 pipe_stats[I915_MAX_PIPES] = { };
> > int pipe;
> >
> > spin_lock(&dev_priv->irq_lock);
> > for_each_pipe(pipe) {
> > - int reg = PIPESTAT(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);
> >
> > /*
> > * Clear the PIPE*STAT regs before the IIR
> > */
> > - if (pipe_stats[pipe] & 0x8000ffff)
> > + mask = PIPESTAT_INT_ENABLE_MASK | PIPE_FIFO_UNDERRUN_STATUS;
>
> Maybe we should add PIPE_FIFO_UNDERRUN_STATUS to the mask only when the
> underrun reporting is enabled. If someone goes and enables underrun
> reporting just after valleyview_pipestat_irq_handler(), we'd
> potentially report a stale underrun.
Ok, will fix it.
--Imre
>
> You get to blame me for that one though, since the bug is already there
> in the current code, which I implemented.
>
> > + if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe))
> > + mask |= dev_priv->pipestat_irq_mask[pipe];
> > + pipe_stats[pipe] &= mask;
> > +
> > + if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> > + PIPESTAT_INT_STATUS_MASK))
> > I915_WRITE(reg, pipe_stats[pipe]);
> > }
> > spin_unlock(&dev_priv->irq_lock);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c998c4f..47e0635 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -997,6 +997,10 @@
> > #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)
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
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
