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

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

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@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)
>   );