On Fri, Sep 07, 2012 at 07:43:43PM -0700, Ben Widawsky wrote:
> Provide a standardized sysfs interface for setting min, and max
> frequencies. The code which reads the limits were lifted from the
> debugfs files. As a brief explanation, the limits are similar to the CPU
> p-states. We have 3 states:
>
> RP0 - ie. max frequency
> RP1 - ie. "preferred min" frequency
> RPn - seriously lowest frequency
>
> Initially Daniel asked me to clamp the writes to supported values, but
> in conforming to the way the cpufreq drivers seem to work, instead
> return -EINVAL (noticed by Jesse in discussion).
> The values can be used by userspace wishing to control the limits of the
> GPU (see the CC list for people who care).
>
> v2 Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> v3: bug fix (Ben) - was passing the MHz value to gen6_set_rps instead of
> the step value. To fix, deal only with step values by doing the divide
> at the top.
>
> v2: add the dropped mutex_unlock in error cases (Chris)
> EINVAL on both too min, or too max (Daniel)
> ---
> drivers/gpu/drm/i915/i915_sysfs.c | 88 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 1da07e3..b67f5d7 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -238,6 +238,48 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute
> return snprintf(buf, PAGE_SIZE, "%d", ret);
> }
>
> +static ssize_t gt_max_freq_mhz_store(struct device *kdev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> + struct drm_device *dev = minor->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + u32 val, rp_state_cap, hw_max, hw_min;
> + ssize_t ret;
> +
> + ret = kstrtou32(buf, 0, &val);
> + if (ret)
> + return ret;
> +
> + val /= GT_FREQUENCY_MULTIPLIER;
> +
> + ret = mutex_lock_interruptible(&dev->struct_mutex);
> + if (ret)
> + return ret;
> +
> + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> + hw_max = (rp_state_cap & 0xff);
> + hw_min = ((rp_state_cap & 0xff0000) >> 16);
> +
> + if (val < hw_min || val > hw_max) {
> + mutex_unlock(&dev->struct_mutex);
> + return -EINVAL;
> + }
> +
> + if (val < dev_priv->rps.min_delay)
> + val = dev_priv->rps.min_delay;
Just forwarding an irc discussion so I don't forget about it:
I think we need return -EINVAL here, too. Otherwise userspace gets an
-EINVAL for hw limits, but if it tries to set up an otherwise illegal
min/max pair we silently fix it up. Same applies for the min delay below.
-Daniel
> +
> + if (dev_priv->rps.cur_delay > val)
> + gen6_set_rps(dev_priv->dev, val);
> +
> + dev_priv->rps.max_delay = val;
> +
> + mutex_unlock(&dev->struct_mutex);
> +
> + return count;
> +}
> +
> static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf)
> {
> struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> @@ -255,9 +297,51 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute
> return snprintf(buf, PAGE_SIZE, "%d", ret);
> }
>
> +static ssize_t gt_min_freq_mhz_store(struct device *kdev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev);
> + struct drm_device *dev = minor->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + u32 val, rp_state_cap, hw_max, hw_min;
> + ssize_t ret;
> +
> + ret = kstrtou32(buf, 0, &val);
> + if (ret)
> + return ret;
> +
> + val /= GT_FREQUENCY_MULTIPLIER;
> +
> + ret = mutex_lock_interruptible(&dev->struct_mutex);
> + if (ret)
> + return ret;
> +
> + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> + hw_max = (rp_state_cap & 0xff);
> + hw_min = ((rp_state_cap & 0xff0000) >> 16);
> +
> + if (val < hw_min || val > hw_max) {
> + mutex_unlock(&dev->struct_mutex);
> + return -EINVAL;
> + }
> +
> + if (val > dev_priv->rps.max_delay)
> + val = dev_priv->rps.max_delay;
> +
> + if (dev_priv->rps.cur_delay < val)
> + gen6_set_rps(dev_priv->dev, val);
> +
> + dev_priv->rps.min_delay = val;
> +
> + mutex_unlock(&dev->struct_mutex);
> +
> + return count;
> +}
> +
> static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
> -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL);
> -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL);
> +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store);
> +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store);
>
> static const struct attribute *gen6_attrs[] = {
> &dev_attr_gt_cur_freq_mhz.attr,
> --
> 1.7.12
>
> _______________________________________________
> 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
