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 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.

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