Thank you Ville for checking my patch. Please see my responses inline
below.
On Wed, 2024-02-21 at 21:05 +0200, Ville Syrjälä wrote:
> 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@xxxxxxxxx>
> > ---
> > 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.
Fixed.
> > + 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.
No it will end up as 42 as it was originally.
>
> > +}
> > +
> > 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?
Fixed.
>
> > + 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.
>
I will add this in next version.
BR,
Jouni Högander
