On Wed, 2025-10-29 at 16:17 +0200, Imre Deak wrote: > On Wed, Oct 29, 2025 at 04:06:07PM +0200, Hogander, Jouni wrote: > > On Tue, 2025-10-28 at 13:35 +0200, Imre Deak wrote: > > > The reason for enabling FEC for an uncompressed stream on an MST > > > link > > > is > > > that the DSC compression is enabled for another stream on the > > > same > > > link. > > > For such an uncompressed stream FEC doesn't need to be supported > > > on > > > the > > > whole path until the (DP-SST) sink DPRX. For instance if a branch > > > device > > > - like a monitor with an MST branch device within it - is plugged > > > to > > > a > > > DFP connector of an MST docking station and the monitor's branch > > > device does not support FEC, the docking station's branch device > > > will > > > still enable the link to the monitor correctly, disabling the FEC > > > on > > > that link as expected. Since it's been verified already that FEC > > > is > > > supported for the compressed stream above, the corresponding > > > check > > > for > > > the uncompressed stream can be dropped: the check for the > > > compressed > > > stream implies already that FEC is supported on the link between > > > the > > > source DPTX and immediate downstream branch device. If FEC is not > > > supported on the whole path until the sink DPRX, FEC will be > > > disabled > > > by > > > a downstream branch device on the path as described above for the > > > MST > > > dock + MST monitor configuration example. > > > > > > This fixes a problem in the above MST dock + MST monitor example, > > > where > > > the dock supports FEC, but the monitor doesn't support it and FEC > > > gets > > > enabled on the link due to DSC getting enabled for another > > > monitor's > > > stream on the same link. > > > > > > Closes: > > > https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14254 > > > Signed-off-by: Imre Deak <[email protected]> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index a845b2612a3fa..21a60b8c880ee 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -299,7 +299,14 @@ int intel_dp_mtp_tu_compute_config(struct > > > intel_dp *intel_dp, > > > * intel_dp_needs_8b10b_fec(). > > > */ > > > crtc_state->fec_enable = > > > intel_dp_needs_8b10b_fec(crtc_state, dsc); > > > - if (crtc_state->fec_enable && > > > + /* > > > + * If FEC gets enabled only because of another > > > compressed > > > stream, FEC > > > + * may not be supported for this uncompressed stream on > > > the > > > whole link > > > + * path until the sink DPRX. In this case a downstream > > > branch device > > > + * will disable FEC for the uncompressed stream as > > > expected > > > and so the > > > + * FEC support doesn't need to be checked for this > > > uncompressed stream. > > > + */ > > > + if (crtc_state->fec_enable && dsc && > > > > Why crtc_state->fec_enable is set if it's not going to enabled and > > not > > even supported in the crtc this crtc_state is for? > > It is going to be enabled as required by all the CRTCs on the same > link, > when any CRTC on this link uses DSC compression. It is also supported > by > the link as explained by the commit message (the CRTC using DSC > ensured > this support, all the way to the sink DPRX). It's not necessarily > supported all the way until the sink DPRX on _this_ CRTC, hence no > need > to check the support for _that_ calling intel_dp_supports_fec().
Thank you for the clarification. Reviewed-by: Jouni Högander <[email protected]> > > > Also there seems to be very similar check in > > mst_stream_compute_config. > > Do we need to change that as well? > > Right. intel_crtc_state::fec_enable isn't yet set at that point, > since > the state computation starts with a zeroed CRTC state. So, that check > should be removed (as a follow-up). > > > BR, > > > > Jouni Högander > > > > > !intel_dp_supports_fec(intel_dp, connector, > > > crtc_state)) > > > return -EINVAL; > > > > >
