[PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
up to 675 MHz on ULT, bu only if extra cooling is provided. There
don't seem to be any strap or VBT bits to tells us this however.

But I did spot something potentially relevant in
VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
knows what its doing and trust the max cdclk in SWF06 if it's higher
than the basic limit specified in Bspec. To avoid regressing anything
let's ignore SWF06 if it indicates a lower limit than Bspec.

Cc: Clint Taylor <clinton.a.taylor@intel.com>
Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---

I'm not at all sure if this is the right way to go about it. Sivakumar,
since you seem to have some ideas on this maybe you can have a look.
I'm not aware of any complaints so far that people can't get the cdclk
as high is they should on specific machines, so not sure if this is really
even needed.

The other open question is what we should do if the VBIOS limit is
lower than what we'd expect based on BSpec. Should we still trust it?
Sadly we can't verify the SWF06 cdclk value in any way since it
only has two bits and we have four possible cdclk values.

 drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 836bbdc239b6..1d70f6bf2934 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
 			dev_priv->max_cdclk_freq = 450000;
 		else
 			dev_priv->max_cdclk_freq = 337500;
-	} else if (IS_BROADWELL(dev))  {
+	} else if (IS_BROADWELL(dev)) {
+		int bios_max_cdclk_freq, max_cdclk_freq;
+
 		/*
-		 * FIXME with extra cooling we can allow
-		 * 540 MHz for ULX and 675 Mhz for ULT.
-		 * How can we know if extra cooling is
-		 * available? PCI ID, VTB, something else?
+		 * With extra cooling we can allow 540 MHz for
+		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
+		 * passes that information in SWF06.
 		 */
-		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
-			dev_priv->max_cdclk_freq = 450000;
-		else if (IS_BDW_ULX(dev))
-			dev_priv->max_cdclk_freq = 450000;
-		else if (IS_BDW_ULT(dev))
-			dev_priv->max_cdclk_freq = 540000;
-		else
-			dev_priv->max_cdclk_freq = 675000;
+		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
+		case 0:
+			bios_max_cdclk_freq = 450000;
+			break;
+		case 1:
+			bios_max_cdclk_freq = 540000;
+			break;
+		case 2:
+			bios_max_cdclk_freq = 337500;
+			break;
+		case 3:
+			bios_max_cdclk_freq = 675000;
+			break;
+		}
+
+		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
+			if (WARN_ON(bios_max_cdclk_freq != 450000))
+				bios_max_cdclk_freq = 450000;
+			max_cdclk_freq = 450000;
+		} else if (IS_BDW_ULX(dev_priv)) {
+			if (WARN_ON(bios_max_cdclk_freq > 540000))
+				bios_max_cdclk_freq = 540000;
+			max_cdclk_freq = 450000;
+		} else if (IS_BDW_ULT(dev_priv)) {
+			max_cdclk_freq = 540000;
+		} else {
+			max_cdclk_freq = 675000;
+		}
+
+		if (bios_max_cdclk_freq > max_cdclk_freq) {
+			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
+				      "assuming extra cooling is present\n",
+				      bios_max_cdclk_freq, max_cdclk_freq);
+			max_cdclk_freq = bios_max_cdclk_freq;
+		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
+			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
+				      "ignoring it\n",
+				      bios_max_cdclk_freq, max_cdclk_freq);
+		}
+
+		dev_priv->max_cdclk_freq = max_cdclk_freq;
 	} else if (IS_CHERRYVIEW(dev)) {
 		dev_priv->max_cdclk_freq = 320000;
 	} else if (IS_VALLEYVIEW(dev)) {
-- 
2.4.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[Ville Syrjälä]

On Fri, Feb 12, 2016 at 05:06:07PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
> up to 675 MHz on ULT, bu only if extra cooling is provided. There
> don't seem to be any strap or VBT bits to tells us this however.
> 
> But I did spot something potentially relevant in
> VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
> the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
> knows what its doing and trust the max cdclk in SWF06 if it's higher
> than the basic limit specified in Bspec. To avoid regressing anything
> let's ignore SWF06 if it indicates a lower limit than Bspec.
> 
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> 
> I'm not at all sure if this is the right way to go about it. Sivakumar,
> since you seem to have some ideas on this maybe you can have a look.
> I'm not aware of any complaints so far that people can't get the cdclk
> as high is they should on specific machines, so not sure if this is really
> even needed.
> 
> The other open question is what we should do if the VBIOS limit is
> lower than what we'd expect based on BSpec. Should we still trust it?
> Sadly we can't verify the SWF06 cdclk value in any way since it
> only has two bits and we have four possible cdclk values.

Oh, and I forgot to mention that this is totally untested.

> 
>  drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
>  1 file changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 836bbdc239b6..1d70f6bf2934 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>  			dev_priv->max_cdclk_freq = 450000;
>  		else
>  			dev_priv->max_cdclk_freq = 337500;
> -	} else if (IS_BROADWELL(dev))  {
> +	} else if (IS_BROADWELL(dev)) {
> +		int bios_max_cdclk_freq, max_cdclk_freq;
> +
>  		/*
> -		 * FIXME with extra cooling we can allow
> -		 * 540 MHz for ULX and 675 Mhz for ULT.
> -		 * How can we know if extra cooling is
> -		 * available? PCI ID, VTB, something else?
> +		 * With extra cooling we can allow 540 MHz for
> +		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
> +		 * passes that information in SWF06.
>  		 */
> -		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> -			dev_priv->max_cdclk_freq = 450000;
> -		else if (IS_BDW_ULX(dev))
> -			dev_priv->max_cdclk_freq = 450000;
> -		else if (IS_BDW_ULT(dev))
> -			dev_priv->max_cdclk_freq = 540000;
> -		else
> -			dev_priv->max_cdclk_freq = 675000;
> +		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
> +		case 0:
> +			bios_max_cdclk_freq = 450000;
> +			break;
> +		case 1:
> +			bios_max_cdclk_freq = 540000;
> +			break;
> +		case 2:
> +			bios_max_cdclk_freq = 337500;
> +			break;
> +		case 3:
> +			bios_max_cdclk_freq = 675000;
> +			break;
> +		}
> +
> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
> +			if (WARN_ON(bios_max_cdclk_freq != 450000))
> +				bios_max_cdclk_freq = 450000;
> +			max_cdclk_freq = 450000;
> +		} else if (IS_BDW_ULX(dev_priv)) {
> +			if (WARN_ON(bios_max_cdclk_freq > 540000))
> +				bios_max_cdclk_freq = 540000;
> +			max_cdclk_freq = 450000;
> +		} else if (IS_BDW_ULT(dev_priv)) {
> +			max_cdclk_freq = 540000;
> +		} else {
> +			max_cdclk_freq = 675000;
> +		}
> +
> +		if (bios_max_cdclk_freq > max_cdclk_freq) {
> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
> +				      "assuming extra cooling is present\n",
> +				      bios_max_cdclk_freq, max_cdclk_freq);
> +			max_cdclk_freq = bios_max_cdclk_freq;
> +		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
> +				      "ignoring it\n",
> +				      bios_max_cdclk_freq, max_cdclk_freq);
> +		}
> +
> +		dev_priv->max_cdclk_freq = max_cdclk_freq;
>  	} else if (IS_CHERRYVIEW(dev)) {
>  		dev_priv->max_cdclk_freq = 320000;
>  	} else if (IS_VALLEYVIEW(dev)) {
> -- 
> 2.4.10

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[Thulasimani, Sivakumar]

On 2/12/2016 8:36 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
> up to 675 MHz on ULT, bu only if extra cooling is provided. There
> don't seem to be any strap or VBT bits to tells us this however.
>
> But I did spot something potentially relevant in
> VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
> the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
> knows what its doing and trust the max cdclk in SWF06 if it's higher
> than the basic limit specified in Bspec. To avoid regressing anything
> let's ignore SWF06 if it indicates a lower limit than Bspec.
>
> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>
> I'm not at all sure if this is the right way to go about it. Sivakumar,
> since you seem to have some ideas on this maybe you can have a look.
> I'm not aware of any complaints so far that people can't get the cdclk
> as high is they should on specific machines, so not sure if this is really
> even needed.
>
> The other open question is what we should do if the VBIOS limit is
> lower than what we'd expect based on BSpec. Should we still trust it?
> Sadly we can't verify the SWF06 cdclk value in any way since it
> only has two bits and we have four possible cdclk values.
The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so
it just backs up the CD Clock before it optimizes for the available LFP. 
if we
are trusting for higher value we should trust it for lower value too.
The OEM might have did this to either reduce max resolution for cheaper
products or might have removed some cooling mechanisms for thinner
designs since we cannot say which of the reasons we should trust the
lower value too.

now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver
from intel alone(it is not programmed from VBT),
since GOP driver can be implemented by anyone and
if anyone implements their own GOP driver we cannot be sure if the
value is valid or not. please check if we can check for "Intel GOP driver".
And if "Intel GOP driver" was used during boot, we can trust the value
100%.  i am not sure how this can be done, so i would recommend
trusting the values with clear debug messages as done below already.

As mentioned in another thread, this needs to be done for SKL too.
i dont have a SKL system so if no one else can make a patch for it
i will have to share an untested patch for it :).
>
>   drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
>   1 file changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 836bbdc239b6..1d70f6bf2934 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>   			dev_priv->max_cdclk_freq = 450000;
>   		else
>   			dev_priv->max_cdclk_freq = 337500;
> -	} else if (IS_BROADWELL(dev))  {
> +	} else if (IS_BROADWELL(dev)) {
> +		int bios_max_cdclk_freq, max_cdclk_freq;
> +
>   		/*
> -		 * FIXME with extra cooling we can allow
> -		 * 540 MHz for ULX and 675 Mhz for ULT.
> -		 * How can we know if extra cooling is
> -		 * available? PCI ID, VTB, something else?
> +		 * With extra cooling we can allow 540 MHz for
> +		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
> +		 * passes that information in SWF06.
>   		 */
> -		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> -			dev_priv->max_cdclk_freq = 450000;
> -		else if (IS_BDW_ULX(dev))
> -			dev_priv->max_cdclk_freq = 450000;
> -		else if (IS_BDW_ULT(dev))
> -			dev_priv->max_cdclk_freq = 540000;
> -		else
> -			dev_priv->max_cdclk_freq = 675000;
> +		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
> +		case 0:
> +			bios_max_cdclk_freq = 450000;
> +			break;
> +		case 1:
> +			bios_max_cdclk_freq = 540000;
> +			break;
> +		case 2:
> +			bios_max_cdclk_freq = 337500;
> +			break;
> +		case 3:
> +			bios_max_cdclk_freq = 675000;
> +			break;
> +		}
> +
> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
> +			if (WARN_ON(bios_max_cdclk_freq != 450000))
> +				bios_max_cdclk_freq = 450000;
> +			max_cdclk_freq = 450000;
> +		} else if (IS_BDW_ULX(dev_priv)) {
> +			if (WARN_ON(bios_max_cdclk_freq > 540000))
> +				bios_max_cdclk_freq = 540000;
> +			max_cdclk_freq = 450000;
> +		} else if (IS_BDW_ULT(dev_priv)) {
> +			max_cdclk_freq = 540000;
> +		} else {
> +			max_cdclk_freq = 675000;
> +		}
> +
> +		if (bios_max_cdclk_freq > max_cdclk_freq) {
> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
> +				      "assuming extra cooling is present\n",
> +				      bios_max_cdclk_freq, max_cdclk_freq);
> +			max_cdclk_freq = bios_max_cdclk_freq;
> +		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
> +				      "ignoring it\n",
> +				      bios_max_cdclk_freq, max_cdclk_freq);
> +		}
for the reasons mentioned above, i would favor trusting lower values too.

