[Intel-gfx] [PATCH] drm/i915: improve SERR_INT clearing for fifo underrun reporting

[Paulo Zanoni]

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