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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 30, 2026 at 05:38:58PM +0000, sashiko-bot@kernel.org wrote:
> 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().

Hmm, I think this could even happen without joiner, if the
connectors' tile groups have somehow changed so that they
no longer match.

I suppose what we actually need is a port_sync_add_affected_crtcs().
Need to think if I can do that without causing extra fastsets on
all the synced crtcs when one of them needs a fastset...

-- 
Ville Syrjälä
Intel