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
