Re: [PATCH] drm/i915/display: Set SPREAD_AMP bit to enable SSC

["Almahallawy, Khaled" <khaled.almahallawy@intel.com>]

On Fri, 2025-09-19 at 13:26 +0300, Jani Nikula wrote:
> On Thu, 18 Sep 2025, Khaled Almahallawy
> <khaled.almahallawy@intel.com> wrote:
> > According to DP Specs[1]:
> > "SSC is enabled or disabled when the SPREAD_AMP bit in the
> > DOWNSPREAD_CTRL register is set or cleared (DPCD 00107h[4]  = 1 or
> > 0,
> > respectively)."
> > 
> > Set the SPREAD_AMP bit before starting link training to ensure SSC
> > is enabled as required.
> > 
> > [1]: DisplayPort Standard v2.1 - Sec 2.2.3.1 De-spreading of the
> > Regenerated Stream Clock
> > 
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > index 27f3716bdc1f..db2ea3c51a5f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -711,9 +711,13 @@ static bool
> > intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
> >  
> >  void intel_dp_link_training_set_mode(struct intel_dp *intel_dp,
> > int link_rate, bool is_vrr)
> >  {
> > +	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  	u8 link_config[2];
> > +	bool ssc_enabled = intel_panel_use_ssc(display) &&
> > +			(intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
> > DP_MAX_DOWNSPREAD_0_5);
> 
> This is actually pretty messy.
> 
> intel_panel_use_ssc() looks at a module parameter, and then at an
> *obsolete* field in the VBT, but may also be quirked away.
> 
> And as the name implies, it's for panels, originally LVDS only.
> 
> Someone needs to look at the VBT spec, there's a handful of places
> mentioning SSC, and *all* usage of intel_panel_use_ssc() in the
> driver,
> and match those properly, using the right part from VBT depending on
> BDB
> version and panel/child device, etc.

Noted, I will give a try to implement this. 

> 
> (Side note, there's Cx0 PHY code looking at DPCD registers for this,
> WTF.)
> 
> This code here would probably benefit from having:
> 
> intel_dp_source_supports_ssc()
> 
> and
> 
> intel_dp_sink_supports_ssc()
> 
> to make all of this clear here.
> 
> > -	link_config[0] = is_vrr ? DP_MSA_TIMING_PAR_IGNORE_EN : 0;
> > +	link_config[0] = (is_vrr ? DP_MSA_TIMING_PAR_IGNORE_EN :
> > 0) |
> > +		(ssc_enabled ? DP_SPREAD_AMP_0_5 : 0);
> 
> Getting complex, maybe something like:
> 
> 	if (is_vrr)
> 		link_config[0] |= DP_MSA_TIMING_PAR_IGNORE_EN;
> 	if (intel_dp_source_supports_ssc() &&
> intel_dp_sink_supports_ssc())
> 		link_config[0] |= DP_SPREAD_AMP_0_5;

Sure, Will incorporate that. 
> 
> >  	link_config[1] = drm_dp_is_uhbr_rate(link_rate) ?
> >  			 DP_SET_ANSI_128B132B : DP_SET_ANSI_8B10B;
> >  	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL,
> > link_config, 2);
> 
Thank You for your review
Khaled