[PATCH] drm/msm/dpu: Remove chatty vbif debug print

[PATCH] drm/msm/dpu: Remove chatty vbif debug print

[Stephen Boyd]

I don't know what this debug print is for but it is super chatty,
throwing 8 lines of debug prints in the logs every time we update a
plane. It looks like it has no value. Let's nuke it so we can get
better logs.

Cc: Sean Paul <sean@poorly.run>
Cc: Abhinav Kumar <abhinavk@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 5e8c3f3e6625..5eb2b2ee09f5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
 	forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl, true);
 
 	for (i = 0; i < qos_tbl->npriority_lvl; i++) {
-		DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
-				params->vbif_idx, params->xin_id, i,
-				qos_tbl->priority_lvl[i]);
 		vbif->ops.set_qos_remap(vbif, params->xin_id, i,
 				qos_tbl->priority_lvl[i]);
 	}
-- 
Sent by a computer, using git, on the internet

[PATCH] drm/msm/dpu: Remove chatty vbif debug print

[Stephen Boyd]

I don't know what this debug print is for but it is super chatty,
throwing 8 lines of debug prints in the logs every time we update a
plane. It looks like it has no value. Let's nuke it so we can get
better logs.

Cc: Sean Paul <sean@poorly.run>
Cc: Abhinav Kumar <abhinavk@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 5e8c3f3e6625..5eb2b2ee09f5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
 	forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl, true);
 
 	for (i = 0; i < qos_tbl->npriority_lvl; i++) {
-		DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
-				params->vbif_idx, params->xin_id, i,
-				qos_tbl->priority_lvl[i]);
 		vbif->ops.set_qos_remap(vbif, params->xin_id, i,
 				qos_tbl->priority_lvl[i]);
 	}
-- 
Sent by a computer, using git, on the internet

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[abhinavk]

On 2020-11-17 09:26, Stephen Boyd wrote:
> I don't know what this debug print is for but it is super chatty,
> throwing 8 lines of debug prints in the logs every time we update a
> plane. It looks like it has no value. Let's nuke it so we can get
> better logs.
> 
> Cc: Sean Paul <sean@poorly.run>
> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> index 5e8c3f3e6625..5eb2b2ee09f5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms 
> *dpu_kms,
>  	forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl, 
> true);
> 
>  	for (i = 0; i < qos_tbl->npriority_lvl; i++) {
> -		DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
> -				params->vbif_idx, params->xin_id, i,
> -				qos_tbl->priority_lvl[i]);

Instead of getting rid of this print, we should optimize the caller of 
this. This is what
we are doing in downstream. So we need to update the property only if we 
are switching from a RT client
to non-RT client for the plane and vice-versa. So we should try to do 
the same thing here.

  	is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
  	if (is_rt != psde->is_rt_pipe) {
  		psde->is_rt_pipe = is_rt;
  		pstate->dirty |= SDE_PLANE_DIRTY_QOS;
  	}


  	if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
  		_dpu_plane_set_qos_remap(plane);

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[abhinavk]

On 2020-11-17 09:26, Stephen Boyd wrote:
> I don't know what this debug print is for but it is super chatty,
> throwing 8 lines of debug prints in the logs every time we update a
> plane. It looks like it has no value. Let's nuke it so we can get
> better logs.
> 
> Cc: Sean Paul <sean@poorly.run>
> Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> index 5e8c3f3e6625..5eb2b2ee09f5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms 
> *dpu_kms,
>  	forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl, 
> true);
> 
>  	for (i = 0; i < qos_tbl->npriority_lvl; i++) {
> -		DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
> -				params->vbif_idx, params->xin_id, i,
> -				qos_tbl->priority_lvl[i]);

Instead of getting rid of this print, we should optimize the caller of 
this. This is what
we are doing in downstream. So we need to update the property only if we 
are switching from a RT client
to non-RT client for the plane and vice-versa. So we should try to do 
the same thing here.

  	is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
  	if (is_rt != psde->is_rt_pipe) {
  		psde->is_rt_pipe = is_rt;
  		pstate->dirty |= SDE_PLANE_DIRTY_QOS;
  	}


  	if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
  		_dpu_plane_set_qos_remap(plane);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[Stephen Boyd]

