* [PATCH 1/2] drm/i915: Enable some sysfs stuff without CONFIG_PM
@ 2012-09-02 7:24 Ben Widawsky
2012-09-02 7:24 ` [PATCH 2/2] drm/i915: Add current GPU freq to sysfs Ben Widawsky
2012-09-05 21:27 ` [PATCH 1/2] drm/i915: Enable some sysfs stuff without CONFIG_PM Daniel Vetter
0 siblings, 2 replies; 6+ messages in thread
From: Ben Widawsky @ 2012-09-02 7:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
The original patch was actually incorrect in stubbing out the sysfs for
l3 parity.
commit 5ab3633d6907018b0b830a720e877c3884d679c3
Author: Hunt Xu <mhuntxu@gmail.com>
Date: Sun Jul 1 03:45:07 2012 +0000
drm/i915: make rc6 in sysfs functions conditional
Unfortunately Hunt didn't respond to my review comments, and Daniel
sucked in the patch again ignoring. Worst of all, I'm too lazy to write
the patch for what I originally wanted, which was to keep rc6 sysfs even
without CONFIG_PM. This simpler patch does enough to enable us to add
more sysfs entries though.
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_sysfs.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index c5ee7ee..da733a3 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -93,6 +93,7 @@ static struct attribute_group rc6_attr_group = {
.name = power_group_name,
.attrs = rc6_attrs
};
+#endif
static int l3_access_valid(struct drm_device *dev, loff_t offset)
{
@@ -206,13 +207,14 @@ void i915_setup_sysfs(struct drm_device *dev)
{
int ret;
+#ifdef CONFIG_PM
if (INTEL_INFO(dev)->gen >= 6) {
ret = sysfs_merge_group(&dev->primary->kdev.kobj,
&rc6_attr_group);
if (ret)
DRM_ERROR("RC6 residency sysfs setup failed\n");
}
-
+#endif
if (HAS_L3_GPU_CACHE(dev)) {
ret = device_create_bin_file(&dev->primary->kdev, &dpf_attrs);
if (ret)
@@ -225,14 +227,3 @@ void i915_teardown_sysfs(struct drm_device *dev)
device_remove_bin_file(&dev->primary->kdev, &dpf_attrs);
sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
}
-#else
-void i915_setup_sysfs(struct drm_device *dev)
-{
- return;
-}
-
-void i915_teardown_sysfs(struct drm_device *dev)
-{
- return;
-}
-#endif /* CONFIG_PM */
--
1.7.12
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] drm/i915: Add current GPU freq to sysfs 2012-09-02 7:24 [PATCH 1/2] drm/i915: Enable some sysfs stuff without CONFIG_PM Ben Widawsky @ 2012-09-02 7:24 ` Ben Widawsky 2012-09-02 8:44 ` Chris Wilson 2012-09-03 8:41 ` Daniel Vetter 2012-09-05 21:27 ` [PATCH 1/2] drm/i915: Enable some sysfs stuff without CONFIG_PM Daniel Vetter 1 sibling, 2 replies; 6+ messages in thread From: Ben Widawsky @ 2012-09-02 7:24 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky, Arjan van de Ven 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 CC: Arjan van de Ven <arjan@linux.intel.com> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Add current GPU freq to sysfs 2012-09-02 7:24 ` [PATCH 2/2] drm/i915: Add current GPU freq to sysfs Ben Widawsky @ 2012-09-02 8:44 ` Chris Wilson 2012-09-03 8:41 ` Daniel Vetter 1 sibling, 0 replies; 6+ messages in thread From: Chris Wilson @ 2012-09-02 8:44 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky, Arjan van de Ven On Sun, 2 Sep 2012 00:24:41 -0700, Ben Widawsky <ben@bwidawsk.net> 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 > > CC: Arjan van de Ven <arjan@linux.intel.com> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > 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; I would have called the struct device *kdev so that we could have our standard nameing convention of struct drm_device *dev. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Add current GPU freq to sysfs 2012-09-02 7:24 ` [PATCH 2/2] drm/i915: Add current GPU freq to sysfs Ben Widawsky 2012-09-02 8:44 ` Chris Wilson @ 2012-09-03 8:41 ` Daniel Vetter 2012-09-03 20:48 ` Ben Widawsky 1 sibling, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2012-09-03 8:41 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx, Arjan van de Ven 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@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@linux.intel.com> > Signed-off-by: Ben Widawsky <ben@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 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 think calling it gt_cur_freq would make sense in case we'll expose _min/_max limits through sysfs, too. - Also, calling it _cur_freq withouth MHz keeps in style with the frequency knobs exposed by cpus ... 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 > --- > 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Add current GPU freq to sysfs 2012-09-03 8:41 ` Daniel Vetter @ 2012-09-03 20:48 ` Ben Widawsky 0 siblings, 0 replies; 6+ messages in thread From: Ben Widawsky @ 2012-09-03 20:48 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Arjan van de Ven 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@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@linux.intel.com> >> Signed-off-by: Ben Widawsky <ben@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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Enable some sysfs stuff without CONFIG_PM 2012-09-02 7:24 [PATCH 1/2] drm/i915: Enable some sysfs stuff without CONFIG_PM Ben Widawsky 2012-09-02 7:24 ` [PATCH 2/2] drm/i915: Add current GPU freq to sysfs Ben Widawsky @ 2012-09-05 21:27 ` Daniel Vetter 1 sibling, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2012-09-05 21:27 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Sun, Sep 02, 2012 at 12:24:40AM -0700, Ben Widawsky wrote: > The original patch was actually incorrect in stubbing out the sysfs for > l3 parity. > commit 5ab3633d6907018b0b830a720e877c3884d679c3 > Author: Hunt Xu <mhuntxu@gmail.com> > Date: Sun Jul 1 03:45:07 2012 +0000 > > drm/i915: make rc6 in sysfs functions conditional > > Unfortunately Hunt didn't respond to my review comments, and Daniel > sucked in the patch again ignoring. Worst of all, I'm too lazy to write > the patch for what I originally wanted, which was to keep rc6 sysfs even > without CONFIG_PM. This simpler patch does enough to enable us to add > more sysfs entries though. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-05 21:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-02 7:24 [PATCH 1/2] drm/i915: Enable some sysfs stuff without CONFIG_PM Ben Widawsky 2012-09-02 7:24 ` [PATCH 2/2] drm/i915: Add current GPU freq to sysfs Ben Widawsky 2012-09-02 8:44 ` Chris Wilson 2012-09-03 8:41 ` Daniel Vetter 2012-09-03 20:48 ` Ben Widawsky 2012-09-05 21:27 ` [PATCH 1/2] drm/i915: Enable some sysfs stuff without CONFIG_PM Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.