Re: [PATCH v5 09/10] drm/msm/dpu: only reassign resources when the encoder is reprogrammed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dpu: dpu_crtc_check_mode_changed() forces modeset on disabled CRTC
--

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

drm/msm/dpu: only reassign resources when the encoder is reprogrammed

This commit restricts DPU CRTC resource reassignment to modeset and connector
changes rather than color management updates, and moves the modeset check for
DSPP reservation into dpu_crtc_check_mode_changed() while removing the CTM
modeset workaround from msm_atomic_check().

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 2ff0dab33710b..048800227ad30 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1489,6 +1489,26 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> +static bool dpu_crtc_needs_dspp(const struct drm_crtc_state *crtc_state)
> +{
> +	return crtc_state->ctm || crtc_state->gamma_lut;
> +}
> +
> +static bool dpu_crtc_has_dspp(const struct drm_crtc_state *crtc_state)
> +{
> +	const struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
> +	unsigned int i;
> +
> +	if (!cstate->num_mixers)
> +		return false;
> +
> +	for (i = 0; i < cstate->num_mixers; i++)
> +		if (!cstate->mixers[i].hw_dspp)
> +			return false;
> +
> +	return true;
> +}
> +
>  /**
>   * dpu_crtc_check_mode_changed: check if full modeset is required
>   * @old_crtc_state:	Previous CRTC state
> @@ -1506,6 +1526,11 @@ int dpu_crtc_check_mode_changed(struct drm_crtc_state *old_crtc_state,
>  
>  	DRM_DEBUG_ATOMIC("%d\n", crtc->base.id);
>  
> +	/* DSPPs are only reserved during a modeset */
> +	if (dpu_crtc_needs_dspp(new_crtc_state) &&
> +	    !dpu_crtc_has_dspp(old_crtc_state))
> +		new_crtc_state->mode_changed = true;

[Severity: High]
What happens here when ctm or gamma_lut is set on a disabled CRTC where
new_crtc_state->enable is false?

In drm_mode_atomic_ioctl() -> drm_atomic_check_only() -> msm_atomic_check() ->
dpu_kms_check_mode_changed() -> dpu_crtc_check_mode_changed(),
dpu_crtc_needs_dspp(new_crtc_state) returns true whenever ctm or gamma_lut is
present regardless of whether new_crtc_state->enable is set.

Because dpu_crtc_assign_resources() skips resource reservation when the CRTC
is disabled:

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;
	...
}

cstate->num_mixers is 0 on a disabled CRTC, so
dpu_crtc_has_dspp(old_crtc_state) is false and dpu_crtc_check_mode_changed()
sets new_crtc_state->mode_changed = true.

If userspace sets CTM or GAMMA_LUT on a disabled CRTC without
DRM_MODE_ATOMIC_ALLOW_MODESET, drm_atomic_check_only() will see
drm_atomic_crtc_needs_modeset(new_crtc_state) and reject the commit with
-EINVAL.

Furthermore, even if CTM or GAMMA_LUT is initially committed on a disabled
CRTC with allow_modeset = true (or via legacy gamma/property ioctls or
drm_fb_helper's setcmap_atomic()), dpu_crtc_assign_resources() still returns
early without reserving mixers or DSPPs. On every subsequent atomic commit
that includes that disabled CRTC in state, dpu_crtc_needs_dspp(new_crtc_state)
&& !dpu_crtc_has_dspp(old_crtc_state) remains true, forcing
new_crtc_state->mode_changed = true and causing any non-modeset commit to fail
with -EINVAL.

Should dpu_crtc_needs_dspp() or dpu_crtc_check_mode_changed() check
new_crtc_state->enable before requiring a modeset for DSPP reservation?

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