Quoting abhinavk@codeaurora.org (2020-11-17 12:34:56)
> On 2020-11-17 09:26, Stephen Boyd wrote:
> > I don't know what this debug print is for but it is super chatty,
> > throwing 8 lines of debug prints in the logs every time we update a
> > plane. It looks like it has no value. Let's nuke it so we can get
> > better logs.
> > 
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> 
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > index 5e8c3f3e6625..5eb2b2ee09f5 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms 
> > *dpu_kms,
> >       forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl, 
> > true);
> > 
> >       for (i = 0; i < qos_tbl->npriority_lvl; i++) {
> > -             DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
> > -                             params->vbif_idx, params->xin_id, i,
> > -                             qos_tbl->priority_lvl[i]);
> 
> Instead of getting rid of this print, we should optimize the caller of 
> this.

Does the print tell us anything? Right now it prints 8 lines where it
feels like it could be trimmed down:

           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:0/3
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:1/3
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:2/4
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:3/4
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:4/5
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:5/5
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:6/6
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:7/6

maybe one line that combines the index into values?

           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 [3 3 4 4 5 5 6 6]

But again I have no idea if this print is really useful. Maybe we can
print it only if the value changes from what was already there?
Basically move the print into dpu_hw_set_qos_remap() and then skip out
early if nothing changed or print and modify the register.

> This is what
> we are doing in downstream. So we need to update the property only if we 
> are switching from a RT client
> to non-RT client for the plane and vice-versa. So we should try to do 
> the same thing here.
> 
>         is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
>         if (is_rt != psde->is_rt_pipe) {
>                 psde->is_rt_pipe = is_rt;
>                 pstate->dirty |= SDE_PLANE_DIRTY_QOS;
>         }
> 
> 
>         if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
>                 _dpu_plane_set_qos_remap(plane);
> 

Sounds great! Can you send the patch?

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[Stephen Boyd]

Quoting abhinavk@codeaurora.org (2020-11-17 12:34:56)
> On 2020-11-17 09:26, Stephen Boyd wrote:
> > I don't know what this debug print is for but it is super chatty,
> > throwing 8 lines of debug prints in the logs every time we update a
> > plane. It looks like it has no value. Let's nuke it so we can get
> > better logs.
> > 
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> 
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > index 5e8c3f3e6625..5eb2b2ee09f5 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms 
> > *dpu_kms,
> >       forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl, 
> > true);
> > 
> >       for (i = 0; i < qos_tbl->npriority_lvl; i++) {
> > -             DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
> > -                             params->vbif_idx, params->xin_id, i,
> > -                             qos_tbl->priority_lvl[i]);
> 
> Instead of getting rid of this print, we should optimize the caller of 
> this.

Does the print tell us anything? Right now it prints 8 lines where it
feels like it could be trimmed down:

           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:0/3
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:1/3
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:2/4
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:3/4
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:4/5
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:5/5
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:6/6
           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:7/6

maybe one line that combines the index into values?

           [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 [3 3 4 4 5 5 6 6]

But again I have no idea if this print is really useful. Maybe we can
print it only if the value changes from what was already there?
Basically move the print into dpu_hw_set_qos_remap() and then skip out
early if nothing changed or print and modify the register.

> This is what
> we are doing in downstream. So we need to update the property only if we 
> are switching from a RT client
> to non-RT client for the plane and vice-versa. So we should try to do 
> the same thing here.
> 
>         is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
>         if (is_rt != psde->is_rt_pipe) {
>                 psde->is_rt_pipe = is_rt;
>                 pstate->dirty |= SDE_PLANE_DIRTY_QOS;
>         }
> 
> 
>         if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
>                 _dpu_plane_set_qos_remap(plane);
> 

Sounds great! Can you send the patch?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[Rob Clark]

On Tue, Nov 17, 2020 at 2:53 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting abhinavk@codeaurora.org (2020-11-17 12:34:56)
> > On 2020-11-17 09:26, Stephen Boyd wrote:
> > > I don't know what this debug print is for but it is super chatty,
> > > throwing 8 lines of debug prints in the logs every time we update a
> > > plane. It looks like it has no value. Let's nuke it so we can get
> > > better logs.
> > >
> > > Cc: Sean Paul <sean@poorly.run>
> > > Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> >
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > > index 5e8c3f3e6625..5eb2b2ee09f5 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > > @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms
> > > *dpu_kms,
> > >       forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl,
> > > true);
> > >
> > >       for (i = 0; i < qos_tbl->npriority_lvl; i++) {
> > > -             DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
> > > -                             params->vbif_idx, params->xin_id, i,
> > > -                             qos_tbl->priority_lvl[i]);
> >
> > Instead of getting rid of this print, we should optimize the caller of
> > this.
>
> Does the print tell us anything? Right now it prints 8 lines where it
> feels like it could be trimmed down:
>
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:0/3
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:1/3
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:2/4
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:3/4
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:4/5
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:5/5
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:6/6
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:7/6
>
> maybe one line that combines the index into values?
>
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 [3 3 4 4 5 5 6 6]
>

