[PATCH 0/2 v6] drm/i915: LVDS channel mode fixes

[PATCH 0/2 v6] drm/i915: LVDS channel mode fixes

[Takashi Iwai]

Hi,

this is yet another respin of the previous LVDS channel mode fix patches.

v1->v2: Fix the register for gen<=4
v2->v3: Check the resolution of the entry to be sure
v3->v4: Optimize the register reference; add a module option
v4->v5:
  Check whether the data points within lvds_fp_data block properly
  Refactor the code to return an lvds_fp_timing pointer
  Renamed the module option to lvds_channel_mode
  Changed the option permission to 0600 to allow changing dynamically
v5->v6:
  Use the BIOS value only when LVDS register is uninitialized at start

thanks

Takashi

[PATCH 1/2] drm/i915: Check VBIOS value for determining LVDS dual channel mode, too

[Takashi Iwai]

Currently i915 driver checks [PCH_]LVDS register bits to decide
whether to set up the dual-link or the single-link mode.  This relies
implicitly on that BIOS initializes the register properly at boot.
However, BIOS doesn't initialize it always.  When the machine is
booted with the closed lid, BIOS skips the LVDS reg initialization.
This ends up in blank output on a machine with a dual-link LVDS when
you open the lid after the boot.

This patch adds a workaround for that problem by checking the initial
LVDS register value in VBT.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 ++
 drivers/gpu/drm/i915/intel_bios.c    |   36 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   30 ++++++++++++++++++++++------
 3 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9689ca3..28397b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -376,6 +376,8 @@ typedef struct drm_i915_private {
 	unsigned int lvds_use_ssc:1;
 	unsigned int display_clock_mode:1;
 	int lvds_ssc_freq;
+	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
+	unsigned int lvds_val; /* used for checking LVDS channel mode */
 	struct {
 		int rate;
 		int lanes;
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 63880e2..f71cbd0 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -173,6 +173,28 @@ get_lvds_dvo_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data,
 	return (struct lvds_dvo_timing *)(entry + dvo_timing_offset);
 }
 
+/* get lvds_fp_timing entry
+ * this function may return NULL if the corresponding entry is invalid
+ */
+static const struct lvds_fp_timing *
+get_lvds_fp_timing(const struct bdb_header *bdb,
+		   const struct bdb_lvds_lfp_data *data,
+		   const struct bdb_lvds_lfp_data_ptrs *ptrs,
+		   int index)
+{
+	size_t data_ofs = (const u8 *)data - (const u8 *)bdb;
+	u16 data_size = ((const u16 *)data)[-1]; /* stored in header */
+	size_t ofs;
+
+	if (index >= ARRAY_SIZE(ptrs->ptr))
+		return NULL;
+	ofs = ptrs->ptr[index].fp_timing_offset;
+	if (ofs < data_ofs ||
+	    ofs + sizeof(struct lvds_fp_timing) > data_ofs + data_size)
+		return NULL;
+	return (const struct lvds_fp_timing *)((const u8 *)bdb + ofs);
+}
+
 /* Try to find integrated panel data */
 static void
 parse_lfp_panel_data(struct drm_i915_private *dev_priv,
@@ -182,6 +204,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 	const struct bdb_lvds_lfp_data *lvds_lfp_data;
 	const struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs;
 	const struct lvds_dvo_timing *panel_dvo_timing;
+	const struct lvds_fp_timing *fp_timing;
 	struct drm_display_mode *panel_fixed_mode;
 	int i, downclock;
 
@@ -243,6 +266,19 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 			      "Normal Clock %dKHz, downclock %dKHz\n",
 			      panel_fixed_mode->clock, 10*downclock);
 	}
+
+	fp_timing = get_lvds_fp_timing(bdb, lvds_lfp_data,
+				       lvds_lfp_data_ptrs,
+				       lvds_options->panel_type);
+	if (fp_timing) {
+		/* check the resolution, just to be sure */
+		if (fp_timing->x_res == panel_fixed_mode->hdisplay &&
+		    fp_timing->y_res == panel_fixed_mode->vdisplay) {
+			dev_priv->bios_lvds_val = fp_timing->lvds_reg_val;
+			DRM_DEBUG_KMS("VBT initial LVDS value %x\n",
+				      dev_priv->bios_lvds_val);
+		}
+	}
 }
 
 /* Try to find sdvo panel data */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f851db7..f5ed808 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -356,6 +356,27 @@ static const intel_limit_t intel_limits_ironlake_display_port = {
 	.find_pll = intel_find_pll_ironlake_dp,
 };
 
