Re: [PATCH 09/25] drm/msm/dpu: dpu_crtc_blend_setup: split mixer and ctl logic
On 06/05/2022 21:56, Abhinav Kumar wrote:
On 2/9/2022 9:25 AM, Dmitry Baryshkov wrote:
The funcitons _dpu_crtc_blend_setup() and _dpu_crtc_blend_setup_mixer()
have an intertwined mixture of CTL and LM-related code. Split these two
functions into LM-specific and CTL-specific parts, making both code
paths clean and observable.
I do see the intention of this change, but there are two things to
consider here.
Let me know what you think of those:
1) Agreed that we are able to split it out but at what cost? We are
repeating some of the loops such as
a) for (i = 0; i < cstate->num_mixers; i++) {
b) drm_atomic_crtc_for_each_plane(
Maybe we should invert these loops, so that we'll through the planes and
only then loop over the mixers.
2) The intertwining is "somewhat" logical here because we are
programming the LMs for which we are staging the planes so it somewhat
goes together
I'll revisit this for v2. I'll move this towards the end of the series,
so it would be more obvious if patch 25/25 is better with this change or
w/o it.
3) dropping sspp idx from this trace removes some useful informatio of
which sspp is staged to which stage of blend
I can add this back to the dpu_crtc_blend_setup_ctl
> trace_dpu_crtc_setup_mixer(DRMID(crtc), DRMID(plane),
> state, pstate, stage_idx,
> - sspp_idx - SSPP_VIG0,
> format->base.pixel_format,
> fb ? fb->modifier : 0);
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 101 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 10 +--
2 files changed, 63 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e6c33022d560..ada7d5750536 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -336,27 +336,23 @@ static void
_dpu_crtc_program_lm_output_roi(struct drm_crtc *crtc)
}
}
-static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
- struct dpu_crtc *dpu_crtc, struct dpu_crtc_mixer *mixer,
- struct dpu_hw_stage_cfg *stage_cfg)
+static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc)
{
+ struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
+ struct dpu_crtc_mixer *mixer = cstate->mixers;
struct drm_plane *plane;
struct drm_framebuffer *fb;
struct drm_plane_state *state;
- struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
struct dpu_plane_state *pstate = NULL;
struct dpu_format *format;
- struct dpu_hw_ctl *ctl = mixer->lm_ctl;
-
+ int i;
uint32_t stage_idx, lm_idx;
- int zpos_cnt[DPU_STAGE_MAX + 1] = { 0 };
bool bg_alpha_enable = false;
- DECLARE_BITMAP(fetch_active, SSPP_MAX);
- memset(fetch_active, 0, sizeof(fetch_active));
- drm_atomic_crtc_for_each_plane(plane, crtc) {
- enum dpu_sspp sspp_idx;
+ for (i = 0; i < cstate->num_mixers; i++)
+ mixer[i].mixer_op_mode = 0;
+ drm_atomic_crtc_for_each_plane(plane, crtc) {
state = plane->state;
if (!state)
continue;
@@ -364,14 +360,10 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
pstate = to_dpu_plane_state(state);
fb = state->fb;
- sspp_idx = pstate->pipe_hw->idx;
- set_bit(sspp_idx, fetch_active);
-
- DRM_DEBUG_ATOMIC("crtc %d stage:%d - plane %d sspp %d fb %d\n",
+ DRM_DEBUG_ATOMIC("crtc %d stage:%d - plane %d fb %d\n",
crtc->base.id,
pstate->stage,
plane->base.id,
- sspp_idx - SSPP_VIG0,
state->fb ? state->fb->base.id : -1);
format =
to_dpu_format(msm_framebuffer_format(pstate->base.fb));
@@ -379,15 +371,8 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
bg_alpha_enable = true;
- stage_idx = zpos_cnt[pstate->stage]++;
- stage_cfg->stage[pstate->stage][stage_idx] =
- sspp_idx;
- stage_cfg->multirect_index[pstate->stage][stage_idx] =
- pstate->multirect_index;
-
trace_dpu_crtc_setup_mixer(DRMID(crtc), DRMID(plane),
state, pstate, stage_idx,
- sspp_idx - SSPP_VIG0,
format->base.pixel_format,
fb ? fb->modifier : 0);
@@ -396,8 +381,6 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
_dpu_crtc_setup_blend_cfg(mixer + lm_idx,
pstate, format);
-
mixer[lm_idx].lm_ctl->ops.update_pending_flush_sspp(mixer[lm_idx].lm_ctl,
sspp_idx);
-
if (bg_alpha_enable && !format->alpha_enable)
mixer[lm_idx].mixer_op_mode
Re: [PATCH 09/25] drm/msm/dpu: dpu_crtc_blend_setup: split mixer and ctl logic
On 2/9/2022 9:25 AM, Dmitry Baryshkov wrote:
The funcitons _dpu_crtc_blend_setup() and _dpu_crtc_blend_setup_mixer()
have an intertwined mixture of CTL and LM-related code. Split these two
functions into LM-specific and CTL-specific parts, making both code
paths clean and observable.
I do see the intention of this change, but there are two things to
consider here.
Let me know what you think of those:
1) Agreed that we are able to split it out but at what cost? We are
repeating some of the loops such as
a) for (i = 0; i < cstate->num_mixers; i++) {
b) drm_atomic_crtc_for_each_plane(
2) The intertwining is "somewhat" logical here because we are
programming the LMs for which we are staging the planes so it somewhat
goes together
3) dropping sspp idx from this trace removes some useful informatio of
which sspp is staged to which stage of blend
>trace_dpu_crtc_setup_mixer(DRMID(crtc), DRMID(plane),
> state, pstate, stage_idx,
> - sspp_idx - SSPP_VIG0,
> format->base.pixel_format,
> fb ? fb->modifier : 0);
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 101 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 10 +--
2 files changed, 63 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e6c33022d560..ada7d5750536 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -336,27 +336,23 @@ static void _dpu_crtc_program_lm_output_roi(struct
drm_crtc *crtc)
}
}
-static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
- struct dpu_crtc *dpu_crtc, struct dpu_crtc_mixer *mixer,
- struct dpu_hw_stage_cfg *stage_cfg)
+static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc)
{
+ struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
+ struct dpu_crtc_mixer *mixer = cstate->mixers;
struct drm_plane *plane;
struct drm_framebuffer *fb;
struct drm_plane_state *state;
- struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
struct dpu_plane_state *pstate = NULL;
struct dpu_format *format;
- struct dpu_hw_ctl *ctl = mixer->lm_ctl;
-
+ int i;
uint32_t stage_idx, lm_idx;
- int zpos_cnt[DPU_STAGE_MAX + 1] = { 0 };
bool bg_alpha_enable = false;
- DECLARE_BITMAP(fetch_active, SSPP_MAX);
- memset(fetch_active, 0, sizeof(fetch_active));
- drm_atomic_crtc_for_each_plane(plane, crtc) {
- enum dpu_sspp sspp_idx;
+ for (i = 0; i < cstate->num_mixers; i++)
+ mixer[i].mixer_op_mode = 0;
+ drm_atomic_crtc_for_each_plane(plane, crtc) {
state = plane->state;
if (!state)
continue;
@@ -364,14 +360,10 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
pstate = to_dpu_plane_state(state);
fb = state->fb;
- sspp_idx = pstate->pipe_hw->idx;
- set_bit(sspp_idx, fetch_active);
-
- DRM_DEBUG_ATOMIC("crtc %d stage:%d - plane %d sspp %d fb %d\n",
+ DRM_DEBUG_ATOMIC("crtc %d stage:%d - plane %d fb %d\n",
crtc->base.id,
pstate->stage,
plane->base.id,
- sspp_idx - SSPP_VIG0,
state->fb ? state->fb->base.id : -1);
format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
@@ -379,15 +371,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
bg_alpha_enable = true;
- stage_idx = zpos_cnt[pstate->stage]++;
- stage_cfg->stage[pstate->stage][stage_idx] =
- sspp_idx;
- stage_cfg->multirect_index[pstate->stage][stage_idx] =
- pstate->multirect_index;
-
trace_dpu_crtc_setup_mixer(DRMID(crtc), DRMID(plane),
state, pstate, stage_idx,
- sspp_idx - SSPP_VIG0,
format->base.pixel_format,
fb ? fb->modifier : 0);
@@ -396,8 +381,6 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
_dpu_crtc_setup_blend_cfg(mixer + lm_idx,
pstate, format);
- mixer[lm_idx].lm_ctl->ops.update_pending_flush_sspp(mixer[lm_idx].lm_ctl, sspp_idx);
-
if (bg_alpha_enable &&
