[04/21] drm/i915: Only use VBT panel mode on eDP if no EDID is found
diff mbox series

Message ID 1317344993-24945-5-git-send-email-keithp@keithp.com
State New, archived
Headers show
Series
  • MacBook Air patch sequence (v2)
Related show

Commit Message

Keith Packard Sept. 30, 2011, 1:09 a.m. UTC
We're going to assume that EDID is more reliable than the VBT tables
for eDP panels, which is notably true on MacBook machines where the
VBT contains completely bogus data.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

Comments

Daniel Vetter Sept. 30, 2011, 4:32 p.m. UTC | #1
On Thu, Sep 29, 2011 at 06:09:36PM -0700, Keith Packard wrote:
> We're going to assume that EDID is more reliable than the VBT tables
> for eDP panels, which is notably true on MacBook machines where the
> VBT contains completely bogus data.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Ok, this is way over my head, just checked whether the patch does what it
claims to - nice exercise in reading dp modeset code ;-)
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Keith Packard Sept. 30, 2011, 5:58 p.m. UTC | #2
On Fri, 30 Sep 2011 18:32:35 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> Ok, this is way over my head, just checked whether the patch does what it
> claims to - nice exercise in reading dp modeset code ;-)

Yeah, it's purely heuristic -- the VBT contains a mode which was
originally for the LVDS. It's unclear whether it should ever be applied
to an eDP panel, but absent other information, it seems like we should
at least consider it.

Many LVDS panels don't bother to include an EDID rom, or the vendor
didn't bother to hook up the DDC wire; presumably it's cheaper for them
to stick more data in the VBIOS than add hardware.  However, there are
some LVDS panels with EDID roms which contain *incorrect* mode data for
the panel (amazing, I know), and so the driver prefers to use the VBT
data when both are present.

eDP, on the other hand, doesn't require any additional wiring (at least)
to connect up the DDC channel, and eDP panels are required to provide
EDID data. So far, in my (very small) sample set, I've got one machine
which provides accurate VBT *and* EDID data (an HP 2540p) and one
machine which provides inaccurate VBT data but accurate EDID data (a
MacBook air). So, I just decided to prefer the EDID data.

One option would be to extract the current mode from the hardware when
the driver starts and use that if present. But, that might mean that
you'd get different modes depending on whether the machine booted with
the lid closed or open, which seems like a bad plan.
Jesse Barnes Oct. 3, 2011, 8:42 p.m. UTC | #3
On Fri, 30 Sep 2011 10:58:26 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Fri, 30 Sep 2011 18:32:35 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > Ok, this is way over my head, just checked whether the patch does what it
> > claims to - nice exercise in reading dp modeset code ;-)
> 
> Yeah, it's purely heuristic -- the VBT contains a mode which was
> originally for the LVDS. It's unclear whether it should ever be applied
> to an eDP panel, but absent other information, it seems like we should
> at least consider it.
> 
> Many LVDS panels don't bother to include an EDID rom, or the vendor
> didn't bother to hook up the DDC wire; presumably it's cheaper for them
> to stick more data in the VBIOS than add hardware.  However, there are
> some LVDS panels with EDID roms which contain *incorrect* mode data for
> the panel (amazing, I know), and so the driver prefers to use the VBT
> data when both are present.
> 
> eDP, on the other hand, doesn't require any additional wiring (at least)
> to connect up the DDC channel, and eDP panels are required to provide
> EDID data. So far, in my (very small) sample set, I've got one machine
> which provides accurate VBT *and* EDID data (an HP 2540p) and one
> machine which provides inaccurate VBT data but accurate EDID data (a
> MacBook air). So, I just decided to prefer the EDID data.
> 
> One option would be to extract the current mode from the hardware when
> the driver starts and use that if present. But, that might mean that
> you'd get different modes depending on whether the machine booted with
> the lid closed or open, which seems like a bad plan.

More than that, I think eDP *requires* an EDID, and it sounds like even
the Air has one (and if any machine didn't, you know it would be an
Apple).

So I'm definitely in favor of this change.

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f0cfb6b..8ab2a88 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1748,7 +1748,16 @@  static int intel_dp_get_modes(struct drm_connector *connector)
 
 	/* if eDP has no EDID, try to use fixed panel mode from VBT */
 	if (is_edp(intel_dp)) {
-		if (dev_priv->panel_fixed_mode != NULL) {
+		/* initialize panel mode from VBT if available for eDP */
+		if (dev_priv->panel_fixed_mode == NULL && dev_priv->lfp_lvds_vbt_mode != NULL) {
+			dev_priv->panel_fixed_mode =
+				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
+			if (dev_priv->panel_fixed_mode) {
+				dev_priv->panel_fixed_mode->type |=
+					DRM_MODE_TYPE_PREFERRED;
+			}
+		}
+		if (dev_priv->panel_fixed_mode) {
 			struct drm_display_mode *mode;
 			mode = drm_mode_duplicate(dev, dev_priv->panel_fixed_mode);
 			drm_mode_probed_add(connector, mode);
@@ -2061,15 +2070,6 @@  intel_dp_init(struct drm_device *dev, int output_reg)
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
 	if (is_edp(intel_dp)) {
-		/* initialize panel mode from VBT if available for eDP */
-		if (dev_priv->lfp_lvds_vbt_mode) {
-			dev_priv->panel_fixed_mode =
-				drm_mode_duplicate(dev, dev_priv->lfp_lvds_vbt_mode);
-			if (dev_priv->panel_fixed_mode) {
-				dev_priv->panel_fixed_mode->type |=
-					DRM_MODE_TYPE_PREFERRED;
-			}
-		}
 		dev_priv->int_edp_connector = connector;
 		intel_panel_setup_backlight(dev);
 	}