Re: [PATCH 10/10] drm/i915: Allow joiner with port sync

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915: port sync dropped when joiner secondary CRTC triggers modeset
--

commit e8591be1b66ac026c8025978e792560a0337567b
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm/i915: Allow joiner with port sync

This commit removes the check preventing pipes using joiner from entering
port sync mode now that modeset enable and disable sequences iterate in
pipe/transcoder order.

> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 2e7f278a1b6c6..3be5b429ac876 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4675,14 +4675,8 @@ static bool m_n_equal(const struct intel_link_m_n *m_n_1,
>  static bool crtcs_port_sync_compatible(const struct intel_crtc_state *crtc_state1,
>  				       const struct intel_crtc_state *crtc_state2)
>  {
> -	/*
> -	 * FIXME the modeset sequence is currently wrong and
> -	 * can't deal with joiner + port sync at the same time.
> -	 *
> -	 * TODO VRR + port sync needs special handling
> -	 */
> +	/* TODO VRR + port sync needs special handling */
>  	return crtc_state1->hw.active && crtc_state2->hw.active &&
> -		!crtc_state1->joiner_pipes && !crtc_state2->joiner_pipes &&

[Severity: High]
Can allowing joiner with port sync here cause port sync to be dropped when an
atomic commit triggers a modeset via a joiner secondary CRTC?

In drivers/gpu/drm/i915/display/intel_display.c:intel_atomic_check(),
drm_atomic_helper_check_modeset() runs connector atomic_check callbacks
before intel_atomic_check_config_and_link() pulls in joined CRTCs:

	ret = drm_atomic_helper_check_modeset(dev, &state->base);
	if (ret)
		goto fail;
	...
	ret = intel_atomic_check_config_and_link(state);

Suppose a joined CRTC pair (primary Pipe A and secondary Pipe B) is active in
port sync mode with a peer CRTC (Pipe C). If userspace submits an atomic
commit targeting the joiner secondary CRTC (Pipe B, which has uapi.enable ==
false and no uapi connectors) that sets uapi.mode_changed = true in
intel_atomic_check() (for example by changing scaling_filter,
sharpness_strength, or vrr_enabled, or clearing inherited on the first
userspace commit), drm_atomic_helper_check_modeset() runs while neither Pipe
A nor its connector is in the atomic state.

Later, intel_atomic_check_config_and_link() -> intel_atomic_check_config() ->
intel_joiner_add_affected_crtcs() adds Pipe A, sets Pipe A's
uapi.mode_changed = true, and adds Pipe A's connector:

	for_each_intel_crtc_in_pipe_mask(display, crtc, modeset_pipes) {
		int ret;

		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);

		crtc_state->uapi.mode_changed = true;

		ret = drm_atomic_add_affected_connectors(&state->base, &crtc->base);

Because drm_atomic_helper_check_modeset() has already finished,
intel_dp_connector_atomic_check() -> intel_modeset_tile_group() and
intel_modeset_synced_crtcs() never run for Pipe A's connector, so Pipe C and
its connector are never added to the atomic state.

When intel_modeset_pipe_config_late() -> intel_ddi_compute_config_late() ->
intel_ddi_port_sync_transcoders() subsequently iterates over connectors in
the atomic state:

	for_each_new_connector_in_state(&state->base, connector, conn_state, i) {
		struct intel_crtc *crtc = to_intel_crtc(conn_state->crtc);
		const struct intel_crtc_state *crtc_state;

it only sees Pipe A's connector, leaving master_transcoder =
INVALID_TRANSCODER and sync_mode_slaves_mask = 0 on Pipe A.

In intel_crtc_check_fastset(), intel_pipe_config_compare() then sees the
mismatch in master_transcoder or sync_mode_slaves_mask against old_crtc_state
and forces a full modeset on Pipe A and Pipe B without Pipe C in the commit.
If Pipe A was the port sync master, Pipe C's hardware transcoder remains
active and enslaved to TRANSCODER_A while TRANSCODER_A is disabled and
re-enabled, and bdw_get_trans_port_sync_config() during
intel_modeset_verify_crtc() reads back a non-zero sync_mode_slaves_mask,
triggering INTEL_DISPLAY_STATE_WARN in verify_crtc_state().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030171327.29829-1-ville.syrjala@linux.intel.com?part=10