Re: [PATCH v2 1/2] drm/i915/mst: Unify fec_enable across mst streams

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 16 Jun 2026, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> FEC is a link-wide property: DP_TP_CTL_FEC_ENABLE is a per-port HW bit
> while crtc_state->fec_enable is per-stream. With DP MST several streams
> share the same port, so if any sibling stream needs FEC the per-port HW
> bit is on for every sibling. If sibling crtc_states disagree the
> following two symptoms appear:
>
> - intel_pipe_config_compare() rejects fastset on the sibling whose new
>   crtc_state->fec_enable disagrees with the old (HW) value
>   ("fastset requirement not met in fec_enable"), forcing an
>   unnecessary full modeset.
> - verify_crtc_state() after commit reports a fec_enable mismatch
>   ("[CRTC:..] mismatch in fec_enable (expected no, found yes)") because
>   the per-port HW bit is read back into every sibling's hw state.
>
> Walk every MST connector on @mst_mgr, pulling currently-active siblings
> into @state if they are not already in it (covers the case where the
> user's commit touches only a subset of MST streams on the link). Then OR
> all sibling fec_enable values together and write the unified result back
> into every sibling crtc_state. The unification only widens
> (false -> true), never narrows, so a stream that genuinely needs FEC
> keeps it.
>
> This runs from intel_dp_mst_atomic_check_link(), which is invoked after
> intel_atomic_check_config_and_link() has finished all per-stream
> compute_config and compute_config_late passes but before
> intel_crtc_check_fastset() and the post-commit verify, so the unified
> value is visible to both checks.

I believe the refcounting should already be in place. See my reply to
patch 2.

Here, it's slightly misleading to say FEC is a "per-port bit". I think
the main point here is that for DP MST it gets set/cleared for the
primary encoder & master transcoder in the first/last stream.

Maybe add debug logging to FEC enable/disable and see if it gets called
too many times or not.

Seems like the confusion comes from the fact that the readout is also
based on the master transcoder register, and throws off the state
checker.

BR,
Jani.


>
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16073
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> Tested-by: Stephen Fuhry <fuhrysteve@gmail.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 70 +++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index bcdc504913471a1ac7d255cde49a907c9f3d88a6..d487f1c90dcd2671754e6c6f28f207f32ace9ee2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -881,6 +881,72 @@ static int intel_dp_mst_check_bw(struct intel_atomic_state *state,
>  	return ret ? : -EAGAIN;
>  }
>  
> +/*
> + * Unify crtc_state->fec_enable across every MST sibling stream on @mst_mgr.
> + */
> +static int intel_dp_mst_unify_fec_enable(struct intel_atomic_state *state,
> +					 struct drm_dp_mst_topology_mgr *mst_mgr)
> +{
> +	struct intel_display *display = to_intel_display(state);
> +	struct drm_connector_list_iter connector_list_iter;
> +	struct intel_connector *connector;
> +	struct intel_crtc *crtcs[I915_MAX_PIPES];
> +	int n_crtcs = 0;
> +	bool need_fec = false;
> +	int ret = 0;
> +	int i;
> +
> +	drm_connector_list_iter_begin(display->drm, &connector_list_iter);
> +	for_each_intel_connector_iter(connector, &connector_list_iter) {
> +		struct intel_digital_connector_state *conn_state;
> +		struct intel_crtc_state *crtc_state;
> +		struct intel_crtc *crtc;
> +
> +		if (&connector->mst.dp->mst.mgr != mst_mgr)
> +			continue;
> +
> +		conn_state = intel_atomic_get_digital_connector_state(state,
> +								      connector);
> +		if (IS_ERR(conn_state)) {
> +			ret = PTR_ERR(conn_state);
> +			break;
> +		}
> +
> +		if (!conn_state->base.crtc)
> +			continue;
> +
> +		crtc = to_intel_crtc(conn_state->base.crtc);
> +		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			ret = PTR_ERR(crtc_state);
> +			break;
> +		}
> +
> +		if (!crtc_state->hw.active)
> +			continue;
> +
> +		if (drm_WARN_ON(display->drm, n_crtcs >= ARRAY_SIZE(crtcs)))
> +			break;
> +
> +		crtcs[n_crtcs++] = crtc;
> +		if (crtc_state->fec_enable)
> +			need_fec = true;
> +	}
> +	drm_connector_list_iter_end(&connector_list_iter);
> +
> +	if (ret || !need_fec)
> +		return ret;
> +
> +	for (i = 0; i < n_crtcs; i++) {
> +		struct intel_crtc_state *crtc_state =
> +			intel_atomic_get_new_crtc_state(state, crtcs[i]);
> +
> +		crtc_state->fec_enable = true;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * intel_dp_mst_atomic_check_link - check all modeset MST link configuration
>   * @state: intel atomic state
> @@ -908,6 +974,10 @@ int intel_dp_mst_atomic_check_link(struct intel_atomic_state *state,
>  	int i;
>  
>  	for_each_new_mst_mgr_in_state(&state->base, mgr, mst_state, i) {
> +		ret = intel_dp_mst_unify_fec_enable(state, mgr);
> +		if (ret)
> +			return ret;
> +
>  		ret = intel_dp_mst_check_dsc_change(state, mgr, limits);
>  		if (ret)
>  			return ret;

-- 
Jani Nikula, Intel