* [PATCH] drm/msm/dpu: update the qos remap only if the client type changes
@ 2020-11-19 21:41 Abhinav Kumar
2020-11-23 23:18 ` Rob Clark
0 siblings, 1 reply; 4+ messages in thread
From: Abhinav Kumar @ 2020-11-19 21:41 UTC (permalink / raw)
To: dri-devel
Cc: Abhinav Kumar, linux-arm-msm, freedreno, robdclark, seanpaul,
swboyd, nganji, aravindh, tanmay, khsieh
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;
+ }
_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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/msm/dpu: update the qos remap only if the client type changes
2020-11-19 21:41 [PATCH] drm/msm/dpu: update the qos remap only if the client type changes Abhinav Kumar
@ 2020-11-23 23:18 ` Rob Clark
2020-11-24 0:38 ` abhinavk
0 siblings, 1 reply; 4+ messages in thread
From: Rob Clark @ 2020-11-23 23:18 UTC (permalink / raw)
To: Abhinav Kumar
Cc: dri-devel, linux-arm-msm, freedreno, Sean Paul, Stephen Boyd,
nganji, aravindh, Tanmay Shah, Kuogee Hsieh
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/msm/dpu: update the qos remap only if the client type changes
2020-11-23 23:18 ` Rob Clark
@ 2020-11-24 0:38 ` abhinavk
2020-11-24 1:02 ` Rob Clark
0 siblings, 1 reply; 4+ messages in thread
From: abhinavk @ 2020-11-24 0:38 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-arm-msm, freedreno, Sean Paul, Stephen Boyd,
nganji, aravindh, Tanmay Shah, Kuogee Hsieh
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
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/msm/dpu: update the qos remap only if the client type changes
2020-11-24 0:38 ` abhinavk
@ 2020-11-24 1:02 ` Rob Clark
0 siblings, 0 replies; 4+ messages in thread
From: Rob Clark @ 2020-11-24 1:02 UTC (permalink / raw)
To: Abhinav Kumar
Cc: dri-devel, linux-arm-msm, freedreno, Sean Paul, Stephen Boyd,
nganji, aravindh, Tanmay Shah, Kuogee Hsieh
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
> >>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-24 1:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-19 21:41 [PATCH] drm/msm/dpu: update the qos remap only if the client type changes Abhinav Kumar
2020-11-23 23:18 ` Rob Clark
2020-11-24 0:38 ` abhinavk
2020-11-24 1:02 ` Rob Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).