Re: [PATCH v2] drm/i915: Debugfs disable RPS boost and idle

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Apr 28, 2014 at 01:53:52PM -0700, Daisy Sun 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.
> 
> Intention: when boost and idle are disabled, we have a clear vision
> of turbo algorithm. It‘s very helpful to verify if the turbo
> algorithm is working as expected.
> Without debugfs hooks, the RPS boost or idle may kicks in at
> anytime and any circumstances.
> 
> V1->V2: Follow Daniel's comment to explain the intention.
> 
> 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..ff71214 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;
> +	struct 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;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);

I'm not really sure why you feel it's necessary to flush the wq here.
Note that you have no real safety since you cannot acquire the lock, and
another event can get queued up after the flush. In other words,
whatever you're trying to do probably can fail.

Also note that without this, a simple atomic_t would suffice for
debugfs_disable_boost.

> +
> +	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

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx