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 <[email protected]> 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/[email protected]?part=1
