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@xxxxxxxxx> wrote: > > On Wed, Nov 13, 2013 at 05:59:43PM -0800, Olof Johansson wrote: > >> From: Duncan Laurie <dlaurie@xxxxxxxxxxxx> > >> > >> 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@xxxxxxxxxxxx> > >> [olof: Fixup for recent base, switched from if/else to single call] > >> Signed-off-by: Olof Johansson <olof@xxxxxxxxx> > >> --- > >> > >> 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@xxxxxxxxx> > > > -Olof Cheers, Rodrigo. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
