[PATCH] drm/msm/dp: Only create debugfs for PRIMARY minor

[PATCH] drm/msm/dp: Only create debugfs for PRIMARY minor

[Bjorn Andersson]

dpu_kms_debugfs_init() and hence dp_debug_get() gets invoked for each
minor being registered. But dp_debug_get() will allocate a new struct
dp_debug for each call and this will be associated as dp->debug.

As such dp_debug will create debugfs files in both the PRIMARY and the
RENDER minor's debugfs directory, but only the last reference will be
remembered.

The only use of this reference today is in the cleanup path in
dp_display_deinit_sub_modules() and the dp_debug_private object does
outlive the debugfs entries in either case, so there doesn't seem to be
any adverse effects of this, but per the code the current behavior is
unexpected, so change it to only create dp_debug for the PRIMARY minor.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3aa67c53dbc0..06773b58bb60 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
 #include <linux/component.h>
 #include <linux/of_irq.h>
 #include <linux/delay.h>
+#include <drm/drm_file.h>
 #include <drm/drm_panel.h>
 
 #include "msm_drv.h"
@@ -1463,6 +1464,10 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 	dev = &dp->pdev->dev;
 
+	/* Only create one set of debugfs per DP instance */
+	if (minor->type != DRM_MINOR_PRIMARY)
+		return;
+
 	dp->debug = dp_debug_get(dev, dp->panel, dp->usbpd,
 					dp->link, dp->dp_display.connector,
 					minor);
-- 
2.29.2

Re: [Freedreno] [PATCH] drm/msm/dp: Only create debugfs for PRIMARY minor

[abhinavk]

On 2021-10-15 16:13, Bjorn Andersson wrote:
> dpu_kms_debugfs_init() and hence dp_debug_get() gets invoked for each
> minor being registered. But dp_debug_get() will allocate a new struct
> dp_debug for each call and this will be associated as dp->debug.
> 
> As such dp_debug will create debugfs files in both the PRIMARY and the
> RENDER minor's debugfs directory, but only the last reference will be
> remembered.
> 
> The only use of this reference today is in the cleanup path in
> dp_display_deinit_sub_modules() and the dp_debug_private object does
> outlive the debugfs entries in either case, so there doesn't seem to be
> any adverse effects of this, but per the code the current behavior is
> unexpected, so change it to only create dp_debug for the PRIMARY minor.
> 

If i understand correctly, today because of this, we get redundant 
debugfs nodes right?

/sys/kernel/debug/dri/<minor_x>/dp_debug
/sys/kernel/debug/dri/<minor_y>/dp_debug

Both of these will hold the same information as they are for the same DP 
controller right?
In that case, this is true even for the other DPU kms information too.

Why not move this check one level up to dpu_kms_debugfs_init?

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 3aa67c53dbc0..06773b58bb60 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -10,6 +10,7 @@
>  #include <linux/component.h>
>  #include <linux/of_irq.h>
>  #include <linux/delay.h>
> +#include <drm/drm_file.h>
>  #include <drm/drm_panel.h>
> 
>  #include "msm_drv.h"
> @@ -1463,6 +1464,10 @@ void msm_dp_debugfs_init(struct msm_dp
> *dp_display, struct drm_minor *minor)
>  	dp = container_of(dp_display, struct dp_display_private, dp_display);
>  	dev = &dp->pdev->dev;
> 
> +	/* Only create one set of debugfs per DP instance */
> +	if (minor->type != DRM_MINOR_PRIMARY)
> +		return;
> +
>  	dp->debug = dp_debug_get(dev, dp->panel, dp->usbpd,
>  					dp->link, dp->dp_display.connector,
>  					minor);

Re: [Freedreno] [PATCH] drm/msm/dp: Only create debugfs for PRIMARY minor

[Bjorn Andersson]

On Fri 15 Oct 18:44 CDT 2021, abhinavk@codeaurora.org wrote:

> On 2021-10-15 16:13, Bjorn Andersson wrote:
> > dpu_kms_debugfs_init() and hence dp_debug_get() gets invoked for each
> > minor being registered. But dp_debug_get() will allocate a new struct
> > dp_debug for each call and this will be associated as dp->debug.
> > 
> > As such dp_debug will create debugfs files in both the PRIMARY and the
> > RENDER minor's debugfs directory, but only the last reference will be
> > remembered.
> > 
> > The only use of this reference today is in the cleanup path in
> > dp_display_deinit_sub_modules() and the dp_debug_private object does
> > outlive the debugfs entries in either case, so there doesn't seem to be
> > any adverse effects of this, but per the code the current behavior is
> > unexpected, so change it to only create dp_debug for the PRIMARY minor.
> > 
> 
> If i understand correctly, today because of this, we get redundant debugfs
> nodes right?
> 
> /sys/kernel/debug/dri/<minor_x>/dp_debug
> /sys/kernel/debug/dri/<minor_y>/dp_debug
> 
> Both of these will hold the same information as they are for the same DP
> controller right?

That's correct, all the dp_debug debugfs files end up in both minors.

The problem is that dp->debug points to the dp_debug struct for one of
them, which afaict doesn't have any really bad effects today - unless
you try to clean up the dp_debug state, but that will clear out the
entire dri/<minor_y>...

> In that case, this is true even for the other DPU kms information too.
> 
> Why not move this check one level up to dpu_kms_debugfs_init?
> 

I was expecting that perhaps some of this information was
minor-specific, but if that isn't the case it sounds reasonable that we
should push this one step up.

Regards,
Bjorn

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 3aa67c53dbc0..06773b58bb60 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/component.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/delay.h>
> > +#include <drm/drm_file.h>
> >  #include <drm/drm_panel.h>
> > 
> >  #include "msm_drv.h"
> > @@ -1463,6 +1464,10 @@ void msm_dp_debugfs_init(struct msm_dp
> > *dp_display, struct drm_minor *minor)
> >  	dp = container_of(dp_display, struct dp_display_private, dp_display);
> >  	dev = &dp->pdev->dev;
> > 
> > +	/* Only create one set of debugfs per DP instance */
> > +	if (minor->type != DRM_MINOR_PRIMARY)
> > +		return;
> > +
> >  	dp->debug = dp_debug_get(dev, dp->panel, dp->usbpd,
> >  					dp->link, dp->dp_display.connector,
> >  					minor);