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

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

[Chris Wilson]

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@chris-wilson.co.uk>
---
 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;
+
+			if (temp & SDVO_PIPE_B_SELECT) {
+				temp &= ~SDVO_PIPE_B_SELECT;
+				I915_WRITE(intel_sdvo->sdvo_reg, temp);
+				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;
+
+			/* 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

Re: [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@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@chris-wilson.co.uk>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

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

[Daniel Vetter]

On Wed, Nov 21, 2012 at 10:44:23AM +0000, Chris Wilson wrote:
> 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@chris-wilson.co.uk>
Queued for -next, thanks for the patch.

> ---
>  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;

All these "is the encoder enabled" and "do we have a crtc for sure" checks
remind me that we still have cruft all over the place, which should die
with the modeset rework. Work for 3.9 I guess ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch