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