On Thu, 2025-10-16 at 23:00 +0300, Imre Deak wrote: > 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]>
Reviewed-by: Jouni Högander <[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? Yes, it shouldn't cause any issue. This is ok to me. Provided rb above. BR, Jouni Högander > > > BR, > > > > Jouni Högander
