Re: [PATCH v2 03/10] drm/i915: Check VBT for CRT port presence on HSW/BDW

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

On Tue, Dec 01, 2015 at 05:28:19PM -0200, Paulo Zanoni wrote:
> 2015-12-01 14:07 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Unfortunatey there appear to quite a few HSW/BDW machines (eg.
> > NUCs, Brix Pro) in the wild with LPT/WPT-H but non-working FDI.
> > FDI training fails every single time on these machines. Dunno,
> > maybe they just didn't bother wiring it up or something?
> 
> I know you clarified this on IRC, but just a little nitpick: when
> reading this, it was not very clear to me whether these machines had
> an actual physical CRT connector or not. If you read this while
> assuming the machine does have a CRT connector, the patch would sound
> like "we fail to properly train FDI, so let's give up and not register
> the CRT connector at all", which is not our case.

I'll try revise a bit.

> 
> >
> > Unfortunately all the fuse bits and whatnot are telling us that
> > the CRT connector is present. And so what we get from this is tons
> > of false positives from the CI systems due to VGA connector forcing.
> >
> > I've not found any way to detect this purely from hardware, so we
> > have to resort to looking at the VBT int_crt_support bit. We used
> > to check this bit on all platforms, but that broke all the old
> > machines, so the check was then restricted to VLV only in
> > commit 84b4e042c470 ("drm/i915: only apply crt_present check on VLV")
> >
> > Considering HSW and VLV VBT probably got defined around the same time,
> > it should be reasonably safe to assume that the bits is sane for
> > HSW/BDW as well. At least I have one copy of some VBT spec here that
> > says it's meant for both VLV and HSW, and it knows about the bit
> > (lists it being valid from version 155 onwards). Also I have two
> > desktop machines with actual CRT ports and both have
> > int_crt_support==1 in their VBTs.
> >
> > v2: Move the platform checks into the VBT parsing code
> >     Also check that the VBT version is at least 155
> 
> You could also add that HAS_DDI platforms are already believing the
> VBT for versions >= 155: see intel_ddi_init(), so we have a precedent.

Can do.

> 
> (by the way, I'm trusting you that VLV VBT versions are always >= 155,
> I couldn't check this)

It is at least on the VLV I have on my desk. But even if it wasn't, I
don't think things should break. We live with shadow VGA connectors on
many platforms, and I don't think VLV would be too upset by one. IIRC
it was mainly a speed optimization to get rid of it.

> 
> With or without any changes:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_bios.c    | 5 ++++-
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index ce82f9c7df24..070470fe9a91 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -356,7 +356,10 @@ parse_general_features(struct drm_i915_private *dev_priv,
> >         general = find_section(bdb, BDB_GENERAL_FEATURES);
> >         if (general) {
> >                 dev_priv->vbt.int_tv_support = general->int_tv_support;
> > -               dev_priv->vbt.int_crt_support = general->int_crt_support;
> > +               /* int_crt_support can't be trusted on earlier platforms */
> > +               if (bdb->version >= 155 &&
> > +                   (HAS_DDI(dev_priv) || IS_VALLEYVIEW(dev_priv)))
> > +                       dev_priv->vbt.int_crt_support = general->int_crt_support;
> >                 dev_priv->vbt.lvds_use_ssc = general->enable_ssc;
> >                 dev_priv->vbt.lvds_ssc_freq =
> >                         intel_bios_ssc_frequency(dev, general->ssc_freq);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e80387dd6582..68822395914b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14263,7 +14263,7 @@ static bool intel_crt_present(struct drm_device *dev)
> >         if (HAS_DDI(dev) && I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES)
> >                 return false;
> >
> > -       if (IS_VALLEYVIEW(dev) && !dev_priv->vbt.int_crt_support)
> > +       if (!dev_priv->vbt.int_crt_support)
> >                 return false;
> >
> >         return true;
> > --
> > 2.4.10
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

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