Re: [Freedreno] [PATCH] drm/msm/dp: Move debugfs files into subdirectory

[abhinavk@codeaurora.org]

On 2021-10-15 16:17, Bjorn Andersson wrote:
> In the cleanup path of the MSM DP driver the DP driver's debugfs files
> are destroyed by invoking debugfs_remove_recursive() on debug->root,
> which during initialization has been set to minor->debugfs_root.
> 
> To allow cleaning up the DP driver's debugfs files either each dentry
> needs to be kept track of or the files needs to be put in a 
> subdirectory
> which can be removed in one go.
> 
> By choosing to put the debugfs files in a subdirectory, based on the
> name of the associated connector this also solves the problem that 
> these
> names would collide as support for multiple DP instances are 
> introduced.
> 
> One alternative solution to the problem with colliding file names would
> have been to put keep track of the individual files and put them under
> the connector's debugfs directory. But while the drm_connector has been
> allocated, its associated debugfs directory has not been created at the
> time of initialization of the dp_debug.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

I have been thinking about this problem ever since multi-DP has been 
posted :)
Creating sub-directories seems right but at the moment it looks like IGT 
which
uses these debugfs nodes doesnt check sub-directories:

https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c#L215

It looks for the DP debugfs nodes under /sys/kernel/debug/dri/*/

We have to fix IGT too to be able to handle multi-DP cases. I will try 
to come up
with a proposal to address this.

Till then, can we go with the other solution to keep track of the 
dentries?

> ---
> 
> This depends on
> https://lore.kernel.org/linux-arm-msm/20211010030435.4000642-1-bjorn.andersson@linaro.org/
> reducing the connector from a double pointer.
> 
>  drivers/gpu/drm/msm/dp/dp_debug.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
> b/drivers/gpu/drm/msm/dp/dp_debug.c
> index da4323556ef3..67da4c69eca1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> @@ -210,26 +210,29 @@ static const struct file_operations 
> test_active_fops = {
>  static int dp_debug_init(struct dp_debug *dp_debug, struct drm_minor 
> *minor)
>  {
>  	int rc = 0;
> +	char path[64];
>  	struct dp_debug_private *debug = container_of(dp_debug,
>  			struct dp_debug_private, dp_debug);
> 
> -	debugfs_create_file("dp_debug", 0444, minor->debugfs_root,
> +	snprintf(path, sizeof(path), "msm_dp-%s", debug->connector->name);
> +
> +	debug->root = debugfs_create_dir(path, minor->debugfs_root);
> +
> +	debugfs_create_file("dp_debug", 0444, debug->root,
>  			debug, &dp_debug_fops);
> 
>  	debugfs_create_file("msm_dp_test_active", 0444,
> -			minor->debugfs_root,
> +			debug->root,
>  			debug, &test_active_fops);
> 
>  	debugfs_create_file("msm_dp_test_data", 0444,
> -			minor->debugfs_root,
> +			debug->root,
>  			debug, &dp_test_data_fops);
> 
>  	debugfs_create_file("msm_dp_test_type", 0444,
> -			minor->debugfs_root,
> +			debug->root,
>  			debug, &dp_test_type_fops);
> 
> -	debug->root = minor->debugfs_root;
> -
>  	return rc;
>  }