On Thu, Nov 14, 2013 at 09:36:10AM -0800, Olof Johansson wrote:
On Thu, Nov 14, 2013 at 8:53 AM, Rodrigo Vivi rodrigo.vivi@gmail.com wrote:
On Wed, Nov 13, 2013 at 05:59:43PM -0800, Olof Johansson wrote:
From: Duncan Laurie dlaurie@chromium.org
We had been using a DMI table workaround to select the right frequency for devices, but this is fragile and must be updated with every new platform.
Instead the default case when VBT is missing is changed to use 120MHz clock for LVDS SSC for these generations.
The docs for 2010-Core, SandyBridge, and IvyBridge all indicate that the reference frequency for LVDS is 120MHz:
"2010 Core" http://intellinuxgraphics.org/IHD_OS_Vol3_Part3r2.pdf page 38 Reference Frequency: 120MHz for CRT and LVDS. 100MHz for the FDI.
"2011 SandyBridge" http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol3_Part3.pdf page 33 Reference Frequency: 120MHz for CRT, HDMI, LVDS. 100MHz for the FDI.
"2012 IvyBridge" http://intellinuxgraphics.org/documentation/IVB/IHD_OS_Vol3_Part4.pdf page 27 Reference Frequency: 120 MHz for CRT, HDMI, LVDS, 100MHz for the FDI.
Checked. You are right. And actually true even for HSW and BDW. 120 is the default and 100 is for test mode.
Yeah, the patch predates HSW/BDW, so it was not mentioned.
Signed-off-by: Duncan Laurie dlaurie@chromium.org [olof: Fixup for recent base, switched from if/else to single call] Signed-off-by: Olof Johansson olof@lixom.net
Daniel,
This applies on top of -next, which I'm presuming is close to your for-3.13 base right now. It'd be good to see this go in since it's needed to boot on Chromebooks (with developer mode off), and is thus blocking testing next/mainline on a regular basis here.
Thanks!
-Olof
drivers/gpu/drm/i915/intel_bios.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 6dd622d733b9..e4fba39631a5 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -790,7 +790,12 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
/* Default to using SSC */ dev_priv->vbt.lvds_use_ssc = 1;
dev_priv->vbt.lvds_ssc_freq = intel_bios_ssc_frequency(dev, 1);
/** Core/SandyBridge/IvyBridge use alternative (120MHz) reference* clock for LVDS.*/dev_priv->vbt.lvds_ssc_freq = intel_bios_ssc_frequency(dev,!HAS_PCH_SPLIT(dev));I'm just not convinced this is the right way to fix this here. Mainly because for most of platforms the alternate is 100 and default is 120. The ideal in my opinion should be to invert the alternate inside ssc_freqeuncy function and use the exception, that is probably IS_PINEVIEW(dev)...
Not sure though... just a guess since this alternate was implemented for pineview...
Seeing the history of some of this code (changes, followed by reverts, etc), I wanted to stay conservative with what we know works from a few years in the field by now.
Stay conservative is good, but what I don't like is just the inverted logic... or maybe just the "alternate" name.
The vbt defaults are only used by Chromebooks, as far as I know, so it's not a code path shared with other platforms today.
Are you sure? I don't know who is really setting vbt or going to defaults, but it is possible to have any one using it right now... anyway another reason to stay conservative...
Also, the ssc_freq bit from the vbios table is passed into this function, so I don't think there's much point in reversing the logic in there just for one of the two code paths.
yeah, maybe just the name alternate is killing me here...
Finally, from elsewhere in the same file:
/*
- The genX designation typically refers to the render engine, so render
- capability related checks should use IS_GEN, while display and other checks
- have their own (e.g. HAS_PCH_SPLIT for ILK+ display, IS_foo for
particular
- chips, etc.).
*/
with this I agree..
Since this is display and not rendering related, that seems like the right thing to base it on instead of generation tests.
I'm open for suggestions on improvements though, of course.
I don't have a real suggestion here... only bikesheds... maybe on the alternate name ;)
But either way, it seems to work and right since default is 120 for those platforms... So, feel free to use: Reviewed-by: Rodrigo Vivi rodrigo.vivi@gmail.com
-Olof
Cheers, Rodrigo.