Re: [PATCH] drm/i915: Sanitize PCH port transcoder select on IBX

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 08, 2018 at 06:36:15PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 08, 2018 at 04:23:48PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-11-08 14:36:35)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > IBX has a documented workaround which states that when we disable the
> > > port we must change its transcoder select to A, otherwise it will
> > > prevent the other port (DP vs. HDMI/SDVO) from using transcoder A.
> > > We implement the workaround during encoder disable, but looks like
> > > some BIOSen leave transcoder B selected even when the port wasn't
> > > actually enabled by the BIOS. That will trip up our asserts
> > > that attempt to make sure we never forget this w/a.
> > > 
> > > Sanitize the transcoder select to A for all disabled PCH
> > > DP/HDMI/SDVO ports. We assume that the port was never enabled
> > > by the BIOS on transcoder B, because if it had we'd actually have
> > > to toggle the port on and back off to properly switch it back to
> > > transcoder A. That would cause some display flicker if transcoder A
> > > is already enabled on some other port, so it's better not to do it
> > > unless absolutely necessary. Since we have no indication that the
> > > transcoder select is misbehaving on the affected machines we can
> > > assume the port was never actually enabled by the BIOS.
> > > 
> > > This cures warning like this during driver load:
> > > IBX PCH DP C still using transcoder B
> > > WARNING: CPU: 2 PID: 172 at drivers/gpu/drm/i915/intel_display.c:1279 assert_pch_dp_disabled+0x9e/0xb0 [i915]
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index ae6d58dbf1ed..71b7bff85e52 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15653,6 +15653,64 @@ static void intel_early_display_was(struct drm_i915_private *dev_priv)
> > >         }
> > >  }
> > >  
> > > +static void ibx_sanitize_pch_hdmi_port(struct drm_i915_private *dev_priv,
> > > +                                      enum port port, i915_reg_t hdmi_reg)
> > > +{
> > > +       u32 val = I915_READ(hdmi_reg);
> > > +
> > > +       if (val & SDVO_ENABLE ||
> > > +           (val & SDVO_PIPE_SEL_MASK) == SDVO_PIPE_SEL(PIPE_A))
> > > +               return;
> > 
> > Hmm, the w/a is also applied in intel_sdvo.c. Do we need to worry about
> > those as well?
> > 
> > Oh my, it's perfectly obvious SDVO is aliased to HDMI on ibx.
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > I would have s/hdmi/svdo/ here so that the labelling was all consistent
> > and added a reminder in the comment below that hdmi is aliased to sdvo.
> 
> I wanted to be consistent with assert_pch_hdmi_disabled(). I think only
> port B can be SDVO actually, or something along those lines. Not sure
> we want to rename all of this for that reason. But a comment seems like
> a very good idea at least. I'll add one.

Tossed in a few comments about SDVOB being an alias for HDMIB
and pushed to dinq. Thanks for the review.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx