Re: [PATCH] drm/i915/display: Refcount for fec enable/disable

[Imre Deak <imre.deak@intel.com>]

On Tue, Jun 02, 2026 at 11:33:09AM -0400, Stephen Fuhry wrote:
> 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.

I found drm-debug-dmesg-fec-patch.txt on the ticket. This seems to be a
6.12 kernel, with the change in this patch applied. There is a strange
delay at boot-up between 13.894595 and 148.167 when nothing seems to
happen.

I see indeed multiple disconnect/reconnect events for the dock starting
like:

[  159.428600] usb 3-6: USB disconnect, device number 10
[  159.428605] usb 3-6.3: USB disconnect, device number 11
[  159.428607] usb 3-6.3.2: USB disconnect, device number 12
[  159.431529] i915 0000:00:02.0: [drm:gen11_hpd_irq_handler [i915]] hotplug event received, stat 0x00040000, dig 0x00008a88, pins 0x00000800, long 0x00000800
[  159.440423] usb 3-6.3.3: USB disconnect, device number 13
[  159.440430] usb 3-6.3.3.1: USB disconnect, device number 14
[  159.442548] usb 3-6.3.3.2: USB disconnect, device number 15
[  159.452345] usb 2-3: USB disconnect, device number 6
[  159.452352] r8152-cfgselector 2-3.1: USB disconnect, device number 7
[  159.521344] usb 3-6.3.3.4: USB disconnect, device number 16
[  159.544816] usb 2-3.3: USB disconnect, device number 8
[  165.839439] i915 0000:00:02.0: [drm:gen11_hpd_irq_handler [i915]] hotplug event received, stat 0x00040000, dig 0x00008a88, pins 0x00000800, long 0x00000800

repeated about 5 times start at 159.428. At first look this seems like a
non-display related issue leading to the disconnection of the whole dock
with all the USB device on it.

To get a better understanding of the root cause could you please provide
a log with the latest drm-tip kernel without any patch applied as I
requested on the ticket?

Thanks.

> 
> Stephen Fuhry
> 
> On Tue, Jun 2, 2026 at 9:39 AM Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Tue, Jun 02, 2026 at 04:35:42PM +0300, Murthy, Arun R wrote:
> > >
> > > > -----Original Message-----
> > > > From: Deak, Imre <imre.deak@intel.com>
> > > > Sent: Tuesday, June 2, 2026 4:17 PM
> > > > To: Murthy, Arun R <arun.r.murthy@intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org;
> > Stephen
> > > > Fuhry <fuhrysteve@gmail.com>
> > > > 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 <arun.r.murthy@intel.com>
> > > > > Tested-by: Stephen Fuhry <fuhrysteve@gmail.com>
> > > > > ---
> > > > >  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
> > > > >
> >