| Message ID | 20220209172520.3719906-10-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dpu: wide planes support | expand |
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 <dmitry.baryshkov@linaro.org> > --- > 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 = 0; > else > @@ -406,17 +389,22 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, > } > } > > - if (ctl->ops.set_active_pipes) > - ctl->ops.set_active_pipes(ctl, fetch_active); > - > _dpu_crtc_program_lm_output_roi(crtc); > + > + for (i = 0; i < cstate->num_mixers; i++) { > + struct dpu_hw_mixer *lm; > + > + lm = mixer[i].hw_lm; > + > + lm->ops.setup_alpha_out(lm, mixer[i].mixer_op_mode); > + > + DRM_DEBUG_ATOMIC("lm %d, op_mode 0x%X\n", > + mixer[i].hw_lm->idx - LM_0, > + mixer[i].mixer_op_mode); > + } > } > > -/** > - * _dpu_crtc_blend_setup - configure crtc mixers > - * @crtc: Pointer to drm crtc structure > - */ > -static void _dpu_crtc_blend_setup(struct drm_crtc *crtc) > +static void _dpu_crtc_blend_setup_ctl(struct drm_crtc *crtc) > { > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); > @@ -425,34 +413,62 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc) > struct dpu_hw_mixer *lm; > struct dpu_hw_stage_cfg stage_cfg; > int i; > + struct drm_plane *plane; > + struct drm_plane_state *state; > + struct dpu_plane_state *pstate = NULL; > + > + uint32_t stage_idx, lm_idx; > + int zpos_cnt[DPU_STAGE_MAX + 1] = { 0 }; > + DECLARE_BITMAP(fetch_active, SSPP_MAX); > > DRM_DEBUG_ATOMIC("%s\n", dpu_crtc->name); > > - for (i = 0; i < cstate->num_mixers; i++) { > - mixer[i].mixer_op_mode = 0; > + for (i = 0; i < cstate->num_mixers; i++) > if (mixer[i].lm_ctl->ops.clear_all_blendstages) > mixer[i].lm_ctl->ops.clear_all_blendstages( > mixer[i].lm_ctl); > - } > > /* initialize stage cfg */ > memset(&stage_cfg, 0, sizeof(struct dpu_hw_stage_cfg)); > > - _dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer, &stage_cfg); > + memset(fetch_active, 0, sizeof(fetch_active)); > + drm_atomic_crtc_for_each_plane(plane, crtc) { > + enum dpu_sspp sspp_idx; > + > + state = plane->state; > + if (!state) > + continue; > + > + pstate = to_dpu_plane_state(state); > + > + sspp_idx = pstate->pipe_hw->idx; > + set_bit(sspp_idx, fetch_active); > + > + 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; > + > + /* blend config update */ > + for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) > + mixer[lm_idx].lm_ctl->ops.update_pending_flush_sspp(mixer[lm_idx].lm_ctl, sspp_idx); > + } > + > + ctl = mixer->lm_ctl; > + if (ctl->ops.set_active_pipes) > + ctl->ops.set_active_pipes(ctl, fetch_active); > > for (i = 0; i < cstate->num_mixers; i++) { > ctl = mixer[i].lm_ctl; > lm = mixer[i].hw_lm; > > - lm->ops.setup_alpha_out(lm, mixer[i].mixer_op_mode); > - > /* stage config flush mask */ > ctl->ops.update_pending_flush_mixer(ctl, > mixer[i].hw_lm->idx); > > - DRM_DEBUG_ATOMIC("lm %d, op_mode 0x%X, ctl %d\n", > + DRM_DEBUG_ATOMIC("lm %d, ctl %d\n", > mixer[i].hw_lm->idx - LM_0, > - mixer[i].mixer_op_mode, > ctl->idx - CTL_0); > > ctl->ops.setup_blendstage(ctl, mixer[i].hw_lm->idx, > @@ -731,7 +747,8 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, > if (unlikely(!cstate->num_mixers)) > return; > > - _dpu_crtc_blend_setup(crtc); > + _dpu_crtc_blend_setup_mixer(crtc); > + _dpu_crtc_blend_setup_ctl(crtc); > > _dpu_crtc_setup_cp_blocks(crtc); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h > index 54d74341e690..ecd2f371374d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h > @@ -632,9 +632,9 @@ TRACE_EVENT(dpu_enc_phys_vid_irq_ctrl, > TRACE_EVENT(dpu_crtc_setup_mixer, > TP_PROTO(uint32_t crtc_id, uint32_t plane_id, > struct drm_plane_state *state, struct dpu_plane_state *pstate, > - uint32_t stage_idx, enum dpu_sspp sspp, uint32_t pixel_format, > + uint32_t stage_idx, uint32_t pixel_format, > uint64_t modifier), > - TP_ARGS(crtc_id, plane_id, state, pstate, stage_idx, sspp, > + TP_ARGS(crtc_id, plane_id, state, pstate, stage_idx, > pixel_format, modifier), > TP_STRUCT__entry( > __field( uint32_t, crtc_id ) > @@ -644,7 +644,6 @@ TRACE_EVENT(dpu_crtc_setup_mixer, > __field_struct( struct drm_rect, dst_rect ) > __field( uint32_t, stage_idx ) > __field( enum dpu_stage, stage ) > - __field( enum dpu_sspp, sspp ) > __field( uint32_t, multirect_idx ) > __field( uint32_t, multirect_mode ) > __field( uint32_t, pixel_format ) > @@ -658,20 +657,19 @@ TRACE_EVENT(dpu_crtc_setup_mixer, > __entry->dst_rect = drm_plane_state_dest(state); > __entry->stage_idx = stage_idx; > __entry->stage = pstate->stage; > - __entry->sspp = sspp; > __entry->multirect_idx = pstate->multirect_index; > __entry->multirect_mode = pstate->multirect_mode; > __entry->pixel_format = pixel_format; > __entry->modifier = modifier; > ), > TP_printk("crtc_id:%u plane_id:%u fb_id:%u src:" DRM_RECT_FP_FMT > - " dst:" DRM_RECT_FMT " stage_idx:%u stage:%d, sspp:%d " > + " dst:" DRM_RECT_FMT " stage_idx:%u stage:%d, " > "multirect_index:%d multirect_mode:%u pix_format:%u " > "modifier:%llu", > __entry->crtc_id, __entry->plane_id, __entry->fb_id, > DRM_RECT_FP_ARG(&__entry->src_rect), > DRM_RECT_ARG(&__entry->dst_rect), > - __entry->stage_idx, __entry->stage, __entry->sspp, > + __entry->stage_idx, __entry->stage, > __entry->multirect_idx, __entry->multirect_mode, > __entry->pixel_format, __entry->modifier) > );
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 <dmitry.baryshkov@linaro.org> >> --- >> 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 = 0; >> else >> @@ -406,17 +389,22 @@ static void _dpu_crtc_blend_setup_mixer(struct >> drm_crtc *crtc, >> } >> } >> - if (ctl->ops.set_active_pipes) >> - ctl->ops.set_active_pipes(ctl, fetch_active); >> - >> _dpu_crtc_program_lm_output_roi(crtc); >> + >> + for (i = 0; i < cstate->num_mixers; i++) { >> + struct dpu_hw_mixer *lm; >> + >> + lm = mixer[i].hw_lm; >> + >> + lm->ops.setup_alpha_out(lm, mixer[i].mixer_op_mode); >> + >> + DRM_DEBUG_ATOMIC("lm %d, op_mode 0x%X\n", >> + mixer[i].hw_lm->idx - LM_0, >> + mixer[i].mixer_op_mode); >> + } >> } >> -/** >> - * _dpu_crtc_blend_setup - configure crtc mixers >> - * @crtc: Pointer to drm crtc structure >> - */ >> -static void _dpu_crtc_blend_setup(struct drm_crtc *crtc) >> +static void _dpu_crtc_blend_setup_ctl(struct drm_crtc *crtc) >> { >> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); >> struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); >> @@ -425,34 +413,62 @@ static void _dpu_crtc_blend_setup(struct >> drm_crtc *crtc) >> struct dpu_hw_mixer *lm; >> struct dpu_hw_stage_cfg stage_cfg; >> int i; >> + struct drm_plane *plane; >> + struct drm_plane_state *state; >> + struct dpu_plane_state *pstate = NULL; >> + >> + uint32_t stage_idx, lm_idx; >> + int zpos_cnt[DPU_STAGE_MAX + 1] = { 0 }; >> + DECLARE_BITMAP(fetch_active, SSPP_MAX); >> DRM_DEBUG_ATOMIC("%s\n", dpu_crtc->name); >> - for (i = 0; i < cstate->num_mixers; i++) { >> - mixer[i].mixer_op_mode = 0; >> + for (i = 0; i < cstate->num_mixers; i++) >> if (mixer[i].lm_ctl->ops.clear_all_blendstages) >> mixer[i].lm_ctl->ops.clear_all_blendstages( >> mixer[i].lm_ctl); >> - } >> /* initialize stage cfg */ >> memset(&stage_cfg, 0, sizeof(struct dpu_hw_stage_cfg)); >> - _dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer, &stage_cfg); >> + memset(fetch_active, 0, sizeof(fetch_active)); >> + drm_atomic_crtc_for_each_plane(plane, crtc) { >> + enum dpu_sspp sspp_idx; >> + >> + state = plane->state; >> + if (!state) >> + continue; >> + >> + pstate = to_dpu_plane_state(state); >> + >> + sspp_idx = pstate->pipe_hw->idx; >> + set_bit(sspp_idx, fetch_active); >> + >> + 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; >> + >> + /* blend config update */ >> + for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) >> + >> mixer[lm_idx].lm_ctl->ops.update_pending_flush_sspp(mixer[lm_idx].lm_ctl, >> sspp_idx); >> + } >> + >> + ctl = mixer->lm_ctl; >> + if (ctl->ops.set_active_pipes) >> + ctl->ops.set_active_pipes(ctl, fetch_active); >> for (i = 0; i < cstate->num_mixers; i++) { >> ctl = mixer[i].lm_ctl; >> lm = mixer[i].hw_lm; >> - lm->ops.setup_alpha_out(lm, mixer[i].mixer_op_mode); >> - >> /* stage config flush mask */ >> ctl->ops.update_pending_flush_mixer(ctl, >> mixer[i].hw_lm->idx); >> - DRM_DEBUG_ATOMIC("lm %d, op_mode 0x%X, ctl %d\n", >> + DRM_DEBUG_ATOMIC("lm %d, ctl %d\n", >> mixer[i].hw_lm->idx - LM_0, >> - mixer[i].mixer_op_mode, >> ctl->idx - CTL_0); >> ctl->ops.setup_blendstage(ctl, mixer[i].hw_lm->idx, >> @@ -731,7 +747,8 @@ static void dpu_crtc_atomic_begin(struct drm_crtc >> *crtc, >> if (unlikely(!cstate->num_mixers)) >> return; >> - _dpu_crtc_blend_setup(crtc); >> + _dpu_crtc_blend_setup_mixer(crtc); >> + _dpu_crtc_blend_setup_ctl(crtc); >> _dpu_crtc_setup_cp_blocks(crtc); >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h >> index 54d74341e690..ecd2f371374d 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h >> @@ -632,9 +632,9 @@ TRACE_EVENT(dpu_enc_phys_vid_irq_ctrl, >> TRACE_EVENT(dpu_crtc_setup_mixer, >> TP_PROTO(uint32_t crtc_id, uint32_t plane_id, >> struct drm_plane_state *state, struct dpu_plane_state *pstate, >> - uint32_t stage_idx, enum dpu_sspp sspp, uint32_t pixel_format, >> + uint32_t stage_idx, uint32_t pixel_format, >> uint64_t modifier), >> - TP_ARGS(crtc_id, plane_id, state, pstate, stage_idx, sspp, >> + TP_ARGS(crtc_id, plane_id, state, pstate, stage_idx, >> pixel_format, modifier), >> TP_STRUCT__entry( >> __field( uint32_t, crtc_id ) >> @@ -644,7 +644,6 @@ TRACE_EVENT(dpu_crtc_setup_mixer, >> __field_struct( struct drm_rect, dst_rect ) >> __field( uint32_t, stage_idx ) >> __field( enum dpu_stage, stage ) >> - __field( enum dpu_sspp, sspp ) >> __field( uint32_t, multirect_idx ) >> __field( uint32_t, multirect_mode ) >> __field( uint32_t, pixel_format ) >> @@ -658,20 +657,19 @@ TRACE_EVENT(dpu_crtc_setup_mixer, >> __entry->dst_rect = drm_plane_state_dest(state); >> __entry->stage_idx = stage_idx; >> __entry->stage = pstate->stage; >> - __entry->sspp = sspp; >> __entry->multirect_idx = pstate->multirect_index; >> __entry->multirect_mode = pstate->multirect_mode; >> __entry->pixel_format = pixel_format; >> __entry->modifier = modifier; >> ), >> TP_printk("crtc_id:%u plane_id:%u fb_id:%u src:" DRM_RECT_FP_FMT >> - " dst:" DRM_RECT_FMT " stage_idx:%u stage:%d, sspp:%d " >> + " dst:" DRM_RECT_FMT " stage_idx:%u stage:%d, " >> "multirect_index:%d multirect_mode:%u pix_format:%u " >> "modifier:%llu", >> __entry->crtc_id, __entry->plane_id, __entry->fb_id, >> DRM_RECT_FP_ARG(&__entry->src_rect), >> DRM_RECT_ARG(&__entry->dst_rect), >> - __entry->stage_idx, __entry->stage, __entry->sspp, >> + __entry->stage_idx, __entry->stage, >> __entry->multirect_idx, __entry->multirect_mode, >> __entry->pixel_format, __entry->modifier) >> );
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 = 0; else @@ -406,17 +389,22 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, } } - if (ctl->ops.set_active_pipes) - ctl->ops.set_active_pipes(ctl, fetch_active); - _dpu_crtc_program_lm_output_roi(crtc); + + for (i = 0; i < cstate->num_mixers; i++) { + struct dpu_hw_mixer *lm; + + lm = mixer[i].hw_lm; + + lm->ops.setup_alpha_out(lm, mixer[i].mixer_op_mode); + + DRM_DEBUG_ATOMIC("lm %d, op_mode 0x%X\n", + mixer[i].hw_lm->idx - LM_0, + mixer[i].mixer_op_mode); + } } -/** - * _dpu_crtc_blend_setup - configure crtc mixers - * @crtc: Pointer to drm crtc structure - */ -static void _dpu_crtc_blend_setup(struct drm_crtc *crtc) +static void _dpu_crtc_blend_setup_ctl(struct drm_crtc *crtc) { struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); @@ -425,34 +413,62 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc) struct dpu_hw_mixer *lm; struct dpu_hw_stage_cfg stage_cfg; int i; + struct drm_plane *plane; + struct drm_plane_state *state; + struct dpu_plane_state *pstate = NULL; + + uint32_t stage_idx, lm_idx; + int zpos_cnt[DPU_STAGE_MAX + 1] = { 0 }; + DECLARE_BITMAP(fetch_active, SSPP_MAX); DRM_DEBUG_ATOMIC("%s\n", dpu_crtc->name); - for (i = 0; i < cstate->num_mixers; i++) { - mixer[i].mixer_op_mode = 0; + for (i = 0; i < cstate->num_mixers; i++) if (mixer[i].lm_ctl->ops.clear_all_blendstages) mixer[i].lm_ctl->ops.clear_all_blendstages( mixer[i].lm_ctl); - } /* initialize stage cfg */ memset(&stage_cfg, 0, sizeof(struct dpu_hw_stage_cfg)); - _dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer, &stage_cfg); + memset(fetch_active, 0, sizeof(fetch_active)); + drm_atomic_crtc_for_each_plane(plane, crtc) { + enum dpu_sspp sspp_idx; + + state = plane->state; + if (!state) + continue; + + pstate = to_dpu_plane_state(state); + + sspp_idx = pstate->pipe_hw->idx; + set_bit(sspp_idx, fetch_active); + + 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; + + /* blend config update */ + for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) + mixer[lm_idx].lm_ctl->ops.update_pending_flush_sspp(mixer[lm_idx].lm_ctl, sspp_idx); + } + + ctl = mixer->lm_ctl; + if (ctl->ops.set_active_pipes) + ctl->ops.set_active_pipes(ctl, fetch_active); for (i = 0; i < cstate->num_mixers; i++) { ctl = mixer[i].lm_ctl; lm = mixer[i].hw_lm; - lm->ops.setup_alpha_out(lm, mixer[i].mixer_op_mode); - /* stage config flush mask */ ctl->ops.update_pending_flush_mixer(ctl, mixer[i].hw_lm->idx); - DRM_DEBUG_ATOMIC("lm %d, op_mode 0x%X, ctl %d\n", + DRM_DEBUG_ATOMIC("lm %d, ctl %d\n", mixer[i].hw_lm->idx - LM_0, - mixer[i].mixer_op_mode, ctl->idx - CTL_0); ctl->ops.setup_blendstage(ctl, mixer[i].hw_lm->idx, @@ -731,7 +747,8 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, if (unlikely(!cstate->num_mixers)) return; - _dpu_crtc_blend_setup(crtc); + _dpu_crtc_blend_setup_mixer(crtc); + _dpu_crtc_blend_setup_ctl(crtc); _dpu_crtc_setup_cp_blocks(crtc); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h index 54d74341e690..ecd2f371374d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h @@ -632,9 +632,9 @@ TRACE_EVENT(dpu_enc_phys_vid_irq_ctrl, TRACE_EVENT(dpu_crtc_setup_mixer, TP_PROTO(uint32_t crtc_id, uint32_t plane_id, struct drm_plane_state *state, struct dpu_plane_state *pstate, - uint32_t stage_idx, enum dpu_sspp sspp, uint32_t pixel_format, + uint32_t stage_idx, uint32_t pixel_format, uint64_t modifier), - TP_ARGS(crtc_id, plane_id, state, pstate, stage_idx, sspp, + TP_ARGS(crtc_id, plane_id, state, pstate, stage_idx, pixel_format, modifier), TP_STRUCT__entry( __field( uint32_t, crtc_id ) @@ -644,7 +644,6 @@ TRACE_EVENT(dpu_crtc_setup_mixer, __field_struct( struct drm_rect, dst_rect ) __field( uint32_t, stage_idx ) __field( enum dpu_stage, stage ) - __field( enum dpu_sspp, sspp ) __field( uint32_t, multirect_idx ) __field( uint32_t, multirect_mode ) __field( uint32_t, pixel_format ) @@ -658,20 +657,19 @@ TRACE_EVENT(dpu_crtc_setup_mixer, __entry->dst_rect = drm_plane_state_dest(state); __entry->stage_idx = stage_idx; __entry->stage = pstate->stage; - __entry->sspp = sspp; __entry->multirect_idx = pstate->multirect_index; __entry->multirect_mode = pstate->multirect_mode; __entry->pixel_format = pixel_format; __entry->modifier = modifier; ), TP_printk("crtc_id:%u plane_id:%u fb_id:%u src:" DRM_RECT_FP_FMT - " dst:" DRM_RECT_FMT " stage_idx:%u stage:%d, sspp:%d " + " dst:" DRM_RECT_FMT " stage_idx:%u stage:%d, " "multirect_index:%d multirect_mode:%u pix_format:%u " "modifier:%llu", __entry->crtc_id, __entry->plane_id, __entry->fb_id, DRM_RECT_FP_ARG(&__entry->src_rect), DRM_RECT_ARG(&__entry->dst_rect), - __entry->stage_idx, __entry->stage, __entry->sspp, + __entry->stage_idx, __entry->stage, __entry->multirect_idx, __entry->multirect_mode, __entry->pixel_format, __entry->modifier) );
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. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- 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(-)