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

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

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.