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 <[email protected]> > > > > --- > > > > 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
