diff mbox series

[v4,4/5] drm/i915/psr: Add IO buffer wake times for LunarLake and beyond

Message ID 20240306083427.2040475-5-jouni.hogander@intel.com (mailing list archive)
State New, archived
Headers show
Series IO and fast wake lines calculation and increase fw sync length | expand

Commit Message

Hogander, Jouni March 6, 2024, 8:34 a.m. UTC 
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@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Ville Syrjälä March 7, 2024, 5:15 p.m. UTC | #1 
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@intel.com>
> ---
>  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.
And the only one of those that I think deserves a comment is skl since 
there we don't get a proper value from bspec directly.

>  }
>  
>  static bool _compute_alpm_params(struct intel_dp *intel_dp,
> -- 
> 2.34.1
Hogander, Jouni March 8, 2024, 8:39 a.m. UTC | #2 
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@intel.com>
> > ---
> >  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

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();

?

BR,

Jouni Högander

> And the only one of those that I think deserves a comment is skl
> since 
> there we don't get a proper value from bspec directly.
> 
> >  }
> >  
> >  static bool _compute_alpm_params(struct intel_dp *intel_dp,
> > -- 
> > 2.34.1
>
Ville Syrjälä March 8, 2024, 10:11 a.m. UTC | #3 
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@intel.com>
> > > ---
> > >  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.

> 
> 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.
Hogander, Jouni March 8, 2024, 11:01 a.m. UTC | #4 
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@intel.com>
> > > > ---
> > > >  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

>
diff mbox series

Patch

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;
 }
 
 static bool _compute_alpm_params(struct intel_dp *intel_dp,