Messages in this thread | | | Date | Wed, 08 Dec 2021 15:54:18 -0800 | From | abhinavk@codeauro ... | Subject | Re: [Freedreno] [PATCH] drm/msm/dp: Move debugfs files into subdirectory |
| |
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; >> > > > }
| |