regards,
Sivakumar
> +
> +		dev_priv->max_cdclk_freq = max_cdclk_freq;
>   	} else if (IS_CHERRYVIEW(dev)) {
>   		dev_priv->max_cdclk_freq = 320000;
>   	} else if (IS_VALLEYVIEW(dev)) {

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Use SWF06 to figure out max cdclk for BDW

[Patchwork]

== Summary ==

Series 3365v1 drm/i915: Use SWF06 to figure out max cdclk for BDW
http://patchwork.freedesktop.org/api/1.0/series/3365/revisions/1/mbox/

Test gem_sync:
        Subgroup basic-bsd:
                dmesg-fail -> PASS       (ilk-hp8440p)
        Subgroup basic-vebox:
                dmesg-fail -> PASS       (hsw-brixbox)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p) UNSTABLE
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (ivb-t430s)
        Subgroup force-load-detect:
                fail       -> SKIP       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (ivb-t430s)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-nuci7        total:162  pass:152  dwarn:0   dfail:0   fail:0   skip:10 
bdw-ultra        total:165  pass:152  dwarn:0   dfail:0   fail:0   skip:13 
bsw-nuc-2        total:165  pass:135  dwarn:1   dfail:0   fail:0   skip:29 
byt-nuc          total:165  pass:141  dwarn:0   dfail:0   fail:0   skip:24 
hsw-brixbox      total:165  pass:151  dwarn:0   dfail:0   fail:0   skip:14 
hsw-gt2          total:165  pass:154  dwarn:0   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:165  pass:116  dwarn:0   dfail:0   fail:0   skip:49 
ivb-t430s        total:165  pass:149  dwarn:0   dfail:0   fail:1   skip:15 
skl-i5k-2        total:165  pass:150  dwarn:0   dfail:0   fail:0   skip:15 
snb-dellxps      total:165  pass:142  dwarn:0   dfail:0   fail:1   skip:22 
snb-x220t        total:165  pass:142  dwarn:0   dfail:0   fail:2   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1402/

