| Message ID | 1372973734-7601-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/7/4 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. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++++---- > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 80b88c8..bbf4df8 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -215,18 +215,26 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev, > bool enable) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT); You could move this declaration down to the "else" case so we don't read the register when enable is true. > > if (enable) { > + I915_WRITE(SERR_INT, > + SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)); > + > if (!cpt_can_enable_serr_int(dev)) > return; > > - I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN | > - SERR_INT_TRANS_B_FIFO_UNDERRUN | > - SERR_INT_TRANS_C_FIFO_UNDERRUN); > - > ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT); > } else { > + uint32_t tmp = I915_READ(SERR_INT); > + > 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 %i\n", Shouldn't this be %c ? > + transcoder_name(pch_transcoder)); > + } > } > } > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 9b51be8..5bba39a 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3881,6 +3881,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)) Both checkpatch.pl and Paulo complain: ERROR: need consistent spacing around '<<' (ctx:VxW) The logic of the patch looks correct now, I think I finally understood it. > > /* digital port hotplug */ > #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */ > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 80b88c8..bbf4df8 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -215,18 +215,26 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev, bool enable) { struct drm_i915_private *dev_priv = dev->dev_private; + bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT); if (enable) { + I915_WRITE(SERR_INT, + SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)); + if (!cpt_can_enable_serr_int(dev)) return; - I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN | - SERR_INT_TRANS_B_FIFO_UNDERRUN | - SERR_INT_TRANS_C_FIFO_UNDERRUN); - ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT); } else { + uint32_t tmp = I915_READ(SERR_INT); + 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 %i\n", + transcoder_name(pch_transcoder)); + } } } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9b51be8..5bba39a 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3881,6 +3881,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. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++++---- drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 13 insertions(+), 4 deletions(-)