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

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

[Bjorn Andersson]

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

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

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

[abhinavk]

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

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

[Bjorn Andersson]

On Fri 15 Oct 16:53 PDT 2021, abhinavk@codeaurora.org wrote:

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

I'm afraid I don't see what you're proposing.

Afaict we need one set of dp_test{type,active,data} per DP controller,
so even doing this by keeping track of the dentries requires that we
rename the files based on some identifier (id or connector name) - which
will cause igt to break.

As such, I think the practical path forward is that we merge the
multi-DP series as currently proposed. This will not cause any issues on
single-DP systems, but on multi-DP systems we will have warnings about
duplicate debugfs entries in the kernel logs.

Then you can figure out how to rework igt to deal with the multiple DP
instances and update the dp_debug interface accordingly.


Which also implies that we should hold this patch back. But if we go
that path, I think we should fix dp_debug_deinit() so that it doesn't
remove /sys/kernel/debug/dri/128 when the DP driver is unloaded.

Regards,
Bjorn

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

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

[abhinavk]

Hi Bjorn

On 2021-10-17 09:42, Bjorn Andersson wrote:
> On Fri 15 Oct 16:53 PDT 2021, abhinavk@codeaurora.org wrote:
> 
>> 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?
>> 
> 
> I'm afraid I don't see what you're proposing.
> 
> Afaict we need one set of dp_test{type,active,data} per DP controller,
> so even doing this by keeping track of the dentries requires that we
> rename the files based on some identifier (id or connector name) - 
> which
> will cause igt to break.

Yes, I also thought the same that there needs to be some identifier.

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

I guess I misunderstood your statement in the commit text thinking that 
you
had some other way to keep track of the dentries as it mentioned that
use a subdirectory OR keep track of each dentry.

> 
> As such, I think the practical path forward is that we merge the
> multi-DP series as currently proposed. This will not cause any issues 
> on
> single-DP systems, but on multi-DP systems we will have warnings about
> duplicate debugfs entries in the kernel logs.
> 
> Then you can figure out how to rework igt to deal with the multiple DP
> instances and update the dp_debug interface accordingly.
> 

Fine with me, I will take care of this.

> 
> Which also implies that we should hold this patch back. But if we go
> that path, I think we should fix dp_debug_deinit() so that it doesn't
> remove /sys/kernel/debug/dri/128 when the DP driver is unloaded.
Yes, lets hold this patch back till I fix multi-DP for IGT.
> 
> Regards,
> Bjorn
> 
>> > ---
>> >
>> > 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;
>> >  }

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

[Bjorn Andersson]

On Mon 18 Oct 11:07 PDT 2021, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 
> On 2021-10-17 09:42, Bjorn Andersson wrote:
> > On Fri 15 Oct 16:53 PDT 2021, abhinavk@codeaurora.org wrote:
> > 
> > > 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?
> > > 
> > 
> > I'm afraid I don't see what you're proposing.
> > 
> > Afaict we need one set of dp_test{type,active,data} per DP controller,
> > so even doing this by keeping track of the dentries requires that we
> > rename the files based on some identifier (id or connector name) - which
> > will cause igt to break.
> 
> Yes, I also thought the same that there needs to be some identifier.
> 
> "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"
> 
> I guess I misunderstood your statement in the commit text thinking that you
> had some other way to keep track of the dentries as it mentioned that
> use a subdirectory OR keep track of each dentry.
> 

No, I did write that code as well and then ditched it.

Unfortunately I don't think it would help you, because we still need to
add some identifier to the file names and preferably we should add that
to the single case as well to make things consistent.

> > 
> > As such, I think the practical path forward is that we merge the
> > multi-DP series as currently proposed. This will not cause any issues on
> > single-DP systems, but on multi-DP systems we will have warnings about
> > duplicate debugfs entries in the kernel logs.
> > 
> > Then you can figure out how to rework igt to deal with the multiple DP
> > instances and update the dp_debug interface accordingly.
> > 
> 
> Fine with me, I will take care of this.
> 

Cool, thanks.

Regards,
Bjorn

> > 
> > Which also implies that we should hold this patch back. But if we go
> > that path, I think we should fix dp_debug_deinit() so that it doesn't
> > remove /sys/kernel/debug/dri/128 when the DP driver is unloaded.
> Yes, lets hold this patch back till I fix multi-DP for IGT.
> > 
> > Regards,
> > Bjorn
> > 
> > > > ---
> > > >
> > > > 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;
> > > >  }

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

[abhinavk]

On 2021-10-18 11:36, Bjorn Andersson wrote:
> On Mon 18 Oct 11:07 PDT 2021, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> On 2021-10-17 09:42, Bjorn Andersson wrote:
>> > On Fri 15 Oct 16:53 PDT 2021, abhinavk@codeaurora.org wrote:
>> >
>> > > 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?
>> > >
>> >
>> > I'm afraid I don't see what you're proposing.
>> >
>> > Afaict we need one set of dp_test{type,active,data} per DP controller,
>> > so even doing this by keeping track of the dentries requires that we
>> > rename the files based on some identifier (id or connector name) - which
>> > will cause igt to break.
>> 
>> Yes, I also thought the same that there needs to be some identifier.
>> 
>> "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"
>> 
>> I guess I misunderstood your statement in the commit text thinking 
>> that you
>> had some other way to keep track of the dentries as it mentioned that
>> use a subdirectory OR keep track of each dentry.
>> 
> 
> No, I did write that code as well and then ditched it.
> 
> Unfortunately I don't think it would help you, because we still need to
> add some identifier to the file names and preferably we should add that
> to the single case as well to make things consistent.
> 
>> >
>> > As such, I think the practical path forward is that we merge the
>> > multi-DP series as currently proposed. This will not cause any issues on
>> > single-DP systems, but on multi-DP systems we will have warnings about
>> > duplicate debugfs entries in the kernel logs.
>> >
>> > Then you can figure out how to rework igt to deal with the multiple DP
>> > instances and update the dp_debug interface accordingly.
>> >
>> 
>> Fine with me, I will take care of this.
>> 
> 
> Cool, thanks.
> 
> Regards,
> Bjorn
> 
Following up on this, Rob has posted the igt change today which i acked.
https://patchwork.freedesktop.org/patch/465930/
With this in place, we can actually go ahead with this change.

Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> >
>> > Which also implies that we should hold this patch back. But if we go
>> > that path, I think we should fix dp_debug_deinit() so that it doesn't
>> > remove /sys/kernel/debug/dri/128 when the DP driver is unloaded.
>> Yes, lets hold this patch back till I fix multi-DP for IGT.
>> >
>> > Regards,
>> > Bjorn
>> >
>> > > > ---
>> > > >
>> > > > 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;
>> > > >  }

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

[abhinavk]

On 2021-10-18 11:36, Bjorn Andersson wrote:
> On Mon 18 Oct 11:07 PDT 2021, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> On 2021-10-17 09:42, Bjorn Andersson wrote:
>> > On Fri 15 Oct 16:53 PDT 2021, abhinavk@codeaurora.org wrote:
>> >
>> > > 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?
>> > >
>> >
>> > I'm afraid I don't see what you're proposing.
>> >
>> > Afaict we need one set of dp_test{type,active,data} per DP controller,
>> > so even doing this by keeping track of the dentries requires that we
>> > rename the files based on some identifier (id or connector name) - which
>> > will cause igt to break.
>> 
>> Yes, I also thought the same that there needs to be some identifier.
>> 
>> "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"
>> 
>> I guess I misunderstood your statement in the commit text thinking 
>> that you
>> had some other way to keep track of the dentries as it mentioned that
>> use a subdirectory OR keep track of each dentry.
>> 
> 
> No, I did write that code as well and then ditched it.
> 
> Unfortunately I don't think it would help you, because we still need to
> add some identifier to the file names and preferably we should add that
> to the single case as well to make things consistent.
> 
>> >
>> > As such, I think the practical path forward is that we merge the
>> > multi-DP series as currently proposed. This will not cause any issues on
>> > single-DP systems, but on multi-DP systems we will have warnings about
>> > duplicate debugfs entries in the kernel logs.
>> >
>> > Then you can figure out how to rework igt to deal with the multiple DP
>> > instances and update the dp_debug interface accordingly.
>> >
>> 
>> Fine with me, I will take care of this.
>> 
> 
> Cool, thanks.
> 
> Regards,
> Bjorn
> 
Following up on this, Rob has posted the igt change today which i acked.
https://patchwork.freedesktop.org/patch/465930/
With this in place, we can actually go ahead with this change.

Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> >
>> > Which also implies that we should hold this patch back. But if we go
>> > that path, I think we should fix dp_debug_deinit() so that it doesn't
>> > remove /sys/kernel/debug/dri/128 when the DP driver is unloaded.
>> Yes, lets hold this patch back till I fix multi-DP for IGT.
>> >
>> > Regards,
>> > Bjorn
>> >
>> > > > ---
>> > > >
>> > > > 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;
>> > > >  }