a4474d338aa8156348cebe58a329a18c8560da1e drm-intel-nightly: 2016y-02m-15d-17h-27m-11s UTC integration manifest
3ec671672f83d1cf74b9c10013221e50575cb1e4 drm/i915: Use SWF06 to figure out max cdclk for BDW

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[Ville Syrjälä]

On Tue, Feb 16, 2016 at 07:13:05AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 2/12/2016 8:36 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
> > up to 675 MHz on ULT, bu only if extra cooling is provided. There
> > don't seem to be any strap or VBT bits to tells us this however.
> >
> > But I did spot something potentially relevant in
> > VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
> > the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
> > knows what its doing and trust the max cdclk in SWF06 if it's higher
> > than the basic limit specified in Bspec. To avoid regressing anything
> > let's ignore SWF06 if it indicates a lower limit than Bspec.
> >
> > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >
> > I'm not at all sure if this is the right way to go about it. Sivakumar,
> > since you seem to have some ideas on this maybe you can have a look.
> > I'm not aware of any complaints so far that people can't get the cdclk
> > as high is they should on specific machines, so not sure if this is really
> > even needed.
> >
> > The other open question is what we should do if the VBIOS limit is
> > lower than what we'd expect based on BSpec. Should we still trust it?
> > Sadly we can't verify the SWF06 cdclk value in any way since it
> > only has two bits and we have four possible cdclk values.
> The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so
> it just backs up the CD Clock before it optimizes for the available LFP. 
> if we
> are trusting for higher value we should trust it for lower value too.
> The OEM might have did this to either reduce max resolution for cheaper
> products or might have removed some cooling mechanisms for thinner
> designs since we cannot say which of the reasons we should trust the
> lower value too.
> 
> now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver
> from intel alone(it is not programmed from VBT),
> since GOP driver can be implemented by anyone and
> if anyone implements their own GOP driver we cannot be sure if the
> value is valid or not. please check if we can check for "Intel GOP driver".
> And if "Intel GOP driver" was used during boot, we can trust the value
> 100%.  i am not sure how this can be done, so i would recommend
> trusting the values with clear debug messages as done below already.

We definitely need a way to validate the register value before we trust
it for lower values. I suppose we might be able to look at bits 31:16
since those should store the current cdclk "decimal" value. If that part
looks reasonable, we might be able to trust the "max cdclk" bits as well.

> 
> As mentioned in another thread, this needs to be done for SKL too.
> i dont have a SKL system so if no one else can make a patch for it
> i will have to share an untested patch for it :).

For SKL it seems a bit easier, since it apparently just stores the max
cdclk in 10.1 decimal fixed point in bits 10:0. So invalid/uninitialized
values would be easier to spot.

Hmm. 11 bits and 10.1 fixed point format, I wonder if it's really
decimal and not binary fixed point...

