On Fri, Nov 15, 2013 at 12:39:32AM +0100, Daniel Vetter wrote: > On Thu, Nov 14, 2013 at 05:40:36PM -0200, Rodrigo Vivi wrote: > > 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> > > Queued for -next, thanks for the patch. Moved to -fixes on Olof's request. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
