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

[Paulo Zanoni]

2013/7/4 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.
>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni