Re: [PATCH 3/6] drm/radeon: Use new drm debugfs file helper

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, Jan 22, 2014 at 11:11:10AM +0100, Christian König wrote:
> Am 21.01.2014 21:33, schrieb Ben Widawsky:
> >The debugfs helper duplicates the functionality used by Armada, so let's
> >just use that.
> >
> >WARNING: only compile tested
> >
> >Cc: Christian König <christian.koenig@amd.com>
> >Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >---
> >  drivers/gpu/drm/radeon/radeon.h     |  5 -----
> >  drivers/gpu/drm/radeon/radeon_ttm.c | 43 ++++++++++++++++++++++---------------
> >  2 files changed, 26 insertions(+), 22 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> >index c5519ca..ceec468 100644
> >--- a/drivers/gpu/drm/radeon/radeon.h
> >+++ b/drivers/gpu/drm/radeon/radeon.h
> >@@ -418,11 +418,6 @@ struct radeon_mman {
> >  	struct ttm_bo_device		bdev;
> >  	bool				mem_global_referenced;
> >  	bool				initialized;
> >-
> >-#if defined(CONFIG_DEBUG_FS)
> >-	struct dentry			*vram;
> >-	struct dentry			*gtt;
> >-#endif
> >  };
> >  /* bo virtual address in a specific vm */
> >diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> >index 77f5b0c..cf7caf2 100644
> >--- a/drivers/gpu/drm/radeon/radeon_ttm.c
> >+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> >@@ -971,27 +971,34 @@ static const struct file_operations radeon_ttm_gtt_fops = {
> >  	.llseek = default_llseek
> >  };
> >+static const struct radeon_debugfs_files {
> >+	const char *name;
> >+	const struct file_operations *fops;
> >+} radeon_debugfs_files[] = {
> >+	{"radeon_vram", &radeon_ttm_vram_fops},
> >+	{"radeon_gtt", &radeon_ttm_gtt_fops},
> >+};
> >  #endif
> >  static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
> >  {
> >  #if defined(CONFIG_DEBUG_FS)
> >  	unsigned count;
> >+	int i, ret;
> >  	struct drm_minor *minor = rdev->ddev->primary;
> >-	struct dentry *ent, *root = minor->debugfs_root;
> >-
> >-	ent = debugfs_create_file("radeon_vram", S_IFREG | S_IRUGO, root,
> >-				  rdev, &radeon_ttm_vram_fops);
> >-	if (IS_ERR(ent))
> >-		return PTR_ERR(ent);
> >-	rdev->mman.vram = ent;
> >-
> >-	ent = debugfs_create_file("radeon_gtt", S_IFREG | S_IRUGO, root,
> >-				  rdev, &radeon_ttm_gtt_fops);
> >-	if (IS_ERR(ent))
> >-		return PTR_ERR(ent);
> >-	rdev->mman.gtt = ent;
> >+	struct dentry *root = minor->debugfs_root;
> >+
> >+	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
> >+		ret = drm_debugfs_create_file(root, minor,
> >+					      radeon_debugfs_files[i].name,
> >+					      radeon_debugfs_files[i].fops,
> >+					      S_IFREG | S_IWUSR);
> >+		if (ret) {
> >+			radeon_ttm_debugfs_fini(rdev);
> >+			return ret;
> >+		}
> >+	}
> >  	count = ARRAY_SIZE(radeon_ttm_debugfs_list);
> >@@ -1010,11 +1017,13 @@ static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
> >  static void radeon_ttm_debugfs_fini(struct radeon_device *rdev)
> >  {
> >  #if defined(CONFIG_DEBUG_FS)
> >+	int i;
> >-	debugfs_remove(rdev->mman.vram);
> >-	rdev->mman.vram = NULL;
> >+	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
> >+		struct drm_info_list *info_list =
> >+			(struct drm_info_list *)&radeon_debugfs_files[i];
> >-	debugfs_remove(rdev->mman.gtt);
> >-	rdev->mman.gtt = NULL;
> >+		drm_debugfs_remove_files(info_list, 1, rdev->ddev->primary);
> 
> This won't work correctly because info_ent doesn't contain this pointer but
> a pointer to the fops instead.

You are correct. It should have been &radeon_debugfs_files[i].fops.

> Additional to that the fops might not be unique (you can use the same
> fops for different files) so they are probably not such a good choice
> as the key.

This is correct, and I elaborate a bit on this in a later patch (or
earlier, I forget). Essentially we have exactly one usage (afaict) of a
caller that wants a fops != key. I didn't want to create the interface
around this possibility, but rather let other extend it when it becomes
more common.

However, if this is requested/required, what I would do is provide a
void *key argument, where a NULL value will result in using fops.


> 
> On the other hand why do the drm layer want to do this bookkeeping of
> all the created files in the first place? We could just use
> debugfs_remove_recursive instead and don't need to mess with this at
> all.
> 
> Regards, Christian.
> 

The answer to the question predates me. I think having the helpers for
the common case, where you only want a show() function is handy. The
decision to split off and continue to shoehorn newer, more featured
debugfs files (as you have done in Radeon with these two, and as we have
done in i915 all over) was not one I made, nor one I really care to fix.

So assuming I fix the bug, what do you want to me do?

> >+	} #endif }
> 
> 

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel