On Fri, 17 Mar 2017, Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> wrote:
> Em Ter, 2017-03-14 às 11:09 +0200, Jani Nikula escreveu:
>> On Mon, 13 Mar 2017, Manasi Navare <manasi.d.navare@xxxxxxxxx> wrote:
>> >
>> > On Mon, Mar 13, 2017 at 11:55:31AM +0200, Jani Nikula wrote:
>> > >
>> > > On Sat, 11 Mar 2017, Manasi Navare <manasi.d.navare@xxxxxxxxx>
>> > > 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@xxxxxxxxx>
>> > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > > > > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> > > > > ---
>> > > > > 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@xxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
