[Intel-gfx] [PATCH 03/10] drm/i915: Check VBT for CRT port presence on HSW/BDW

[Chris Wilson]

On Tue, Dec 01, 2015 at 03:57:37PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 01, 2015 at 01:41:49PM +0000, Chris Wilson wrote:
> > On Tue, Dec 01, 2015 at 03:08:34PM +0200, ville.syrjala at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at 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?
> > > 
> > > 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
> > > HSW desktop machines with actual CRT ports and both have
> > > int_crt_support==1 in their VBTs.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index e80387dd6582..29ea4c458ab3 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 ((HAS_DDI(dev) || IS_VALLEYVIEW(dev)) && !dev_priv->vbt.int_crt_support)
> > 
> > Would it not be better to move this knowledge to vbt if it is version
> > dependent?
> > 
> > 	vbt.int_crtc_support = (UNKNOWN, NOT_PRESENT, PRESENT)
> > 
> > then
> > 
> > 	if (dev_priv->vbt.int_crt_support == NOT_PRESENT)
> > >  		return false;
> 
> I'm not sure the UNKNWON value really buys us anything.

Don't you need a flag to say when the .int_crt_support can be trusted?

> But I suppose we
> could just do this:
> 
>         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;
> +               if (HAS_DDI(dev_priv) || IS_VALLEYIEW(dev_priv))
> +                       dev_priv->vbt.int_crt_support = general->int_crt_support;
> 
> The other option is to make it check the version. But I'm not sure if
> that's entirely safe. For extra paranoia I guess we could do both,
> just in case there's some early VLV/HSW out there with <155 VBT
> version.

I would, if only to get better documentation on the changes. I also
wouldn't put it past someone to build a new VBIOS with an old tool...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre