On Fri, 2024-03-08 at 12:11 +0200, Ville Syrjälä wrote:
> On Fri, Mar 08, 2024 at 08:39:36AM +0000, Hogander, Jouni wrote:
> > On Thu, 2024-03-07 at 19:15 +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 06, 2024 at 10:34:26AM +0200, Jouni Högander wrote:
> > > > IO buffer wake time used for IO wake calculation is dependent
> > > > on
> > > > port clock
> > > > on LunarLake and beyond. Take this into account in
> > > > get_io_buffer_wake_time.
> > > >
> > > > Bspec: 65450
> > > >
> > > > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_psr.c | 9 ++++++++-
> > > > 1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index e1df0ece9fa3..e8410e770da8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1160,6 +1160,9 @@ static bool
> > > > _lnl_compute_alpm_params(struct
> > > > intel_dp *intel_dp,
> > > > * This is not directly mentioned in Bspec. There are 50 us io
> > > > wake time and 32
> > > > * us fast wake time. Clearly preharge pulses are not
> > > > (improperly)
> > > > included in
> > > > * 32 us fast wake time. 50 us - 32 us = 18 us.
> > > > + *
> > > > + * For DISPLAY_VER >= 20
> > > > + * RBR 15us, HBR1 11us, higher rates 10us
> > > > */
> > > > static int get_io_buffer_wake_time(const struct
> > > > intel_crtc_state
> > > > *crtc_state)
> > > > {
> > > > @@ -1167,8 +1170,12 @@ static int get_io_buffer_wake_time(const
> > > > struct intel_crtc_state *crtc_state)
> > > >
> > > > if (DISPLAY_VER(i915) < 12)
> > > > return 18;
> > > > - else
> > > > + else if (DISPLAY_VER(i915) < 20 || crtc_state-
> > > > >port_clock >
> > > > 270000)
> > > > return 10;
> > > > + else if (crtc_state->port_clock > 162000)
> > > > + return 11;
> > > > + else
> > > > + return 15;
> > >
> > > That's getting rather messy. I think having separate functions
> > > for
> > > skl vs. tgl vs. mtl (assuming the ver==20 is mtl) would be
> > > cleaner.
> >
> > ver == 20 == lnl
> > ver == 14 == mtl
>
> Hmm. I thought 14 was just bumped to 20 because someone didn't
> understand that 0x14==20.
This is in intel_display_device.c:
static const struct {
u16 ver;
u16 rel;
const struct intel_display_device_info *display;
} gmdid_display_map[] = {
{ 14, 0, &xe_lpdp_display },
{ 20, 0, &xe2_lpd_display },
};
>
> >
> > So you are thinking get_io_buffer_wake_time would do:
> >
> > if (DISPLAY_VER(i915) >= 20)
> > lnl_get_io_buffer_wake_time();
> > else if (DISPLAY_VER(i915) >= 12)
> > tgl_get_io_buffer_wake_time();
> > else
> > skl_get_buffer_wake_time();
> >
> > ?
>
> Yeah. Personally I would also drop the "get_" part from the function
> names. That just makes me think it's getting a reference or
> something.
Done. Please check new version.
BR,
Jouni Högander
>
