[Intel-gfx] [PATCH 2/2] drm/i915/vbt: split out defaults that are set when there is no VBT

[Paulo Zanoni]

Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:
> On Mon, 13 Mar 2017, Manasi Navare <manasi.d.navare at intel.com> wrote:
> > 
> > On Mon, Mar 13, 2017 at 11:55:31AM +0200, Jani Nikula wrote:
> > > 
> > > On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare at intel.com>
> > > wrote:
> > > > 
> > > > On Fri, Mar 10, 2017 at 03:27:58PM +0200, Jani Nikula wrote:
> > > > > 
> > > > > The main thing are the DDI ports. If there's a VBT that says
> > > > > there are
> > > > > no outputs, we should trust that, and not have semi-random
> > > > > defaults. Unfortunately, the defaults have resulted in some
> > > > > Chromebooks
> > > > > without VBT to rely on this behaviour, so we split out the
> > > > > defaults for
> > > > > the missing VBT case.
> > > > > 
> > > > > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++++-
> > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_bios.c
> > > > > b/drivers/gpu/drm/i915/intel_bios.c
> > > > > index 710988d72253..639d45c1dd2e 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_bios.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > > > > @@ -1341,6 +1341,7 @@ parse_device_mapping(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	return;
> > > > >  }
> > > > >  
> > > > > +/* Common defaults which may be overridden by VBT. */
> > > > >  static void
> > > > >  init_vbt_defaults(struct drm_i915_private *dev_priv)
> > > > >  {
> > > > > @@ -1377,6 +1378,18 @@ init_vbt_defaults(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  			&dev_priv->vbt.ddi_port_info[port];
> > > > >  
> > > > >  		info->hdmi_level_shift =
> > > > > HDMI_LEVEL_SHIFT_UNKNOWN;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +/* Defaults to initialize only if there is no VBT. */
> > > > > +static void
> > > > > +init_vbt_missing_defaults(struct drm_i915_private *dev_priv)
> > > > > +{
> > > > > +	enum port port;
> > > > > +
> > > > > +	for (port = PORT_A; port < I915_MAX_PORTS; port++) {
> > > > > +		struct ddi_vbt_port_info *info =
> > > > > +			&dev_priv->vbt.ddi_port_info[port];
> > > > >  
> > > > >  		info->supports_dvi = (port != PORT_A && port
> > > > > != PORT_E);
> > > > >  		info->supports_hdmi = info->supports_dvi;
> > > > > @@ -1516,8 +1529,10 @@ void intel_bios_init(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  	parse_ddi_ports(dev_priv, bdb);
> > > > >  
> > > > >  out:
> > > > > -	if (!vbt)
> > > > > +	if (!vbt) {
> > > > >  		DRM_INFO("Failed to find VBIOS tables
> > > > > (VBT)\n");
> > > > > +		init_vbt_missing_defaults(dev_priv);
> > > > > +	}
> > > > 
> > > > So in case there is no VBT, this will set supports_DP flag on
> > > > Port A.
> > > > What is there is no VBT and there is no eDP on Port A?
> > > > In this case it will still try to link train on Port A and
> > > > fail..?
> > > > I am not sure if this case exists, but just a thought looking
> > > > at it.
> > > 
> > > It's possible the case exists, but the point is that the
> > > behaviour for
> > > the no-VBT case remains the same before and after this patch.
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > 
> > Ok agreed. In that case Reviewed-by: Manasi Navare
> > <manasi.d.navare at intel.com>
> 
> Pushed to drm-intel-next-queued, thanks for the review.
> 
> I really hope there are no machines out there that have a crippled
> VBT
> with no child device config. I guess we'll find out...

I have access to this very interesting machine with DDB version 163 and
a child device size config that's 1 instead of the expected 33.

So what happens here is that since the VBT is supposed to be valid we
don't end up calling init_vbt_missing_defauilts(). We return early from
parse_device_mapping(), which means we don't set vbt.child_dev_num,
which means that parse_ddi_port() returns early. So info->supports_*
stays false, and intel_ddi_init() fails.

Given your commit message it seems that we should properly be able to
distinguish between "VBT correctly says that there's no output" and
"VBT is drunk and should go home" in order to fix this problem.

I can confirm that reverting this patch makes display great again^w^w
work again. So unfortunately I'll have to call regression on this
patch.

> 
> BR,
> Jani.
> 
> 
> > 
> > 
> > Regards
> > Manasi
> > 
> >  
> > > 
> > > > 
> > > > If such a case does not exist, then this will solve our problem
> > > > of
> > > > current failures because leaving defaults on Port A. So in that
> > > > case
> > > > it lgtm.
> > > > 
> > > > Regards
> > > > Manasi
> > > > 
> > > > 
> > > > > 
> > > > >  
> > > > >  	if (bios)
> > > > >  		pci_unmap_rom(pdev, bios);
> > > > > -- 
> > > > > 2.1.4
> > > > > 
> > > 
> > > -- 
> > > Jani Nikula, Intel Open Source Technology Center
>