Re: [PATCH v5 07/10] drm/msm/dpu: clear the DSPP pointer when no DSPP is assigned

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dpu: clearing hw_dspp in dpu_crtc_assign_resources() leaves DSPP enabled
--

commit 1d1b56e93b802629a163cf47cb75949dc3b0cdc9
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dpu: clear the DSPP pointer when no DSPP is assigned

This commit updates dpu_crtc_assign_resources() to explicitly set
cstate->mixers[i].hw_dspp to NULL when the new topology does not assign a
DSPP block to a mixer, preventing stale DSPP pointers from persisting across
state duplications.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 1f8fce366a1d3..2ff0dab33710b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1480,8 +1480,8 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc,
>  
>  		cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]);
>  		cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]);
> -		if (i < num_dspp)
> -			cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]);
> +		cstate->mixers[i].hw_dspp = i < num_dspp ?
> +			to_dpu_hw_dspp(hw_dspp[i]) : NULL;

[Severity: High]
Does clearing cstate->mixers[i].hw_dspp here leave the DSPP PCC and GC
sub-blocks stuck enabled in hardware with stale coefficients and LUTs when
userspace disables color management?

In DPU hardware, each DSPP block sits inline in the datapath between its
layer mixer and pingpong block (per lm_cfg->dspp in the catalog), with no
CTL active-bit bypass mux. The only place in the driver that disables PCC
and GC in the DSPP registers and stages a DSPP flush on the CTL is
dpu_crtc_atomic_begin() -> _dpu_crtc_setup_cp_blocks():

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:_dpu_crtc_setup_cp_blocks() {
	...
	for (i = 0; i < cstate->num_mixers; i++) {
		ctl = mixer[i].lm_ctl;
		dspp = mixer[i].hw_dspp;

		if (!dspp)
			continue;

		if (dspp->ops.setup_pcc) {
			if (!state->ctm) {
				dspp->ops.setup_pcc(dspp, NULL);
			} else {
				_dpu_crtc_get_pcc_coeff(state, &cfg);
				dspp->ops.setup_pcc(dspp, &cfg);
			}

			/* stage config flush mask */
			ctl->ops.update_pending_flush_dspp(ctl,
				mixer[i].hw_dspp->idx, DPU_DSPP_PCC);
		}
	...
}

When userspace enables ctm or gamma_lut, dpu_crtc_get_topology() sets
topology.num_dspp = topology.num_lm, dpu_crtc_assign_resources() populates
cstate->mixers[i].hw_dspp, and _dpu_crtc_setup_cp_blocks() enables PCC
and/or GC in the DSPP block and flushes it.

When userspace later disables both ctm and gamma_lut by setting them to
NULL, crtc_state->color_mgmt_changed is set (and msm_atomic_check() also
sets mode_changed = true when ctm transitions to NULL), causing
dpu_crtc_atomic_check() to call dpu_crtc_assign_resources():

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_atomic_check() {
	...
	if (crtc_state->mode_changed || crtc_state->connectors_changed ||
	    crtc_state->color_mgmt_changed) {
		rc = dpu_crtc_assign_resources(crtc, crtc_state);
		if (rc < 0)
			return rc;
	}
	...
}

Inside dpu_crtc_assign_resources(), dpu_crtc_get_topology() sees both
crtc_state->ctm and crtc_state->gamma_lut as NULL and leaves
topology.num_dspp as 0:

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_get_topology() {
	...
	if (crtc_state->ctm || crtc_state->gamma_lut)
		topology.num_dspp = topology.num_lm;
	...
}

While dpu_rm_reserve() still assigns the same layer mixer,
dpu_crtc_assign_resources() now sets cstate->mixers[i].hw_dspp to NULL.
When _dpu_crtc_setup_cp_blocks() subsequently runs in
dpu_crtc_atomic_begin(), it sees dspp == NULL and continues early, skipping
dspp->ops.setup_pcc(dspp, NULL), dspp->ops.setup_gc(dspp, NULL), and
ctl->ops.update_pending_flush_dspp().

A later commit in the series ("drm/msm/dpu: only reassign resources when
the encoder is reprogrammed") stops calling dpu_crtc_assign_resources() on
color-management-only commits where mode_changed and connectors_changed are
false. However, won't the DSPP sub-blocks still be left enabled whenever
ctm and gamma_lut are cleared in the same atomic commit as a modeset
(where mode_changed or connectors_changed is true), or when a CRTC with
active ctm or gamma_lut is disabled (since dpu_crtc_atomic_begin() returns
early on !crtc->state->enable and dpu_encoder_helper_phys_cleanup() does
not reset or flush DSPP sub-blocks) and a CRTC is subsequently enabled on
the same layer mixer without ctm or gamma_lut?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-fd-kms-fix-smmu-v5-0-a5b0b284aead@oss.qualcomm.com?part=7