[Freedreno] [PATCH 09/25] drm/msm/dpu: dpu_crtc_blend_setup: split mixer and ctl logic
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri May 6 20:14:09 UTC 2022
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 at 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)
>> );
--
With best wishes
Dmitry
More information about the Freedreno
mailing list