Re: [PATCH 6/8] drm/msm/dpu: workaround res re-assignment issues by pairing enc/crtc

[sashiko-bot@kernel.org]

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 <xz.ca.cn@gmail.com>

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 <xz.ca.cn@gmail.com>

[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/20261026-msm-dpu-res-clean-v1-0-f46130a8a717@gmail.com?part=6