On Mon, Sep 16, 2013 at 02:56:43PM -0700, Tom.O'Rourke@xxxxxxxxx wrote:
> From: Tom O'Rourke <Tom.O'Rourke@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