or possibly convert to a tracepoint (so it doesn't spam the drm_trace buffer)

BR,
-R

> But again I have no idea if this print is really useful. Maybe we can
> print it only if the value changes from what was already there?
> Basically move the print into dpu_hw_set_qos_remap() and then skip out
> early if nothing changed or print and modify the register.
>
> > This is what
> > we are doing in downstream. So we need to update the property only if we
> > are switching from a RT client
> > to non-RT client for the plane and vice-versa. So we should try to do
> > the same thing here.
> >
> >         is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
> >         if (is_rt != psde->is_rt_pipe) {
> >                 psde->is_rt_pipe = is_rt;
> >                 pstate->dirty |= SDE_PLANE_DIRTY_QOS;
> >         }
> >
> >
> >         if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
> >                 _dpu_plane_set_qos_remap(plane);
> >
>
> Sounds great! Can you send the patch?

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[Rob Clark]

On Tue, Nov 17, 2020 at 2:53 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting abhinavk@codeaurora.org (2020-11-17 12:34:56)
> > On 2020-11-17 09:26, Stephen Boyd wrote:
> > > I don't know what this debug print is for but it is super chatty,
> > > throwing 8 lines of debug prints in the logs every time we update a
> > > plane. It looks like it has no value. Let's nuke it so we can get
> > > better logs.
> > >
> > > Cc: Sean Paul <sean@poorly.run>
> > > Cc: Abhinav Kumar <abhinavk@codeaurora.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> >
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > > index 5e8c3f3e6625..5eb2b2ee09f5 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > > @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms
> > > *dpu_kms,
> > >       forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl,
> > > true);
> > >
> > >       for (i = 0; i < qos_tbl->npriority_lvl; i++) {
> > > -             DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
> > > -                             params->vbif_idx, params->xin_id, i,
> > > -                             qos_tbl->priority_lvl[i]);
> >
> > Instead of getting rid of this print, we should optimize the caller of
> > this.
>
> Does the print tell us anything? Right now it prints 8 lines where it
> feels like it could be trimmed down:
>
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:0/3
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:1/3
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:2/4
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:3/4
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:4/5
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:5/5
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:6/6
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:7/6
>
> maybe one line that combines the index into values?
>
>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 [3 3 4 4 5 5 6 6]
>

