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

[Christian König <christian.koenig@xxxxxxx>]

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


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