| Message ID | 20180504141349.5878-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 04 May 2018, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > VBT seems to have some bits to tell us whether the internal LVDS port > has something hooked up. In theory one might expect the VBT to not have > a child device for the LVDS port if there's no panel hooked up, but > in practice many VBTs still add the child device. The "LVDS config" bits > seem more reliable though, so let's check those. > > So far we've used the "LVDS config" bits to check for eDP support on > ILK+, and disable the internal LVDS when the value is 3. That value > is actually documented as "Both internal LVDS and SDVO LVDS", but in > practice it looks to mean "eDP" on all the ilk+ VBTs I've seen. So let's > keep that interpretation, but for pre-ILK we will consider the value > 3 to also indicate the presence of the internal LVDS. > > Currently we have 25 DMI matches for the "no internal LVDS" quirk. In an > effort to reduce that let's toss in a WARN when the DMI match and VBT > both tell us that the internal LVDS is not present. The hope is that > people will report a bug, and then we can just nuke the corresponding > entry from the DMI quirk list. Credits to Jani for this idea. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ondrej Zary <linux@rainbow-software.org> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105468 > References: https://lists.freedesktop.org/archives/intel-gfx/2018-March/158408.html > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 20 ++++++++++++++++++-- > drivers/gpu/drm/i915/intel_lvds.c | 14 +++++++++----- > drivers/gpu/drm/i915/intel_vbt_defs.h | 1 + > 4 files changed, 29 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 04e27806e581..ffd0c8d33336 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1052,6 +1052,7 @@ struct intel_vbt_data { > unsigned int lvds_vbt:1; > unsigned int int_crt_support:1; > unsigned int lvds_use_ssc:1; > + unsigned int int_lvds_support:1; > unsigned int display_clock_mode:1; > unsigned int fdi_rx_polarity_inverted:1; > unsigned int panel_type:4; > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > index 702d3fab97fc..f75541b11f11 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -518,8 +518,21 @@ parse_driver_features(struct drm_i915_private *dev_priv, > if (!driver) > return; > I like the changes in intel_lvds_init(), particularly the fact that the eDP check is abstracted away from there. But the stuff below is really subtle. I'll walk through it with commentary. > - if (driver->lvds_config == BDB_DRIVER_FEATURE_EDP) > - dev_priv->vbt.edp.support = 1; > + if (INTEL_GEN(dev_priv) >= 5) { Okay, so the eDP check in intel_lvds_init() is wrapped in HAS_PCH_SPLIT(), but VLV/CHV don't have LVDS so switching to a gen 5 check here is fine. We also only have eDP since gen 5. So far so good. > + if (driver->lvds_config != BDB_DRIVER_FEATURE_INT_LVDS) > + dev_priv->vbt.int_lvds_support = 0; Here's the change for IBX/CPT (the only gen 5+ that can have LVDS). It used to be that value 3 disabled LVDS, but now 0, 2 and 3 disable LVDS. It stands to reason 0 and 2 disable LVDS, but looking at the spec, it seems odd that value 3 disables LVDS... So for IBX/CPT we disable LVDS more per VBT. > + > + /* > + * Note that the VBT spec doesn't agree with this > + * interpretation, but real world VBTs seem to. > + */ > + if (driver->lvds_config == BDB_DRIVER_FEATURE_EDP) > + dev_priv->vbt.edp.support = 1; In a surprise twist, this is now useless. We could remove it altogether, as it was only ever used to disable LVDS. And some likely useless logging. Really. Maybe then we should throw BDB_DRIVER_FEATURE_EDP macro out in favor of BDB_DRIVER_FEATURE_INT_SDVO_LVDS too, to match the spec. > + } else { > + if (driver->lvds_config != BDB_DRIVER_FEATURE_INT_LVDS && > + driver->lvds_config != BDB_DRIVER_FEATURE_INT_SDVO_LVDS) > + dev_priv->vbt.int_lvds_support = 0; For pre-gen5 we start looking at VBT for clues. We enable LVDS for values 1 and 3, disable otherwise. This matches the spec (unlike for gen5+). This is likely the useful change for the referenced bug, and many of the quirks. Overall I like the direction here, very much, but at the same time I'm wondering if we should split this up a bit just for caution. First part, all the changes in intel_lvds_init() you have, as is, and throwing edp.support out, but keeping the same logic for LVDS vs. no LVDS. Second part, actually changing the logic here for setting int_lvds_support. Minimizes and localizes the logic changing patch in case we need to revert it. How does that sound? Too wimpy? ;) BR, Jani. > + } > > DRM_DEBUG_KMS("DRRS State Enabled:%d\n", driver->drrs_enabled); > /* > @@ -1512,6 +1525,9 @@ init_vbt_defaults(struct drm_i915_private *dev_priv) > dev_priv->vbt.int_tv_support = 1; > dev_priv->vbt.int_crt_support = 1; > > + /* driver features */ > + dev_priv->vbt.int_lvds_support = 1; > + > /* Default to using SSC */ > dev_priv->vbt.lvds_use_ssc = 1; > /* > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index d35d2d50f595..f6596c5ce973 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -962,8 +962,16 @@ void intel_lvds_init(struct drm_i915_private *dev_priv) > return; > > /* Skip init on machines we know falsely report LVDS */ > - if (dmi_check_system(intel_no_lvds)) > + if (dmi_check_system(intel_no_lvds)) { > + WARN(!dev_priv->vbt.int_lvds_support, > + "Useless DMI match. Internal LVDS support disabled by VBT\n"); > return; > + } > + > + if (!dev_priv->vbt.int_lvds_support) { > + DRM_DEBUG_KMS("Internal LVDS support disabled by VBT.\n"); > + return; > + } > > if (HAS_PCH_SPLIT(dev_priv)) > lvds_reg = PCH_LVDS; > @@ -975,10 +983,6 @@ void intel_lvds_init(struct drm_i915_private *dev_priv) > if (HAS_PCH_SPLIT(dev_priv)) { > if ((lvds & LVDS_DETECTED) == 0) > return; > - if (dev_priv->vbt.edp.support) { > - DRM_DEBUG_KMS("disable LVDS for eDP support\n"); > - return; > - } > } > > pin = GMBUS_PIN_PANEL; > diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h > index 458468237b5f..a1d5e39f9a00 100644 > --- a/drivers/gpu/drm/i915/intel_vbt_defs.h > +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h > @@ -635,6 +635,7 @@ struct bdb_sdvo_lvds_options { > #define BDB_DRIVER_FEATURE_NO_LVDS 0 > #define BDB_DRIVER_FEATURE_INT_LVDS 1 > #define BDB_DRIVER_FEATURE_SDVO_LVDS 2 > +#define BDB_DRIVER_FEATURE_INT_SDVO_LVDS 3 > #define BDB_DRIVER_FEATURE_EDP 3 > > struct bdb_driver_features {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 04e27806e581..ffd0c8d33336 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1052,6 +1052,7 @@ struct intel_vbt_data { unsigned int lvds_vbt:1; unsigned int int_crt_support:1; unsigned int lvds_use_ssc:1; + unsigned int int_lvds_support:1; unsigned int display_clock_mode:1; unsigned int fdi_rx_polarity_inverted:1; unsigned int panel_type:4; diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 702d3fab97fc..f75541b11f11 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -518,8 +518,21 @@ parse_driver_features(struct drm_i915_private *dev_priv, if (!driver) return; - if (driver->lvds_config == BDB_DRIVER_FEATURE_EDP) - dev_priv->vbt.edp.support = 1; + if (INTEL_GEN(dev_priv) >= 5) { + if (driver->lvds_config != BDB_DRIVER_FEATURE_INT_LVDS) + dev_priv->vbt.int_lvds_support = 0; + + /* + * Note that the VBT spec doesn't agree with this + * interpretation, but real world VBTs seem to. + */ + if (driver->lvds_config == BDB_DRIVER_FEATURE_EDP) + dev_priv->vbt.edp.support = 1; + } else { + if (driver->lvds_config != BDB_DRIVER_FEATURE_INT_LVDS && + driver->lvds_config != BDB_DRIVER_FEATURE_INT_SDVO_LVDS) + dev_priv->vbt.int_lvds_support = 0; + } DRM_DEBUG_KMS("DRRS State Enabled:%d\n", driver->drrs_enabled); /* @@ -1512,6 +1525,9 @@ init_vbt_defaults(struct drm_i915_private *dev_priv) dev_priv->vbt.int_tv_support = 1; dev_priv->vbt.int_crt_support = 1; + /* driver features */ + dev_priv->vbt.int_lvds_support = 1; + /* Default to using SSC */ dev_priv->vbt.lvds_use_ssc = 1; /* diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index d35d2d50f595..f6596c5ce973 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -962,8 +962,16 @@ void intel_lvds_init(struct drm_i915_private *dev_priv) return; /* Skip init on machines we know falsely report LVDS */ - if (dmi_check_system(intel_no_lvds)) + if (dmi_check_system(intel_no_lvds)) { + WARN(!dev_priv->vbt.int_lvds_support, + "Useless DMI match. Internal LVDS support disabled by VBT\n"); return; + } + + if (!dev_priv->vbt.int_lvds_support) { + DRM_DEBUG_KMS("Internal LVDS support disabled by VBT.\n"); + return; + } if (HAS_PCH_SPLIT(dev_priv)) lvds_reg = PCH_LVDS; @@ -975,10 +983,6 @@ void intel_lvds_init(struct drm_i915_private *dev_priv) if (HAS_PCH_SPLIT(dev_priv)) { if ((lvds & LVDS_DETECTED) == 0) return; - if (dev_priv->vbt.edp.support) { - DRM_DEBUG_KMS("disable LVDS for eDP support\n"); - return; - } } pin = GMBUS_PIN_PANEL; diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h index 458468237b5f..a1d5e39f9a00 100644 --- a/drivers/gpu/drm/i915/intel_vbt_defs.h +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h @@ -635,6 +635,7 @@ struct bdb_sdvo_lvds_options { #define BDB_DRIVER_FEATURE_NO_LVDS 0 #define BDB_DRIVER_FEATURE_INT_LVDS 1 #define BDB_DRIVER_FEATURE_SDVO_LVDS 2 +#define BDB_DRIVER_FEATURE_INT_SDVO_LVDS 3 #define BDB_DRIVER_FEATURE_EDP 3 struct bdb_driver_features {