From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 2/3] drm/i915: implement IBX hdmi transcoder select workaround Date: Wed, 30 May 2012 11:39:11 +0100 Message-ID: <1338374382_354653@CP5-2952> References: <1338373918-1532-1-git-send-email-daniel.vetter@ffwll.ch> <1338373918-1532-2-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (smtp.fireflyinternet.com [109.228.6.236]) by gabe.freedesktop.org (Postfix) with ESMTP id B43BA9E7BF for ; Wed, 30 May 2012 03:39:47 -0700 (PDT) In-Reply-To: <1338373918-1532-2-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Intel Graphics Development Cc: Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org On Wed, 30 May 2012 12:31:57 +0200, Daniel Vetter wrote: > Bspec Vol 3, Part 3, Section 3.8.1.1, bit 30: > > "[DevIBX] Writing to this bit only takes effect when port is enabled. > Due to hardware issue it is required that this bit be cleared when port > is disabled. To clear this bit software must temporarily enable this > port on transcoder A." > > Unfortunately the public Bspec misses totally out on the same language > for HDMIB. Internal Bspec also mentions that one of the bad > side-effects is that DPx can fail to light up on transcoder A if HDMIx > is disabled but using transcoder B. > > I've found this while reviewing Bsepc. We already implement the same > workaround for the DP ports. > > Also replace a magic 1 with PIPE_B I've found while looking through the > code. > > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_hdmi.c | 29 ++++++++++++++++++++++++++++- > 1 files changed, 28 insertions(+), 1 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 4c6f141..4ebdcf1 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -382,7 +382,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder, > > if (HAS_PCH_CPT(dev)) > sdvox |= PORT_TRANS_SEL_CPT(intel_crtc->pipe); > - else if (intel_crtc->pipe == 1) > + else if (intel_crtc->pipe == PIPE_B) > sdvox |= SDVO_PIPE_B_SELECT; > > I915_WRITE(intel_hdmi->sdvox_reg, sdvox); > @@ -405,6 +405,33 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode) > > temp = I915_READ(intel_hdmi->sdvox_reg); > > + /* 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->crtc; > + struct intel_crtc *intel_crtc; > + > + if (crtc) > + intel_crtc = to_intel_crtc(crtc); > + else > + intel_crtc = NULL; If you extracted pipe = crtc ? to_intel_crtc(crtc)->pipe : -1; then that would make the following code less ugly. > + > + if (mode != DRM_MODE_DPMS_ON && (temp & SDVO_PIPE_B_SELECT)) { Split this into a nested if: if (mode != DPMS_MODE_DPMS_ON) { if (temp & SDVO_PIPE_B_SELECT) { // w/a } } else { // restore } > + temp &= ~SDVO_PIPE_B_SELECT; > + I915_WRITE(intel_hdmi->sdvox_reg, temp); > + POSTING_READ(intel_hdmi->sdvox_reg); Should we not worry about the HW w/a requiring this to be written twice? static void intel_hdmi_write_sdvox(intel_hdmi, u32 val) { struct drm_i915_private *dev_priv = intel_hdmi->base.dev->dev_private; I915_WRITE(intel_hdmi->sdvox_reg, val); POSTING_READ(intel_hdmi->sdvo_reg); /* HW workaround, need to write this twice for issues that may result * in first write getting masked. */ if (dev_priv->info.has_pch_split) { I915_WRITE(intel_hdmi->sdvox_reg, val); POSTING_READ(intel_hdmi->sdvox_reg); } } > + > + if (intel_crtc) > + intel_wait_for_vblank(dev, intel_crtc->pipe); > + else > + msleep(50); > + } else if (mode == DRM_MODE_DPMS_ON){ > + /* Restore the transcoder select bit. */ > + if (intel_crtc->pipe == PIPE_B) > + enable_bits |= SDVO_PIPE_B_SELECT; > + } > + } > + > /* HW workaround, need to toggle enable bit off and on for 12bpc, but > * we do this anyway which shows more stable in testing. > */ -- Chris Wilson, Intel Open Source Technology Centre