Re: [PATCH v2 16/23] drm/msm/dcp: Integrate the DSI bridge with KMS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/msm/dcp: dcp_crtc_atomic_begin() opens session on non-modeset commits
--

commit 9b8003ef1934260f1a1ec514fc980b7068b0a204
Author: Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>

drm/msm/dcp: Integrate the DSI bridge with KMS

This commit connects the DCP DSI bridge to the DCP KMS pipeline by adding
helpers to prepare the CRTC display session during atomic_begin and check
whether planes require dirtyfb updates during atomic_check.

> diff --git a/drivers/gpu/drm/msm/dcp/dcp_crtc.c b/drivers/gpu/drm/msm/dcp/dcp_crtc.c
> index ec4f74c2920f8..173a07df71c5f 100644
> --- a/drivers/gpu/drm/msm/dcp/dcp_crtc.c
> +++ b/drivers/gpu/drm/msm/dcp/dcp_crtc.c
> @@ -147,6 +147,12 @@ static void dcp_crtc_atomic_begin(struct drm_crtc *crtc,
>  		return;
>  	}
>  
> +	ret = dcp_dpu_kms_prepare_crtc_session(dcp_kms, crtc);
> +	if (ret) {
> +		cstate->hfi_error = ret;
> +		return;
> +	}

[Severity: Medium]
Should dcp_dpu_kms_prepare_crtc_session() only open a new session when
drm_atomic_crtc_needs_modeset(crtc->state) is true, or can calling it
unconditionally in dcp_crtc_atomic_begin() leave the session partially
initialized on non-modeset commits?

In msm_atomic_commit_tail(), drm_atomic_helper_commit_planes() (which calls
dcp_crtc_atomic_begin()) runs before drm_atomic_helper_commit_modeset_enables()
(which calls dcp_crtc_atomic_enable(), dcp_dsi_bridge_atomic_pre_enable(), and
dcp_dsi_bridge_atomic_enable(), and only runs when
drm_atomic_crtc_needs_modeset(crtc->state) is true).

If an initial modeset commit fails in dcp_dsi_bridge_prepare_session() or in
dcp_dsi_bridge_atomic_enable():

drivers/gpu/drm/msm/dcp/dcp_dsi_bridge.c:dcp_dsi_bridge_atomic_enable() {
	...
	ret = dcp_hfi_display_post_enable(kms->hfi, dsi_bridge->session_id);
	if (ret) {
		drm_err(bridge->dev,
			"DSI%u: HFI DISPLAY_POST_ENABLE failed: %d\n",
			dsi_bridge->intf_idx, ret);
		goto err_close;
	}
	...
err_close:
	dcp_hfi_clear_power_control_cb(kms->hfi, kms);
	dcp_hfi_display_close(kms->hfi, dsi_bridge->session_id);
	dsi_bridge->session_id   = 0;
	dsi_bridge->session_open = false;
	dcp_dsi_bridge_clear_session(dsi_bridge);
}

the session is closed and dsi_bridge->session_open is reset to false, while
crtc->state->active remains true.

On the next non-modeset plane update or pageflip where
drm_atomic_crtc_needs_modeset(crtc->state) is false,
dcp_crtc_atomic_begin() -> dcp_dpu_kms_prepare_crtc_session() ->
dcp_dsi_bridge_prepare_session() sees !dsi_bridge->session_open, opens a new
HFI session, sets dsi_bridge->session_open = true, and publishes session_id to
the CRTC:

drivers/gpu/drm/msm/dcp/dcp_dsi_bridge.c:dcp_dsi_bridge_prepare_session() {
	...
	dsi_bridge->session_id   = opened_session_id;
	dsi_bridge->session_open = true;
	dcp_dsi_bridge_publish_session(dsi_bridge);
	...
}

Because drm_atomic_helper_commit_modeset_enables() skips the CRTC and bridge
enable callbacks on a non-modeset commit, dcp_crtc_atomic_enable() and
dcp_dsi_bridge_atomic_enable() never run, skipping HFI_MODE_DPMS_ON,
drm_crtc_vblank_on(), panel bridge enable, HFI_COMMAND_DISPLAY_POST_ENABLE,
and HFI_EVENT_FRAME_SCAN_COMPLETE registration.

When dcp_crtc_commit_kickoff() then submits frame triggers for that session,
dcp_crtc_wait_for_commit_done() will wait on frame_done_wq for an event that
was never registered and time out after 500ms on every subsequent commit:

drivers/gpu/drm/msm/dcp/dcp_crtc.c:dcp_crtc_wait_for_commit_done() {
	...
	if (!wait_event_timeout(dcp_crtc->frame_done_wq,
				atomic_read(&dcp_crtc->pending_kickoff_cnt) == 0,
				timeout)) {
		drm_err(crtc->dev,
			"frame done timeout for CRTC%u\n",
			dcp_crtc->display_id);
		atomic_set(&dcp_crtc->pending_kickoff_cnt, 0);
	}
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=16