| Message ID | 1373403525-1612-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/7/9 Daniel Vetter <daniel.vetter@ffwll.ch>: > The current code won't report any fifo underruns on cpt if just one > pipe has fifo underrun reporting disabled. We can't enable the > interrupts, but we can still check the per-transcoder bits and so > report the underrun delayed if: > - We always clear the transcoder's bit (and none of the other bits) > when enabling. > - We check the transcoder's bit after disabling (to avoid racing with > the interrupt handler). > > v2: I've forgotten to actually remove the old SERR_INT clearing. > > v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also > noticed a logic bug: When an underrun interrupt fires we report it > both in the interrupt handler and when checking for underruns when > disabling it in cpt_set_fifo_underrun_reporting. But that second check > is only required if the interrupt is disabled and we're switching of > underrun reporting (e.g. because we're disabling the crtc). Hence > check for that condition. > > At first I wanted to rework the code to pass that bit of information > from the uppper functions down to cpt_set_fifo_underrun_reporting. But > that turned out too messy. Hence the quick&dirty check whether the > south error interrupt source is masked off or not. > > v4: Streamline the control flow a bit. > > v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo. > > v6: Review from Paulo: > - Reorder the was_enabled assignment to only read the register when we > need it. Also add a comment that we need to do that before updating > the register. > - s/%i/%c/ fix for the debug output. > - Fix the checkpath complaint in the SERR_INT_TRANS_FIFO_UNDERRUN > #define. > > Cc: Paulo Zanoni <przanoni@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 13 +++++++++++++ > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index d7d9afd..dd9d999 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -217,12 +217,25 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev, > struct drm_i915_private *dev_priv = dev->dev_private; > > if (enable) { > + I915_WRITE(SERR_INT, > + SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)); > + > if (!cpt_can_enable_serr_int(dev)) > return; > > ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT); At this point, you should have the following lines: I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN | SERR_INT_TRANS_B_FIFO_UNDERRUN | SERR_INT_TRANS_C_FIFO_UNDERRUN); Previous versions of this patch were removing these lines, but on this patch these lines just don't exist. There's probably something wrong with your branches. > } else { > + uint32_t tmp = I915_READ(SERR_INT); > + bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT); > + > + /* Change the state _after_ we've read out the current one. */ > ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT); > + > + if (!was_enabled && > + (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) { > + DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n", > + transcoder_name(pch_transcoder)); > + } > } > } > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index e9c50fa..7e2684f 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3882,6 +3882,7 @@ > #define SERR_INT_TRANS_C_FIFO_UNDERRUN (1<<6) > #define SERR_INT_TRANS_B_FIFO_UNDERRUN (1<<3) > #define SERR_INT_TRANS_A_FIFO_UNDERRUN (1<<0) > +#define SERR_INT_TRANS_FIFO_UNDERRUN(pipe) (1<<(pipe*3)) > > /* digital port hotplug */ > #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */ > -- > 1.8.1.4 >
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index d7d9afd..dd9d999 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -217,12 +217,25 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev, struct drm_i915_private *dev_priv = dev->dev_private; if (enable) { + I915_WRITE(SERR_INT, + SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)); + if (!cpt_can_enable_serr_int(dev)) return; ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT); } else { + uint32_t tmp = I915_READ(SERR_INT); + bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT); + + /* Change the state _after_ we've read out the current one. */ ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT); + + if (!was_enabled && + (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) { + DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n", + transcoder_name(pch_transcoder)); + } } } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index e9c50fa..7e2684f 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3882,6 +3882,7 @@ #define SERR_INT_TRANS_C_FIFO_UNDERRUN (1<<6) #define SERR_INT_TRANS_B_FIFO_UNDERRUN (1<<3) #define SERR_INT_TRANS_A_FIFO_UNDERRUN (1<<0) +#define SERR_INT_TRANS_FIFO_UNDERRUN(pipe) (1<<(pipe*3)) /* digital port hotplug */ #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */
The current code won't report any fifo underruns on cpt if just one pipe has fifo underrun reporting disabled. We can't enable the interrupts, but we can still check the per-transcoder bits and so report the underrun delayed if: - We always clear the transcoder's bit (and none of the other bits) when enabling. - We check the transcoder's bit after disabling (to avoid racing with the interrupt handler). v2: I've forgotten to actually remove the old SERR_INT clearing. v3: Use transcoder_name as suggested by Paulo Zanoni. Paulo also noticed a logic bug: When an underrun interrupt fires we report it both in the interrupt handler and when checking for underruns when disabling it in cpt_set_fifo_underrun_reporting. But that second check is only required if the interrupt is disabled and we're switching of underrun reporting (e.g. because we're disabling the crtc). Hence check for that condition. At first I wanted to rework the code to pass that bit of information from the uppper functions down to cpt_set_fifo_underrun_reporting. But that turned out too messy. Hence the quick&dirty check whether the south error interrupt source is masked off or not. v4: Streamline the control flow a bit. v5: s/pipe/pch transcoder/ in the dmesg output, suggested by Paulo. v6: Review from Paulo: - Reorder the was_enabled assignment to only read the register when we need it. Also add a comment that we need to do that before updating the register. - s/%i/%c/ fix for the debug output. - Fix the checkpath complaint in the SERR_INT_TRANS_FIFO_UNDERRUN #define. Cc: Paulo Zanoni <przanoni@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 13 +++++++++++++ drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 14 insertions(+)