+static bool is_dual_link_lvds(struct drm_i915_private *dev_priv,
+			      unsigned int reg)
+{
+	unsigned int val;
+
+	if (dev_priv->lvds_val)
+		val = dev_priv->lvds_val;
+	else {
+		/* BIOS should set the proper LVDS register value at boot, but
+		 * in reality, it doesn't set the value when the lid is closed;
+		 * we need to check "the value to be set" in VBT when LVDS
+		 * register is uninitialized.
+		 */
+		val = I915_READ(reg);
+		if (!(val & ~LVDS_DETECTED))
+			val = dev_priv->bios_lvds_val;
+		dev_priv->lvds_val = val;
+	}
+	return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
+}
+
 static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
 						int refclk)
 {
@@ -364,8 +385,7 @@ static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
 	const intel_limit_t *limit;
 
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
-		if ((I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) ==
-		    LVDS_CLKB_POWER_UP) {
+		if (is_dual_link_lvds(dev_priv, PCH_LVDS)) {
 			/* LVDS dual channel */
 			if (refclk == 100000)
 				limit = &intel_limits_ironlake_dual_lvds_100m;
@@ -393,8 +413,7 @@ static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
 	const intel_limit_t *limit;
 
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
-		if ((I915_READ(LVDS) & LVDS_CLKB_POWER_MASK) ==
-		    LVDS_CLKB_POWER_UP)
+		if (is_dual_link_lvds(dev_priv, LVDS))
 			/* LVDS with dual channel */
 			limit = &intel_limits_g4x_dual_channel_lvds;
 		else
@@ -531,8 +550,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 		 * reliably set up different single/dual channel state, if we
 		 * even can.
 		 */
-		if ((I915_READ(LVDS) & LVDS_CLKB_POWER_MASK) ==
-		    LVDS_CLKB_POWER_UP)
+		if (is_dual_link_lvds(dev_priv, LVDS))
 			clock.p2 = limit->p2.p2_fast;
 		else
 			clock.p2 = limit->p2.p2_slow;
-- 
1.7.9.2

[PATCH 2/2] drm/i915: Add lvds_channel module option

[Takashi Iwai]

Add a new module optoin lvds_channel to specify the LVDS channel mode
explicitly instead of probing the LVDS register value set by BIOS.
This will be helpful when VBT is broken or incompatible with the
current code.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=42842

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.c      |    6 ++++++
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |    4 ++++
 3 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 308f819..92875f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -80,6 +80,12 @@ MODULE_PARM_DESC(lvds_downclock,
 		"Use panel (LVDS/eDP) downclocking for power savings "
 		"(default: false)");
 
+int i915_lvds_channel_mode __read_mostly;
+module_param_named(lvds_channel_mode, i915_lvds_channel_mode, int, 0600);
+MODULE_PARM_DESC(lvds_channel_mode,
+		 "Specify LVDS channel mode "
+		 "(0=probe BIOS [default], 1=single-channel, 2=dual-channel)");
+
 int i915_panel_use_ssc __read_mostly = -1;
 module_param_named(lvds_use_ssc, i915_panel_use_ssc, int, 0600);
 MODULE_PARM_DESC(lvds_use_ssc,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 28397b8..bff3817 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1015,6 +1015,7 @@ extern int i915_panel_ignore_lid __read_mostly;
 extern unsigned int i915_powersave __read_mostly;
 extern int i915_semaphores __read_mostly;
 extern unsigned int i915_lvds_downclock __read_mostly;
+extern int i915_lvds_channel_mode __read_mostly;
 extern int i915_panel_use_ssc __read_mostly;
 extern int i915_vbt_sdvo_panel_type __read_mostly;
 extern int i915_enable_rc6 __read_mostly;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f5ed808..b46bea0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -361,6 +361,10 @@ static bool is_dual_link_lvds(struct drm_i915_private *dev_priv,
 {
 	unsigned int val;
 
+	/* use the module option value if specified */
+	if (i915_lvds_channel_mode > 0)
+		return i915_lvds_channel_mode == 2;
+
 	if (dev_priv->lvds_val)
 		val = dev_priv->lvds_val;
 	else {
-- 
1.7.9.2

Re: [PATCH 1/2] drm/i915: Check VBIOS value for determining LVDS dual channel mode, too

[Daniel Vetter]

On Tue, Mar 20, 2012 at 01:07:05PM +0100, Takashi Iwai wrote:
> Currently i915 driver checks [PCH_]LVDS register bits to decide
> whether to set up the dual-link or the single-link mode.  This relies
> implicitly on that BIOS initializes the register properly at boot.
> However, BIOS doesn't initialize it always.  When the machine is
> booted with the closed lid, BIOS skips the LVDS reg initialization.
> This ends up in blank output on a machine with a dual-link LVDS when
> you open the lid after the boot.
> 
> This patch adds a workaround for that problem by checking the initial
> LVDS register value in VBT.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Reviewed-by: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=37742

Not tested by the reporter, but sounds exactly like the issue this patch
here tries to fix.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/2] drm/i915: Check VBIOS value for determining LVDS dual channel mode, too

[Paulo Zanoni]

2012/3/20 Takashi Iwai <tiwai@suse.de>:
>
> This patch adds a workaround for that problem by checking the initial
> LVDS register value in VBT.
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Reviewed-by: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Tested on 4 laptops. Two of them with docking stations so I didn't
need to open the lid, press the power button and then close the lid.

They were all working before the patch, and they keep working after
the patch. The test was basically booting with closed lid, then after
gdm appears on the vga monitor, open the lid and check if lvds still
works.

Tested-By: Paulo Zanoni <paulo.r.zanoni@intel.com>

-- 
Paulo Zanoni

Re: [PATCH 2/2] drm/i915: Add lvds_channel module option

[Daniel Vetter]

On Tue, Mar 20, 2012 at 01:07:06PM +0100, Takashi Iwai wrote:
> Add a new module optoin lvds_channel to specify the LVDS channel mode
> explicitly instead of probing the LVDS register value set by BIOS.
> This will be helpful when VBT is broken or incompatible with the
> current code.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=42842
> 
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> Reviewed-by: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

I've queued these two here for -next, thanks for the patches.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48