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

["Christian König" <christian.koenig@amd.com>]

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 }
>>


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel