Re: [PATCH] drm/msm/dpu: don't reassign resources on color management changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm/dpu: legacy CTM check in msm_atomic_check() overrides dpu_crtc_check_mode_changed()
--

commit d6607b12584d4668388dbb5415d60630f0aba092
Author: Beat M. Liechti <beat@softbase.tech>

drm/msm/dpu: don't reassign resources on color management changes

This commit stops reallocating hardware resources in dpu_crtc_atomic_check()
on plain color_mgmt_changed updates, and instead forces a full modeset in
dpu_crtc_check_mode_changed() when the combined CTM or GAMMA_LUT requirement
for DSPP blocks changes on an enabled CRTC.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 77a2d1f6700d0..d1cd3c6e2e3ab 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1516,6 +1516,20 @@ int dpu_crtc_check_mode_changed(struct drm_crtc_state *old_crtc_state,
>  	    (!clone_mode_requested && clone_mode_enabled))
>  		new_crtc_state->mode_changed = true;
>  
> +	/*
> +	 * DSPP blocks are reserved only when CTM or GAMMA_LUT is set (see
> +	 * dpu_crtc_get_topology()), so enabling or disabling color management
> +	 * changes the hardware topology. Changing resources on an active CRTC
> +	 * requires a full modeset; otherwise the CTL/LM blocks get swapped
> +	 * underneath a running pipeline.
> +	 */
> +	if (new_crtc_state->enable &&
> +	    (!!(old_crtc_state->ctm || old_crtc_state->gamma_lut) !=
> +	     !!(new_crtc_state->ctm || new_crtc_state->gamma_lut))) {
> +		new_crtc_state->mode_changed = true;
> +		new_crtc_state->state->allow_modeset = true;
> +	}

[Severity: Medium]
In dpu_crtc_check_mode_changed(), this check tests the combined
(ctm || gamma_lut) state on an enabled CRTC so that a modeset is only forced
when DSPP block reservation in dpu_crtc_get_topology() actually changes.

However, msm_atomic_check() in drivers/gpu/drm/msm/msm_atomic.c runs right
before kms->funcs->check_mode_changed() and still contains the earlier
CTM-only loop:

drivers/gpu/drm/msm/msm_atomic.c:msm_atomic_check() {
	/*
	 * FIXME: stop setting allow_modeset and move this check to the DPU
	 * driver.
	 */
	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
				      new_crtc_state, i) {
		if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
		    (!old_crtc_state->ctm && new_crtc_state->ctm)) {
			new_crtc_state->mode_changed = true;
			state->allow_modeset = true;
		}
	}

	if (kms && kms->funcs && kms->funcs->check_mode_changed)
		ret = kms->funcs->check_mode_changed(kms, state);
}

Because msm_atomic_check() unconditionally sets new_crtc_state->mode_changed
and state->allow_modeset whenever ctm is toggled, won't toggling ctm while
gamma_lut is already active still force an unnecessary full modeset and
resource reallocation even though DSPP blocks are already reserved, and also
bypass the new_crtc_state->enable guard when disabling a CRTC that had ctm
set?

Since DPU is the only sub-driver in drivers/gpu/drm/msm/ that enables color
management via drm_crtc_enable_color_mgmt(), should that CTM loop in
msm_atomic_check() be removed now that dpu_crtc_check_mode_changed() handles
both ctm and gamma_lut?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101095754.33840-1-beat@softbase.tech?part=1