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

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

On Thu, Oct 16, 2025 at 09:23:26PM +0300, Jouni Hogander wrote:
> 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?

Yes, I missed this, thanks for noticing it. Yes, it can be removed after
patch 6. There is a subtle interaction between
(a) intel_dp_dsc_compute_config() -> intel_dp_needs_8b10b_fec() and
(b) intel_dp_mtp_tu_compute_config() -> intel_dp_needs_8b10b_fec(),
whereby (b) can see intel_crtc_state::fec_enable being set, because (a)
has set it. Then (b) has to reset it for UHBR, as patch 2 and 3
explains. Removing the if (fec_enable) return true; case above reduces
that subtlety actually, which is good.

However, the check above shouldn't cause an issue (after patch 2) and
intel_crtc_state::fec_enable will still be computed twice in the end.
That's still a bit subtle and so I'd remove the check in a separate
patch, as a follow-up, to keep things isolated. Would you be ok with
that?

> BR,
> 
> Jouni Högander