or possibly convert to a tracepoint (so it doesn't spam the drm_trace buffer)

BR,
-R

> But again I have no idea if this print is really useful. Maybe we can
> print it only if the value changes from what was already there?
> Basically move the print into dpu_hw_set_qos_remap() and then skip out
> early if nothing changed or print and modify the register.
>
> > This is what
> > we are doing in downstream. So we need to update the property only if we
> > are switching from a RT client
> > to non-RT client for the plane and vice-versa. So we should try to do
> > the same thing here.
> >
> >         is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
> >         if (is_rt != psde->is_rt_pipe) {
> >                 psde->is_rt_pipe = is_rt;
> >                 pstate->dirty |= SDE_PLANE_DIRTY_QOS;
> >         }
> >
> >
> >         if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
> >                 _dpu_plane_set_qos_remap(plane);
> >
>
> Sounds great! Can you send the patch?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[abhinavk]

Hi Stephen

On 2020-11-18 07:49, Rob Clark wrote:
> On Tue, Nov 17, 2020 at 2:53 PM Stephen Boyd <swboyd@chromium.org> 
> wrote:
>> 
>> Quoting abhinavk@codeaurora.org (2020-11-17 12:34:56)
>> > On 2020-11-17 09:26, Stephen Boyd wrote:
>> > > I don't know what this debug print is for but it is super chatty,
>> > > throwing 8 lines of debug prints in the logs every time we update a
>> > > plane. It looks like it has no value. Let's nuke it so we can get
>> > > better logs.
>> > >
>> > > Cc: Sean Paul <sean@poorly.run>
>> > > Cc: Abhinav Kumar <abhinavk@codeaurora.org>
>> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>> >
>> > > ---
>> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
>> > >  1 file changed, 3 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> > > index 5e8c3f3e6625..5eb2b2ee09f5 100644
>> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> > > @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms
>> > > *dpu_kms,
>> > >       forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl,
>> > > true);
>> > >
>> > >       for (i = 0; i < qos_tbl->npriority_lvl; i++) {
>> > > -             DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
>> > > -                             params->vbif_idx, params->xin_id, i,
>> > > -                             qos_tbl->priority_lvl[i]);
>> >
>> > Instead of getting rid of this print, we should optimize the caller of
>> > this.
>> 
>> Does the print tell us anything? Right now it prints 8 lines where it
>> feels like it could be trimmed down:
>> 
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:0/3
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:1/3
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:2/4
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:3/4
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:4/5
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:5/5
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:6/6
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:7/6
>> 
>> maybe one line that combines the index into values?
>> 
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 [3 3 4 4 5 5 6 6]
>> 
> 
> or possibly convert to a tracepoint (so it doesn't spam the drm_trace 
> buffer)
> 
> BR,
> -R
> 
>> But again I have no idea if this print is really useful. Maybe we can
>> print it only if the value changes from what was already there?
>> Basically move the print into dpu_hw_set_qos_remap() and then skip out
>> early if nothing changed or print and modify the register.
>> 
>> > This is what
>> > we are doing in downstream. So we need to update the property only if we
>> > are switching from a RT client
>> > to non-RT client for the plane and vice-versa. So we should try to do
>> > the same thing here.
>> >
>> >         is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
>> >         if (is_rt != psde->is_rt_pipe) {
>> >                 psde->is_rt_pipe = is_rt;
>> >                 pstate->dirty |= SDE_PLANE_DIRTY_QOS;
>> >         }
>> >
>> >
>> >         if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
>> >                 _dpu_plane_set_qos_remap(plane);
>> >
>> 
>> Sounds great! Can you send the patch?

Will finalize approach and send the patch in a day or two.

Thanks

Abhinav

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[abhinavk]

Hi Stephen

On 2020-11-18 07:49, Rob Clark wrote:
> On Tue, Nov 17, 2020 at 2:53 PM Stephen Boyd <swboyd@chromium.org> 
> wrote:
>> 
>> Quoting abhinavk@codeaurora.org (2020-11-17 12:34:56)
>> > On 2020-11-17 09:26, Stephen Boyd wrote:
>> > > I don't know what this debug print is for but it is super chatty,
>> > > throwing 8 lines of debug prints in the logs every time we update a
>> > > plane. It looks like it has no value. Let's nuke it so we can get
>> > > better logs.
>> > >
>> > > Cc: Sean Paul <sean@poorly.run>
>> > > Cc: Abhinav Kumar <abhinavk@codeaurora.org>
>> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>> >
>> > > ---
>> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
>> > >  1 file changed, 3 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> > > index 5e8c3f3e6625..5eb2b2ee09f5 100644
>> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> > > @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms
>> > > *dpu_kms,
>> > >       forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl,
>> > > true);
>> > >
>> > >       for (i = 0; i < qos_tbl->npriority_lvl; i++) {
>> > > -             DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
>> > > -                             params->vbif_idx, params->xin_id, i,
>> > > -                             qos_tbl->priority_lvl[i]);
>> >
>> > Instead of getting rid of this print, we should optimize the caller of
>> > this.
>> 
>> Does the print tell us anything? Right now it prints 8 lines where it
>> feels like it could be trimmed down:
>> 
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:0/3
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:1/3
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:2/4
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:3/4
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:4/5
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:5/5
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:6/6
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:7/6
>> 
>> maybe one line that combines the index into values?
>> 
>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 [3 3 4 4 5 5 6 6]
>> 
> 
> or possibly convert to a tracepoint (so it doesn't spam the drm_trace 
> buffer)
> 
> BR,
> -R
> 
>> But again I have no idea if this print is really useful. Maybe we can
>> print it only if the value changes from what was already there?
>> Basically move the print into dpu_hw_set_qos_remap() and then skip out
>> early if nothing changed or print and modify the register.
>> 
>> > This is what
>> > we are doing in downstream. So we need to update the property only if we
>> > are switching from a RT client
>> > to non-RT client for the plane and vice-versa. So we should try to do
>> > the same thing here.
>> >
>> >         is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
>> >         if (is_rt != psde->is_rt_pipe) {
>> >                 psde->is_rt_pipe = is_rt;
>> >                 pstate->dirty |= SDE_PLANE_DIRTY_QOS;
>> >         }
>> >
>> >
>> >         if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
>> >                 _dpu_plane_set_qos_remap(plane);
>> >
>> 
>> Sounds great! Can you send the patch?

