* [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7 @ 2013-11-14 1:59 Olof Johansson 2013-11-14 16:53 ` Rodrigo Vivi 0 siblings, 1 reply; 6+ messages in thread From: Olof Johansson @ 2013-11-14 1:59 UTC (permalink / raw) To: Daniel Vetter Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Duncan Laurie, Olof Johansson 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. 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)); DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq); for (port = PORT_A; port < I915_MAX_PORTS; port++) { -- 1.8.4.1.601.g02b3b1d ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7 2013-11-14 1:59 [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7 Olof Johansson @ 2013-11-14 16:53 ` Rodrigo Vivi 2013-11-14 17:36 ` Olof Johansson 0 siblings, 1 reply; 6+ messages in thread From: Rodrigo Vivi @ 2013-11-14 16:53 UTC (permalink / raw) To: Olof Johansson Cc: Daniel Vetter, intel-gfx, linux-kernel, dri-devel, Duncan Laurie 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. > > 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... > DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->vbt.lvds_ssc_freq); > > for (port = PORT_A; port < I915_MAX_PORTS; port++) { > -- > 1.8.4.1.601.g02b3b1d > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7 2013-11-14 16:53 ` Rodrigo Vivi @ 2013-11-14 17:36 ` Olof Johansson 2013-11-14 19:40 ` Rodrigo Vivi 0 siblings, 1 reply; 6+ messages in thread From: Olof Johansson @ 2013-11-14 17:36 UTC (permalink / raw) To: Rodrigo Vivi Cc: Daniel Vetter, intel-gfx, linux-kernel, DRI mailing list, Duncan Laurie 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. 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. 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. 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.). */ 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. -Olof ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7 2013-11-14 17:36 ` Olof Johansson @ 2013-11-14 19:40 ` Rodrigo Vivi 2013-11-14 23:39 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Rodrigo Vivi @ 2013-11-14 19:40 UTC (permalink / raw) To: Olof Johansson Cc: Daniel Vetter, intel-gfx, linux-kernel, DRI mailing list, Duncan Laurie 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7 2013-11-14 19:40 ` Rodrigo Vivi @ 2013-11-14 23:39 ` Daniel Vetter 2013-11-15 8:57 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2013-11-14 23:39 UTC (permalink / raw) To: Rodrigo Vivi Cc: Olof Johansson, Daniel Vetter, intel-gfx, linux-kernel, DRI mailing list, Duncan Laurie 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@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> Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7 2013-11-14 23:39 ` Daniel Vetter @ 2013-11-15 8:57 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2013-11-15 8:57 UTC (permalink / raw) To: Rodrigo Vivi, Olof Johansson, intel-gfx, linux-kernel, DRI mailing list, Duncan Laurie 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@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> > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-15 8:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-11-14 1:59 [PATCH] i915: Use 120MHz LVDS SSC clock for gen5/gen6/gen7 Olof Johansson 2013-11-14 16:53 ` Rodrigo Vivi 2013-11-14 17:36 ` Olof Johansson 2013-11-14 19:40 ` Rodrigo Vivi 2013-11-14 23:39 ` Daniel Vetter 2013-11-15 8:57 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®