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

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

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