Re: [PATCH v2 3/7] drm/i915/dp: Export helper to determine if FEC on non-UHBR links is required

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Thu, 2025-10-16 at 20:18 +0300, Imre Deak wrote:
> On Thu, Oct 16, 2025 at 07:56:32PM +0300, Jouni Hogander wrote:
> > On Wed, 2025-10-15 at 19:19 +0300, Imre Deak wrote:
> > > Export the helper function to determine if FEC is required on a
> > > non-
> > > UHBR
> > > (8b10b) SST or MST link. A follow up change will take this into
> > > use
> > > for
> > > MST as well.
> > > 
> > > While at it determine the output type from the CRTC state, which
> > > allows
> > > dropping the intel_dp argument. Also make the function return the
> > > required FEC state, instead of setting this in the CRTC state,
> > > which
> > > allows only querying this requirement, without changing the
> > > state.
> > > 
> > > Also rename the function to intel_dp_needs_8b10b_fec(), to
> > > clarify
> > > that
> > > the function determines if FEC is required on an 8b10b link (on
> > > 128b132b
> > > links FEC is always enabled by the HW implicitly, so the function
> > > will
> > > return false for that case).
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c     | 21 +++++++++++++--
> > > ----
> > > --
> > >  drivers/gpu/drm/i915/display/intel_dp.h     |  2 ++
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c |  2 +-
> > >  3 files changed, 16 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index b523c4e661412..3ffb015004c54 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -2365,24 +2365,29 @@ static int
> > > intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > >  	return 0;
> > >  }
> > >  
> > > -static void intel_dp_fec_compute_config(struct intel_dp
> > > *intel_dp,
> > > -					struct intel_crtc_state
> > > *crtc_state)
> > > +/*
> > > + * Return whether FEC must be enabled for 8b10b SST or MST
> > > links. On
> > > 128b132b
> > > + * links FEC is always enabled implicitly by the HW, so this
> > > function returns
> > > + * false for that case.
> > > + */
> > > +bool intel_dp_needs_8b10b_fec(const struct intel_crtc_state
> > > *crtc_state,
> > > +			      bool dsc_enabled_on_crtc)
> > >  {
> > >  	if (intel_dp_is_uhbr(crtc_state))
> > > -		return;
> > > +		return false;
> > >  
> > >  	if (crtc_state->fec_enable)
> > > -		return;
> > > +		return true;
> > 
> > Not really changed in this patch: Do you know in what kind of case
> > "crtc_state->fec_enable == true" before intel_dp_needs_8b10b_fec is
> > called?
> 
> Yes, that's another corner which should've been documented at least:
> that's the case when there are two or more CRTCs on a 8b10b MST link,
> only one/some of them enabling DSC (but not all). For instance,
> CRTC#1
> enables DSC, but CRTC#0 does not enable DSC. First CRTC#0's state is
> computed (due to the regular order of CRTC#0/1 etc. state
> computation)
> and since DSC is not enabled on it, it will compute
> intel_crtc_state::fec_enable=false. Then CRTC#1 will compute
> fec_enable=true, due to it enabling DSC.
> 
> After all CRTCs computed their state, through
> intel_atomic_check_config_and_link() -> intel_atomic_check_config(),
> the
> following path will detect that the FEC enabled state is different
> for
> the CRTCs on the same MST link (whereas the FEC enabled state should
> be
> the same for all the CRTCs on the link, since FEC is the property of
> the
> link encoding). After detecting this, the state of all the CRTCs on
> the
> link will be recomputed with FEC forced on now for all (forced, even
> if
> DSC is not enabled for a CRTC):
> 
> intel_atomic_check_config_and_link() -> intel_link_bw_atomic_check()
> -> 
> check_all_link_config() -> intel_dp_mst_atomic_check_link() ->
> intel_dp_mst_check_fec_change().
> 
> The above will set the intel_link_bw_limits::force_fec_pipes mask for
> both CRTC#0 and CRTC#1, then both CRTCs' state gets recomputed,
> during
> which intel_crtc_state::fec_enable will be set upfront in
> intel_modeset_pipe_config(). The above intel_dp_dsc_compute_config()
> ->
> intel_dp_needs_8b10b_fec() will be called after all the above (during
> the second round of state computation) and so with
> intel_crtc_state::fec_enable already set. This set state must be
> preserved for CRTC#0 as well, even though it doesn't enable DSC.

Thank you for the clarification. Patch 6 converts
intel_dp_mst_check_fec_change -> intel_dp_mst_check_dsc_change. I.e.
force_fec_pipes mask is not set and it doesn't exist. Instead there is
force_dsc_pipes mask. Maybe this could be dropped in patch 6 as
intel_modeset_pipe_config is setting compression_enabled_on_link
instead and this is also checked in intel_dp_needs_8b10b_fec? 

BR,

Jouni Högander

> 
> > BR,
> > 
> > Jouni Högander
> > >  
> > >  	/*
> > >  	 * Though eDP v1.5 supports FEC with DSC, unlike DP, it
> > > is
> > > optional.
> > >  	 * Since, FEC is a bandwidth overhead, continue to not
> > > enable it for
> > >  	 * eDP. Until, there is a good reason to do so.
> > >  	 */
> > > -	if (intel_dp_is_edp(intel_dp))
> > > -		return;
> > > +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP))
> > > +		return false;
> > >  
> > > -	crtc_state->fec_enable = true;
> > > +	return dsc_enabled_on_crtc;
> > >  }
> > >  
> > >  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > > @@ -2404,7 +2409,7 @@ int intel_dp_dsc_compute_config(struct
> > > intel_dp
> > > *intel_dp,
> > >  	 * FIXME: set the FEC enabled state once pipe_config-
> > > > port_clock is
> > >  	 * already known, so the UHBR/non-UHBR mode can be
> > > determined.
> > >  	 */
> > > -	intel_dp_fec_compute_config(intel_dp, pipe_config);
> > > +	pipe_config->fec_enable =
> > > intel_dp_needs_8b10b_fec(pipe_config, true);
> > >  
> > >  	if (!intel_dp_dsc_supports_format(connector,
> > > pipe_config-
> > > > output_format))
> > >  		return -EINVAL;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> > > b/drivers/gpu/drm/i915/display/intel_dp.h
> > > index b379443e0211e..55059bd5c7efb 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > > @@ -73,6 +73,8 @@ void intel_dp_encoder_flush_work(struct
> > > drm_encoder
> > > *encoder);
> > >  int intel_dp_compute_config(struct intel_encoder *encoder,
> > >  			    struct intel_crtc_state
> > > *pipe_config,
> > >  			    struct drm_connector_state
> > > *conn_state);
> > > +bool intel_dp_needs_8b10b_fec(const struct intel_crtc_state
> > > *crtc_state,
> > > +			      bool dsc_enabled_on_crtc);
> > >  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  				struct intel_crtc_state
> > > *pipe_config,
> > >  				struct drm_connector_state
> > > *conn_state,
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index 27e952a67c343..d0590b5ffffd7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -296,7 +296,7 @@ int intel_dp_mtp_tu_compute_config(struct
> > > intel_dp *intel_dp,
> > >  	/*
> > >  	 * NOTE: The following must reset crtc_state->fec_enable
> > > for
> > > UHBR/DSC
> > >  	 * after it was set by intel_dp_dsc_compute_config() ->
> > > -	 * intel_dp_fec_compute_config().
> > > +	 * intel_dp_needs_8b10b_fec().
> > >  	 */
> > >  	if (dsc) {
> > >  		if (!intel_dp_supports_fec(intel_dp, connector,
> > > crtc_state))
> >