[Intel-gfx] [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7

[Daniel Vetter]

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 at gmail.com> wrote:
> > > On Wed, Nov 13, 2013 at 05:59:43PM -0800, Olof Johansson wrote:
> > >> From: Duncan Laurie <dlaurie at 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 at chromium.org>
> > >> [olof: Fixup for recent base, switched from if/else to single call]
> > >> Signed-off-by: Olof Johansson <olof at 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 at gmail.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch