Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm/dpu: clearing hw_dspp in dpu_crtc_assign_resources() leaves DSPP enabled --
commit 1d1b56e93b802629a163cf47cb75949dc3b0cdc9 Author: Dmitry Baryshkov <[email protected]> drm/msm/dpu: clear the DSPP pointer when no DSPP is assigned This commit updates dpu_crtc_assign_resources() to explicitly set cstate->mixers[i].hw_dspp to NULL when the new topology does not assign a DSPP block to a mixer, preventing stale DSPP pointers from persisting across state duplications. > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 1f8fce366a1d3..2ff0dab33710b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1480,8 +1480,8 @@ static int dpu_crtc_assign_resources(struct drm_crtc > *crtc, > > cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); > cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); > - if (i < num_dspp) > - cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); > + cstate->mixers[i].hw_dspp = i < num_dspp ? > + to_dpu_hw_dspp(hw_dspp[i]) : NULL; [Severity: High] Does clearing cstate->mixers[i].hw_dspp here leave the DSPP PCC and GC sub-blocks stuck enabled in hardware with stale coefficients and LUTs when userspace disables color management? In DPU hardware, each DSPP block sits inline in the datapath between its layer mixer and pingpong block (per lm_cfg->dspp in the catalog), with no CTL active-bit bypass mux. The only place in the driver that disables PCC and GC in the DSPP registers and stages a DSPP flush on the CTL is dpu_crtc_atomic_begin() -> _dpu_crtc_setup_cp_blocks(): drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:_dpu_crtc_setup_cp_blocks() { ... for (i = 0; i < cstate->num_mixers; i++) { ctl = mixer[i].lm_ctl; dspp = mixer[i].hw_dspp; if (!dspp) continue; if (dspp->ops.setup_pcc) { if (!state->ctm) { dspp->ops.setup_pcc(dspp, NULL); } else { _dpu_crtc_get_pcc_coeff(state, &cfg); dspp->ops.setup_pcc(dspp, &cfg); } /* stage config flush mask */ ctl->ops.update_pending_flush_dspp(ctl, mixer[i].hw_dspp->idx, DPU_DSPP_PCC); } ... } When userspace enables ctm or gamma_lut, dpu_crtc_get_topology() sets topology.num_dspp = topology.num_lm, dpu_crtc_assign_resources() populates cstate->mixers[i].hw_dspp, and _dpu_crtc_setup_cp_blocks() enables PCC and/or GC in the DSPP block and flushes it. When userspace later disables both ctm and gamma_lut by setting them to NULL, crtc_state->color_mgmt_changed is set (and msm_atomic_check() also sets mode_changed = true when ctm transitions to NULL), causing dpu_crtc_atomic_check() to call dpu_crtc_assign_resources(): drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_atomic_check() { ... if (crtc_state->mode_changed || crtc_state->connectors_changed || crtc_state->color_mgmt_changed) { rc = dpu_crtc_assign_resources(crtc, crtc_state); if (rc < 0) return rc; } ... } Inside dpu_crtc_assign_resources(), dpu_crtc_get_topology() sees both crtc_state->ctm and crtc_state->gamma_lut as NULL and leaves topology.num_dspp as 0: drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_get_topology() { ... if (crtc_state->ctm || crtc_state->gamma_lut) topology.num_dspp = topology.num_lm; ... } While dpu_rm_reserve() still assigns the same layer mixer, dpu_crtc_assign_resources() now sets cstate->mixers[i].hw_dspp to NULL. When _dpu_crtc_setup_cp_blocks() subsequently runs in dpu_crtc_atomic_begin(), it sees dspp == NULL and continues early, skipping dspp->ops.setup_pcc(dspp, NULL), dspp->ops.setup_gc(dspp, NULL), and ctl->ops.update_pending_flush_dspp(). A later commit in the series ("drm/msm/dpu: only reassign resources when the encoder is reprogrammed") stops calling dpu_crtc_assign_resources() on color-management-only commits where mode_changed and connectors_changed are false. However, won't the DSPP sub-blocks still be left enabled whenever ctm and gamma_lut are cleared in the same atomic commit as a modeset (where mode_changed or connectors_changed is true), or when a CRTC with active ctm or gamma_lut is disabled (since dpu_crtc_atomic_begin() returns early on !crtc->state->enable and dpu_encoder_helper_phys_cleanup() does not reset or flush DSPP sub-blocks) and a CRTC is subsequently enabled on the same layer mixer without ctm or gamma_lut? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
