[Intel-gfx] [PATCH 2/2] drm/i915: Add current GPU freq to sysfs

[Ben Widawsky]

On 2012-09-03 01:41, Daniel Vetter wrote:
> On Sun, Sep 02, 2012 at 12:24:41AM -0700, 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 at 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
>>
>> CC: Arjan van de Ven <arjan at linux.intel.com>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>
> I've just noticed that your sloppy maintainer totally missed to pick 
> up
> Jesse's gt power consumption interface:
>
> http://lists.freedesktop.org/archives/intel-gfx/2012-June/018404.html
>

After looking at the sysfs interfaces a bit, I think it makes sense to 
take my patch, and force Jesse to redo his on top of mine. You owe Jesse 
one sparkling wine.

> Hence a bikeshed for the sysfs filename:
> - render_ prefix is not accurate, this is for all of gt. I hence vote 
> for
>   a gt_

I only went with render_ since we're just exposing Rps. I have no 
attachment to the name otherwise (I initially had a patch which called 
it rps, in fact). Alternatively we can make gt_, and then create 
symlinks to it called render, video, whatever. Sounds like overkill, but 
I feel the name gt will become passe at some point, and I really like 
having a descriptive file name.

> - I think calling it gt_cur_freq would make sense in case we'll 
> expose
>   _min/_max limits through sysfs, too.

"cur" is of course redundant, and to me implicit, unless we do indeed 
add the other ones. I actually prefer it without cur, but I really don't 
care enough to argue further.

> - Also, calling it _cur_freq withouth MHz keeps in style with the
>   frequency knobs exposed by cpus ...

I prefer mhz, but I really don't care. Arjan doesn't either so long as 
it's in the name. One thing which sucks about hz is if we ever break the 
32b barrier, we have to deal with the same crap we do in residency.

>
> Now I guess I should go back to my trace point patch and adjust it to
> expose plain Hz, too ... Anyone got some bikeshed on this?
> -Daniel
>

If you don't change the mhz->hz, can you please just suck in the 
patches with whatever name suits you (unless of course, anything I said 
above was interesting). If we all agree to change mhz->hz, I will 
resubmit the patches.

>> ---
>>  drivers/gpu/drm/i915/i915_sysfs.c | 31 
>> +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
>> b/drivers/gpu/drm/i915/i915_sysfs.c
>> index da733a3..0cb479d 100644
>> --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> @@ -203,6 +203,30 @@ static struct bin_attribute dpf_attrs = {
>>  	.mmap = NULL
>>  };
>>
>> +static ssize_t render_frequency_mhz_show(struct device *dev,
>> +				     struct device_attribute *attr, char *buf)
>> +{
>> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, 
>> kdev);
>> +	struct drm_device *drm_dev = dminor->dev;
>> +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
>> +	int ret;
>> +
>> +	ret = i915_mutex_lock_interruptible(drm_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dev_priv->rps.cur_delay * 50;
>> +	mutex_unlock(&drm_dev->struct_mutex);
>> +
>> +	return snprintf(buf, PAGE_SIZE, "%d", ret);
>> +}
>> +
>> +static struct device_attribute gen6_attrs[] = {
>> +	__ATTR_RO(render_frequency_mhz),
>> +	__ATTR_NULL,
>> +};
>> +
>> +
>>  void i915_setup_sysfs(struct drm_device *dev)
>>  {
>>  	int ret;
>> @@ -220,10 +244,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 = device_create_file(&dev->primary->kdev, gen6_attrs);
>> +		if (ret)
>> +			DRM_ERROR("gen6 sysfs setup failed\n");
>> +	}
>>  }
>>
>>  void i915_teardown_sysfs(struct drm_device *dev)
>>  {
>> +	device_remove_file(&dev->primary->kdev, gen6_attrs);
>>  	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
>>  	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
>>  }
>> --
>> 1.7.12
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center