| Message ID | 1302804827-11597-6-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Ben, Am Donnerstag, den 14.04.2011, 11:13 -0700 schrieb Ben Widawsky: > userspace to the forcewake reference count via debugfs. > > v2: > use new spin_locks instead of struct_mutex in my opinion these remarks should not go into the commit message. Reading the commit log the reader is not interested in what patch iteration some change was introduced. > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- Reviewing patches the comment, what changed in what iteration, is useful though. So it should go after the `---` and be followed by a `---`, so that `git am` ignores it. commit message S-o-b: Joe User <j@example.org> --- v2: changed foo --- diff > drivers/gpu/drm/i915/i915_debugfs.c | 77 +++++++++++++++++++++++++++++++++++ > 1 files changed, 77 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index cc3818f..fed41b9 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1181,6 +1181,17 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data) > return 0; > } > > +static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data) > +{ > + struct drm_info_node *node = (struct drm_info_node *) m->private; > + struct drm_device *dev = node->minor->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + seq_printf(m, "forcewake count = %d\n", dev_priv->forcewake_count); > + > + return 0; > +} > + > static int > i915_wedged_open(struct inode *inode, > struct file *filp) > @@ -1283,6 +1294,65 @@ static int i915_wedged_create(struct dentry *root, struct drm_minor *minor) > return drm_add_fake_info_node(minor, ent, &i915_wedged_fops); > } > > +static int i915_forcewake_open(struct inode *inode, struct file *file) > +{ > + struct drm_device *dev = inode->i_private; > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + if (!IS_GEN6(dev)) > + return 0; > + > + /* > + * Paranoid people might say that it's bad that we can potentially hang > + * userspace spinning on this lock. However, if we cannot acquire this > + * lock it means that almost certainly the driver has hung, is not > + * unload-able, and the GPU is not resettable. Therefore hanging here is > + * probably a minor inconvenience not to be seen my almost every user. s/my/by/ […] Thanks, Paul
On Thu, 14 Apr 2011 21:56:02 +0200, Paul Menzel <paulepanter@users.sourceforge.net> wrote: > Dear Ben, > > > Am Donnerstag, den 14.04.2011, 11:13 -0700 schrieb Ben Widawsky: > > userspace to the forcewake reference count via debugfs. > > > > v2: > > use new spin_locks instead of struct_mutex > > in my opinion these remarks should not go into the commit message. > Reading the commit log the reader is not interested in what patch > iteration some change was introduced. In principle, I differ. I appreciate knowing the evolution of a patch as it winds its way upstream. From those notes, I can infer what questions were asked, how much attention the patch received, what the major criticisms were and how they were addressed. Important insights should we ever need revisit the patch again later. In an ideal world, each of these would be expounded upon in the changelog itself so that we had a concise discussion of the what/why/how (and even who) addressing all the salient background points and debating the wisdom of the various approaches to fixing the problem, before describing the ins-and-out of the actual fix implemented. In this particular case, I agree (and had planned to drop them after seeing "v2: no change" ;-). After the discussion of why we need a spin lock in the opening patch, further mentioning of the mutex is then irrelevant. But Ben... I seemed to have missed the real reason why we need the spinlock. You have to remind me or else I will keep whining on like a broken record. ;-) -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index cc3818f..fed41b9 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1181,6 +1181,17 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data) return 0; } +static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data) +{ + struct drm_info_node *node = (struct drm_info_node *) m->private; + struct drm_device *dev = node->minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + + seq_printf(m, "forcewake count = %d\n", dev_priv->forcewake_count); + + return 0; +} + static int i915_wedged_open(struct inode *inode, struct file *filp) @@ -1283,6 +1294,65 @@ static int i915_wedged_create(struct dentry *root, struct drm_minor *minor) return drm_add_fake_info_node(minor, ent, &i915_wedged_fops); } +static int i915_forcewake_open(struct inode *inode, struct file *file) +{ + struct drm_device *dev = inode->i_private; + struct drm_i915_private *dev_priv = dev->dev_private; + + if (!IS_GEN6(dev)) + return 0; + + /* + * Paranoid people might say that it's bad that we can potentially hang + * userspace spinning on this lock. However, if we cannot acquire this + * lock it means that almost certainly the driver has hung, is not + * unload-able, and the GPU is not resettable. Therefore hanging here is + * probably a minor inconvenience not to be seen my almost every user. + */ + spin_lock_irq(&dev_priv->forcewake_lock); + gen6_gt_force_wake_get(dev_priv); + spin_unlock_irq(&dev_priv->forcewake_lock); + + return 0; +} + +int i915_forcewake_release(struct inode *inode, struct file *file) +{ + struct drm_device *dev = inode->i_private; + struct drm_i915_private *dev_priv = dev->dev_private; + + if (!IS_GEN6(dev)) + return 0; + + /* see i915_forcewake_open() */ + spin_lock_irq(&dev_priv->forcewake_lock); + gen6_gt_force_wake_put(dev_priv); + spin_unlock_irq(&dev_priv->forcewake_lock); + + return 0; +} + +static const struct file_operations i915_forcewake_fops = { + .owner = THIS_MODULE, + .open = i915_forcewake_open, + .release = i915_forcewake_release, +}; + +static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor) +{ + struct drm_device *dev = minor->dev; + struct dentry *ent; + + ent = debugfs_create_file("i915_forcewake_user", + S_IRWXU, + root, dev, + &i915_forcewake_fops); + if (IS_ERR(ent)) + return PTR_ERR(ent); + + return 0; +} + static struct drm_info_list i915_debugfs_list[] = { {"i915_capabilities", i915_capabilities, 0}, {"i915_gem_objects", i915_gem_object_info, 0}, @@ -1319,6 +1389,7 @@ static struct drm_info_list i915_debugfs_list[] = { {"i915_sr_status", i915_sr_status, 0}, {"i915_opregion", i915_opregion, 0}, {"i915_gem_framebuffer", i915_gem_framebuffer_info, 0}, + {"i915_gen6_forcewake_count", i915_gen6_forcewake_count_info, 0}, }; #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list) @@ -1330,6 +1401,10 @@ int i915_debugfs_init(struct drm_minor *minor) if (ret) return ret; + ret = i915_forcewake_create(minor->debugfs_root, minor); + if (ret) + return ret; + return drm_debugfs_create_files(i915_debugfs_list, I915_DEBUGFS_ENTRIES, minor->debugfs_root, minor); @@ -1339,6 +1414,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor) { drm_debugfs_remove_files(i915_debugfs_list, I915_DEBUGFS_ENTRIES, minor); + drm_debugfs_remove_files((struct drm_info_list *) &i915_forcewake_fops, + 1, minor); drm_debugfs_remove_files((struct drm_info_list *) &i915_wedged_fops, 1, minor); }
userspace to the forcewake reference count via debugfs. v2: use new spin_locks instead of struct_mutex Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_debugfs.c | 77 +++++++++++++++++++++++++++++++++++ 1 files changed, 77 insertions(+), 0 deletions(-)