Re: [PATCH] drm/i915/dp_mst: Fix check for FEC support for an uncompressed stream

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

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 <imre.deak@intel.com>
> > > ---
> > >  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 <jouni.hogander@intel.com>

> 
> > 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;
> > >  
> >