> -----Original Message-----
> From: Deak, Imre <[email protected]>
> Sent: Tuesday, June 2, 2026 7:09 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 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.
>
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(based on the 2 gitlab issues around the fec):
- intel_pipe_config_compare() rejects fastset on the sibling whose new
crtc_state->fec_enable disagrees with the old (HW) value
forcing an unnecessary full modeset.
- verify_crtc_state() after commit reports a fec_enable mismatch
because the per-port HW bit is read back into every sibling's hw state.
Along with this refcount will have to unify the fec handling across the mst
streams.
This unify will be done by pulling the non-active state(mst stream not part of
the atomic commit, but is a sibling of the mst stream) and then will be a OR
operation across the mst streams.
Will be pushing a patch implementing this as well.
Thanks and Regards,
Arun R Murthy
--------------------
> > 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
> > > >
