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 at amd.com>
>>> Signed-off-by: Ben Widawsky <ben at 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 }
>>
