[PATCH] drm/msm/dpu: update the qos remap only if the client type changes
abhinavk at codeaurora.org
abhinavk at codeaurora.org
Tue Nov 24 00:38:44 UTC 2020
Hi Rob
On 2020-11-23 15:18, Rob Clark wrote:
> On Thu, Nov 19, 2020 at 1:41 PM Abhinav Kumar <abhinavk at 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 at 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
>>
More information about the dri-devel
mailing list