Re: [Intel-gfx] [PATCH v3 2/3] drm/i915: Consult VBT "LVDS config" bits to determine whether internal LVDS is present

[Ville Syrjälä]

On Tue, May 08, 2018 at 05:08:35PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> 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.
> 
> v2: Split the basic int_lvds_support thing to a separate patch (Jani)
> v3: Rebase
> 
> Cc: Jani Nikula 
> Cc: Ondrej Zary 
> 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ä 
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 16 +---
>  drivers/gpu/drm/i915/intel_lvds.c |  5 -
>  drivers/gpu/drm/i915/intel_vbt_defs.h |  2 +-
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c 
> b/drivers/gpu/drm/i915/intel_bios.c
> index 62cc9d3e20f5..f3545b5aabbc 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -518,9 +518,19 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>   if (!driver)
>   return;
>  
> - if (INTEL_GEN(dev_priv) >= 5 &&
> - driver->lvds_config == BDB_DRIVER_FEATURE_EDP)
> - dev_priv->vbt.int_lvds_support = 0;
> + if (INTEL_GEN(dev_priv) >= 5) {
> + /*
> +  * Note that we consider BDB_DRIVER_FEATURE_INT_SDVO_LVDS
> +  * to mean "eDP". The VBT spec doesn't agree with that
> +  * interpretation, but real world VBTs seem to.
> +  */
> + if (driver->lvds_config != BDB_DRIVER_FEATURE_INT_LVDS)
> + dev_priv->vbt.int_lvds_support = 0;
> + } 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;

Ugh. Actually we can't do this. I have a 85x machine myself that
has an LVDS panel but the VBT LVDS config bits set to 0 :(

Looking at the revision history in some VBT spec, I see a note 
"0.92 | Add two definitions for VBT value of LVDS Active Config
(00b and 11b values defined) | 06/13/2005". Unfortunately no
information which VBT version this corresponds with.

I do have a 945gm machine with VBT version 134 that does have the
LVDS config bits correctly populated. I believe the 2005 date does
correspond pretty well with the release of 945 chipsets, so 945gm
might be the first platform where we can actually trust this.

So I think we either have to just scrap this patch or limit it
either to 945+ or VBT versions >= 134 (and we could refine that
later if we discover older versions that have the bits populated
already).

As to the bug report that started this whole thing, I think I'll
just go ahead an apply the DMI quirk patch as that's a 85x machine
and clearly we can't use the VBT information there.

> + }
>  
>   DRM_DEBUG_KMS("DRRS State Enabled:%d\n", driver->drrs_enabled);
>   /*
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c 
> b/drivers/gpu/drm/i915/intel_lvds.c
> index 05d012358df8..5e5a77927369 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -962,8 +962,11 @@ 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");
> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h 
> b/drivers/gpu/drm/i915/intel_vbt_defs.h
> index 

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915: Consult VBT "LVDS config" bits to determine whether internal LVDS is present

[Jani Nikula]

On Tue, 08 May 2018, Ville Syrjala  wrote:
> From: Ville Syrjälä 
>
> 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.
>
> v2: Split the basic int_lvds_support thing to a separate patch (Jani)
> v3: Rebase
>
> Cc: Jani Nikula 
> Cc: Ondrej Zary 
> 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ä 

Reviewed-by: Jani Nikula 

> ---
>  drivers/gpu/drm/i915/intel_bios.c | 16 +---
>  drivers/gpu/drm/i915/intel_lvds.c |  5 -
>  drivers/gpu/drm/i915/intel_vbt_defs.h |  2 +-
>  3 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c 
> b/drivers/gpu/drm/i915/intel_bios.c
> index 62cc9d3e20f5..f3545b5aabbc 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -518,9 +518,19 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>   if (!driver)
>   return;
>  
> - if (INTEL_GEN(dev_priv) >= 5 &&
> - driver->lvds_config == BDB_DRIVER_FEATURE_EDP)
> - dev_priv->vbt.int_lvds_support = 0;
> + if (INTEL_GEN(dev_priv) >= 5) {
> + /*
> +  * Note that we consider BDB_DRIVER_FEATURE_INT_SDVO_LVDS
> +  * to mean "eDP". The VBT spec doesn't agree with that
> +  * interpretation, but real world VBTs seem to.
> +  */
> + if (driver->lvds_config != BDB_DRIVER_FEATURE_INT_LVDS)
> + dev_priv->vbt.int_lvds_support = 0;
> + } 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);
>   /*
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c 
> b/drivers/gpu/drm/i915/intel_lvds.c
> index 05d012358df8..5e5a77927369 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -962,8 +962,11 @@ 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");
> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h 
> b/drivers/gpu/drm/i915/intel_vbt_defs.h
> index 458468237b5f..39c804624179 100644
> --- a/drivers/gpu/drm/i915/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
> @@ -635,7 +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_EDP   3
> +#define BDB_DRIVER_FEATURE_INT_SDVO_LVDS 3
>  
>  struct bdb_driver_features {
>   u8 boot_dev_algorithm:1;

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