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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Seg, 2017-03-20 às 11:38 +0200, Jani Nikula escreveu:
> On Fri, 17 Mar 2017, Paulo Zanoni <paulo.r.zanoni@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@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@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@intel.com>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Jani Nikula <jani.nikula@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@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.


>  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
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx