Am 23.01.2014 04:20, schrieb Ben Widawsky:
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@xxxxxxx>
Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
---
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?
If you want to clean it up then clean it up completely.
The helper to add a debugfs file with only a show function should
probably be a general debugfs feature instead of being part of DRM.
So as long as nobody can come up with a good reason why DRM does this
housekeeping of all created debugfs file I would say use
debugfs_remove_recursive instead and kill the complete drm debugfs helpers.
Christian.
+ } #endif }
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
