diff mbox series

drm/msm/dpu: update the qos remap only if the client type changes

Message ID 20201119214145.10182-1-abhinavk@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/dpu: update the qos remap only if the client type changes | expand

Commit Message

Abhinav Kumar Nov. 19, 2020, 9:41 p.m. UTC
Update the qos remap only if the client type changes for the plane.
This will avoid unnecessary register programming and also avoid log
spam from the dpu_vbif_set_qos_remap() function.

Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 17 +++++++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  7 +++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 12 ++++++++++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |  5 +++++
 5 files changed, 41 insertions(+), 2 deletions(-)

Comments

Rob Clark Nov. 23, 2020, 11:18 p.m. UTC | #1
On Thu, Nov 19, 2020 at 1:41 PM Abhinav Kumar <abhinavk@codeaurora.org> wrote:
>
> Update the qos remap only if the client type changes for the plane.
> This will avoid unnecessary register programming and also avoid log
> spam from the dpu_vbif_set_qos_remap() function.
>
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 17 +++++++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  7 +++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 ++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 12 ++++++++++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |  5 +++++
>  5 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index d4662e8184cc..3867da47c683 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1037,6 +1037,23 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>         return rc;
>  }
>
> +void dpu_crtc_set_qos_dirty(struct drm_crtc *crtc)
> +{
> +       struct drm_plane *plane;
> +       struct drm_plane_state *state;
> +       struct dpu_plane_state *pstate;
> +
> +       drm_atomic_crtc_for_each_plane(plane, crtc) {
> +               state = plane->state;
> +               if (!state)
> +                       continue;
> +
> +               pstate = to_dpu_plane_state(state);
> +
> +               pstate->dirty |= DPU_PLANE_DIRTY_QOS;
> +       }
> +}
> +
>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
>  {
>         struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index cec3474340e8..8ba11de605bc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -231,6 +231,13 @@ static inline int dpu_crtc_frame_pending(struct drm_crtc *crtc)
>   */
>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
>
> +/**
> + * dpu_crtc_set_qos_dirty - update plane dirty flag to include
> + * QoS reprogramming
> + * @crtc: Pointer to drm crtc structure
> + */
> +void dpu_crtc_set_qos_dirty(struct drm_crtc *crtc);
> +
>  /**
>   * dpu_crtc_vblank_callback - called on vblank irq, issues completion events
>   * @crtc: Pointer to drm crtc object
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index f7f5c258b553..c2db9dd6ec67 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1001,6 +1001,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>
>         trace_dpu_enc_mode_set(DRMID(drm_enc));
>
> +       dpu_crtc_set_qos_dirty(drm_enc->crtc);
> +
>         if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
>                 msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 7ea90d25a3b6..f91d31a31e14 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1066,6 +1066,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>         struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>         struct drm_crtc *crtc = state->crtc;
>         struct drm_framebuffer *fb = state->fb;
> +       bool is_rt_pipe;
>         const struct dpu_format *fmt =
>                 to_dpu_format(msm_framebuffer_format(fb));
>
> @@ -1075,7 +1076,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>
>         pstate->pending = true;
>
> -       pdpu->is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
> +       is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
>         _dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
>
>         DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
> @@ -1181,8 +1182,15 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>                 _dpu_plane_set_ot_limit(plane, crtc);
>         }
>
> -       _dpu_plane_set_qos_remap(plane);
> +       if (is_rt_pipe != pdpu->is_rt_pipe) {
> +               pdpu->is_rt_pipe = is_rt_pipe;
> +               pstate->dirty |= DPU_PLANE_DIRTY_QOS;
> +       }
>
> +       if (pstate->dirty & DPU_PLANE_DIRTY_QOS) {
> +               _dpu_plane_set_qos_remap(plane);
> +               pstate->dirty = 0x0;
> +       }

So in the end, this looks roughly like "set qos remap on modesets or
switching between right/left pipe"?  Couldn't this be simpler if in
plane->atomic_check() you do something like:

   dpu_plane_state->needs_qos_remap = atomic_state->allow_modeset;

and then in plane->atomic_update:

    if (pstate->needs_qos_remap || (is_rt_pipe != pdpu->is_rt_pipe) {
         pdpu->is_rt_pipe = is_rt_pipe;
         _dpu_plane_set_qos_remap(plane)
    }

?

BR,
-R


>         _dpu_plane_calc_bw(plane, fb);
>
>         _dpu_plane_calc_clk(plane);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> index ca83b8753d59..47abd3686a86 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> @@ -14,11 +14,15 @@
>  #include "dpu_hw_mdss.h"
>  #include "dpu_hw_sspp.h"
>
> +/* dirty bits to update QOS */
> +#define DPU_PLANE_DIRTY_QOS 0x1
> +
>  /**
>   * struct dpu_plane_state: Define dpu extension of drm plane state object
>   * @base:      base drm plane state object
>   * @aspace:    pointer to address space for input/output buffers
>   * @stage:     assigned by crtc blender
> + * @dirty:     bitmask for which pipe h/w config functions need to be updated
>   * @multirect_index: index of the rectangle of SSPP
>   * @multirect_mode: parallel or time multiplex multirect mode
>   * @pending:   whether the current update is still pending
> @@ -32,6 +36,7 @@ struct dpu_plane_state {
>         struct drm_plane_state base;
>         struct msm_gem_address_space *aspace;
>         enum dpu_stage stage;
> +       uint32_t dirty;
>         uint32_t multirect_index;
>         uint32_t multirect_mode;
>         bool pending;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Abhinav Kumar Nov. 24, 2020, 12:38 a.m. UTC | #2
Hi Rob

On 2020-11-23 15:18, Rob Clark wrote:
> On Thu, Nov 19, 2020 at 1:41 PM Abhinav Kumar <abhinavk@codeaurora.org> 
> wrote:
>> 
>> Update the qos remap only if the client type changes for the plane.
>> This will avoid unnecessary register programming and also avoid log
>> spam from the dpu_vbif_set_qos_remap() function.
>> 
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 17 +++++++++++++++++
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  7 +++++++
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 ++
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 12 ++++++++++--
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |  5 +++++
>>  5 files changed, 41 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index d4662e8184cc..3867da47c683 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -1037,6 +1037,23 @@ static int dpu_crtc_atomic_check(struct 
>> drm_crtc *crtc,
>>         return rc;
>>  }
>> 
>> +void dpu_crtc_set_qos_dirty(struct drm_crtc *crtc)
>> +{
>> +       struct drm_plane *plane;
>> +       struct drm_plane_state *state;
>> +       struct dpu_plane_state *pstate;
>> +
>> +       drm_atomic_crtc_for_each_plane(plane, crtc) {
>> +               state = plane->state;
>> +               if (!state)
>> +                       continue;
>> +
>> +               pstate = to_dpu_plane_state(state);
>> +
>> +               pstate->dirty |= DPU_PLANE_DIRTY_QOS;
>> +       }
>> +}
>> +
>>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
>>  {
>>         struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index cec3474340e8..8ba11de605bc 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -231,6 +231,13 @@ static inline int dpu_crtc_frame_pending(struct 
>> drm_crtc *crtc)
>>   */
>>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
>> 
>> +/**
>> + * dpu_crtc_set_qos_dirty - update plane dirty flag to include
>> + * QoS reprogramming
>> + * @crtc: Pointer to drm crtc structure
>> + */
>> +void dpu_crtc_set_qos_dirty(struct drm_crtc *crtc);
>> +
>>  /**
>>   * dpu_crtc_vblank_callback - called on vblank irq, issues completion 
>> events
>>   * @crtc: Pointer to drm crtc object
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index f7f5c258b553..c2db9dd6ec67 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1001,6 +1001,8 @@ static void dpu_encoder_virt_mode_set(struct 
>> drm_encoder *drm_enc,
>> 
>>         trace_dpu_enc_mode_set(DRMID(drm_enc));
>> 
>> +       dpu_crtc_set_qos_dirty(drm_enc->crtc);
>> +
>>         if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && 
>> priv->dp)
>>                 msm_dp_display_mode_set(priv->dp, drm_enc, mode, 
>> adj_mode);
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> index 7ea90d25a3b6..f91d31a31e14 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> @@ -1066,6 +1066,7 @@ static void dpu_plane_sspp_atomic_update(struct 
>> drm_plane *plane)
>>         struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>         struct drm_crtc *crtc = state->crtc;
>>         struct drm_framebuffer *fb = state->fb;
>> +       bool is_rt_pipe;
>>         const struct dpu_format *fmt =
>>                 to_dpu_format(msm_framebuffer_format(fb));
>> 
>> @@ -1075,7 +1076,7 @@ static void dpu_plane_sspp_atomic_update(struct 
>> drm_plane *plane)
>> 
>>         pstate->pending = true;
>> 
>> -       pdpu->is_rt_pipe = (dpu_crtc_get_client_type(crtc) != 
>> NRT_CLIENT);
>> +       is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
>>         _dpu_plane_set_qos_ctrl(plane, false, 
>> DPU_PLANE_QOS_PANIC_CTRL);
>> 
>>         DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " 
>> DRM_RECT_FMT
>> @@ -1181,8 +1182,15 @@ static void dpu_plane_sspp_atomic_update(struct 
>> drm_plane *plane)
>>                 _dpu_plane_set_ot_limit(plane, crtc);
>>         }
>> 
>> -       _dpu_plane_set_qos_remap(plane);
>> +       if (is_rt_pipe != pdpu->is_rt_pipe) {
>> +               pdpu->is_rt_pipe = is_rt_pipe;
>> +               pstate->dirty |= DPU_PLANE_DIRTY_QOS;
>> +       }
>> 
>> +       if (pstate->dirty & DPU_PLANE_DIRTY_QOS) {
>> +               _dpu_plane_set_qos_remap(plane);
>> +               pstate->dirty = 0x0;
>> +       }
> 
> So in the end, this looks roughly like "set qos remap on modesets or
> switching between right/left pipe"?  Couldn't this be simpler if in
> plane->atomic_check() you do something like:
> 
>    dpu_plane_state->needs_qos_remap = atomic_state->allow_modeset;
> 
> and then in plane->atomic_update:
> 
>     if (pstate->needs_qos_remap || (is_rt_pipe != pdpu->is_rt_pipe) {
>          pdpu->is_rt_pipe = is_rt_pipe;
>          _dpu_plane_set_qos_remap(plane)
>     }
> 
> ?
> 
> BR,
> -R
Thanks for the suggestion, Yes this will make it much simpler. Let me 
update it.

Just one clarification, I believe you meant that
dpu_plane_state->needs_qos_remap = 
drm_atomic_crtc_needs_modeset(crtc_state)

and then the rest of it looks fine to me.

> 
> 
>>         _dpu_plane_calc_bw(plane, fb);
>> 
>>         _dpu_plane_calc_clk(plane);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>> index ca83b8753d59..47abd3686a86 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>> @@ -14,11 +14,15 @@
>>  #include "dpu_hw_mdss.h"
>>  #include "dpu_hw_sspp.h"
>> 
>> +/* dirty bits to update QOS */
>> +#define DPU_PLANE_DIRTY_QOS 0x1
>> +
>>  /**
>>   * struct dpu_plane_state: Define dpu extension of drm plane state 
>> object
>>   * @base:      base drm plane state object
>>   * @aspace:    pointer to address space for input/output buffers
>>   * @stage:     assigned by crtc blender
>> + * @dirty:     bitmask for which pipe h/w config functions need to be 
>> updated
>>   * @multirect_index: index of the rectangle of SSPP
>>   * @multirect_mode: parallel or time multiplex multirect mode
>>   * @pending:   whether the current update is still pending
>> @@ -32,6 +36,7 @@ struct dpu_plane_state {
>>         struct drm_plane_state base;
>>         struct msm_gem_address_space *aspace;
>>         enum dpu_stage stage;
>> +       uint32_t dirty;
>>         uint32_t multirect_index;
>>         uint32_t multirect_mode;
>>         bool pending;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>>
Rob Clark Nov. 24, 2020, 1:02 a.m. UTC | #3
On Mon, Nov 23, 2020 at 4:38 PM <abhinavk@codeaurora.org> wrote:
>
> Hi Rob
>
> On 2020-11-23 15:18, Rob Clark wrote:
> > On Thu, Nov 19, 2020 at 1:41 PM Abhinav Kumar <abhinavk@codeaurora.org>
> > wrote:
> >>
> >> Update the qos remap only if the client type changes for the plane.
> >> This will avoid unnecessary register programming and also avoid log
> >> spam from the dpu_vbif_set_qos_remap() function.
> >>
> >> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> >> ---
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 17 +++++++++++++++++
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  7 +++++++
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 ++
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 12 ++++++++++--
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |  5 +++++
> >>  5 files changed, 41 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> index d4662e8184cc..3867da47c683 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> @@ -1037,6 +1037,23 @@ static int dpu_crtc_atomic_check(struct
> >> drm_crtc *crtc,
> >>         return rc;
> >>  }
> >>
> >> +void dpu_crtc_set_qos_dirty(struct drm_crtc *crtc)
> >> +{
> >> +       struct drm_plane *plane;
> >> +       struct drm_plane_state *state;
> >> +       struct dpu_plane_state *pstate;
> >> +
> >> +       drm_atomic_crtc_for_each_plane(plane, crtc) {
> >> +               state = plane->state;
> >> +               if (!state)
> >> +                       continue;
> >> +
> >> +               pstate = to_dpu_plane_state(state);
> >> +
> >> +               pstate->dirty |= DPU_PLANE_DIRTY_QOS;
> >> +       }
> >> +}
> >> +
> >>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
> >>  {
> >>         struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> >> index cec3474340e8..8ba11de605bc 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> >> @@ -231,6 +231,13 @@ static inline int dpu_crtc_frame_pending(struct
> >> drm_crtc *crtc)
> >>   */
> >>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> >>
> >> +/**
> >> + * dpu_crtc_set_qos_dirty - update plane dirty flag to include
> >> + * QoS reprogramming
> >> + * @crtc: Pointer to drm crtc structure
> >> + */
> >> +void dpu_crtc_set_qos_dirty(struct drm_crtc *crtc);
> >> +
> >>  /**
> >>   * dpu_crtc_vblank_callback - called on vblank irq, issues completion
> >> events
> >>   * @crtc: Pointer to drm crtc object
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index f7f5c258b553..c2db9dd6ec67 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -1001,6 +1001,8 @@ static void dpu_encoder_virt_mode_set(struct
> >> drm_encoder *drm_enc,
> >>
> >>         trace_dpu_enc_mode_set(DRMID(drm_enc));
> >>
> >> +       dpu_crtc_set_qos_dirty(drm_enc->crtc);
> >> +
> >>         if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS &&
> >> priv->dp)
> >>                 msm_dp_display_mode_set(priv->dp, drm_enc, mode,
> >> adj_mode);
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> index 7ea90d25a3b6..f91d31a31e14 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >> @@ -1066,6 +1066,7 @@ static void dpu_plane_sspp_atomic_update(struct
> >> drm_plane *plane)
> >>         struct dpu_plane_state *pstate = to_dpu_plane_state(state);
> >>         struct drm_crtc *crtc = state->crtc;
> >>         struct drm_framebuffer *fb = state->fb;
> >> +       bool is_rt_pipe;
> >>         const struct dpu_format *fmt =
> >>                 to_dpu_format(msm_framebuffer_format(fb));
> >>
> >> @@ -1075,7 +1076,7 @@ static void dpu_plane_sspp_atomic_update(struct
> >> drm_plane *plane)
> >>
> >>         pstate->pending = true;
> >>
> >> -       pdpu->is_rt_pipe = (dpu_crtc_get_client_type(crtc) !=
> >> NRT_CLIENT);
> >> +       is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
> >>         _dpu_plane_set_qos_ctrl(plane, false,
> >> DPU_PLANE_QOS_PANIC_CTRL);
> >>
> >>         DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u "
> >> DRM_RECT_FMT
> >> @@ -1181,8 +1182,15 @@ static void dpu_plane_sspp_atomic_update(struct
> >> drm_plane *plane)
> >>                 _dpu_plane_set_ot_limit(plane, crtc);
> >>         }
> >>
> >> -       _dpu_plane_set_qos_remap(plane);
> >> +       if (is_rt_pipe != pdpu->is_rt_pipe) {
> >> +               pdpu->is_rt_pipe = is_rt_pipe;
> >> +               pstate->dirty |= DPU_PLANE_DIRTY_QOS;
> >> +       }
> >>
> >> +       if (pstate->dirty & DPU_PLANE_DIRTY_QOS) {
> >> +               _dpu_plane_set_qos_remap(plane);
> >> +               pstate->dirty = 0x0;
> >> +       }
> >
> > So in the end, this looks roughly like "set qos remap on modesets or
> > switching between right/left pipe"?  Couldn't this be simpler if in
> > plane->atomic_check() you do something like:
> >
> >    dpu_plane_state->needs_qos_remap = atomic_state->allow_modeset;
> >
> > and then in plane->atomic_update:
> >
> >     if (pstate->needs_qos_remap || (is_rt_pipe != pdpu->is_rt_pipe) {
> >          pdpu->is_rt_pipe = is_rt_pipe;
> >          _dpu_plane_set_qos_remap(plane)
> >     }
> >
> > ?
> >
> > BR,
> > -R
> Thanks for the suggestion, Yes this will make it much simpler. Let me
> update it.
>
> Just one clarification, I believe you meant that
> dpu_plane_state->needs_qos_remap =
> drm_atomic_crtc_needs_modeset(crtc_state)

yeah, that looks better.. thx

BR,
-R

> and then the rest of it looks fine to me.
>
> >
> >
> >>         _dpu_plane_calc_bw(plane, fb);
> >>
> >>         _dpu_plane_calc_clk(plane);
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> >> index ca83b8753d59..47abd3686a86 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> >> @@ -14,11 +14,15 @@
> >>  #include "dpu_hw_mdss.h"
> >>  #include "dpu_hw_sspp.h"
> >>
> >> +/* dirty bits to update QOS */
> >> +#define DPU_PLANE_DIRTY_QOS 0x1
> >> +
> >>  /**
> >>   * struct dpu_plane_state: Define dpu extension of drm plane state
> >> object
> >>   * @base:      base drm plane state object
> >>   * @aspace:    pointer to address space for input/output buffers
> >>   * @stage:     assigned by crtc blender
> >> + * @dirty:     bitmask for which pipe h/w config functions need to be
> >> updated
> >>   * @multirect_index: index of the rectangle of SSPP
> >>   * @multirect_mode: parallel or time multiplex multirect mode
> >>   * @pending:   whether the current update is still pending
> >> @@ -32,6 +36,7 @@ struct dpu_plane_state {
> >>         struct drm_plane_state base;
> >>         struct msm_gem_address_space *aspace;
> >>         enum dpu_stage stage;
> >> +       uint32_t dirty;
> >>         uint32_t multirect_index;
> >>         uint32_t multirect_mode;
> >>         bool pending;
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> >> Forum,
> >> a Linux Foundation Collaborative Project
> >>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index d4662e8184cc..3867da47c683 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1037,6 +1037,23 @@  static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 	return rc;
 }
 
+void dpu_crtc_set_qos_dirty(struct drm_crtc *crtc)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *state;
+	struct dpu_plane_state *pstate;
+
+	drm_atomic_crtc_for_each_plane(plane, crtc) {
+		state = plane->state;
+		if (!state)
+			continue;
+
+		pstate = to_dpu_plane_state(state);
+
+		pstate->dirty |= DPU_PLANE_DIRTY_QOS;
+	}
+}
+
 int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
 {
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index cec3474340e8..8ba11de605bc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -231,6 +231,13 @@  static inline int dpu_crtc_frame_pending(struct drm_crtc *crtc)
  */
 int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
 
+/**
+ * dpu_crtc_set_qos_dirty - update plane dirty flag to include
+ * QoS reprogramming
+ * @crtc: Pointer to drm crtc structure
+ */
+void dpu_crtc_set_qos_dirty(struct drm_crtc *crtc);
+
 /**
  * dpu_crtc_vblank_callback - called on vblank irq, issues completion events
  * @crtc: Pointer to drm crtc object
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index f7f5c258b553..c2db9dd6ec67 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1001,6 +1001,8 @@  static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 
 	trace_dpu_enc_mode_set(DRMID(drm_enc));
 
+	dpu_crtc_set_qos_dirty(drm_enc->crtc);
+
 	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
 		msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 7ea90d25a3b6..f91d31a31e14 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1066,6 +1066,7 @@  static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
 	struct dpu_plane_state *pstate = to_dpu_plane_state(state);
 	struct drm_crtc *crtc = state->crtc;
 	struct drm_framebuffer *fb = state->fb;
+	bool is_rt_pipe;
 	const struct dpu_format *fmt =
 		to_dpu_format(msm_framebuffer_format(fb));
 
@@ -1075,7 +1076,7 @@  static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
 
 	pstate->pending = true;
 
-	pdpu->is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
+	is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
 	_dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
 
 	DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
@@ -1181,8 +1182,15 @@  static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
 		_dpu_plane_set_ot_limit(plane, crtc);
 	}
 
-	_dpu_plane_set_qos_remap(plane);
+	if (is_rt_pipe != pdpu->is_rt_pipe) {
+		pdpu->is_rt_pipe = is_rt_pipe;
+		pstate->dirty |= DPU_PLANE_DIRTY_QOS;
+	}
 
+	if (pstate->dirty & DPU_PLANE_DIRTY_QOS) {
+		_dpu_plane_set_qos_remap(plane);
+		pstate->dirty = 0x0;
+	}
 	_dpu_plane_calc_bw(plane, fb);
 
 	_dpu_plane_calc_clk(plane);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
index ca83b8753d59..47abd3686a86 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
@@ -14,11 +14,15 @@ 
 #include "dpu_hw_mdss.h"
 #include "dpu_hw_sspp.h"
 
+/* dirty bits to update QOS */
+#define DPU_PLANE_DIRTY_QOS 0x1
+
 /**
  * struct dpu_plane_state: Define dpu extension of drm plane state object
  * @base:	base drm plane state object
  * @aspace:	pointer to address space for input/output buffers
  * @stage:	assigned by crtc blender
+ * @dirty:	bitmask for which pipe h/w config functions need to be updated
  * @multirect_index: index of the rectangle of SSPP
  * @multirect_mode: parallel or time multiplex multirect mode
  * @pending:	whether the current update is still pending
@@ -32,6 +36,7 @@  struct dpu_plane_state {
 	struct drm_plane_state base;
 	struct msm_gem_address_space *aspace;
 	enum dpu_stage stage;
+	uint32_t dirty;
 	uint32_t multirect_index;
 	uint32_t multirect_mode;
 	bool pending;