[Intel-gfx] [PATCH] drm/i915: Apply the IBX transcoder A w/a for HDMI to SDVO as well

[Paulo Zanoni]

Hi

2012/11/21 Chris Wilson <chris at chris-wilson.co.uk>:
> As the SDVO/HDMI registers are multiplex, it is safe to assume that the
> w/a required for HDMI on IbexPoint, namely that the SDVO register cannot
> both be disabled and have selected transcoder B, is also required for
> SDVO. At least the modeset state checker detects that the transcoder
> selection is left in the undefined state, and so it appears sensible to
> apply the w/a:
>
> [ 1814.480052] WARNING: at drivers/gpu/drm/i915/intel_display.c:1487 assert_pch_hdmi_disabled+0xad/0xb5()
> [ 1814.480053] Hardware name: Libretto W100
> [ 1814.480054] IBX PCH hdmi port still using transcoder B
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=57066
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

So now the code is the same on both HDMI and SDVO, which is good. The
comments below are ideas for possible follow-up patches touching both
the HDMI and SDVO code paths:

> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   38 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 92baac2..b302ef4 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1228,6 +1228,30 @@ static void intel_disable_sdvo(struct intel_encoder *encoder)
>
>         temp = I915_READ(intel_sdvo->sdvo_reg);
>         if ((temp & SDVO_ENABLE) != 0) {
> +               /* HW workaround for IBX, we need to move the port to
> +                * transcoder A before disabling it. */
> +               if (HAS_PCH_IBX(encoder->base.dev)) {
> +                       struct drm_crtc *crtc = encoder->base.crtc;
> +                       int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;

In the modeset-rework world, can we really reach this place with a
NULL crtc? Maybe we can do a follow-up patch removing this check and
the ugly "msleep(50)" below?

> +
> +                       if (temp & SDVO_PIPE_B_SELECT) {
> +                               temp &= ~SDVO_PIPE_B_SELECT;
> +                               I915_WRITE(intel_sdvo->sdvo_reg, temp);
> +                               POSTING_READ(intel_sdvo->sdvo_reg);
> +

Spec says "To clear this bit software must temporarily enable this
port on transcoder A". So doesn't that mean we need to do something
like the following?

temp &= ~SDVO_PIPE_B_SELECT;
I915_WRITE(intel_sdvo->sdvo_reg, temp); /* Disable, on transcoder A */
POSTING_READ(intel_sdvo->sdvo_reg);
I915_WRITE(intel_sdvo->sdvo_reg, temp | SDVO_ENABLE); /* Temporarily
enable on transcoder A" */
POSTING_READ(intel_sdvo->sdvo_reg);
I915_WRITE(intel_sdvo->sdvo_reg, temp); /* Disable agian, still on
transcoder A" */
POSTING_READ(intel_sdvo->sdvo_reg);



> +                               /* Again we need to write this twice. */
> +                               I915_WRITE(intel_sdvo->sdvo_reg, temp);
> +                               POSTING_READ(intel_sdvo->sdvo_reg);
> +
> +                               /* Transcoder selection bits only update
> +                                * effectively on vblank. */
> +                               if (crtc)
> +                                       intel_wait_for_vblank(encoder->base.dev, pipe);
> +                               else
> +                                       msleep(50);
> +                       }
> +               }
> +
>                 intel_sdvo_write_sdvox(intel_sdvo, temp & ~SDVO_ENABLE);
>         }
>  }
> @@ -1244,8 +1268,20 @@ static void intel_enable_sdvo(struct intel_encoder *encoder)
>         u8 status;
>
>         temp = I915_READ(intel_sdvo->sdvo_reg);
> -       if ((temp & SDVO_ENABLE) == 0)
> +       if ((temp & SDVO_ENABLE) == 0) {
> +               /* HW workaround for IBX, we need to move the port
> +                * to transcoder A before disabling it. */
> +               if (HAS_PCH_IBX(dev)) {
> +                       struct drm_crtc *crtc = encoder->base.crtc;
> +                       int pipe = crtc ? to_intel_crtc(crtc)->pipe : -1;

Same "can we reach this place with a null crtc?" question applies to this chunk.

> +
> +                       /* Restore the transcoder select bit. */
> +                       if (pipe == PIPE_B)
> +                               temp |= SDVO_PIPE_B_SELECT;
> +               }
> +
>                 intel_sdvo_write_sdvox(intel_sdvo, temp | SDVO_ENABLE);
> +       }
>         for (i = 0; i < 2; i++)
>                 intel_wait_for_vblank(dev, intel_crtc->pipe);
>
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni