On Wed, 2024-02-21 at 23:45 +0200, Ville Syrjälä wrote:
> On Wed, Feb 21, 2024 at 09:16:22PM +0200, Ville Syrjälä wrote:
> > On Wed, Feb 21, 2024 at 09:05:43PM +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.
> > >
> > > > + 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.
> >
> > Hmm. The older platforms are apparently using fast_wake=32
> > which implies zero precharge pulses. That definitely does
> > not match what we program into the AUX control register...
>
> Looks like Windows just uses:
> pre-tgl:
> fast_wake=50
> io_fast_wake=50
> tgl-mtl:
> fast_wake=42
> io_fast_wake=42
With my patches we will have
pre-tgl:
fast_wake=50
io_fast_wake=44
tgl-mtl:
fast_wake=44
io_fast_wake=44
>
> Also for pre-tgl they clamp these to 5-8 instead of using
> the min=7 we have. For tgl+ they do clamp to 7-12.
At least Bspec 4289 say it's 7-8.
BR,
Jouni Högander
> And if the values exceed those limits they just proceed
> blindly with the clamped values, which is pretty dodgy.
>
