Re: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 21, 2024 at 09:53:21AM +0200, Jouni Högander wrote:
> Current fast and IO wake lines calculation is assuming fast wake sync
> length is 18 pulses. Let's improve this by checking the actual length.
> 
> Also 10 us IO buffer wake time is currently assumed. This is not the case
> with LunarLake and beyond. Fix this by adding getter for IO wake time and
> return values there according to Bspec.
> 
> Bspec: 65450
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 40 +++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 72cadad09db5..4a1e07411716 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1150,6 +1150,28 @@ static bool _lnl_compute_alpm_params(struct intel_dp *intel_dp,
>  	return true;
>  }
>  
> +/*
> + * From Bspec:
> + *
> + * For Xe2 and beyond
> + * RBR 15us, HBR1 11us, higher rates 10us
> + *
> + * For pre-Xe2
> + * 10 us
> + */
> +static int get_io_wake_time(struct intel_dp *intel_dp,

No point in passing that. You can dig out the i915 from the crtc state.

> +			struct intel_crtc_state *crtc_state)

const

> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> +	if (DISPLAY_VER(i915) < 20 || crtc_state->port_clock > 270000)
> +		return 10;
> +	else if (crtc_state->port_clock > 162000)
> +		return 11;
> +	else
> +		return 15;

The new rate dependent stuff should be a separate patch.

And looks like the 10 usec will give us 44 usec io wake time, so
that should probably be a separate patch as well, to avoid
any functional changes when we introduce the formula.

> +}
> +
>  static bool _compute_alpm_params(struct intel_dp *intel_dp,
>  				 struct intel_crtc_state *crtc_state)
>  {
> @@ -1157,13 +1179,17 @@ static bool _compute_alpm_params(struct intel_dp *intel_dp,
>  	int io_wake_lines, io_wake_time, fast_wake_lines, fast_wake_time;
>  	u8 max_wake_lines;
>  
> -	if (DISPLAY_VER(i915) >= 12) {
> -		io_wake_time = 42;
> -		/*
> -		 * According to Bspec it's 42us, but based on testing
> -		 * it is not enough -> use 45 us.
> -		 */
> -		fast_wake_time = 45;
> +	if (intel_dp->get_aux_fw_sync_len) {
> +		int io_wake_time = get_io_wake_time(intel_dp, crtc_state);

Looks like this will shadow the variable you're trying to change.
Does the compiler not complain about this?

> +		int tfw_exit_latency = 20; /* eDP spec */
> +		int phy_wake = 4;	   /* eDP spec */
> +		int preamble = 8;	   /* eDP spec */
> +		int precharge = intel_dp->get_aux_fw_sync_len() - preamble;
> +
> +		io_wake_time = max(precharge, io_wake_time) + preamble +
> +			phy_wake + tfw_exit_latency;
> +		fast_wake_time = precharge + preamble + phy_wake +
> +			tfw_exit_latency;
>  
>  		/* TODO: Check how we can use ALPM_CTL fast wake extended field */
>  		max_wake_lines = 12;

I would also convert the older platforms to use the formula.
We do need to reverse calculate the io buffer on latency since
AFAICS it's not directly specified in bspec. But I think
that's better than not converting it since with the formula we
can't totally screw things up when eg. changing the precharge
length.

-- 
Ville Syrjälä
Intel