| Message ID | 1390336402-2049-3-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. 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. > + } > #endif > }
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 } > >
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@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? 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 } >>
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); + } #endif }
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(-)