diff mbox

drm/i915: Fix GT frequency rounding

Message ID 1447435781-23416-1-git-send-email-mika.kuoppala@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mika Kuoppala Nov. 13, 2015, 5:29 p.m. UTC
When we set and later readback a frequency value through
sysfs interface, igt/pm_rpm assumes that we get same value back
if it matches hw granularity.

On bxt we have found out that this is not always the case.
Currently frequency - hw ratio - frequency conversions round down,
with few exceptions on platforms that have more specific conversions.
On bxt the supported range can be for example from 100Mhz to 650Mhz.
Midpoint is then calculated by test to be 375 which pm_rps uses to find a
closest hw supported frequency. That is 366 (ratio 22),
which it then writes back. But as the rounding down kicks in,
driver actually sets 350 instead of 366, as 366 is 2/3 below 22 * 50/3.

Fix this by rounding to closest instead of rounding down in
freq-ratio-freq conversions.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92768
Testcase: igt/pm_rps/basic-api
Tested-by: Bob Paauwe <bob.j.paauwe@intel.com>
Cc: Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä Nov. 13, 2015, 5:47 p.m. UTC | #1
On Fri, Nov 13, 2015 at 07:29:41PM +0200, Mika Kuoppala wrote:
> When we set and later readback a frequency value through
> sysfs interface, igt/pm_rpm assumes that we get same value back
> if it matches hw granularity.
> 
> On bxt we have found out that this is not always the case.
> Currently frequency - hw ratio - frequency conversions round down,
> with few exceptions on platforms that have more specific conversions.
> On bxt the supported range can be for example from 100Mhz to 650Mhz.
> Midpoint is then calculated by test to be 375 which pm_rps uses to find a
> closest hw supported frequency. That is 366 (ratio 22),
> which it then writes back. But as the rounding down kicks in,
> driver actually sets 350 instead of 366, as 366 is 2/3 below 22 * 50/3.
> 
> Fix this by rounding to closest instead of rounding down in
> freq-ratio-freq conversions.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92768
> Testcase: igt/pm_rps/basic-api
> Tested-by: Bob Paauwe <bob.j.paauwe@intel.com>
> Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 647c0ff..740623f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7139,7 +7139,8 @@ static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
>  	if (IS_GEN9(dev_priv->dev))
> -		return (val * GT_FREQUENCY_MULTIPLIER) / GEN9_FREQ_SCALER;
> +		return DIV_ROUND_CLOSEST(val * GT_FREQUENCY_MULTIPLIER,
> +					 GEN9_FREQ_SCALER);
>  	else if (IS_CHERRYVIEW(dev_priv->dev))
>  		return chv_gpu_freq(dev_priv, val);
>  	else if (IS_VALLEYVIEW(dev_priv->dev))
> @@ -7151,13 +7152,14 @@ int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  int intel_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  {
>  	if (IS_GEN9(dev_priv->dev))
> -		return (val * GEN9_FREQ_SCALER) / GT_FREQUENCY_MULTIPLIER;
> +		return DIV_ROUND_CLOSEST(val * GEN9_FREQ_SCALER,
> +					 GT_FREQUENCY_MULTIPLIER);
>  	else if (IS_CHERRYVIEW(dev_priv->dev))
>  		return chv_freq_opcode(dev_priv, val);
>  	else if (IS_VALLEYVIEW(dev_priv->dev))
>  		return byt_freq_opcode(dev_priv, val);
>  	else
> -		return val / GT_FREQUENCY_MULTIPLIER;
> +		return DIV_ROUND_CLOSEST(val, GT_FREQUENCY_MULTIPLIER);

lgtm

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  struct request_boost {
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Paauwe, Bob J Nov. 13, 2015, 7:38 p.m. UTC | #2
On Fri, 13 Nov 2015 19:29:41 +0200
Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:

> When we set and later readback a frequency value through
> sysfs interface, igt/pm_rpm assumes that we get same value back
> if it matches hw granularity.
> 
> On bxt we have found out that this is not always the case.
> Currently frequency - hw ratio - frequency conversions round down,
> with few exceptions on platforms that have more specific conversions.
> On bxt the supported range can be for example from 100Mhz to 650Mhz.
> Midpoint is then calculated by test to be 375 which pm_rps uses to find a
> closest hw supported frequency. That is 366 (ratio 22),
> which it then writes back. But as the rounding down kicks in,
> driver actually sets 350 instead of 366, as 366 is 2/3 below 22 * 50/3.
> 
> Fix this by rounding to closest instead of rounding down in
> freq-ratio-freq conversions.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92768
> Testcase: igt/pm_rps/basic-api
> Tested-by: Bob Paauwe <bob.j.paauwe@intel.com>
> Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 647c0ff..740623f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7139,7 +7139,8 @@ static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
>  	if (IS_GEN9(dev_priv->dev))
> -		return (val * GT_FREQUENCY_MULTIPLIER) / GEN9_FREQ_SCALER;
> +		return DIV_ROUND_CLOSEST(val * GT_FREQUENCY_MULTIPLIER,
> +					 GEN9_FREQ_SCALER);
>  	else if (IS_CHERRYVIEW(dev_priv->dev))
>  		return chv_gpu_freq(dev_priv, val);
>  	else if (IS_VALLEYVIEW(dev_priv->dev))
> @@ -7151,13 +7152,14 @@ int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  int intel_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  {
>  	if (IS_GEN9(dev_priv->dev))
> -		return (val * GEN9_FREQ_SCALER) / GT_FREQUENCY_MULTIPLIER;
> +		return DIV_ROUND_CLOSEST(val * GEN9_FREQ_SCALER,
> +					 GT_FREQUENCY_MULTIPLIER);
>  	else if (IS_CHERRYVIEW(dev_priv->dev))
>  		return chv_freq_opcode(dev_priv, val);
>  	else if (IS_VALLEYVIEW(dev_priv->dev))
>  		return byt_freq_opcode(dev_priv, val);
>  	else
> -		return val / GT_FREQUENCY_MULTIPLIER;
> +		return DIV_ROUND_CLOSEST(val, GT_FREQUENCY_MULTIPLIER);
>  }
>  
>  struct request_boost {

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>
Jani Nikula Nov. 16, 2015, 1:28 p.m. UTC | #3
On Fri, 13 Nov 2015, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> On Fri, 13 Nov 2015 19:29:41 +0200
> Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
>
>> When we set and later readback a frequency value through
>> sysfs interface, igt/pm_rpm assumes that we get same value back
>> if it matches hw granularity.
>> 
>> On bxt we have found out that this is not always the case.
>> Currently frequency - hw ratio - frequency conversions round down,
>> with few exceptions on platforms that have more specific conversions.
>> On bxt the supported range can be for example from 100Mhz to 650Mhz.
>> Midpoint is then calculated by test to be 375 which pm_rps uses to find a
>> closest hw supported frequency. That is 366 (ratio 22),
>> which it then writes back. But as the rounding down kicks in,
>> driver actually sets 350 instead of 366, as 366 is 2/3 below 22 * 50/3.
>> 
>> Fix this by rounding to closest instead of rounding down in
>> freq-ratio-freq conversions.
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92768
>> Testcase: igt/pm_rps/basic-api
>> Tested-by: Bob Paauwe <bob.j.paauwe@intel.com>
>> Cc: Bob Paauwe <bob.j.paauwe@intel.com>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 647c0ff..740623f 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -7139,7 +7139,8 @@ static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
>>  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
>>  {
>>  	if (IS_GEN9(dev_priv->dev))
>> -		return (val * GT_FREQUENCY_MULTIPLIER) / GEN9_FREQ_SCALER;
>> +		return DIV_ROUND_CLOSEST(val * GT_FREQUENCY_MULTIPLIER,
>> +					 GEN9_FREQ_SCALER);
>>  	else if (IS_CHERRYVIEW(dev_priv->dev))
>>  		return chv_gpu_freq(dev_priv, val);
>>  	else if (IS_VALLEYVIEW(dev_priv->dev))
>> @@ -7151,13 +7152,14 @@ int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
>>  int intel_freq_opcode(struct drm_i915_private *dev_priv, int val)
>>  {
>>  	if (IS_GEN9(dev_priv->dev))
>> -		return (val * GEN9_FREQ_SCALER) / GT_FREQUENCY_MULTIPLIER;
>> +		return DIV_ROUND_CLOSEST(val * GEN9_FREQ_SCALER,
>> +					 GT_FREQUENCY_MULTIPLIER);
>>  	else if (IS_CHERRYVIEW(dev_priv->dev))
>>  		return chv_freq_opcode(dev_priv, val);
>>  	else if (IS_VALLEYVIEW(dev_priv->dev))
>>  		return byt_freq_opcode(dev_priv, val);
>>  	else
>> -		return val / GT_FREQUENCY_MULTIPLIER;
>> +		return DIV_ROUND_CLOSEST(val, GT_FREQUENCY_MULTIPLIER);
>>  }
>>  
>>  struct request_boost {
>
> Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 647c0ff..740623f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7139,7 +7139,8 @@  static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
 int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
 	if (IS_GEN9(dev_priv->dev))
-		return (val * GT_FREQUENCY_MULTIPLIER) / GEN9_FREQ_SCALER;
+		return DIV_ROUND_CLOSEST(val * GT_FREQUENCY_MULTIPLIER,
+					 GEN9_FREQ_SCALER);
 	else if (IS_CHERRYVIEW(dev_priv->dev))
 		return chv_gpu_freq(dev_priv, val);
 	else if (IS_VALLEYVIEW(dev_priv->dev))
@@ -7151,13 +7152,14 @@  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
 int intel_freq_opcode(struct drm_i915_private *dev_priv, int val)
 {
 	if (IS_GEN9(dev_priv->dev))
-		return (val * GEN9_FREQ_SCALER) / GT_FREQUENCY_MULTIPLIER;
+		return DIV_ROUND_CLOSEST(val * GEN9_FREQ_SCALER,
+					 GT_FREQUENCY_MULTIPLIER);
 	else if (IS_CHERRYVIEW(dev_priv->dev))
 		return chv_freq_opcode(dev_priv, val);
 	else if (IS_VALLEYVIEW(dev_priv->dev))
 		return byt_freq_opcode(dev_priv, val);
 	else
-		return val / GT_FREQUENCY_MULTIPLIER;
+		return DIV_ROUND_CLOSEST(val, GT_FREQUENCY_MULTIPLIER);
 }
 
 struct request_boost {