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

[Jani Nikula <jani.nikula@intel.com>]

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. 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
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx