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

[abhinavk at codeaurora.org]

On 2021-10-18 11:36, Bjorn Andersson wrote:
> On Mon 18 Oct 11:07 PDT 2021, abhinavk at codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> On 2021-10-17 09:42, Bjorn Andersson wrote:
>> > On Fri 15 Oct 16:53 PDT 2021, abhinavk at 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 at 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 at 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;
>> > > >  }