Re: [PATCH 09/25] drm/msm/dpu: dpu_crtc_blend_setup: split mixer and ctl logic

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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