> >
> >   drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
> >   1 file changed, 47 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 836bbdc239b6..1d70f6bf2934 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
> >   			dev_priv->max_cdclk_freq = 450000;
> >   		else
> >   			dev_priv->max_cdclk_freq = 337500;
> > -	} else if (IS_BROADWELL(dev))  {
> > +	} else if (IS_BROADWELL(dev)) {
> > +		int bios_max_cdclk_freq, max_cdclk_freq;
> > +
> >   		/*
> > -		 * FIXME with extra cooling we can allow
> > -		 * 540 MHz for ULX and 675 Mhz for ULT.
> > -		 * How can we know if extra cooling is
> > -		 * available? PCI ID, VTB, something else?
> > +		 * With extra cooling we can allow 540 MHz for
> > +		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
> > +		 * passes that information in SWF06.
> >   		 */
> > -		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> > -			dev_priv->max_cdclk_freq = 450000;
> > -		else if (IS_BDW_ULX(dev))
> > -			dev_priv->max_cdclk_freq = 450000;
> > -		else if (IS_BDW_ULT(dev))
> > -			dev_priv->max_cdclk_freq = 540000;
> > -		else
> > -			dev_priv->max_cdclk_freq = 675000;
> > +		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
> > +		case 0:
> > +			bios_max_cdclk_freq = 450000;
> > +			break;
> > +		case 1:
> > +			bios_max_cdclk_freq = 540000;
> > +			break;
> > +		case 2:
> > +			bios_max_cdclk_freq = 337500;
> > +			break;
> > +		case 3:
> > +			bios_max_cdclk_freq = 675000;
> > +			break;
> > +		}
> > +
> > +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
> > +			if (WARN_ON(bios_max_cdclk_freq != 450000))
> > +				bios_max_cdclk_freq = 450000;
> > +			max_cdclk_freq = 450000;
> > +		} else if (IS_BDW_ULX(dev_priv)) {
> > +			if (WARN_ON(bios_max_cdclk_freq > 540000))
> > +				bios_max_cdclk_freq = 540000;
> > +			max_cdclk_freq = 450000;
> > +		} else if (IS_BDW_ULT(dev_priv)) {
> > +			max_cdclk_freq = 540000;
> > +		} else {
> > +			max_cdclk_freq = 675000;
> > +		}
> > +
> > +		if (bios_max_cdclk_freq > max_cdclk_freq) {
> > +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
> > +				      "assuming extra cooling is present\n",
> > +				      bios_max_cdclk_freq, max_cdclk_freq);
> > +			max_cdclk_freq = bios_max_cdclk_freq;
> > +		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
> > +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
> > +				      "ignoring it\n",
> > +				      bios_max_cdclk_freq, max_cdclk_freq);
> > +		}
> for the reasons mentioned above, i would favor trusting lower values too.
> 
> regards,
> Sivakumar
> > +
> > +		dev_priv->max_cdclk_freq = max_cdclk_freq;
> >   	} else if (IS_CHERRYVIEW(dev)) {
> >   		dev_priv->max_cdclk_freq = 320000;
> >   	} else if (IS_VALLEYVIEW(dev)) {

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[Thulasimani, Sivakumar]

[-- Attachment #1.1: Type: text/plain, Size: 6957 bytes --]



On 2/16/2016 3:35 PM, Ville Syrjälä wrote:
> On Tue, Feb 16, 2016 at 07:13:05AM +0530, Thulasimani, Sivakumar wrote:
>>
>> On 2/12/2016 8:36 PM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
>>> up to 675 MHz on ULT, bu only if extra cooling is provided. There
>>> don't seem to be any strap or VBT bits to tells us this however.
>>>
>>> But I did spot something potentially relevant in
>>> VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
>>> the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
>>> knows what its doing and trust the max cdclk in SWF06 if it's higher
>>> than the basic limit specified in Bspec. To avoid regressing anything
>>> let's ignore SWF06 if it indicates a lower limit than Bspec.
>>>
>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>>> Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>
>>> I'm not at all sure if this is the right way to go about it. Sivakumar,
>>> since you seem to have some ideas on this maybe you can have a look.
>>> I'm not aware of any complaints so far that people can't get the cdclk
>>> as high is they should on specific machines, so not sure if this is really
>>> even needed.
>>>
>>> The other open question is what we should do if the VBIOS limit is
>>> lower than what we'd expect based on BSpec. Should we still trust it?
>>> Sadly we can't verify the SWF06 cdclk value in any way since it
>>> only has two bits and we have four possible cdclk values.
>> The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so
>> it just backs up the CD Clock before it optimizes for the available LFP.
>> if we
>> are trusting for higher value we should trust it for lower value too.
>> The OEM might have did this to either reduce max resolution for cheaper
>> products or might have removed some cooling mechanisms for thinner
>> designs since we cannot say which of the reasons we should trust the
>> lower value too.
>>
>> now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver
>> from intel alone(it is not programmed from VBT),
>> since GOP driver can be implemented by anyone and
>> if anyone implements their own GOP driver we cannot be sure if the
>> value is valid or not. please check if we can check for "Intel GOP driver".
>> And if "Intel GOP driver" was used during boot, we can trust the value
>> 100%.  i am not sure how this can be done, so i would recommend
>> trusting the values with clear debug messages as done below already.
> We definitely need a way to validate the register value before we trust
> it for lower values. I suppose we might be able to look at bits 31:16
> since those should store the current cdclk "decimal" value. If that part
> looks reasonable, we might be able to trust the "max cdclk" bits as well.
it seems the bits 31:16 are used only by VBIOS and not GOP, so that wont 
help.
we need to come up with some other method to confirm the value or verify
if intel gop is loaded. i will get back if i can find such a mechanism.
>> As mentioned in another thread, this needs to be done for SKL too.
>> i dont have a SKL system so if no one else can make a patch for it
>> i will have to share an untested patch for it :).
> For SKL it seems a bit easier, since it apparently just stores the max
> cdclk in 10.1 decimal fixed point in bits 10:0. So invalid/uninitialized
> values would be easier to spot.
>
> Hmm. 11 bits and 10.1 fixed point format, I wonder if it's really
> decimal and not binary fixed point...
it uses the same value programmed in CDCLK_CTL register.
i.e 01 0011 0011 1b    = 308.57MHz

regards,
Sivakumar
>>>    drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
>>>    1 file changed, 47 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 836bbdc239b6..1d70f6bf2934 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>>>    			dev_priv->max_cdclk_freq = 450000;
>>>    		else
>>>    			dev_priv->max_cdclk_freq = 337500;
>>> -	} else if (IS_BROADWELL(dev))  {
>>> +	} else if (IS_BROADWELL(dev)) {
>>> +		int bios_max_cdclk_freq, max_cdclk_freq;
>>> +
>>>    		/*
>>> -		 * FIXME with extra cooling we can allow
>>> -		 * 540 MHz for ULX and 675 Mhz for ULT.
>>> -		 * How can we know if extra cooling is
>>> -		 * available? PCI ID, VTB, something else?
>>> +		 * With extra cooling we can allow 540 MHz for
>>> +		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
>>> +		 * passes that information in SWF06.
>>>    		 */
>>> -		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
>>> -			dev_priv->max_cdclk_freq = 450000;
>>> -		else if (IS_BDW_ULX(dev))
>>> -			dev_priv->max_cdclk_freq = 450000;
>>> -		else if (IS_BDW_ULT(dev))
>>> -			dev_priv->max_cdclk_freq = 540000;
>>> -		else
>>> -			dev_priv->max_cdclk_freq = 675000;
>>> +		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
>>> +		case 0:
>>> +			bios_max_cdclk_freq = 450000;
>>> +			break;
>>> +		case 1:
>>> +			bios_max_cdclk_freq = 540000;
>>> +			break;
>>> +		case 2:
>>> +			bios_max_cdclk_freq = 337500;
>>> +			break;
>>> +		case 3:
>>> +			bios_max_cdclk_freq = 675000;
>>> +			break;
>>> +		}
>>> +
>>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
>>> +			if (WARN_ON(bios_max_cdclk_freq != 450000))
>>> +				bios_max_cdclk_freq = 450000;
>>> +			max_cdclk_freq = 450000;
>>> +		} else if (IS_BDW_ULX(dev_priv)) {
>>> +			if (WARN_ON(bios_max_cdclk_freq > 540000))
>>> +				bios_max_cdclk_freq = 540000;
>>> +			max_cdclk_freq = 450000;
>>> +		} else if (IS_BDW_ULT(dev_priv)) {
>>> +			max_cdclk_freq = 540000;
>>> +		} else {
>>> +			max_cdclk_freq = 675000;
>>> +		}
>>> +
>>> +		if (bios_max_cdclk_freq > max_cdclk_freq) {
>>> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
>>> +				      "assuming extra cooling is present\n",
>>> +				      bios_max_cdclk_freq, max_cdclk_freq);
>>> +			max_cdclk_freq = bios_max_cdclk_freq;
>>> +		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
>>> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
>>> +				      "ignoring it\n",
>>> +				      bios_max_cdclk_freq, max_cdclk_freq);
>>> +		}
>> for the reasons mentioned above, i would favor trusting lower values too.
>>
>> regards,
>> Sivakumar
>>> +
>>> +		dev_priv->max_cdclk_freq = max_cdclk_freq;
>>>    	} else if (IS_CHERRYVIEW(dev)) {
>>>    		dev_priv->max_cdclk_freq = 320000;
>>>    	} else if (IS_VALLEYVIEW(dev)) {


[-- Attachment #1.2: Type: text/html, Size: 8740 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[Ville Syrjälä]

On Tue, Feb 23, 2016 at 04:22:01PM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 2/16/2016 3:35 PM, Ville Syrjälä wrote:
> > On Tue, Feb 16, 2016 at 07:13:05AM +0530, Thulasimani, Sivakumar wrote:
> >>
> >> On 2/12/2016 8:36 PM, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
> >>> up to 675 MHz on ULT, bu only if extra cooling is provided. There
> >>> don't seem to be any strap or VBT bits to tells us this however.
> >>>
> >>> But I did spot something potentially relevant in
> >>> VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
> >>> the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
> >>> knows what its doing and trust the max cdclk in SWF06 if it's higher
> >>> than the basic limit specified in Bspec. To avoid regressing anything
> >>> let's ignore SWF06 if it indicates a lower limit than Bspec.
> >>>
> >>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> >>> Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>
> >>> I'm not at all sure if this is the right way to go about it. Sivakumar,
> >>> since you seem to have some ideas on this maybe you can have a look.
> >>> I'm not aware of any complaints so far that people can't get the cdclk
> >>> as high is they should on specific machines, so not sure if this is really
> >>> even needed.
> >>>
> >>> The other open question is what we should do if the VBIOS limit is
> >>> lower than what we'd expect based on BSpec. Should we still trust it?
> >>> Sadly we can't verify the SWF06 cdclk value in any way since it
> >>> only has two bits and we have four possible cdclk values.
> >> The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so
> >> it just backs up the CD Clock before it optimizes for the available LFP.
> >> if we
> >> are trusting for higher value we should trust it for lower value too.
> >> The OEM might have did this to either reduce max resolution for cheaper
> >> products or might have removed some cooling mechanisms for thinner
> >> designs since we cannot say which of the reasons we should trust the
> >> lower value too.
> >>
> >> now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver
> >> from intel alone(it is not programmed from VBT),
> >> since GOP driver can be implemented by anyone and
> >> if anyone implements their own GOP driver we cannot be sure if the
> >> value is valid or not. please check if we can check for "Intel GOP driver".
> >> And if "Intel GOP driver" was used during boot, we can trust the value
> >> 100%.  i am not sure how this can be done, so i would recommend
> >> trusting the values with clear debug messages as done below already.
> > We definitely need a way to validate the register value before we trust
> > it for lower values. I suppose we might be able to look at bits 31:16
> > since those should store the current cdclk "decimal" value. If that part
> > looks reasonable, we might be able to trust the "max cdclk" bits as well.
> it seems the bits 31:16 are used only by VBIOS and not GOP, so that wont 
> help.
> we need to come up with some other method to confirm the value or verify
> if intel gop is loaded. i will get back if i can find such a mechanism.

Oh, that's unfortunate. IIRC we use some other SWF register to check if
something already initialized things in the cdclk sanitation path.
Not sure if the same could be used here.

> >> As mentioned in another thread, this needs to be done for SKL too.
> >> i dont have a SKL system so if no one else can make a patch for it
> >> i will have to share an untested patch for it :).
> > For SKL it seems a bit easier, since it apparently just stores the max
> > cdclk in 10.1 decimal fixed point in bits 10:0. So invalid/uninitialized
> > values would be easier to spot.
> >
> > Hmm. 11 bits and 10.1 fixed point format, I wonder if it's really
> > decimal and not binary fixed point...
> it uses the same value programmed in CDCLK_CTL register.
> i.e 01 0011 0011 1b    = 308.57MHz

OK, so binary fixed point. Maybe I should file a bug against bspec to
remove this confusing "decimal" naming scheme from the cdclk stuff...

> 
> regards,
> Sivakumar
> >>>    drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
> >>>    1 file changed, 47 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 836bbdc239b6..1d70f6bf2934 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
> >>>    			dev_priv->max_cdclk_freq = 450000;
> >>>    		else
> >>>    			dev_priv->max_cdclk_freq = 337500;
> >>> -	} else if (IS_BROADWELL(dev))  {
> >>> +	} else if (IS_BROADWELL(dev)) {
> >>> +		int bios_max_cdclk_freq, max_cdclk_freq;
> >>> +
> >>>    		/*
> >>> -		 * FIXME with extra cooling we can allow
> >>> -		 * 540 MHz for ULX and 675 Mhz for ULT.
> >>> -		 * How can we know if extra cooling is
> >>> -		 * available? PCI ID, VTB, something else?
> >>> +		 * With extra cooling we can allow 540 MHz for
> >>> +		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
> >>> +		 * passes that information in SWF06.
> >>>    		 */
> >>> -		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> >>> -			dev_priv->max_cdclk_freq = 450000;
> >>> -		else if (IS_BDW_ULX(dev))
> >>> -			dev_priv->max_cdclk_freq = 450000;
> >>> -		else if (IS_BDW_ULT(dev))
> >>> -			dev_priv->max_cdclk_freq = 540000;
> >>> -		else
> >>> -			dev_priv->max_cdclk_freq = 675000;
> >>> +		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
> >>> +		case 0:
> >>> +			bios_max_cdclk_freq = 450000;
> >>> +			break;
> >>> +		case 1:
> >>> +			bios_max_cdclk_freq = 540000;
> >>> +			break;
> >>> +		case 2:
> >>> +			bios_max_cdclk_freq = 337500;
> >>> +			break;
> >>> +		case 3:
> >>> +			bios_max_cdclk_freq = 675000;
> >>> +			break;
> >>> +		}
> >>> +
> >>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
> >>> +			if (WARN_ON(bios_max_cdclk_freq != 450000))
> >>> +				bios_max_cdclk_freq = 450000;
> >>> +			max_cdclk_freq = 450000;
> >>> +		} else if (IS_BDW_ULX(dev_priv)) {
> >>> +			if (WARN_ON(bios_max_cdclk_freq > 540000))
> >>> +				bios_max_cdclk_freq = 540000;
> >>> +			max_cdclk_freq = 450000;
> >>> +		} else if (IS_BDW_ULT(dev_priv)) {
> >>> +			max_cdclk_freq = 540000;
> >>> +		} else {
> >>> +			max_cdclk_freq = 675000;
> >>> +		}
> >>> +
> >>> +		if (bios_max_cdclk_freq > max_cdclk_freq) {
> >>> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
> >>> +				      "assuming extra cooling is present\n",
> >>> +				      bios_max_cdclk_freq, max_cdclk_freq);
> >>> +			max_cdclk_freq = bios_max_cdclk_freq;
> >>> +		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
> >>> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
> >>> +				      "ignoring it\n",
> >>> +				      bios_max_cdclk_freq, max_cdclk_freq);
> >>> +		}
> >> for the reasons mentioned above, i would favor trusting lower values too.
> >>
> >> regards,
> >> Sivakumar
> >>> +
> >>> +		dev_priv->max_cdclk_freq = max_cdclk_freq;
> >>>    	} else if (IS_CHERRYVIEW(dev)) {
> >>>    		dev_priv->max_cdclk_freq = 320000;
> >>>    	} else if (IS_VALLEYVIEW(dev)) {
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[Thulasimani, Sivakumar]

On 2/23/2016 4:35 PM, Ville Syrjälä wrote:
> On Tue, Feb 23, 2016 at 04:22:01PM +0530, Thulasimani, Sivakumar wrote:
>>
>> On 2/16/2016 3:35 PM, Ville Syrjälä wrote:
>>> On Tue, Feb 16, 2016 at 07:13:05AM +0530, Thulasimani, Sivakumar wrote:
>>>> On 2/12/2016 8:36 PM, ville.syrjala@linux.intel.com wrote:
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>
>>>>> Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
>>>>> up to 675 MHz on ULT, bu only if extra cooling is provided. There
>>>>> don't seem to be any strap or VBT bits to tells us this however.
>>>>>
>>>>> But I did spot something potentially relevant in
>>>>> VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
>>>>> the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
>>>>> knows what its doing and trust the max cdclk in SWF06 if it's higher
>>>>> than the basic limit specified in Bspec. To avoid regressing anything
>>>>> let's ignore SWF06 if it indicates a lower limit than Bspec.
>>>>>
>>>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>>>>> Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> ---
>>>>>
>>>>> I'm not at all sure if this is the right way to go about it. Sivakumar,
>>>>> since you seem to have some ideas on this maybe you can have a look.
>>>>> I'm not aware of any complaints so far that people can't get the cdclk
>>>>> as high is they should on specific machines, so not sure if this is really
>>>>> even needed.
>>>>>
>>>>> The other open question is what we should do if the VBIOS limit is
>>>>> lower than what we'd expect based on BSpec. Should we still trust it?
>>>>> Sadly we can't verify the SWF06 cdclk value in any way since it
>>>>> only has two bits and we have four possible cdclk values.
>>>> The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so
>>>> it just backs up the CD Clock before it optimizes for the available LFP.
>>>> if we
>>>> are trusting for higher value we should trust it for lower value too.
>>>> The OEM might have did this to either reduce max resolution for cheaper
>>>> products or might have removed some cooling mechanisms for thinner
>>>> designs since we cannot say which of the reasons we should trust the
>>>> lower value too.
>>>>
>>>> now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver
>>>> from intel alone(it is not programmed from VBT),
>>>> since GOP driver can be implemented by anyone and
>>>> if anyone implements their own GOP driver we cannot be sure if the
>>>> value is valid or not. please check if we can check for "Intel GOP driver".
>>>> And if "Intel GOP driver" was used during boot, we can trust the value
>>>> 100%.  i am not sure how this can be done, so i would recommend
>>>> trusting the values with clear debug messages as done below already.
>>> We definitely need a way to validate the register value before we trust
>>> it for lower values. I suppose we might be able to look at bits 31:16
>>> since those should store the current cdclk "decimal" value. If that part
>>> looks reasonable, we might be able to trust the "max cdclk" bits as well.
>> it seems the bits 31:16 are used only by VBIOS and not GOP, so that wont
>> help.
>> we need to come up with some other method to confirm the value or verify
>> if intel gop is loaded. i will get back if i can find such a mechanism.
> Oh, that's unfortunate. IIRC we use some other SWF register to check if
> something already initialized things in the cdclk sanitation path.
> Not sure if the same could be used here.
i am not aware of any other SWF register used for cdclk related flow so 
cant help there.
had a discussion with local folks and it seems like  there is
no easy way out atleast for BDW.

SKL register using literal values is helpful in
verifying against available set.

my best guess would be to handle it for BDW is as follows
if BIT0:1 == 0 && current_cdclk != 450MHz
           we are not in intel GOP driver so take the current clock as max
if BIT0:1 == 0 && current_cdclk == 450MHz
         cant say which GOP driver was used, so limit to current clock 
as max
if BIT0:1 != 0
        probability of some other component setting these two bits is 
very low :)
        so assume that we are in intel gop driver and trust the value.
         we have checks to limit clock for the SKU so it should not be a 
