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