Re: [PATCH v9 08/10] drm/msm/dpu1: Carry the mixers over to created CRTC states

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/msm/dpu: stale mixers and dead CTLs copied in
  dpu_crtc_atomic_check()
--

commit 1b109324be16b20d23b18d888d3626797044015c
Author: Maxime Ripard <mripard@kernel.org>

drm/msm/dpu1: Carry the mixers over to created CRTC states

This commit copies mixers and CTLs from the old CRTC state to the new
CRTC state in dpu_crtc_atomic_check() when the new state has no mixers
assigned and no modeset occurred.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 77a2d1f6700d0..97deadc976ed7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1540,6 +1540,27 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  		rc = dpu_crtc_assign_resources(crtc, crtc_state);
>  		if (rc < 0)
>  			return rc;
> +	} else if (!cstate->num_mixers) {
> +		struct dpu_crtc_state *old_cstate =
> +			to_dpu_crtc_state(drm_atomic_get_old_crtc_state(state,
> +									crtc));
> +
> +		/*
> +		 * A CRTC that keeps its configuration keeps its mixers.
> +		 * A duplicated state carries them over, but a state
> +		 * created through atomic_create_state() comes with
> +		 * none, so copy them from the current state like a
> +		 * duplicated state would, to make sure the checks and
> +		 * the commit see the mixers in use.
> +		 */
> +		if (old_cstate->num_mixers) {
> +			cstate->num_mixers = old_cstate->num_mixers;
> +			memcpy(cstate->mixers, old_cstate->mixers,
> +			       sizeof(cstate->mixers));
> +			cstate->num_ctls = old_cstate->num_ctls;
> +			memcpy(cstate->hw_ctls, old_cstate->hw_ctls,
> +			       sizeof(cstate->hw_ctls));
> +		}
>  	}
>  
>  	if (crtc_state->planes_changed || crtc_state->zpos_changed) {

[Severity: Medium]
Can this copy stale mixers onto a disabled CRTC state when
crtc_state->enable is false?

When a CRTC is disabled in an earlier commit that uses
dpu_crtc_duplicate_state(), dpu_crtc_assign_resources() releases the
resources in dpu_rm and returns early on !crtc_state->enable without
clearing cstate->num_mixers or cstate->mixers[]:

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_assign_resources() {
    ...
	dpu_rm_release(global_state, crtc);

	if (!crtc_state->enable)
		return 0;
    ...
}

If a subsequent commit built with drm_atomic_commit_fill_with_defaults()
leaves that CRTC disabled, mode_changed, connectors_changed, and
color_mgmt_changed are all false.  This branch then copies the already
released mixers from old_cstate into the new disabled cstate, where
consumers that inspect cstate->num_mixers without checking
crtc_state->enable (such as dpu_crtc_set_crc_source() via
dpu_crtc_setup_lm_misr()) can access hardware resources that may have
been reassigned to another active CRTC.

Also, cstate->num_ctls and cstate->hw_ctls are unused fields in struct
dpu_crtc_state that dpu_crtc_assign_resources() never populates (CTL
pointers are stored in cstate->mixers[i].lm_ctl).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-drm-reset-state-flag-v9-0-ddb0e26324a0@kernel.org?part=8