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

Mon Mar 20 20:47:48 UTC 2017

On Mon, Mar 20, 2017 at 01:57:51PM -0300, Paulo Zanoni wrote:
> Em Seg, 2017-03-20 às 11:38 +0200, Jani Nikula escreveu:
> > On Fri, 17 Mar 2017, Paulo Zanoni <paulo.r.zanoni at intel.com> wrote:
> > > 
> > > 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'm not sure it's possible to distinguish between the two. I thought
> > we'd be able to rely on the former.
> 
> Are you sure? The code I'm hitting is that child_dev_size != expected
> size, which translates to "we failed to parse the VBT". Is this exactly
> what you see in the no-output machines? I would expect no-output
> machines would have a VBT that actually parses correctly but
> intentionally leaves you with child_device_num = 0.
> 
> Then, what we'd need to do would be to make those parse_something
> functions return error codes when the VBT was malformed, so we'd be
> able to differentiate between intentional child_dev_num=0 or parse
> failures.
> 
>

In the no-output machine cases, like the SKL desktop with on eDP on
Port A, it reports child_device_num=0 and so we should not call the
init_vbt_missing_defaults() function at all. I think we need a way to find
if the VBT was malformed/parese error.

Regards
Manasi

> >  If we have to change
> > "init_vbt_missing_defaults" to "init_child_dev_missing_defaults",
> > then
> > we'll never be able to handle the case where vbt correctly states
> > there
> > are no child devices. :(
> > 
> > BR,
> > Jani.
> > 
> > 
> > > 
> > > 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
> > > > 
> > 