| Message ID | 1353494663-17453-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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(-)