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
