Re: [PATCH 1/3] drm/i915: fix up the IBX transcoder B check

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 12, 2012 at 08:20:12AM -0700, Jesse Barnes wrote:
> On Mon, 10 Sep 2012 21:58:29 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > This has been added in
> > 
> > commit de9a35abb3b343a25065449234e47a76c4f3454a
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Tue Jun 5 11:03:40 2012 +0200
> > 
> >     drm/i915: assert that the IBX port transcoder select w/a is implemented
> > 
> > Unfortunately I've failed to notice that these checks are not just
> > called for the port that is about to be disabled, but for all (which
> > makes sense for an assert ...), and the WARN missfired when disabling
> > another pipe than the one with the dp port.
> > 
> > Hence also check whether the port is actually disabled.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54688
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f26fb3f..b8e5a51 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1376,7 +1376,8 @@ static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
> >  	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
> >  	     reg, pipe_name(pipe));
> >  
> > -	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> > +	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & DP_PORT_EN) == 0
> > +	     && (val & DP_PIPEB_SELECT),
> >  	     "IBX PCH dp port still using transcoder B\n");
> >  }
> >  
> > @@ -1388,7 +1389,8 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
> >  	     "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
> >  	     reg, pipe_name(pipe));
> >  
> > -	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & SDVO_PIPE_B_SELECT),
> > +	WARN(HAS_PCH_IBX(dev_priv->dev) && (val & PORT_ENABLE) == 0
> > +	     && (val & SDVO_PIPE_B_SELECT),
> >  	     "IBX PCH hdmi port still using transcoder B\n");
> >  }
> >  
> 
> Won't these warn if the port is disabled rather than enabled?
> Shouldn't we be checking for (val & PORT_ENABLE) != 0 *and* pipe B is
> selected?

Recap from our irc discussion: This is for an ibx workaround, where we
can't let a disabled port stay on transcoder B (for it prevents the other
encoder on the same port from reliably getting enabled). The "is this port
properly disabled" check is above the diff context.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch