[Intel-gfx] [PATCH] drm/i915: Finish enabling rps before use by sysfs or debugfs

[Daniel Vetter]

On Mon, Sep 16, 2013 at 02:56:43PM -0700, Tom.O'Rourke at intel.com wrote:
> From: Tom O'Rourke <Tom.O'Rourke at intel.com>
> 
> Enabling rps (turbo setup) was put in a work queue because it may
> take quite awhile.  This change flushes the work queue to initialize
> rps values before use by sysfs or debugfs.  Specifically,
> rps.delayed_resume_work is flushed before using rps.hw_max,
> rps.max_delay, rps.min_delay, or rps.cur_delay.
> 
> This change fixes a problem in sysfs where show functions using
> uninitialized values show incorrect values and store functions
> using uninitialized values in range checks incorrectly fail to
> store valid input values.  This change also addresses similar use
> before initialized problems in debugfs.
> 
> Change-Id: Ib9c4f2066b65013094cb9278fc17958a964836e7
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke at intel.com>

I think we can exercise this by going through a suspend/resume cycle and
racing concurrent reads/writes of the sysfs files in a 2nd process. With
the igt_fork and igt_system_suspend_autoresume helpers in our kernel
testsuite repro this should be a really quick testcase to write ...

I'd go for a generic "read all sysfs files" approach to increase the
chances of catching other similar fallout in the future.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   12 ++++++++++++
>  drivers/gpu/drm/i915/i915_sysfs.c   |   10 ++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1d77624..52e90a1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -843,6 +843,8 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	if (IS_GEN5(dev)) {
>  		u16 rgvswctl = I915_READ16(MEMSWCTL);
>  		u16 rgvstat = I915_READ16(MEMSTAT_ILK);
> @@ -1321,6 +1323,8 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>  		return 0;
>  	}
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
>  	if (ret)
>  		return ret;
> @@ -1972,6 +1976,8 @@ i915_max_freq_get(void *data, u64 *val)
>  	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
>  		return -ENODEV;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
>  	if (ret)
>  		return ret;
> @@ -1996,6 +2002,8 @@ i915_max_freq_set(void *data, u64 val)
>  	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
>  		return -ENODEV;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val);
>  
>  	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> @@ -2034,6 +2042,8 @@ i915_min_freq_get(void *data, u64 *val)
>  	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
>  		return -ENODEV;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
>  	if (ret)
>  		return ret;
> @@ -2058,6 +2068,8 @@ i915_min_freq_set(void *data, u64 val)
>  	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
>  		return -ENODEV;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val);
>  
>  	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index d572435..270892b 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -213,6 +213,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (IS_VALLEYVIEW(dev_priv->dev)) {
>  		u32 freq;
> @@ -245,6 +247,8 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (IS_VALLEYVIEW(dev_priv->dev))
>  		ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.max_delay);
> @@ -269,6 +273,8 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev,
>  	if (ret)
>  		return ret;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
>  	if (IS_VALLEYVIEW(dev_priv->dev)) {
> @@ -317,6 +323,8 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (IS_VALLEYVIEW(dev_priv->dev))
>  		ret = vlv_gpu_freq(dev_priv->mem_freq, dev_priv->rps.min_delay);
> @@ -341,6 +349,8 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
>  	if (ret)
>  		return ret;
>  
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
>  	if (IS_VALLEYVIEW(dev)) {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at 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