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

[Paulo Zanoni]

2013/6/12 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).

I guess you'll have to update this patch due to my bikeshed on IRC for
patch 9. Anyway, comments below:


>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 15 +++++++++++----
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 685ad84..8627043 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -210,16 +210,23 @@ 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;
> -
> -               I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
> -                                    SERR_INT_TRANS_B_FIFO_UNDERRUN |
> -                                    SERR_INT_TRANS_C_FIFO_UNDERRUN);
>         }
>
>         ibx_display_interrupt_update(dev_priv, SDE_ERROR_CPT,
>                                      enable ? SDE_ERROR_CPT : 0);
> +
> +       if (!enable) {
> +               uint32_t tmp = I915_READ(SERR_INT);
> +
> +               if (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))
> +                       DRM_DEBUG_KMS("uncleared pch fifo underrun on pipe %i\n",
> +                                     pch_transcoder);

Use %c and transcoder_name(pch_transcoder), otherwise you trigger
people's OCDs again :)

Also, I think the logic in this patch is inverted.

Imagine everything is running correctly and the bits are all in the
ideal state. Then we get an underrun report on transcoder A. We'll
detect this inside cpt_serr_int_handler, call
intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, false), which
will call cpt_set_fifo_underrun_reporting(dev, TRANSCODER_A, false).
Then, inside your cpt_set_fifo_underrun_reporting, we'll disable
SDE_ERROR_CPT, then read SERR_INT, see that the bit is 1 and print the
DRM_DEBUG_KMS message you added. We'll return from
cpt_set_fifo_underrun_reporting, then return from
intel_set_pch_fifo_underrun_reporting, and then we'll print an error
message again inside cpt_serr_int_handler. So we'll print 2 messages
for a single underrun.

Instead of checking if the bit is 1 before disabling it, you should
check if the bit is 1 before re-enabling it. The problem that you're
trying to fix is that we don't report underruns while SDE_ERROR_CPT is
disabled, so the solution is to ask "did we get any underruns while
SDE_ERROR_CPT was disabled?", so inside the "if (enable)" case we need
to check if the bit was 1, and on the "if (!enable)" case we need to
clear the bit. Also, we should probably only print this "undetected
pch fifo underrun" message if crtc->pch_fifo_underrun_disabled is
false (otherwise we may report underruns on the same pipe multiple
times).

I hope I'm not confused :)


> +       }
>  }
>
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b1fdca9..86e3987 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3880,6 +3880,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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni