Re: [PATCH 6/7 v3] drm/i915: Add setters for min/max frequency

[Daniel Vetter <daniel@ffwll.ch>]

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@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@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@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