Will finalize approach and send the patch in a day or two.

Thanks

Abhinav
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[abhinavk]

On 2020-11-18 12:03, abhinavk@codeaurora.org wrote:
> Hi Stephen
> 
> On 2020-11-18 07:49, Rob Clark wrote:
>> On Tue, Nov 17, 2020 at 2:53 PM Stephen Boyd <swboyd@chromium.org> 
>> wrote:
>>> 
>>> Quoting abhinavk@codeaurora.org (2020-11-17 12:34:56)
>>> > On 2020-11-17 09:26, Stephen Boyd wrote:
>>> > > I don't know what this debug print is for but it is super chatty,
>>> > > throwing 8 lines of debug prints in the logs every time we update a
>>> > > plane. It looks like it has no value. Let's nuke it so we can get
>>> > > better logs.
>>> > >
>>> > > Cc: Sean Paul <sean@poorly.run>
>>> > > Cc: Abhinav Kumar <abhinavk@codeaurora.org>
>>> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>> >
>>> > > ---
>>> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
>>> > >  1 file changed, 3 deletions(-)
>>> > >
>>> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> > > index 5e8c3f3e6625..5eb2b2ee09f5 100644
>>> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> > > @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms
>>> > > *dpu_kms,
>>> > >       forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl,
>>> > > true);
>>> > >
>>> > >       for (i = 0; i < qos_tbl->npriority_lvl; i++) {
>>> > > -             DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
>>> > > -                             params->vbif_idx, params->xin_id, i,
>>> > > -                             qos_tbl->priority_lvl[i]);
>>> >
>>> > Instead of getting rid of this print, we should optimize the caller of
>>> > this.
>>> 
>>> Does the print tell us anything? Right now it prints 8 lines where it
>>> feels like it could be trimmed down:
>>> 
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:0/3
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:1/3
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:2/4
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:3/4
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:4/5
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:5/5
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:6/6
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:7/6
>>> 
>>> maybe one line that combines the index into values?
>>> 
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 [3 3 4 4 5 5 6 
>>> 6]
>>> 
>> 
>> or possibly convert to a tracepoint (so it doesn't spam the drm_trace 
>> buffer)
>> 
>> BR,
>> -R
>> 
>>> But again I have no idea if this print is really useful. Maybe we can
>>> print it only if the value changes from what was already there?
>>> Basically move the print into dpu_hw_set_qos_remap() and then skip 
>>> out
>>> early if nothing changed or print and modify the register.
>>> 
>>> > This is what
>>> > we are doing in downstream. So we need to update the property only if we
>>> > are switching from a RT client
>>> > to non-RT client for the plane and vice-versa. So we should try to do
>>> > the same thing here.
>>> >
>>> >         is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
>>> >         if (is_rt != psde->is_rt_pipe) {
>>> >                 psde->is_rt_pipe = is_rt;
>>> >                 pstate->dirty |= SDE_PLANE_DIRTY_QOS;
>>> >         }
>>> >
>>> >
>>> >         if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
>>> >                 _dpu_plane_set_qos_remap(plane);
>>> >
>>> 
>>> Sounds great! Can you send the patch?
> 
> Will finalize approach and send the patch in a day or two.
> 
> Thanks
> 
> Abhinav

