* Thinkpad T420 and single/dual channel lvds @ 2012-03-14 12:37 Helge Bahmann 2012-03-14 14:45 ` Adam Jackson 0 siblings, 1 reply; 15+ messages in thread From: Helge Bahmann @ 2012-03-14 12:37 UTC (permalink / raw) To: intel-gfx; +Cc: hcb Hi everyone, Booting a Thinkpad T420 with the lid closed results and opening it subsequently results in an unusable picture on the panel (and no amount of resetting makes it usable): The even pixels show the nominal content of the framebuffer (with the right half missing) whlie the odd pixels show a red-/blueish flicker. A little bit of investigation revealed that with the lid closed on boot, the panel ends up driven in lvds single channel mode, which the panel probably does not like. It appears that the i915 driver relies on the BIOS setting up the PCH_LVDS register for dual channel and will otherwise not ever consider switching. While the following hack makes my display work, it is quite obviously not the right thing to do -- could you investigate if there is a "proper" way to determine whether the panel is supposed to be driven dual channel? Best regards Helge diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f851db7..2e51dc3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -364,8 +364,8 @@ 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 (1 || ((I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == + LVDS_CLKB_POWER_UP)) { /* LVDS dual channel */ if (refclk == 100000) limit = &intel_limits_ironlake_dual_lvds_100m; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-14 12:37 Thinkpad T420 and single/dual channel lvds Helge Bahmann @ 2012-03-14 14:45 ` Adam Jackson 2012-03-14 16:44 ` Takashi Iwai 0 siblings, 1 reply; 15+ messages in thread From: Adam Jackson @ 2012-03-14 14:45 UTC (permalink / raw) To: Helge Bahmann; +Cc: intel-gfx, hcb [-- Attachment #1.1: Type: text/plain, Size: 1268 bytes --] On Wed, 2012-03-14 at 13:37 +0100, Helge Bahmann wrote: > Hi everyone, > > Booting a Thinkpad T420 with the lid closed results and opening it subsequently results in an > unusable picture on the panel (and no amount of resetting makes it usable): The even pixels show > the nominal content of the framebuffer (with the right half missing) whlie the odd pixels show a > red-/blueish flicker. > > A little bit of investigation revealed that with the lid closed on boot, the panel ends up driven in > lvds single channel mode, which the panel probably does not like. It appears that the i915 driver > relies on the BIOS setting up the PCH_LVDS register for dual channel and will otherwise not ever > consider switching. > > While the following hack makes my display work, it is quite obviously not the right thing to do -- > could you investigate if there is a "proper" way to determine whether the panel is supposed to be > driven dual channel? Nice find! There may or may not be a bit for this in the VBT in the BIOS. But the more reliably correct thing I suspect would be to just look at the preferred mode for the panel and assume it's dual-link LVDS if the pixel clock is >112MHz, since that's the crossover frequency. - ajax [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-14 14:45 ` Adam Jackson @ 2012-03-14 16:44 ` Takashi Iwai 2012-03-14 18:09 ` Adam Jackson 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2012-03-14 16:44 UTC (permalink / raw) To: Adam Jackson; +Cc: intel-gfx, hcb At Wed, 14 Mar 2012 10:45:06 -0400, Adam Jackson wrote: > > [1 <multipart/signed (7bit)>] > [1.1 <text/plain; UTF-8 (quoted-printable)>] > On Wed, 2012-03-14 at 13:37 +0100, Helge Bahmann wrote: > > Hi everyone, > > > > Booting a Thinkpad T420 with the lid closed results and opening it subsequently results in an > > unusable picture on the panel (and no amount of resetting makes it usable): The even pixels show > > the nominal content of the framebuffer (with the right half missing) whlie the odd pixels show a > > red-/blueish flicker. > > > > A little bit of investigation revealed that with the lid closed on boot, the panel ends up driven in > > lvds single channel mode, which the panel probably does not like. It appears that the i915 driver > > relies on the BIOS setting up the PCH_LVDS register for dual channel and will otherwise not ever > > consider switching. > > > > While the following hack makes my display work, it is quite obviously not the right thing to do -- > > could you investigate if there is a "proper" way to determine whether the panel is supposed to be > > driven dual channel? > > Nice find! > > There may or may not be a bit for this in the VBT in the BIOS. But the > more reliably correct thing I suspect would be to just look at the > preferred mode for the panel and assume it's dual-link LVDS if the pixel > clock is >112MHz, since that's the crossover frequency. Coincidently, we hit the same issue with a HP laptop, and wondered how is the best way to fix. I hoped BIOS could handle better, i.e. setting the power bits no matter whether the lid is opened or not. But it doesn't set unless the lid is once opened. (Interestingly, the bits remain even if you close the lid again before booting. Just opening once seems triggering the probing of LVDS panel in BIOS and let it setting the right values.) FWIW, when I check ironalke_crtc_mode_set(), the clock of the HD+ LVDS mode (1600x900) is 107800 (refclk 120000), while a similar machine with a HD panel (1366x768) shows 76300. So, I'm not sure whether 112MHz could be a right threshold. thanks, Takashi ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-14 16:44 ` Takashi Iwai @ 2012-03-14 18:09 ` Adam Jackson 2012-03-15 13:15 ` Takashi Iwai 0 siblings, 1 reply; 15+ messages in thread From: Adam Jackson @ 2012-03-14 18:09 UTC (permalink / raw) To: Takashi Iwai; +Cc: intel-gfx, hcb [-- Attachment #1.1: Type: text/plain, Size: 1338 bytes --] On Wed, 2012-03-14 at 17:44 +0100, Takashi Iwai wrote: > At Wed, 14 Mar 2012 10:45:06 -0400, > Adam Jackson wrote: > > There may or may not be a bit for this in the VBT in the BIOS. But the > > more reliably correct thing I suspect would be to just look at the > > preferred mode for the panel and assume it's dual-link LVDS if the pixel > > clock is >112MHz, since that's the crossover frequency. > > Coincidently, we hit the same issue with a HP laptop, and wondered how > is the best way to fix. I hoped BIOS could handle better, > i.e. setting the power bits no matter whether the lid is opened or > not. But it doesn't set unless the lid is once opened. > > (Interestingly, the bits remain even if you close the lid again before > booting. Just opening once seems triggering the probing of LVDS > panel in BIOS and let it setting the right values.) > > FWIW, when I check ironalke_crtc_mode_set(), the clock of the HD+ > LVDS mode (1600x900) is 107800 (refclk 120000), while a similar > machine with a HD panel (1366x768) shows 76300. So, I'm not sure > whether 112MHz could be a right threshold. Hm, fair point. Would be interesting to compare the VBTs between the two boots. The lvds_fp_timing struct might actually have the data we want here regardless of how the machine booted. - ajax [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 198 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-14 18:09 ` Adam Jackson @ 2012-03-15 13:15 ` Takashi Iwai 2012-03-15 13:25 ` Chris Wilson 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2012-03-15 13:15 UTC (permalink / raw) To: Adam Jackson; +Cc: intel-gfx, hcb At Wed, 14 Mar 2012 14:09:19 -0400, Adam Jackson wrote: > > On Wed, 2012-03-14 at 17:44 +0100, Takashi Iwai wrote: > > At Wed, 14 Mar 2012 10:45:06 -0400, > > Adam Jackson wrote: > > > There may or may not be a bit for this in the VBT in the BIOS. But the > > > more reliably correct thing I suspect would be to just look at the > > > preferred mode for the panel and assume it's dual-link LVDS if the pixel > > > clock is >112MHz, since that's the crossover frequency. > > > > Coincidently, we hit the same issue with a HP laptop, and wondered how > > is the best way to fix. I hoped BIOS could handle better, > > i.e. setting the power bits no matter whether the lid is opened or > > not. But it doesn't set unless the lid is once opened. > > > > (Interestingly, the bits remain even if you close the lid again before > > booting. Just opening once seems triggering the probing of LVDS > > panel in BIOS and let it setting the right values.) > > > > FWIW, when I check ironalke_crtc_mode_set(), the clock of the HD+ > > LVDS mode (1600x900) is 107800 (refclk 120000), while a similar > > machine with a HD panel (1366x768) shows 76300. So, I'm not sure > > whether 112MHz could be a right threshold. > > Hm, fair point. > > Would be interesting to compare the VBTs between the two boots. The > lvds_fp_timing struct might actually have the data we want here > regardless of how the machine booted. It seems that BIOS doesn't change VTBs at boot no matter whether the lid is opened or not. It just skips the LVDS initialization in it. I dumbed the VBIOS data in both lid-open and close cases, and they are identical. Then I strated looking at VBT entries, and compared the outputs of two similar machines with different resolutions. Then I found out that a different value is set in lvds_fp_timing.lvds_reg_val for the default panel mode indeed. (This isn't set in all entries but only in the native one.) The patch below is to add a check of this LVDS reg value in VBT. It worked on a couple of HP laptops here. Can anyone check whether it works (and doesn't regress) on other machines, preferably from other vendors? Ideally it would be safer if we can verify the initial LVDS value in VBT more strictly... thanks, Takashi --- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] drm/i915: Check VBIOS value for determining LVDS dual channel mode, too 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. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_bios.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++------ 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9689ca3..8c8e488 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -376,6 +376,7 @@ 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 */ 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..e04f06d 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -173,6 +173,25 @@ get_lvds_dvo_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data, return (struct lvds_dvo_timing *)(entry + dvo_timing_offset); } +/* read the initial LVDS register value for the given panel mode */ +static unsigned int get_lvds_reg_val(const struct bdb_header *bdb, + const struct bdb_lvds_lfp_data_ptrs *ptrs, + int index) +{ + unsigned int ofs; + const struct lvds_fp_timing *timing; + + if (index >= ARRAY_SIZE(ptrs->ptr)) + return 0; + ofs = ptrs->ptr[index].fp_timing_offset; + if (ofs + sizeof(*timing) > bdb->bdb_size) + return 0; + timing = (const struct lvds_fp_timing *)((const u8 *)bdb + ofs); + if (timing->lvds_reg_val == -1) + return 0; /* just to be sure */ + return timing->lvds_reg_val; +} + /* Try to find integrated panel data */ static void parse_lfp_panel_data(struct drm_i915_private *dev_priv, @@ -243,6 +262,9 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, "Normal Clock %dKHz, downclock %dKHz\n", panel_fixed_mode->clock, 10*downclock); } + + dev_priv->bios_lvds_val = get_lvds_reg_val(bdb, lvds_lfp_data_ptrs, + lvds_options->panel_type); } /* 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..2a61dab 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -356,6 +356,23 @@ 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) +{ + /* BIOS should set the proper LVDS register value at boot, but + * in reality, it doesn't set the value when the lid is closed; + * thus when a machine is booted with the lid closed, the LVDS + * reg value can't be trusted. So we need to check "the value + * to be set" in VBT at first. + */ + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == + LVDS_CLKB_POWER_UP) + return true; + if ((I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == + LVDS_CLKB_POWER_UP) + return true; + return false; +} + static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc, int refclk) { @@ -364,8 +381,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)) { /* LVDS dual channel */ if (refclk == 100000) limit = &intel_limits_ironlake_dual_lvds_100m; @@ -393,8 +409,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 with dual channel */ limit = &intel_limits_g4x_dual_channel_lvds; else @@ -531,8 +546,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)) clock.p2 = limit->p2.p2_fast; else clock.p2 = limit->p2.p2_slow; -- 1.7.9.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-15 13:15 ` Takashi Iwai @ 2012-03-15 13:25 ` Chris Wilson 2012-03-15 13:30 ` Takashi Iwai 0 siblings, 1 reply; 15+ messages in thread From: Chris Wilson @ 2012-03-15 13:25 UTC (permalink / raw) To: Takashi Iwai, Adam Jackson; +Cc: intel-gfx, hcb On Thu, 15 Mar 2012 14:15:54 +0100, Takashi Iwai <tiwai@suse.de> wrote: > +static bool is_dual_link_lvds(struct drm_i915_private *dev_priv) > +{ > + /* BIOS should set the proper LVDS register value at boot, but > + * in reality, it doesn't set the value when the lid is closed; > + * thus when a machine is booted with the lid closed, the LVDS > + * reg value can't be trusted. So we need to check "the value > + * to be set" in VBT at first. > + */ > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > + LVDS_CLKB_POWER_UP) > + return true; > + if ((I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == This is either PCH_LVDS or LVDS depending on the generation. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-15 13:25 ` Chris Wilson @ 2012-03-15 13:30 ` Takashi Iwai 2012-03-15 14:42 ` Takashi Iwai 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2012-03-15 13:30 UTC (permalink / raw) To: Chris Wilson; +Cc: hcb, intel-gfx At Thu, 15 Mar 2012 13:25:08 +0000, Chris Wilson wrote: > > On Thu, 15 Mar 2012 14:15:54 +0100, Takashi Iwai <tiwai@suse.de> wrote: > > +static bool is_dual_link_lvds(struct drm_i915_private *dev_priv) > > +{ > > + /* BIOS should set the proper LVDS register value at boot, but > > + * in reality, it doesn't set the value when the lid is closed; > > + * thus when a machine is booted with the lid closed, the LVDS > > + * reg value can't be trusted. So we need to check "the value > > + * to be set" in VBT at first. > > + */ > > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > > + LVDS_CLKB_POWER_UP) > > + return true; > > + if ((I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == > > This is either PCH_LVDS or LVDS depending on the generation. Oh, right. The revised patch is below. thanks, Takashi --- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH v2] drm/i915: Check VBIOS value for determining LVDS dual channel mode, too 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. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- v1->v2: Fix the register for gen<=4 drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_bios.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++------ 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9689ca3..8c8e488 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -376,6 +376,7 @@ 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 */ 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..e04f06d 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -173,6 +173,25 @@ get_lvds_dvo_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data, return (struct lvds_dvo_timing *)(entry + dvo_timing_offset); } +/* read the initial LVDS register value for the given panel mode */ +static unsigned int get_lvds_reg_val(const struct bdb_header *bdb, + const struct bdb_lvds_lfp_data_ptrs *ptrs, + int index) +{ + unsigned int ofs; + const struct lvds_fp_timing *timing; + + if (index >= ARRAY_SIZE(ptrs->ptr)) + return 0; + ofs = ptrs->ptr[index].fp_timing_offset; + if (ofs + sizeof(*timing) > bdb->bdb_size) + return 0; + timing = (const struct lvds_fp_timing *)((const u8 *)bdb + ofs); + if (timing->lvds_reg_val == -1) + return 0; /* just to be sure */ + return timing->lvds_reg_val; +} + /* Try to find integrated panel data */ static void parse_lfp_panel_data(struct drm_i915_private *dev_priv, @@ -243,6 +262,9 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, "Normal Clock %dKHz, downclock %dKHz\n", panel_fixed_mode->clock, 10*downclock); } + + dev_priv->bios_lvds_val = get_lvds_reg_val(bdb, lvds_lfp_data_ptrs, + lvds_options->panel_type); } /* 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..314af26 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -356,6 +356,23 @@ 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) +{ + /* BIOS should set the proper LVDS register value at boot, but + * in reality, it doesn't set the value when the lid is closed; + * thus when a machine is booted with the lid closed, the LVDS + * reg value can't be trusted. So we need to check "the value + * to be set" in VBT at first. + */ + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == + LVDS_CLKB_POWER_UP) + return true; + if ((I915_READ(reg) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP) + return true; + return false; +} + static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc, int refclk) { @@ -364,8 +381,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 +409,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 +546,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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-15 13:30 ` Takashi Iwai @ 2012-03-15 14:42 ` Takashi Iwai 2012-03-16 15:33 ` Rodrigo Vivi 2012-03-16 19:55 ` Adam Jackson 0 siblings, 2 replies; 15+ messages in thread From: Takashi Iwai @ 2012-03-15 14:42 UTC (permalink / raw) To: Adam Jackson; +Cc: intel-gfx, hcb At Thu, 15 Mar 2012 14:30:35 +0100, Takashi Iwai wrote: > > At Thu, 15 Mar 2012 13:25:08 +0000, > Chris Wilson wrote: > > > > On Thu, 15 Mar 2012 14:15:54 +0100, Takashi Iwai <tiwai@suse.de> wrote: > > > +static bool is_dual_link_lvds(struct drm_i915_private *dev_priv) > > > +{ > > > + /* BIOS should set the proper LVDS register value at boot, but > > > + * in reality, it doesn't set the value when the lid is closed; > > > + * thus when a machine is booted with the lid closed, the LVDS > > > + * reg value can't be trusted. So we need to check "the value > > > + * to be set" in VBT at first. > > > + */ > > > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > > > + LVDS_CLKB_POWER_UP) > > > + return true; > > > + if ((I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == > > > > This is either PCH_LVDS or LVDS depending on the generation. > > Oh, right. The revised patch is below. One more change. Now it checks the resolution to be sure. Takashi --- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH v3] drm/i915: Check VBIOS value for determining LVDS dual channel mode, too 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. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- v1->v2: Fix the register for gen<=4 v2->v3: Check the resolution of the entry to be sure drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_bios.c | 26 ++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++------ 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9689ca3..8c8e488 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -376,6 +376,7 @@ 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 */ 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..6b93f92 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -173,6 +173,27 @@ get_lvds_dvo_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data, return (struct lvds_dvo_timing *)(entry + dvo_timing_offset); } +/* read the initial LVDS register value for the given panel mode */ +static unsigned int get_lvds_reg_val(const struct bdb_header *bdb, + const struct bdb_lvds_lfp_data_ptrs *ptrs, + int index, + struct drm_display_mode *mode) +{ + unsigned int ofs; + const struct lvds_fp_timing *timing; + + if (index >= ARRAY_SIZE(ptrs->ptr)) + return 0; + ofs = ptrs->ptr[index].fp_timing_offset; + if (ofs + sizeof(*timing) > bdb->bdb_size) + return 0; + timing = (const struct lvds_fp_timing *)((const u8 *)bdb + ofs); + /* check the resolution, just to be sure */ + if (timing->x_res != mode->hdisplay || timing->y_res != mode->vdisplay) + return 0; + return timing->lvds_reg_val; +} + /* Try to find integrated panel data */ static void parse_lfp_panel_data(struct drm_i915_private *dev_priv, @@ -243,6 +264,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, "Normal Clock %dKHz, downclock %dKHz\n", panel_fixed_mode->clock, 10*downclock); } + + dev_priv->bios_lvds_val = get_lvds_reg_val(bdb, lvds_lfp_data_ptrs, + lvds_options->panel_type, + panel_fixed_mode); + 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..314af26 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -356,6 +356,23 @@ 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) +{ + /* BIOS should set the proper LVDS register value at boot, but + * in reality, it doesn't set the value when the lid is closed; + * thus when a machine is booted with the lid closed, the LVDS + * reg value can't be trusted. So we need to check "the value + * to be set" in VBT at first. + */ + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == + LVDS_CLKB_POWER_UP) + return true; + if ((I915_READ(reg) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP) + return true; + return false; +} + static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc, int refclk) { @@ -364,8 +381,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 +409,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 +546,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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-15 14:42 ` Takashi Iwai @ 2012-03-16 15:33 ` Rodrigo Vivi 2012-03-16 19:55 ` Adam Jackson 1 sibling, 0 replies; 15+ messages in thread From: Rodrigo Vivi @ 2012-03-16 15:33 UTC (permalink / raw) To: Takashi Iwai; +Cc: hcb, intel-gfx Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> On Thu, Mar 15, 2012 at 11:42 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Thu, 15 Mar 2012 14:30:35 +0100, > Takashi Iwai wrote: >> >> At Thu, 15 Mar 2012 13:25:08 +0000, >> Chris Wilson wrote: >> > >> > On Thu, 15 Mar 2012 14:15:54 +0100, Takashi Iwai <tiwai@suse.de> wrote: >> > > +static bool is_dual_link_lvds(struct drm_i915_private *dev_priv) >> > > +{ >> > > + /* BIOS should set the proper LVDS register value at boot, but >> > > + * in reality, it doesn't set the value when the lid is closed; >> > > + * thus when a machine is booted with the lid closed, the LVDS >> > > + * reg value can't be trusted. So we need to check "the value >> > > + * to be set" in VBT at first. >> > > + */ >> > > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == >> > > + LVDS_CLKB_POWER_UP) >> > > + return true; >> > > + if ((I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == >> > >> > This is either PCH_LVDS or LVDS depending on the generation. >> >> Oh, right. The revised patch is below. > > One more change. Now it checks the resolution to be sure. > > > Takashi > > --- > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH v3] drm/i915: Check VBIOS value for determining LVDS dual channel mode, too > > 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. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > v1->v2: Fix the register for gen<=4 > v2->v3: Check the resolution of the entry to be sure > > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 26 ++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++------ > 3 files changed, 47 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 9689ca3..8c8e488 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -376,6 +376,7 @@ 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 */ > 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..6b93f92 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -173,6 +173,27 @@ get_lvds_dvo_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data, > return (struct lvds_dvo_timing *)(entry + dvo_timing_offset); > } > > +/* read the initial LVDS register value for the given panel mode */ > +static unsigned int get_lvds_reg_val(const struct bdb_header *bdb, > + const struct bdb_lvds_lfp_data_ptrs *ptrs, > + int index, > + struct drm_display_mode *mode) > +{ > + unsigned int ofs; > + const struct lvds_fp_timing *timing; > + > + if (index >= ARRAY_SIZE(ptrs->ptr)) > + return 0; > + ofs = ptrs->ptr[index].fp_timing_offset; > + if (ofs + sizeof(*timing) > bdb->bdb_size) > + return 0; > + timing = (const struct lvds_fp_timing *)((const u8 *)bdb + ofs); > + /* check the resolution, just to be sure */ > + if (timing->x_res != mode->hdisplay || timing->y_res != mode->vdisplay) > + return 0; > + return timing->lvds_reg_val; > +} > + > /* Try to find integrated panel data */ > static void > parse_lfp_panel_data(struct drm_i915_private *dev_priv, > @@ -243,6 +264,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, > "Normal Clock %dKHz, downclock %dKHz\n", > panel_fixed_mode->clock, 10*downclock); > } > + > + dev_priv->bios_lvds_val = get_lvds_reg_val(bdb, lvds_lfp_data_ptrs, > + lvds_options->panel_type, > + panel_fixed_mode); > + 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..314af26 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -356,6 +356,23 @@ 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) > +{ > + /* BIOS should set the proper LVDS register value at boot, but > + * in reality, it doesn't set the value when the lid is closed; > + * thus when a machine is booted with the lid closed, the LVDS > + * reg value can't be trusted. So we need to check "the value > + * to be set" in VBT at first. > + */ > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > + LVDS_CLKB_POWER_UP) > + return true; > + if ((I915_READ(reg) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP) > + return true; > + return false; > +} > + > static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc, > int refclk) > { > @@ -364,8 +381,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 +409,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 +546,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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br GPG: 0x905BE242 @ wwwkeys.pgp.net ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-15 14:42 ` Takashi Iwai 2012-03-16 15:33 ` Rodrigo Vivi @ 2012-03-16 19:55 ` Adam Jackson 2012-03-16 20:29 ` Takashi Iwai 1 sibling, 1 reply; 15+ messages in thread From: Adam Jackson @ 2012-03-16 19:55 UTC (permalink / raw) To: Takashi Iwai; +Cc: intel-gfx, hcb On 3/15/12 10:42 AM, Takashi Iwai wrote: > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f851db7..314af26 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -356,6 +356,23 @@ 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) > +{ > + /* BIOS should set the proper LVDS register value at boot, but > + * in reality, it doesn't set the value when the lid is closed; > + * thus when a machine is booted with the lid closed, the LVDS > + * reg value can't be trusted. So we need to check "the value > + * to be set" in VBT at first. > + */ > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > + LVDS_CLKB_POWER_UP) > + return true; Would slightly prefer if this was more like: if (dev_priv->bios_lvds_val) return !!(dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK); Since that way it eliminates some useless register reads in the normal case even for single-link. Not going to insist on it though. Looks really good otherwise, thanks! Reviewed-by: Adam Jackson <ajax@redhat.com> - ajax ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-16 19:55 ` Adam Jackson @ 2012-03-16 20:29 ` Takashi Iwai 2012-03-18 17:50 ` Daniel Vetter 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2012-03-16 20:29 UTC (permalink / raw) To: Adam Jackson; +Cc: intel-gfx, hcb At Fri, 16 Mar 2012 15:55:52 -0400, Adam Jackson wrote: > > On 3/15/12 10:42 AM, Takashi Iwai wrote: > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index f851db7..314af26 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -356,6 +356,23 @@ 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) > > +{ > > + /* BIOS should set the proper LVDS register value at boot, but > > + * in reality, it doesn't set the value when the lid is closed; > > + * thus when a machine is booted with the lid closed, the LVDS > > + * reg value can't be trusted. So we need to check "the value > > + * to be set" in VBT at first. > > + */ > > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > > + LVDS_CLKB_POWER_UP) > > + return true; > > Would slightly prefer if this was more like: > > if (dev_priv->bios_lvds_val) > return !!(dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK); > > Since that way it eliminates some useless register reads in the normal > case even for single-link. Not going to insist on it though. Sounds reasonable, yes. Also, it'd be good to have a module option to override the lvds channel setup, e.g. lvds_channel=0 for probing BIOS like this, lvds_channel=1 and 2 are to set the single and dual-channel mode forcibly, respectively. If you guys think it's worth, I'll write an additional patch after fixing this as suggested. > Looks really good otherwise, thanks! > > Reviewed-by: Adam Jackson <ajax@redhat.com> Thanks! Takashi ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-16 20:29 ` Takashi Iwai @ 2012-03-18 17:50 ` Daniel Vetter 2012-03-18 18:01 ` Andreas Heider 2012-03-18 20:24 ` Takashi Iwai 0 siblings, 2 replies; 15+ messages in thread From: Daniel Vetter @ 2012-03-18 17:50 UTC (permalink / raw) To: Takashi Iwai; +Cc: hcb, intel-gfx On Fri, Mar 16, 2012 at 09:29:24PM +0100, Takashi Iwai wrote: > At Fri, 16 Mar 2012 15:55:52 -0400, > Adam Jackson wrote: > > > > On 3/15/12 10:42 AM, Takashi Iwai wrote: > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index f851db7..314af26 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -356,6 +356,23 @@ 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) > > > +{ > > > + /* BIOS should set the proper LVDS register value at boot, but > > > + * in reality, it doesn't set the value when the lid is closed; > > > + * thus when a machine is booted with the lid closed, the LVDS > > > + * reg value can't be trusted. So we need to check "the value > > > + * to be set" in VBT at first. > > > + */ > > > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > > > + LVDS_CLKB_POWER_UP) > > > + return true; > > > > Would slightly prefer if this was more like: > > > > if (dev_priv->bios_lvds_val) > > return !!(dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK); > > > > Since that way it eliminates some useless register reads in the normal > > case even for single-link. Not going to insist on it though. > > Sounds reasonable, yes. > > Also, it'd be good to have a module option to override the lvds > channel setup, e.g. lvds_channel=0 for probing BIOS like this, > lvds_channel=1 and 2 are to set the single and dual-channel mode > forcibly, respectively. If you guys think it's worth, I'll write an > additional patch after fixing this as suggested. I think we can wait with adding debug options until this blows up. And if there are indeed broken bios out there, we need a full quirk table anyway. I'll wait for the new patch with Adam's suggestion for merging into -next. -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-18 17:50 ` Daniel Vetter @ 2012-03-18 18:01 ` Andreas Heider 2012-03-18 20:26 ` Takashi Iwai 2012-03-18 20:24 ` Takashi Iwai 1 sibling, 1 reply; 15+ messages in thread From: Andreas Heider @ 2012-03-18 18:01 UTC (permalink / raw) To: Daniel Vetter; +Cc: Takashi Iwai, intel-gfx, hcb Am 18.03.2012 um 18:50 schrieb Daniel Vetter: > On Fri, Mar 16, 2012 at 09:29:24PM +0100, Takashi Iwai wrote: >> At Fri, 16 Mar 2012 15:55:52 -0400, >> Adam Jackson wrote: >>> >>> On 3/15/12 10:42 AM, Takashi Iwai wrote: >>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index f851db7..314af26 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -356,6 +356,23 @@ 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) >>>> +{ >>>> + /* BIOS should set the proper LVDS register value at boot, but >>>> + * in reality, it doesn't set the value when the lid is closed; >>>> + * thus when a machine is booted with the lid closed, the LVDS >>>> + * reg value can't be trusted. So we need to check "the value >>>> + * to be set" in VBT at first. >>>> + */ >>>> + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == >>>> + LVDS_CLKB_POWER_UP) >>>> + return true; >>> >>> Would slightly prefer if this was more like: >>> >>> if (dev_priv->bios_lvds_val) >>> return !!(dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK); >>> >>> Since that way it eliminates some useless register reads in the normal >>> case even for single-link. Not going to insist on it though. >> >> Sounds reasonable, yes. >> >> Also, it'd be good to have a module option to override the lvds >> channel setup, e.g. lvds_channel=0 for probing BIOS like this, >> lvds_channel=1 and 2 are to set the single and dual-channel mode >> forcibly, respectively. If you guys think it's worth, I'll write an >> additional patch after fixing this as suggested. > > I think we can wait with adding debug options until this blows up. And if > there are indeed broken bios out there, we need a full quirk table anyway. > > I'll wait for the new patch with Adam's suggestion for merging into -next. > -Daniel Hi, I tried this patch on a Macbook Pro 6,2 and indeed still need to manually set lvds_channel=2. It'd be great if this patch with the override parameter or some other way to fix it would end up in mainline. Cheers, Andreas > -- > Daniel Vetter > Mail: daniel@ffwll.ch > Mobile: +41 (0)79 365 57 48 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-18 18:01 ` Andreas Heider @ 2012-03-18 20:26 ` Takashi Iwai 0 siblings, 0 replies; 15+ messages in thread From: Takashi Iwai @ 2012-03-18 20:26 UTC (permalink / raw) To: Andreas Heider; +Cc: intel-gfx, hcb At Sun, 18 Mar 2012 19:01:21 +0100, Andreas Heider wrote: > > Am 18.03.2012 um 18:50 schrieb Daniel Vetter: > > On Fri, Mar 16, 2012 at 09:29:24PM +0100, Takashi Iwai wrote: > >> At Fri, 16 Mar 2012 15:55:52 -0400, > >> Adam Jackson wrote: > >>> > >>> On 3/15/12 10:42 AM, Takashi Iwai wrote: > >>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>>> index f851db7..314af26 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_display.c > >>>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>>> @@ -356,6 +356,23 @@ 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) > >>>> +{ > >>>> + /* BIOS should set the proper LVDS register value at boot, but > >>>> + * in reality, it doesn't set the value when the lid is closed; > >>>> + * thus when a machine is booted with the lid closed, the LVDS > >>>> + * reg value can't be trusted. So we need to check "the value > >>>> + * to be set" in VBT at first. > >>>> + */ > >>>> + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > >>>> + LVDS_CLKB_POWER_UP) > >>>> + return true; > >>> > >>> Would slightly prefer if this was more like: > >>> > >>> if (dev_priv->bios_lvds_val) > >>> return !!(dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK); > >>> > >>> Since that way it eliminates some useless register reads in the normal > >>> case even for single-link. Not going to insist on it though. > >> > >> Sounds reasonable, yes. > >> > >> Also, it'd be good to have a module option to override the lvds > >> channel setup, e.g. lvds_channel=0 for probing BIOS like this, > >> lvds_channel=1 and 2 are to set the single and dual-channel mode > >> forcibly, respectively. If you guys think it's worth, I'll write an > >> additional patch after fixing this as suggested. > > > > I think we can wait with adding debug options until this blows up. And if > > there are indeed broken bios out there, we need a full quirk table anyway. > > > > I'll wait for the new patch with Adam's suggestion for merging into -next. > > -Daniel > > Hi, > > I tried this patch on a Macbook Pro 6,2 and indeed still need to manually set lvds_channel=2. OK, that's expected. Basically my patch would fix only the case where the default BIOS works but doesn't work in some exceptional case (e.g. boot with the closed lid). > It'd be great if this patch with the override parameter or some other way to fix it would end up in mainline. I think the module option would be good. It's needed to give an easy way for testing. Then we can add the entries in some quirk tables. thanks, Takashi ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Thinkpad T420 and single/dual channel lvds 2012-03-18 17:50 ` Daniel Vetter 2012-03-18 18:01 ` Andreas Heider @ 2012-03-18 20:24 ` Takashi Iwai 1 sibling, 0 replies; 15+ messages in thread From: Takashi Iwai @ 2012-03-18 20:24 UTC (permalink / raw) To: Daniel Vetter; +Cc: hcb, intel-gfx At Sun, 18 Mar 2012 18:50:31 +0100, Daniel Vetter wrote: > > On Fri, Mar 16, 2012 at 09:29:24PM +0100, Takashi Iwai wrote: > > At Fri, 16 Mar 2012 15:55:52 -0400, > > Adam Jackson wrote: > > > > > > On 3/15/12 10:42 AM, Takashi Iwai wrote: > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index f851db7..314af26 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -356,6 +356,23 @@ 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) > > > > +{ > > > > + /* BIOS should set the proper LVDS register value at boot, but > > > > + * in reality, it doesn't set the value when the lid is closed; > > > > + * thus when a machine is booted with the lid closed, the LVDS > > > > + * reg value can't be trusted. So we need to check "the value > > > > + * to be set" in VBT at first. > > > > + */ > > > > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > > > > + LVDS_CLKB_POWER_UP) > > > > + return true; > > > > > > Would slightly prefer if this was more like: > > > > > > if (dev_priv->bios_lvds_val) > > > return !!(dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK); > > > > > > Since that way it eliminates some useless register reads in the normal > > > case even for single-link. Not going to insist on it though. > > > > Sounds reasonable, yes. > > > > Also, it'd be good to have a module option to override the lvds > > channel setup, e.g. lvds_channel=0 for probing BIOS like this, > > lvds_channel=1 and 2 are to set the single and dual-channel mode > > forcibly, respectively. If you guys think it's worth, I'll write an > > additional patch after fixing this as suggested. > > I think we can wait with adding debug options until this blows up. And if > there are indeed broken bios out there, we need a full quirk table anyway. The module option is often the easiest way to check, so I believe it's a good thing. It won't break the current behavior, at least. > I'll wait for the new patch with Adam's suggestion for merging into -next. I'll respin the patches tomorrow with people's suggestions. thanks, Takashi ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-03-18 20:26 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-14 12:37 Thinkpad T420 and single/dual channel lvds Helge Bahmann 2012-03-14 14:45 ` Adam Jackson 2012-03-14 16:44 ` Takashi Iwai 2012-03-14 18:09 ` Adam Jackson 2012-03-15 13:15 ` Takashi Iwai 2012-03-15 13:25 ` Chris Wilson 2012-03-15 13:30 ` Takashi Iwai 2012-03-15 14:42 ` Takashi Iwai 2012-03-16 15:33 ` Rodrigo Vivi 2012-03-16 19:55 ` Adam Jackson 2012-03-16 20:29 ` Takashi Iwai 2012-03-18 17:50 ` Daniel Vetter 2012-03-18 18:01 ` Andreas Heider 2012-03-18 20:26 ` Takashi Iwai 2012-03-18 20:24 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.