problem
        to trust the value in register.


regards,
Sivakumar
>>>> As mentioned in another thread, this needs to be done for SKL too.
>>>> i dont have a SKL system so if no one else can make a patch for it
>>>> i will have to share an untested patch for it :).
>>> For SKL it seems a bit easier, since it apparently just stores the max
>>> cdclk in 10.1 decimal fixed point in bits 10:0. So invalid/uninitialized
>>> values would be easier to spot.
>>>
>>> Hmm. 11 bits and 10.1 fixed point format, I wonder if it's really
>>> decimal and not binary fixed point...
>> it uses the same value programmed in CDCLK_CTL register.
>> i.e 01 0011 0011 1b    = 308.57MHz
> OK, so binary fixed point. Maybe I should file a bug against bspec to
> remove this confusing "decimal" naming scheme from the cdclk stuff...
>
>> regards,
>> Sivakumar
>>>>>     drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
>>>>>     1 file changed, 47 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 836bbdc239b6..1d70f6bf2934 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
>>>>>     			dev_priv->max_cdclk_freq = 450000;
>>>>>     		else
>>>>>     			dev_priv->max_cdclk_freq = 337500;
>>>>> -	} else if (IS_BROADWELL(dev))  {
>>>>> +	} else if (IS_BROADWELL(dev)) {
>>>>> +		int bios_max_cdclk_freq, max_cdclk_freq;
>>>>> +
>>>>>     		/*
>>>>> -		 * FIXME with extra cooling we can allow
>>>>> -		 * 540 MHz for ULX and 675 Mhz for ULT.
>>>>> -		 * How can we know if extra cooling is
>>>>> -		 * available? PCI ID, VTB, something else?
>>>>> +		 * With extra cooling we can allow 540 MHz for
>>>>> +		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
>>>>> +		 * passes that information in SWF06.
>>>>>     		 */
>>>>> -		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
>>>>> -			dev_priv->max_cdclk_freq = 450000;
>>>>> -		else if (IS_BDW_ULX(dev))
>>>>> -			dev_priv->max_cdclk_freq = 450000;
>>>>> -		else if (IS_BDW_ULT(dev))
>>>>> -			dev_priv->max_cdclk_freq = 540000;
>>>>> -		else
>>>>> -			dev_priv->max_cdclk_freq = 675000;
>>>>> +		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
>>>>> +		case 0:
>>>>> +			bios_max_cdclk_freq = 450000;
>>>>> +			break;
>>>>> +		case 1:
>>>>> +			bios_max_cdclk_freq = 540000;
>>>>> +			break;
>>>>> +		case 2:
>>>>> +			bios_max_cdclk_freq = 337500;
>>>>> +			break;
>>>>> +		case 3:
>>>>> +			bios_max_cdclk_freq = 675000;
>>>>> +			break;
>>>>> +		}
>>>>> +
>>>>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
>>>>> +			if (WARN_ON(bios_max_cdclk_freq != 450000))
>>>>> +				bios_max_cdclk_freq = 450000;
>>>>> +			max_cdclk_freq = 450000;
>>>>> +		} else if (IS_BDW_ULX(dev_priv)) {
>>>>> +			if (WARN_ON(bios_max_cdclk_freq > 540000))
>>>>> +				bios_max_cdclk_freq = 540000;
>>>>> +			max_cdclk_freq = 450000;
>>>>> +		} else if (IS_BDW_ULT(dev_priv)) {
>>>>> +			max_cdclk_freq = 540000;
>>>>> +		} else {
>>>>> +			max_cdclk_freq = 675000;
>>>>> +		}
>>>>> +
>>>>> +		if (bios_max_cdclk_freq > max_cdclk_freq) {
>>>>> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
>>>>> +				      "assuming extra cooling is present\n",
>>>>> +				      bios_max_cdclk_freq, max_cdclk_freq);
>>>>> +			max_cdclk_freq = bios_max_cdclk_freq;
>>>>> +		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
>>>>> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
>>>>> +				      "ignoring it\n",
>>>>> +				      bios_max_cdclk_freq, max_cdclk_freq);
>>>>> +		}
>>>> for the reasons mentioned above, i would favor trusting lower values too.
>>>>
>>>> regards,
>>>> Sivakumar
>>>>> +
>>>>> +		dev_priv->max_cdclk_freq = max_cdclk_freq;
>>>>>     	} else if (IS_CHERRYVIEW(dev)) {
>>>>>     		dev_priv->max_cdclk_freq = 320000;
>>>>>     	} else if (IS_VALLEYVIEW(dev)) {

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW

[Ville Syrjälä]

On Tue, Feb 23, 2016 at 06:40:37PM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 2/23/2016 4:35 PM, Ville Syrjälä wrote:
> > On Tue, Feb 23, 2016 at 04:22:01PM +0530, Thulasimani, Sivakumar wrote:
> >>
> >> On 2/16/2016 3:35 PM, Ville Syrjälä wrote:
> >>> On Tue, Feb 16, 2016 at 07:13:05AM +0530, Thulasimani, Sivakumar wrote:
> >>>> On 2/12/2016 8:36 PM, ville.syrjala@linux.intel.com wrote:
> >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>
> >>>>> Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or
> >>>>> up to 675 MHz on ULT, bu only if extra cooling is provided. There
> >>>>> don't seem to be any strap or VBT bits to tells us this however.
> >>>>>
> >>>>> But I did spot something potentially relevant in
> >>>>> VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass
> >>>>> the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware
> >>>>> knows what its doing and trust the max cdclk in SWF06 if it's higher
> >>>>> than the basic limit specified in Bspec. To avoid regressing anything
> >>>>> let's ignore SWF06 if it indicates a lower limit than Bspec.
> >>>>>
> >>>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
> >>>>> Cc: "Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>
> >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>> ---
> >>>>>
> >>>>> I'm not at all sure if this is the right way to go about it. Sivakumar,
> >>>>> since you seem to have some ideas on this maybe you can have a look.
> >>>>> I'm not aware of any complaints so far that people can't get the cdclk
> >>>>> as high is they should on specific machines, so not sure if this is really
> >>>>> even needed.
> >>>>>
> >>>>> The other open question is what we should do if the VBIOS limit is
> >>>>> lower than what we'd expect based on BSpec. Should we still trust it?
> >>>>> Sadly we can't verify the SWF06 cdclk value in any way since it
> >>>>> only has two bits and we have four possible cdclk values.
> >>>> The value stored in SWF06 is the CD Clock VBIOS/GOP sees during boot. so
> >>>> it just backs up the CD Clock before it optimizes for the available LFP.
> >>>> if we
> >>>> are trusting for higher value we should trust it for lower value too.
> >>>> The OEM might have did this to either reduce max resolution for cheaper
> >>>> products or might have removed some cooling mechanisms for thinner
> >>>> designs since we cannot say which of the reasons we should trust the
> >>>> lower value too.
> >>>>
> >>>> now comes to tough part :(, this SWF06 is saved by VBIOS/GOP driver
> >>>> from intel alone(it is not programmed from VBT),
> >>>> since GOP driver can be implemented by anyone and
> >>>> if anyone implements their own GOP driver we cannot be sure if the
> >>>> value is valid or not. please check if we can check for "Intel GOP driver".
> >>>> And if "Intel GOP driver" was used during boot, we can trust the value
> >>>> 100%.  i am not sure how this can be done, so i would recommend
> >>>> trusting the values with clear debug messages as done below already.
> >>> We definitely need a way to validate the register value before we trust
> >>> it for lower values. I suppose we might be able to look at bits 31:16
> >>> since those should store the current cdclk "decimal" value. If that part
> >>> looks reasonable, we might be able to trust the "max cdclk" bits as well.
> >> it seems the bits 31:16 are used only by VBIOS and not GOP, so that wont
> >> help.
> >> we need to come up with some other method to confirm the value or verify
> >> if intel gop is loaded. i will get back if i can find such a mechanism.
> > Oh, that's unfortunate. IIRC we use some other SWF register to check if
> > something already initialized things in the cdclk sanitation path.
> > Not sure if the same could be used here.
> i am not aware of any other SWF register used for cdclk related flow so 
> cant help there.

I think the register was more of a canary and not directly related to
cdclk. And it was... SWF18. Hmm. Based on the spec that would indicate
some kind of initial pipe->connector mapping. So if there are no
displays connected, I suppose it might end up showing nothing either.
So yeah, probably not suitable for this stuff after all :(

> had a discussion with local folks and it seems like  there is
> no easy way out atleast for BDW.
> 
> SKL register using literal values is helpful in
> verifying against available set.
> 
> my best guess would be to handle it for BDW is as follows
> if BIT0:1 == 0 && current_cdclk != 450MHz
>            we are not in intel GOP driver so take the current clock as max
> if BIT0:1 == 0 && current_cdclk == 450MHz
>          cant say which GOP driver was used, so limit to current clock 
> as max
> if BIT0:1 != 0
>         probability of some other component setting these two bits is 
> very low :)
>         so assume that we are in intel gop driver and trust the value.
>          we have checks to limit clock for the SKU so it should not be a 
> problem
>         to trust the value in register.
> 
> 
> regards,
> Sivakumar
> >>>> As mentioned in another thread, this needs to be done for SKL too.
> >>>> i dont have a SKL system so if no one else can make a patch for it
> >>>> i will have to share an untested patch for it :).
> >>> For SKL it seems a bit easier, since it apparently just stores the max
> >>> cdclk in 10.1 decimal fixed point in bits 10:0. So invalid/uninitialized
> >>> values would be easier to spot.
> >>>
> >>> Hmm. 11 bits and 10.1 fixed point format, I wonder if it's really
> >>> decimal and not binary fixed point...
> >> it uses the same value programmed in CDCLK_CTL register.
> >> i.e 01 0011 0011 1b    = 308.57MHz
> > OK, so binary fixed point. Maybe I should file a bug against bspec to
> > remove this confusing "decimal" naming scheme from the cdclk stuff...
> >
> >> regards,
> >> Sivakumar
> >>>>>     drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
> >>>>>     1 file changed, 47 insertions(+), 13 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index 836bbdc239b6..1d70f6bf2934 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev)
> >>>>>     			dev_priv->max_cdclk_freq = 450000;
> >>>>>     		else
> >>>>>     			dev_priv->max_cdclk_freq = 337500;
> >>>>> -	} else if (IS_BROADWELL(dev))  {
> >>>>> +	} else if (IS_BROADWELL(dev)) {
> >>>>> +		int bios_max_cdclk_freq, max_cdclk_freq;
> >>>>> +
> >>>>>     		/*
> >>>>> -		 * FIXME with extra cooling we can allow
> >>>>> -		 * 540 MHz for ULX and 675 Mhz for ULT.
> >>>>> -		 * How can we know if extra cooling is
> >>>>> -		 * available? PCI ID, VTB, something else?
> >>>>> +		 * With extra cooling we can allow 540 MHz for
> >>>>> +		 * ULX and 675 Mhz for ULT. Assume VBIOS/GOP
> >>>>> +		 * passes that information in SWF06.
> >>>>>     		 */
> >>>>> -		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT)
> >>>>> -			dev_priv->max_cdclk_freq = 450000;
> >>>>> -		else if (IS_BDW_ULX(dev))
> >>>>> -			dev_priv->max_cdclk_freq = 450000;
> >>>>> -		else if (IS_BDW_ULT(dev))
> >>>>> -			dev_priv->max_cdclk_freq = 540000;
> >>>>> -		else
> >>>>> -			dev_priv->max_cdclk_freq = 675000;
> >>>>> +		switch (I915_READ(SWF_ILK(0x06)) & 0x3) {
> >>>>> +		case 0:
> >>>>> +			bios_max_cdclk_freq = 450000;
> >>>>> +			break;
> >>>>> +		case 1:
> >>>>> +			bios_max_cdclk_freq = 540000;
> >>>>> +			break;
> >>>>> +		case 2:
> >>>>> +			bios_max_cdclk_freq = 337500;
> >>>>> +			break;
> >>>>> +		case 3:
> >>>>> +			bios_max_cdclk_freq = 675000;
> >>>>> +			break;
> >>>>> +		}
> >>>>> +
> >>>>> +		if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) {
> >>>>> +			if (WARN_ON(bios_max_cdclk_freq != 450000))
> >>>>> +				bios_max_cdclk_freq = 450000;
> >>>>> +			max_cdclk_freq = 450000;
> >>>>> +		} else if (IS_BDW_ULX(dev_priv)) {
> >>>>> +			if (WARN_ON(bios_max_cdclk_freq > 540000))
> >>>>> +				bios_max_cdclk_freq = 540000;
> >>>>> +			max_cdclk_freq = 450000;
> >>>>> +		} else if (IS_BDW_ULT(dev_priv)) {
> >>>>> +			max_cdclk_freq = 540000;
> >>>>> +		} else {
> >>>>> +			max_cdclk_freq = 675000;
> >>>>> +		}
> >>>>> +
> >>>>> +		if (bios_max_cdclk_freq > max_cdclk_freq) {
> >>>>> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), "
> >>>>> +				      "assuming extra cooling is present\n",
> >>>>> +				      bios_max_cdclk_freq, max_cdclk_freq);
> >>>>> +			max_cdclk_freq = bios_max_cdclk_freq;
> >>>>> +		} else if (bios_max_cdclk_freq < max_cdclk_freq) {
> >>>>> +			DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), "
> >>>>> +				      "ignoring it\n",
> >>>>> +				      bios_max_cdclk_freq, max_cdclk_freq);
> >>>>> +		}
> >>>> for the reasons mentioned above, i would favor trusting lower values too.
> >>>>
> >>>> regards,
> >>>> Sivakumar
> >>>>> +
> >>>>> +		dev_priv->max_cdclk_freq = max_cdclk_freq;
> >>>>>     	} else if (IS_CHERRYVIEW(dev)) {
> >>>>>     		dev_priv->max_cdclk_freq = 320000;
> >>>>>     	} else if (IS_VALLEYVIEW(dev)) {

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx