Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/msm/dpu: disjoint possible_crtcs in _dpu_kms_drm_obj_init() breaks CWB - [Medium] drm/msm/dpu: pairing encoder i to crtc i deprives encoders of cursor planes - [Medium] drm/msm/dpu: broken possible_crtcs pairing in _dpu_kms_drm_obj_init() --
commit 92f85d682d2387cdaeb71bf5aeb4e7434bd2e013 Author: Xi Zhang <[email protected]> drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc This commit pairs each encoder with a dedicated CRTC in _dpu_kms_drm_obj_init() when dev->mode_config.num_crtc >= num_encoders so that modesets keep the same CRTC and release old DPU resources before allocating new ones during atomic_check. > drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc > > Consider the scenario with 4 crtcs, 3 encoders/displays are > connected. DPU resources is nearly maxed-out, and mode set is > performed on a display which has acquired many dpu resources. > A new crtc is selected by kernel drm for the mode set, > and during atomic_check, the previous crtc won't have a chance > to release its old resources from global_state before the new > crtc tries to allocate new ones. Leading to atomic_check failure. > > By having the same crtc for encoder when possible, during > atomic_check each individual crtc will always be able to release > old resources before allocating new ones. > > Signed-off-by: Xi Zhang <[email protected]> [Severity: Medium] Can the scenario with 4 CRTCs and 3 encoders described in the commit message occur, and does pairing encoders to CRTCs resolve cross-CRTC resource reassignment in dpu_crtc_atomic_check()? In drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c:_dpu_kms_drm_obj_init(), the number of created CRTCs is capped at num_encoders: max_crtc_count = min(catalog->mixer_count, num_encoders); ... max_crtc_count = min(max_crtc_count, primary_planes_idx); /* Create one CRTC per encoder */ for (i = 0; i < max_crtc_count; i++) { crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]); so dev->mode_config.num_crtc is never greater than num_encoders. In addition, drm_atomic_helper_check_planes() iterates over CRTCs in ascending CRTC index order and calls dpu_crtc_atomic_check(). Inside drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c, both dpu_crtc_assign_resources() and dpu_crtc_reassign_planes() release and immediately reserve resources per CRTC: dpu_rm_release(global_state, crtc); if (!crtc_state->enable) return 0; topology = dpu_crtc_get_topology(crtc, dpu_kms, crtc_state); ret = dpu_rm_reserve(&dpu_kms->rm, global_state, crtc_state->crtc, &topology); and: dpu_rm_release_all_sspp(global_state, crtc); return dpu_crtc_assign_planes(crtc, crtc_state); Even with 1:1 encoder-to-CRTC pairing, if a multi-display atomic commit disables or reduces resources on a higher-indexed CRTC while enabling or increasing resources on a lower-indexed CRTC, won't the lower-indexed CRTC still attempt to reserve from global_state before the higher-indexed CRTC has released its old resources? > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index da3556eb6ecc2..13099d6e3ac95 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -879,9 +879,18 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > } > } > > - /* All CRTCs are compatible with all encoders */ > - drm_for_each_encoder(encoder, dev) > - encoder->possible_crtcs = (1 << dev->mode_config.num_crtc) - 1; > + /* All CRTCs are compatible with all encoders. > + * Nevertheless, if enough CRTCs are available, pair up crtc/encoder > + * for better resource re-assignments during atomic_check > + * from userspace mode set. > + */ > + i = 0; > + drm_for_each_encoder(encoder, dev) { > + if (dev->mode_config.num_crtc >= num_encoders) > + encoder->possible_crtcs = 1 << i++; [Severity: High] Does assigning encoder->possible_crtcs = 1 << i++ break Concurrent Writeback (CWB) clone mode on platforms where catalog->cwb_count > 0? Earlier in _dpu_kms_drm_obj_init(), DSI and Writeback encoders are marked as clones via dpu_encoder_get_clones(): drm_for_each_encoder(encoder, dev) { num_encoders++; if (catalog->cwb_count > 0) encoder->possible_clones = dpu_encoder_get_clones(encoder); } where drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_get_clones() pairs DRM_MODE_ENCODER_DSI and DRM_MODE_ENCODER_VIRTUAL encoders so both can attach to the same drm_crtc. When dev->mode_config.num_crtc >= num_encoders, this loop gives every encoder a mutually exclusive single-bit possible_crtcs mask. When userspace submits an atomic commit attaching both a DSI encoder and a Writeback encoder to the same CRTC for CWB, drm_atomic_helper_check_modeset() -> update_connector_routing() checks drm_encoder_crtc_ok(): if (!drm_encoder_crtc_ok(new_encoder, new_connector_state->crtc)) { drm_dbg_atomic(connector->dev, "[ENCODER:%d:%s] incompatible with [CRTC:%d:%s]\n", new_encoder->base.id, new_encoder->name, new_connector_state->crtc->base.id, new_connector_state->crtc->name); return -EINVAL; } Because the DSI and Writeback encoders no longer share any common CRTC bit in possible_crtcs, won't all CWB clone commits be rejected with -EINVAL? [Severity: Medium] Also in _dpu_kms_drm_obj_init(), won't locking encoder i to CRTC i prevent displays on higher-indexed encoders from using hardware cursor planes? Earlier in _dpu_kms_drm_obj_init(), DRM_PLANE_TYPE_CURSOR planes are only created for SSPPs with BIT(DPU_SSPP_CURSOR) (at most 1 or 2 across DPU catalogs): if ((catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)) && cursor_planes_idx < max_crtc_count) type = DRM_PLANE_TYPE_CURSOR; and passed to dpu_crtc_init(): for (i = 0; i < max_crtc_count; i++) { crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]); Therefore, CRTCs at index i >= cursor_planes_idx are initialized with cursor_planes[i] == NULL (so crtc->cursor == NULL). Because _dpu_kms_setup_displays() registers encoders in fixed controller order (DSI, DP controllers, HDMI, Writeback), an active display on an encoder at index i >= cursor_planes_idx (such as an eDP panel on dp[3] on multi-DP laptops) is now forced onto CRTC i where crtc->cursor is NULL, even when CRTC 0 and CRTC 1 are unused. When userspace calls DRM_IOCTL_MODE_CURSOR or DRM_IOCTL_MODE_CURSOR2 on that CRTC, drivers/gpu/drm/drm_plane.c:drm_mode_cursor_common() sees crtc->cursor == NULL: if (crtc->cursor) { ... ret = drm_mode_cursor_universal(crtc, req, file_priv, &ctx); goto out; } if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { ret = -ENXIO; goto out; } and fails with -ENXIO (or -EFAULT for DRM_MODE_CURSOR_MOVE) because dpu_crtc_funcs does not implement legacy cursor callbacks, while also preventing drm_atomic_helper_update_plane() from setting state->legacy_cursor_update for async cursor updates in can_do_async(). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
