> -----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.
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
> >
