Re: [PATCH 2/8] drm/msm/dpu: save LMs in encoder mode_set for cleanup use later

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm/dpu: incomplete blendstage reset in dpu_encoder_helper_reset_mixers()
--

commit d3a43e897b19cb72f6d34115067a2c68f2bef217
Author: Xi Zhang <xz.ca.cn@gmail.com>

drm/msm/dpu: save LMs in encoder mode_set for cleanup use later

This commit caches layer mixers in dpu_encoder_virt during atomic_mode_set so
that dpu_encoder_helper_reset_mixers() cleans up the previously assigned
mixers rather than querying the post-swap global state. It also removes the
ctl->ops.setup_blendstage() call from dpu_encoder_helper_reset_mixers().

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 8d2b4d2726f27..09afc0681248f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

[ ... ]

> @@ -2183,42 +2194,34 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
>  
>  static void dpu_encoder_helper_reset_mixers(struct dpu_encoder_phys *phys_enc)
>  {
> -	int i, num_lm;
> -	struct dpu_global_state *global_state;
> -	struct dpu_hw_blk *hw_lm[2];
> -	struct dpu_hw_mixer *hw_mixer[2];
> +	int i;
>  	struct dpu_hw_ctl *ctl = phys_enc->hw_ctl;
> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(phys_enc->parent);
>  
>  	/* reset all mixers for this encoder */
>  	if (ctl->ops.clear_all_blendstages)
>  		ctl->ops.clear_all_blendstages(ctl);
>  
> -	global_state = dpu_kms_get_existing_global_state(phys_enc->dpu_kms);
> +	for (i = 0; i < ARRAY_SIZE(dpu_enc->hw_mixer); i++) {
> +		if (dpu_enc->hw_mixer[i]) {
> +			if (ctl->ops.update_pending_flush_mixer)
> +				ctl->ops.update_pending_flush_mixer(ctl, dpu_enc->hw_mixer[i]->idx);
>  
> -	num_lm = dpu_rm_get_assigned_resources(&phys_enc->dpu_kms->rm, global_state,
> -		phys_enc->parent->crtc, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
> -
> -	for (i = 0; i < num_lm; i++) {
> -		hw_mixer[i] = to_dpu_hw_mixer(hw_lm[i]);
> -		if (ctl->ops.update_pending_flush_mixer)
> -			ctl->ops.update_pending_flush_mixer(ctl, hw_mixer[i]->idx);
> -
> -		/* clear all blendstages */
> -		if (ctl->ops.setup_blendstage)
> -			ctl->ops.setup_blendstage(ctl, hw_mixer[i]->idx, NULL);
> -
> -		if (hw_mixer[i]->ops.clear_all_blendstages)
> -			hw_mixer[i]->ops.clear_all_blendstages(hw_mixer[i]);
> +			/* clear all blendstages */
> +			if (dpu_enc->hw_mixer[i]->ops.clear_all_blendstages)
> +				dpu_enc->hw_mixer[i]->ops.clear_all_blendstages(
> +					dpu_enc->hw_mixer[i]);
> +		}
> +	}

[Severity: Medium]
Does ctl->ops.clear_all_blendstages() completely cover what
ctl->ops.setup_blendstage(ctl, hw_mixer[i]->idx, NULL) was doing here?

When dpu_encoder_virt_atomic_disable() -> dpu_encoder_helper_phys_cleanup() ->
dpu_encoder_helper_reset_mixers() runs on DPU hardware with core_major_ver < 12,
ctl->ops.clear_all_blendstages invokes dpu_hw_ctl_clear_all_blendstages() in
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c:

static void dpu_hw_ctl_clear_all_blendstages(struct dpu_hw_ctl *ctx)
{
	struct dpu_hw_blk_reg_map *c = &ctx->hw;
	int i;

	for (i = 0; i < ctx->mixer_count; i++) {
		enum dpu_lm mixer_id = ctx->mixer_hw_caps[i].id;

		DPU_REG_WRITE(c, CTL_LAYER(mixer_id), 0);
		DPU_REG_WRITE(c, CTL_LAYER_EXT(mixer_id), 0);
		DPU_REG_WRITE(c, CTL_LAYER_EXT2(mixer_id), 0);
		DPU_REG_WRITE(c, CTL_LAYER_EXT3(mixer_id), 0);
	}

	DPU_REG_WRITE(c, CTL_FETCH_PIPE_ACTIVE, 0);
}

By contrast, dpu_hw_ctl_setup_blendstage() with stage_cfg == NULL did:

	mixercfg[0] = CTL_MIXER_BORDER_OUT; /* always set BORDER_OUT */

	if (!stage_cfg)
		goto exit;
...
exit:
	DPU_REG_WRITE(c, CTL_LAYER(lm), mixercfg[0]);
	DPU_REG_WRITE(c, CTL_LAYER_EXT(lm), mixercfg[1]);
	DPU_REG_WRITE(c, CTL_LAYER_EXT2(lm), mixercfg[2]);
	DPU_REG_WRITE(c, CTL_LAYER_EXT3(lm), mixercfg[3]);
	if (ctx->mdss_ver->core_major_ver >= 9)
		DPU_REG_WRITE(c, CTL_LAYER_EXT4(lm), mixercfg[4]);

Without ctl->ops.setup_blendstage(), on DPU v9.x through v11.x platforms
CTL_LAYER_EXT4(lm) is never cleared during mixer reset (leaving any staged
SSPP_DMA4 or SSPP_DMA5 bits set), and on DPU < v5 (where
ctl->ops.reset_intf_cfg is not implemented) CTL_LAYER(lm) is left as 0
instead of having CTL_MIXER_BORDER_OUT set.

Should dpu_hw_ctl_clear_all_blendstages() be updated to also clear
CTL_LAYER_EXT4(mixer_id) on core_major_ver >= 9 (and preserve
CTL_MIXER_BORDER_OUT if needed), or should the setup_blendstage() call be
kept?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-msm-dpu-res-clean-v1-0-f46130a8a717@gmail.com?part=2