Re: [PATCH 4/7 v2] drm/i915: Add current GPU freq to sysfs

[Ben Widawsky <ben@bwidawsk.net>]

On 2012-09-06 14:09, Michael Larabel wrote:
> Have you discussed this with Radeon/Nouveau to see if they would
> adopt a common sysfs interface? In the past, Eugeni Dodonov and I had
> talked about this with a desire to have the open-source drivers 
> expose
> the useful information for monitoring in a similar manner for helping
> userspace (current/max/min clock speeds, voltage, etc). [My personal
> interest is from monitoring this information in a more standardized
> way from the Phoronix Test Suite system monitoring components rather
> than writing driver-specific paths like it is now.]
>
> Martin (CCed) has also expressed interest from the Nouveau side in
> calling for some standardization.
>
> -- Michael

We can... I did very briefly look at what Nouveau does with the hwmon 
interface, and what DRM provided, and thought it'd be more work than I 
wanted to sign up for (which is just about nothing sysfs other than 
display). Thanks for bringing him into the thread though.

>
> On 09/06/2012 03:54 PM, Ben Widawsky wrote:
>> Userspace applications such as PowerTOP are interesting in being 
>> able to
>> read the current GPU frequency. The patch itself sets up a generic 
>> array
>> for gen6 attributes so we can easily add other items in the future 
>> (and
>> it also happens to be just about the cleanest way to do this).
>>
>> The patch is a nice addition to
>> commit 1ac02185dff3afac146d745ba220dc6672d1d162
>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Date:   Thu Aug 30 13:26:48 2012 +0200
>>
>>      drm/i915: add a tracepoint for gpu frequency changes
>>
>> Reading the GPU frequncy can be done by reading a file like:
>> /sys/class/drm/card0/render_frequency_mhz
>>
>> v2: rename the sysfs file to gt_cur_freq_mhz (Daniel)
>> Use kdev naming (Chris)
>> Use DEVICE_ATTR over __ATTR (Ben)
>> Add min/max freq (Ben/Daniel)
>> user the new #define for frequency multiplier
>>
>> CC: Arjan van de Ven <arjan@linux.intel.com>
>> CC: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>   drivers/gpu/drm/i915/i915_sysfs.c | 71 
>> +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 71 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
>> b/drivers/gpu/drm/i915/i915_sysfs.c
>> index c701a17..6c7f905 100644
>> --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> @@ -203,6 +203,70 @@ static struct bin_attribute dpf_attrs = {
>>   	.mmap = NULL
>>   };
>>   +static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>> +				    struct device_attribute *attr, char *buf)
>> +{
>> +	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;
>> +	int ret;
>> +
>> +	ret = i915_mutex_lock_interruptible(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER;
>> +	mutex_unlock(&dev->struct_mutex);
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%d", ret);
>> +}
>> +
>> +static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct 
>> device_attribute *attr, char *buf)
>> +{
>> +	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;
>> +	int ret;
>> +
>> +	ret = i915_mutex_lock_interruptible(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER;
>> +	mutex_unlock(&dev->struct_mutex);
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%d", ret);
>> +}
>> +
>> +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);
>> +	struct drm_device *dev = minor->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	int ret;
>> +
>> +	ret = i915_mutex_lock_interruptible(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER;
>> +	mutex_unlock(&dev->struct_mutex);
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%d", ret);
>> +}
>> +
>> +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 const struct attribute *gen6_attrs[] = {
>> +	&dev_attr_gt_cur_freq_mhz.attr,
>> +	&dev_attr_gt_max_freq_mhz.attr,
>> +	&dev_attr_gt_min_freq_mhz.attr,
>> +	NULL,
>> +};
>> +
>> +
>>   void i915_setup_sysfs(struct drm_device *dev)
>>   {
>>   	int ret;
>> @@ -220,10 +284,17 @@ void i915_setup_sysfs(struct drm_device *dev)
>>   		if (ret)
>>   			DRM_ERROR("l3 parity sysfs setup failed\n");
>>   	}
>> +
>> +	if (INTEL_INFO(dev)->gen >= 6) {
>> +		ret = sysfs_create_files(&dev->primary->kdev.kobj, gen6_attrs);
>> +		if (ret)
>> +			DRM_ERROR("gen6 sysfs setup failed\n");
>> +	}
>>   }
>>   void i915_teardown_sysfs(struct drm_device *dev)
>>   {
>> +	sysfs_remove_files(&dev->primary->kdev.kobj, gen6_attrs);
>>   	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
>>   	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
>>   }

-- 
Ben Widawsky, Intel Open Source Technology Center