patch has been posted here for review : 
https://patchwork.freedesktop.org/patch/401929/

Re: [Freedreno] [PATCH] drm/msm/dpu: Remove chatty vbif debug print

[abhinavk]

On 2020-11-18 12:03, abhinavk@codeaurora.org wrote:
> Hi Stephen
> 
> On 2020-11-18 07:49, Rob Clark wrote:
>> On Tue, Nov 17, 2020 at 2:53 PM Stephen Boyd <swboyd@chromium.org> 
>> wrote:
>>> 
>>> Quoting abhinavk@codeaurora.org (2020-11-17 12:34:56)
>>> > On 2020-11-17 09:26, Stephen Boyd wrote:
>>> > > I don't know what this debug print is for but it is super chatty,
>>> > > throwing 8 lines of debug prints in the logs every time we update a
>>> > > plane. It looks like it has no value. Let's nuke it so we can get
>>> > > better logs.
>>> > >
>>> > > Cc: Sean Paul <sean@poorly.run>
>>> > > Cc: Abhinav Kumar <abhinavk@codeaurora.org>
>>> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>> >
>>> > > ---
>>> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 3 ---
>>> > >  1 file changed, 3 deletions(-)
>>> > >
>>> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> > > index 5e8c3f3e6625..5eb2b2ee09f5 100644
>>> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> > > @@ -245,9 +245,6 @@ void dpu_vbif_set_qos_remap(struct dpu_kms
>>> > > *dpu_kms,
>>> > >       forced_on = mdp->ops.setup_clk_force_ctrl(mdp, params->clk_ctrl,
>>> > > true);
>>> > >
>>> > >       for (i = 0; i < qos_tbl->npriority_lvl; i++) {
>>> > > -             DPU_DEBUG("vbif:%d xin:%d lvl:%d/%d\n",
>>> > > -                             params->vbif_idx, params->xin_id, i,
>>> > > -                             qos_tbl->priority_lvl[i]);
>>> >
>>> > Instead of getting rid of this print, we should optimize the caller of
>>> > this.
>>> 
>>> Does the print tell us anything? Right now it prints 8 lines where it
>>> feels like it could be trimmed down:
>>> 
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:0/3
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:1/3
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:2/4
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:3/4
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:4/5
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:5/5
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:6/6
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 lvl:7/6
>>> 
>>> maybe one line that combines the index into values?
>>> 
>>>            [drm:dpu_vbif_set_qos_remap] vbif:0 xin:0 [3 3 4 4 5 5 6 
>>> 6]
>>> 
>> 
>> or possibly convert to a tracepoint (so it doesn't spam the drm_trace 
>> buffer)
>> 
>> BR,
>> -R
>> 
>>> But again I have no idea if this print is really useful. Maybe we can
>>> print it only if the value changes from what was already there?
>>> Basically move the print into dpu_hw_set_qos_remap() and then skip 
>>> out
>>> early if nothing changed or print and modify the register.
>>> 
>>> > This is what
>>> > we are doing in downstream. So we need to update the property only if we
>>> > are switching from a RT client
>>> > to non-RT client for the plane and vice-versa. So we should try to do
>>> > the same thing here.
>>> >
>>> >         is_rt = sde_crtc_is_rt_client(crtc, crtc->state);
>>> >         if (is_rt != psde->is_rt_pipe) {
>>> >                 psde->is_rt_pipe = is_rt;
>>> >                 pstate->dirty |= SDE_PLANE_DIRTY_QOS;
>>> >         }
>>> >
>>> >
>>> >         if (pstate->dirty & DPU_PLANE_DIRTY_QOS)
>>> >                 _dpu_plane_set_qos_remap(plane);
>>> >
>>> 
>>> Sounds great! Can you send the patch?
> 
> Will finalize approach and send the patch in a day or two.
> 
> Thanks
> 
> Abhinav

patch has been posted here for review : 
https://patchwork.freedesktop.org/patch/401929/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel