public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Debugfs disable RPS boost and idle
@ 2014-04-25 17:29 Daisy Sun
  2014-04-25 17:41 ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Daisy Sun @ 2014-04-25 17:29 UTC (permalink / raw)
  To: intel-gfx

RP frequency request is affected by 2 modules: normal turbo
algorithm and RPS boost algorithm. By adding RPS boost algorithm
to the mix, the final frequency becomes relatively unpredictable.
Add a switch to enable/disable RPS boost functionality. When
disabled, RP frequency will follow the normal turbo algorithm only

Issue: VIZ-3801
Signed-off-by: Daisy Sun <daisy.sun@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_pm.c     |  8 ++++++--
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1e83ae4..2077bbd 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3486,6 +3486,45 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops,
 			i915_drop_caches_get, i915_drop_caches_set,
 			"0x%08llx\n");
 
+static int i915_rps_disable_boost_get(void *data, u64 *val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private *dev_priv = dev->dev_private;
+
+	if (INTEL_INFO(dev)->gen < 6)
+		return -ENODEV;
+
+	*val = dev_priv->rps.debugfs_disable_boost;
+
+	return 0;
+}
+
+static int i915_rps_disable_boost_set(void *data, u64 val)
+{
+	struct drm_device *dev = data;
+	drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	DRM_DEBUG_DRIVER("Setting RPS disable Boost-Idle mode to %s\n",
+				 val ? "on" : "off");
+
+	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
+	if (ret)
+		return ret;
+
+	dev_priv->rps.debugfs_disable_boost = val;
+
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_rps_disable_boost_fops,
+		i915_rps_disable_boost_get, i915_rps_disable_boost_set,
+			"%llu\n");
+
 static int
 i915_max_freq_get(void *data, u64 *val)
 {
@@ -3821,6 +3860,7 @@ static const struct i915_debugfs_files {
 	{"i915_wedged", &i915_wedged_fops},
 	{"i915_max_freq", &i915_max_freq_fops},
 	{"i915_min_freq", &i915_min_freq_fops},
+	{"i915_rps_disable_boost", &i915_rps_disable_boost_fops},
 	{"i915_cache_sharing", &i915_cache_sharing_fops},
 	{"i915_ring_stop", &i915_ring_stop_fops},
 	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 272aa7a..9c427da 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -847,6 +847,7 @@ struct intel_gen6_power_mgmt {
 	int last_adj;
 	enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
 
+	bool debugfs_disable_boost;
 	bool enabled;
 	struct delayed_work delayed_resume_work;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 75c1c76..6acac14 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3163,7 +3163,9 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
-	if (dev_priv->rps.enabled) {
+
+	if (dev_priv->rps.enabled
+		&& !dev_priv->rps.debugfs_disable_boost) {
 		if (IS_VALLEYVIEW(dev))
 			vlv_set_rps_idle(dev_priv);
 		else
@@ -3178,7 +3180,9 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
-	if (dev_priv->rps.enabled) {
+
+	if (dev_priv->rps.enabled
+		&& !dev_priv->rps.debugfs_disable_boost) {
 		if (IS_VALLEYVIEW(dev))
 			valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit);
 		else
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Debugfs disable RPS boost and idle
  2014-04-25 17:29 [PATCH] drm/i915: Debugfs disable RPS boost and idle Daisy Sun
@ 2014-04-25 17:41 ` Daniel Vetter
  2014-04-25 21:16   ` Sun, Daisy
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2014-04-25 17:41 UTC (permalink / raw)
  To: Daisy Sun; +Cc: intel-gfx

On Fri, Apr 25, 2014 at 7:29 PM, Daisy Sun <daisy.sun@intel.com> wrote:
> RP frequency request is affected by 2 modules: normal turbo
> algorithm and RPS boost algorithm. By adding RPS boost algorithm
> to the mix, the final frequency becomes relatively unpredictable.
> Add a switch to enable/disable RPS boost functionality. When
> disabled, RP frequency will follow the normal turbo algorithm only

This only really describes what the patch does, not why the rps boost
is a problem and why exactly we need to disable it. If you want to set
a fixed frequency then we have interfaces in sysfs for that, if the
booster does something stupid then we need to fix that.
-Daniel

>
> Issue: VIZ-3801
> Signed-off-by: Daisy Sun <daisy.sun@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_pm.c     |  8 ++++++--
>  3 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1e83ae4..2077bbd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3486,6 +3486,45 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops,
>                         i915_drop_caches_get, i915_drop_caches_set,
>                         "0x%08llx\n");
>
> +static int i915_rps_disable_boost_get(void *data, u64 *val)
> +{
> +       struct drm_device *dev = data;
> +       drm_i915_private *dev_priv = dev->dev_private;
> +
> +       if (INTEL_INFO(dev)->gen < 6)
> +               return -ENODEV;
> +
> +       *val = dev_priv->rps.debugfs_disable_boost;
> +
> +       return 0;
> +}
> +
> +static int i915_rps_disable_boost_set(void *data, u64 val)
> +{
> +       struct drm_device *dev = data;
> +       drm_i915_private *dev_priv = dev->dev_private;
> +       int ret;
> +
> +       flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
> +       DRM_DEBUG_DRIVER("Setting RPS disable Boost-Idle mode to %s\n",
> +                                val ? "on" : "off");
> +
> +       ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> +       if (ret)
> +               return ret;
> +
> +       dev_priv->rps.debugfs_disable_boost = val;
> +
> +       mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +       return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(i915_rps_disable_boost_fops,
> +               i915_rps_disable_boost_get, i915_rps_disable_boost_set,
> +                       "%llu\n");
> +
>  static int
>  i915_max_freq_get(void *data, u64 *val)
>  {
> @@ -3821,6 +3860,7 @@ static const struct i915_debugfs_files {
>         {"i915_wedged", &i915_wedged_fops},
>         {"i915_max_freq", &i915_max_freq_fops},
>         {"i915_min_freq", &i915_min_freq_fops},
> +       {"i915_rps_disable_boost", &i915_rps_disable_boost_fops},
>         {"i915_cache_sharing", &i915_cache_sharing_fops},
>         {"i915_ring_stop", &i915_ring_stop_fops},
>         {"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 272aa7a..9c427da 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -847,6 +847,7 @@ struct intel_gen6_power_mgmt {
>         int last_adj;
>         enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
>
> +       bool debugfs_disable_boost;
>         bool enabled;
>         struct delayed_work delayed_resume_work;
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 75c1c76..6acac14 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3163,7 +3163,9 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>         struct drm_device *dev = dev_priv->dev;
>
>         mutex_lock(&dev_priv->rps.hw_lock);
> -       if (dev_priv->rps.enabled) {
> +
> +       if (dev_priv->rps.enabled
> +               && !dev_priv->rps.debugfs_disable_boost) {
>                 if (IS_VALLEYVIEW(dev))
>                         vlv_set_rps_idle(dev_priv);
>                 else
> @@ -3178,7 +3180,9 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv)
>         struct drm_device *dev = dev_priv->dev;
>
>         mutex_lock(&dev_priv->rps.hw_lock);
> -       if (dev_priv->rps.enabled) {
> +
> +       if (dev_priv->rps.enabled
> +               && !dev_priv->rps.debugfs_disable_boost) {
>                 if (IS_VALLEYVIEW(dev))
>                         valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit);
>                 else
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Debugfs disable RPS boost and idle
  2014-04-25 17:41 ` Daniel Vetter
@ 2014-04-25 21:16   ` Sun, Daisy
  2014-04-26  9:07     ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Sun, Daisy @ 2014-04-25 21:16 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx


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

Thanks for the timely reply, Daniel.

The intention to bring debugfs for RPS boost and idle is when boost and 
idle are disabled, we are able to have a clear vision of what normal 
turbo algorithm.
It‘s very helpful to verify if the turbo algorithm is working as 
expected, at least from the VPG validation team.

Without the debugfs hooks, the RPS boost or idle may kicks in at anytime 
and any circumstances, which makes it complicated.
It does not mean RPS boost and idle is doing anything wrong, that's a 
different story. A fixed frequency setting is useful in some other cases 
but not enough to verify turbo algorithm.

I'd like to add the purpose in commit msg if it's not clear.

On 4/25/2014 10:41 AM, Daniel Vetter wrote:
> On Fri, Apr 25, 2014 at 7:29 PM, Daisy Sun <daisy.sun@intel.com> wrote:
>> RP frequency request is affected by 2 modules: normal turbo
>> algorithm and RPS boost algorithm. By adding RPS boost algorithm
>> to the mix, the final frequency becomes relatively unpredictable.
>> Add a switch to enable/disable RPS boost functionality. When
>> disabled, RP frequency will follow the normal turbo algorithm only
> This only really describes what the patch does, not why the rps boost
> is a problem and why exactly we need to disable it. If you want to set
> a fixed frequency then we have interfaces in sysfs for that, if the
> booster does something stupid then we need to fix that.
> -Daniel
>
>> Issue: VIZ-3801
>> Signed-off-by: Daisy Sun <daisy.sun@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_drv.h     |  1 +
>>   drivers/gpu/drm/i915/intel_pm.c     |  8 ++++++--
>>   3 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 1e83ae4..2077bbd 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3486,6 +3486,45 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_drop_caches_fops,
>>                          i915_drop_caches_get, i915_drop_caches_set,
>>                          "0x%08llx\n");
>>
>> +static int i915_rps_disable_boost_get(void *data, u64 *val)
>> +{
>> +       struct drm_device *dev = data;
>> +       drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +       if (INTEL_INFO(dev)->gen < 6)
>> +               return -ENODEV;
>> +
>> +       *val = dev_priv->rps.debugfs_disable_boost;
>> +
>> +       return 0;
>> +}
>> +
>> +static int i915_rps_disable_boost_set(void *data, u64 val)
>> +{
>> +       struct drm_device *dev = data;
>> +       drm_i915_private *dev_priv = dev->dev_private;
>> +       int ret;
>> +
>> +       flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>> +
>> +       DRM_DEBUG_DRIVER("Setting RPS disable Boost-Idle mode to %s\n",
>> +                                val ? "on" : "off");
>> +
>> +       ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
>> +       if (ret)
>> +               return ret;
>> +
>> +       dev_priv->rps.debugfs_disable_boost = val;
>> +
>> +       mutex_unlock(&dev_priv->rps.hw_lock);
>> +
>> +       return 0;
>> +}
>> +
>> +DEFINE_SIMPLE_ATTRIBUTE(i915_rps_disable_boost_fops,
>> +               i915_rps_disable_boost_get, i915_rps_disable_boost_set,
>> +                       "%llu\n");
>> +
>>   static int
>>   i915_max_freq_get(void *data, u64 *val)
>>   {
>> @@ -3821,6 +3860,7 @@ static const struct i915_debugfs_files {
>>          {"i915_wedged", &i915_wedged_fops},
>>          {"i915_max_freq", &i915_max_freq_fops},
>>          {"i915_min_freq", &i915_min_freq_fops},
>> +       {"i915_rps_disable_boost", &i915_rps_disable_boost_fops},
>>          {"i915_cache_sharing", &i915_cache_sharing_fops},
>>          {"i915_ring_stop", &i915_ring_stop_fops},
>>          {"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 272aa7a..9c427da 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -847,6 +847,7 @@ struct intel_gen6_power_mgmt {
>>          int last_adj;
>>          enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
>>
>> +       bool debugfs_disable_boost;
>>          bool enabled;
>>          struct delayed_work delayed_resume_work;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 75c1c76..6acac14 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3163,7 +3163,9 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
>>          struct drm_device *dev = dev_priv->dev;
>>
>>          mutex_lock(&dev_priv->rps.hw_lock);
>> -       if (dev_priv->rps.enabled) {
>> +
>> +       if (dev_priv->rps.enabled
>> +               && !dev_priv->rps.debugfs_disable_boost) {
>>                  if (IS_VALLEYVIEW(dev))
>>                          vlv_set_rps_idle(dev_priv);
>>                  else
>> @@ -3178,7 +3180,9 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv)
>>          struct drm_device *dev = dev_priv->dev;
>>
>>          mutex_lock(&dev_priv->rps.hw_lock);
>> -       if (dev_priv->rps.enabled) {
>> +
>> +       if (dev_priv->rps.enabled
>> +               && !dev_priv->rps.debugfs_disable_boost) {
>>                  if (IS_VALLEYVIEW(dev))
>>                          valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit);
>>                  else
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>


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

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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Debugfs disable RPS boost and idle
  2014-04-25 21:16   ` Sun, Daisy
@ 2014-04-26  9:07     ` Daniel Vetter
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-04-26  9:07 UTC (permalink / raw)
  To: Sun, Daisy; +Cc: intel-gfx

On Fri, Apr 25, 2014 at 11:16 PM, Sun, Daisy <daisy.sun@intel.com> wrote:
> Thanks for the timely reply, Daniel.
>
> The intention to bring debugfs for RPS boost and idle is when boost and idle
> are disabled, we are able to have a clear vision of what normal turbo
> algorithm.
> It‘s very helpful to verify if the turbo algorithm is working as expected,
> at least from the VPG validation team.
>
> Without the debugfs hooks, the RPS boost or idle may kicks in at anytime and
> any circumstances, which makes it complicated.
> It does not mean RPS boost and idle is doing anything wrong, that's a
> different story. A fixed frequency setting is useful in some other cases but
> not enough to verify turbo algorithm.
>
> I'd like to add the purpose in commit msg if it's not clear.

Yeah if this is useful for the hw validation team then a debugfs
interface makes sense. But the patch must mention this motivation
since if you try to use this in production to work around issues with
the turbo booster then that's not good ;-) Please update the commit
message with these important details and resubmit the patch.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-04-26  9:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-25 17:29 [PATCH] drm/i915: Debugfs disable RPS boost and idle Daisy Sun
2014-04-25 17:41 ` Daniel Vetter
2014-04-25 21:16   ` Sun, Daisy
2014-04-26  9:07     ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox