I can share some observations from the debug logs captured on the affected
system (drm.debug=0x10e, Alder Lake-P [8086:46a6], ThinkPad dock, 1080p on
pipe B + 4K DSC on pipe C) that may help clarify the root cause.
Imre's assertion about the enable/disable gating appears to be correct from
the logs. The active_links sequence across every modeset cycle in the
captured trace is always clean:
disable: 2 -> 1 -> 0 (intel_mst_disable_dp, intel_encoders_post_disable)
enable: 0 -> 1 -> 2 (intel_mst_pre_enable_dp, intel_mst_enable_dp)
No interleaving, no double-calls. intel_ddi_enable_fec and
intel_ddi_disable_fec are each called exactly once per link per modeset
cycle, as expected.
The mismatch and cascade are instead driven by repeated failures in the
fastset check. In the cable-unplug log, 8 "fastset requirement not met,
forcing full modeset" messages fire for both pipes between t=110.803s and
t=110.899s -- before a single active_links change occurs. After replug,
there are dozens more at ~20ms intervals (t=125.5s to t=126.3s) as the
driver re-probes the dock, each one attempting and failing to commit a
modeset.
The direction of the first mismatch after dock connect is notable. The
very first fastset failure shows:
[CRTC:186:pipe C] fastset requirement not met in fec_enable
(expected no, found yes)
[CRTC:186:pipe C] fastset requirement not met in
hw.pipe_mode.crtc_hdisplay
(expected 1920, found 3840)
[CRTC:186:pipe C] fastset requirement not met in dsc.config.pic_width
(expected 0, found 3840)
The SW computed state has pipe C at 1920x1080 with no DSC and no FEC, while
HW has it at 3840x2160 with DSC and FEC active (from the previous
session/firmware state). The modeset that follows clears FEC. The next
compute correctly selects DSC (and therefore FEC), triggering the opposite
mismatch ("expected yes, found no"). This back-and-forth drives the cascade.
The question of why the first SW compute produces fec_enable=no for pipe C
when DSC is needed seems to be where the actual root cause lies -- whether
that's a mode-negotiation timing issue (compositor requesting 1080p before
4K+DSC is negotiated) or something in the compute ordering in
intel_dp_mst_compute_config.
Full log (drm.debug=0x10e, cable unplug/replug) is attached to issue #16303
if useful.
Stephen Fuhry
On Tue, Jun 2, 2026 at 9:39 AM Imre Deak <[email protected]> wrote:
> On Tue, Jun 02, 2026 at 04:35:42PM +0300, Murthy, Arun R wrote:
> >
> > > -----Original Message-----
> > > From: Deak, Imre <[email protected]>
> > > Sent: Tuesday, June 2, 2026 4:17 PM
> > > To: Murthy, Arun R <[email protected]>
> > > Cc: [email protected]; [email protected];
> Stephen
> > > Fuhry <[email protected]>
> > > Subject: Re: [PATCH] drm/i915/display: Refcount for fec enable/disable
> > >
> > > On Mon, Jun 01, 2026 at 07:59:43PM +0530, Arun R Murthy wrote:
> > > > The FEC_ENABLE bit is per port basis and is enabled/disabled on ddi
> > > > pre_enable and post_disable. This fec is shared across the mst
> streams
> > > > and can be enabled per stream basis as well.
> > > > So have a refcount to track the usage of FEC and then enable/disable
> > > > accordingly.
> > > >
> > > > Closes:
> > > > https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16073
> > > > Signed-off-by: Arun R Murthy <[email protected]>
> > > > Tested-by: Stephen Fuhry <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_ddi.c | 66
> +++++++++++++++++++
> > > > drivers/gpu/drm/i915/display/intel_ddi.h | 1 +
> > > > .../drm/i915/display/intel_display_types.h | 12 ++++
> > > > .../drm/i915/display/intel_modeset_setup.c | 6 ++
> > > > 4 files changed, 85 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index 86520848892e..e12a3d6d6a67 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -2096,6 +2096,47 @@ void intel_ddi_disable_clock(struct
> intel_encoder
> > > *encoder)
> > > > encoder->disable_clock(encoder);
> > > > }
> > > >
> > > > +/**
> > > > + * intel_ddi_seed_fec_refcounts - Seed per-port FEC refcounts from
> > > > +active CRTCs
> > > > + * @display: display device
> > > > + *
> > > > + * intel_digital_port::fec_active_streams is the per-port refcount
> > > > +that gates
> > > > + * programming of the shared DP_TP_CTL_FEC_ENABLE bit. After initial
> > > > +HW state
> > > > + * readout (driver load, resume, GPU reset takeover), the persistent
> > > > + * crtc_state->fec_enable values reflect what HW currently has; we
> > > > +need to
> > > > + * align the refcount with that so the first paired disable doesn't
> > > > +underflow
> > > > + * and the next enable doesn't incorrectly skip programming the HW
> bit.
> > > > + *
> > > > + * Must be called once after intel_modeset_readout_hw_state(),
> before
> > > > +any new
> > > > + * modeset commit can run.
> > > > + */
> > > > +void intel_ddi_seed_fec_refcounts(struct intel_display *display) {
> > > > + struct intel_crtc *crtc;
> > > > +
> > > > + for_each_intel_crtc(display->drm, crtc) {
> > > > + const struct intel_crtc_state *crtc_state =
> > > > + to_intel_crtc_state(crtc->base.state);
> > > > + struct intel_encoder *encoder;
> > > > +
> > > > + if (!crtc_state->hw.active || !crtc_state->fec_enable)
> > > > + continue;
> > > > +
> > > > + for_each_intel_encoder(display->drm, encoder) {
> > > > + struct intel_digital_port *dig_port;
> > > > +
> > > > + if (encoder->base.crtc != &crtc->base)
> > > > + continue;
> > > > + if (!intel_encoder_is_dig_port(encoder))
> > > > + continue;
> > > > +
> > > > + dig_port = enc_to_dig_port(encoder);
> > > > + dig_port->fec_active_streams++;
> > > > + break;
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > void intel_ddi_sanitize_encoder_pll_mapping(struct intel_encoder
> > > > *encoder) {
> > > > struct intel_display *display = to_intel_display(encoder); @@
> > > > -2413,12 +2454,22 @@ static void intel_ddi_enable_fec(struct
> intel_encoder
> > > *encoder,
> > > > const struct intel_crtc_state
> *crtc_state) {
> > > > struct intel_display *display = to_intel_display(encoder);
> > > > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > > int i;
> > > > int ret;
> > > >
> > > > if (!crtc_state->fec_enable)
> > > > return;
> > > >
> > > > + /*
> > > > + * FEC is link-wide: DP_TP_CTL_FEC_ENABLE is per-port while
> > > > + * crtc_state->fec_enable is per-stream. For DP MST, several
> streams
> > > > + * on this port share the bit. Only program HW on the first stream
> > > > + * needing FEC; subsequent streams just bump the refcount.
> > > > + */
> > > > + if (dig_port->fec_active_streams++ > 0)
> > > > + return;
> > >
> > > This doesn't make sense to me. FEC is enabled for the MST link and if
> it's
> > > enabled then fec_enabled is set in the crtc_state for all the streams
> in the MST
> > > topology. intel_ddi_enable_fec() will be called only for the first MST
> stream
> > > being enabled and intel_ddi_disable_fec() will be called only for the
> last MST
> > > stream being disabled. So I don't see why the above refcounting would
> be
> > > needed.
> >
> > The logs mentioned in the above listed gitlab issue shows mismatch in
> > fec enable/disable in the MST scenario. Hence added this refcount
> > logic to overcome the mismatch.
>
> The root cause for the mismatch should be better understood then. I
> still think that it's something else than the lack of refcounting.
>
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> > > > +
> > > > intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state),
> > > > 0, DP_TP_CTL_FEC_ENABLE);
> > > >
> > > > @@ -2454,10 +2505,25 @@ static void intel_ddi_disable_fec(struct
> > > intel_encoder *encoder,
> > > > const struct intel_crtc_state
> *crtc_state) {
> > > > struct intel_display *display = to_intel_display(encoder);
> > > > + struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > > >
> > > > if (!crtc_state->fec_enable)
> > > > return;
> > > >
> > > > + /*
> > > > + * FEC is a link-wide property and DP_TP_CTL_FEC_ENABLE is a
> per-port
> > > > + * register, but crtc_state->fec_enable is per-stream. For DP MST,
> > > > + * multiple streams on the same port share this bit. Refcount the
> > > > + * active FEC users on the port and only clear the HW bit when the
> > > > + * last user goes away, otherwise tearing down one MST stream would
> > > > + * disable FEC for sibling streams still using it.
> > > > + */
> > > > + if (drm_WARN_ON(display->drm, dig_port->fec_active_streams <= 0))
> > > > + return;
> > > > +
> > > > + if (--dig_port->fec_active_streams > 0)
> > > > + return;
> > > > +
> > > > intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state),
> > > > DP_TP_CTL_FEC_ENABLE, 0);
> > > > intel_de_posting_read(display, dp_tp_ctl_reg(encoder, crtc_state));
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h
> > > > b/drivers/gpu/drm/i915/display/intel_ddi.h
> > > > index 580ecb09b8b6..3678c28a0dc9 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.h
> > > > @@ -78,6 +78,7 @@ int intel_ddi_toggle_hdcp_bits(struct intel_encoder
> > > *intel_encoder,
> > > > enum transcoder cpu_transcoder,
> > > > bool enable, u32 hdcp_mask); void
> > > > intel_ddi_sanitize_encoder_pll_mapping(struct intel_encoder
> *encoder);
> > > > +void intel_ddi_seed_fec_refcounts(struct intel_display *display);
> > > > int intel_ddi_level(struct intel_encoder *encoder,
> > > > const struct intel_crtc_state *crtc_state,
> > > > int lane);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index f44be5c689ae..84bd0d993197 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1987,6 +1987,18 @@ struct intel_digital_port {
> > > > struct ref_tracker *ddi_io_wakeref;
> > > > struct ref_tracker *aux_wakeref;
> > > >
> > > > + /*
> > > > + * Number of active streams on this port currently using FEC.
> > > > + *
> > > > + * DP_TP_CTL_FEC_ENABLE is a per-port (link-wide) HW bit, but
> > > > + * crtc_state->fec_enable is per-stream. For DP MST several streams
> > > > + * share the same port and therefore the same FEC enable bit. Track
> > > > + * how many active streams want FEC so that the HW bit is only
> > > > + * programmed on the first enable and only cleared on the last
> > > > + * disable. Modified under the modeset locks.
> > > > + */
> > > > + int fec_active_streams;
> > > > +
> > > > struct intel_tc_port *tc;
> > > >
> > > > struct {
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > > b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > > index e88082c8caac..14f038b8ef81 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> > > > @@ -950,6 +950,12 @@ void intel_modeset_setup_hw_state(struct
> > > > intel_display *display,
> > > >
> > > > intel_modeset_readout_hw_state(display);
> > > >
> > > > + /*
> > > > + * Seed per-port FEC refcounts from the just-populated active
> > > > + * crtc_states before anything can issue an enable/disable.
> > > > + */
> > > > + intel_ddi_seed_fec_refcounts(display);
> > > > +
> > > > /* HW state is read out, now we need to sanitize this mess. */
> > > > get_encoder_power_domains(display);
> > > >
> > > > --
> > > > 2.25.1
> > > >
>
