2013/7/9 Daniel Vetter <daniel.vetter at 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 at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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
>
--
Paulo Zanoni
