Re: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation
Hi Jouni,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Jouni-H-gander/drm-i915-display-Add-aux-function-pointer-for-fast-wake-sync-pulse-count/20240221-160220
base: git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:
https://lore.kernel.org/r/20240221075322.2764209-3-jouni.hogander%40intel.com
patch subject: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines
calculation
config: i386-randconfig-r081-20240223
(https://download.01.org/0day-ci/archive/20240225/202402250758.kqbqxyrz-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202402250758.kqbqxyrz-...@intel.com/
smatch warnings:
drivers/gpu/drm/i915/display/intel_psr.c:1203 _compute_alpm_params() error:
uninitialized symbol 'io_wake_time'.
vim +/io_wake_time +1203 drivers/gpu/drm/i915/display/intel_psr.c
7903f1d36c3d97 Jouni Högander 2024-02-21 1174
96a24945731fe9 Jouni Högander 2024-01-30 1175 static bool
_compute_alpm_params(struct intel_dp *intel_dp,
cb42e8ede5b475 Jouni Högander 2023-02-21 1176
struct intel_crtc_state *crtc_state)
cb42e8ede5b475 Jouni Högander 2023-02-21 1177 {
cb42e8ede5b475 Jouni Högander 2023-02-21 1178 struct drm_i915_private
*i915 = dp_to_i915(intel_dp);
cb42e8ede5b475 Jouni Högander 2023-02-21 1179 int io_wake_lines,
io_wake_time, fast_wake_lines, fast_wake_time;
cb42e8ede5b475 Jouni Högander 2023-02-21 1180 u8 max_wake_lines;
cb42e8ede5b475 Jouni Högander 2023-02-21 1181
7903f1d36c3d97 Jouni Högander 2024-02-21 1182 if
(intel_dp->get_aux_fw_sync_len) {
7903f1d36c3d97 Jouni Högander 2024-02-21 1183 int
io_wake_time = get_io_wake_time(intel_dp, crtc_state);
This declaration shadows the earlier io_wake_time declaration.
7903f1d36c3d97 Jouni Högander 2024-02-21 1184 int
tfw_exit_latency = 20; /* eDP spec */
7903f1d36c3d97 Jouni Högander 2024-02-21 1185 int phy_wake =
4; /* eDP spec */
7903f1d36c3d97 Jouni Högander 2024-02-21 1186 int preamble =
8; /* eDP spec */
7903f1d36c3d97 Jouni Högander 2024-02-21 1187 int precharge =
intel_dp->get_aux_fw_sync_len() - preamble;
7903f1d36c3d97 Jouni Högander 2024-02-21 1188
7903f1d36c3d97 Jouni Högander 2024-02-21 1189 io_wake_time =
max(precharge, io_wake_time) + preamble +
7903f1d36c3d97 Jouni Högander 2024-02-21 1190
phy_wake + tfw_exit_latency;
7903f1d36c3d97 Jouni Högander 2024-02-21 1191 fast_wake_time
= precharge + preamble + phy_wake +
7903f1d36c3d97 Jouni Högander 2024-02-21 1192
tfw_exit_latency;
29f3067a236ac5 Jouni Högander 2024-01-30 1193
29f3067a236ac5 Jouni Högander 2024-01-30 1194 /* TODO: Check
how we can use ALPM_CTL fast wake extended field */
cb42e8ede5b475 Jouni Högander 2023-02-21 1195 max_wake_lines
= 12;
cb42e8ede5b475 Jouni Högander 2023-02-21 1196 } else {
cb42e8ede5b475 Jouni Högander 2023-02-21 1197 io_wake_time =
50;
cb42e8ede5b475 Jouni Högander 2023-02-21 1198 fast_wake_time
= 32;
cb42e8ede5b475 Jouni Högander 2023-02-21 1199 max_wake_lines
= 8;
cb42e8ede5b475 Jouni Högander 2023-02-21 1200 }
cb42e8ede5b475 Jouni Högander 2023-02-21 1201
cb42e8ede5b475 Jouni Högander 2023-02-21 1202 io_wake_lines =
intel_usecs_to_scanlines(
ef0af9db2a2125 Jouni Högander 2023-06-20 @1203
_state->hw.adjusted_mode, io_wake_time);
Uninitialized
cb42e8ede5b475 Jouni Högander 2023-02-21 1204 fast_wake_lines =
intel_usecs_to_scanlines(
ef0af9db2a2125 Jouni Högander 2023-06-20 1205
_state->hw.adjusted_mode, fast_wake_time);
cb42e8ede5b475 Jouni Högander 2023-02-21 1206
cb42e8ede5b475 Jouni Högander 2023-02-21 1207 if (io_wake_lines >
max_wake_lines ||
cb42e8ede5b475 Jouni Högander 2023-02-21 1208 fast_wake_lines >
max_wake_lines)
cb42e8ede5b475 Jouni Högander 2023-02-21 1209 return false;
cb42e8ede5b475 Jouni Högander 2023-02-21 1210
29f3067a236ac5 Jouni Högander 2024-01-30 1211 if
(!_lnl_compute_alpm_params(intel_dp, crtc_state))
29f3067a236ac5 Jouni Högander 2024-01-30 1212 return false;
29f3067a236ac5 Jouni Högande
Re: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation
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
> > > > ---
> > > > 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 >
> > > > 27)
> > > > + 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
>
>
Re: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation
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
> > ---
> > 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 >
> > 27)
> > + 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
Re: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation
Hi Jouni, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-tip/drm-tip next-20240221] [cannot apply to drm-intel/for-linux-next-fixes linus/master v6.8-rc5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Jouni-H-gander/drm-i915-display-Add-aux-function-pointer-for-fast-wake-sync-pulse-count/20240221-160220 base: git://anongit.freedesktop.org/drm-intel for-linux-next patch link: https://lore.kernel.org/r/20240221075322.2764209-3-jouni.hogander%40intel.com patch subject: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240222/202402220859.k3osmrci-...@intel.com/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 36adfec155de366d722f2bac8ff9162289dcf06c) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240222/202402220859.k3osmrci-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202402220859.k3osmrci-...@intel.com/ All warnings (new ones prefixed by >>): In file included from drivers/gpu/drm/i915/display/intel_psr.c:28: In file included from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:15: In file included from drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h:11: In file included from drivers/gpu/drm/xe/xe_bo.h:11: In file included from drivers/gpu/drm/xe/xe_bo_types.h:9: In file included from include/linux/iosys-map.h:10: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:78: include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 547 | val = __raw_readb(PCI_IOBASE + addr); | ~~ ^ include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~ ^ include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) | ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' 102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) | ^ In file included from drivers/gpu/drm/i915/display/intel_psr.c:28: In file included from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:15: In file included from drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h:11: In file included from drivers/gpu/drm/xe/xe_bo.h:11: In file included from drivers/gpu/drm/xe/xe_bo_types.h:9: In file included from include/linux/iosys-map.h:10: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:78: include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~ ^ include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) | ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' 115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) | ^ In file included from drivers/gpu/drm/i915/display/intel_psr.c:28: In file included from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:15: In file included from drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h:11: In file included from drivers/gpu/drm/xe/xe_bo.h:11: In file included from drivers/gpu/drm/xe/xe_bo_types.h:9: In file included from include/linux/iosys-map.h:10: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:78: include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arit
Re: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation
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
> > > ---
> > > 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 > 27)
> > > + 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
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.
And if the values exceed those limits they just proceed
blindly with the clamped values, which is pretty dodgy.
--
Ville Syrjälä
Intel
Re: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation
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
> > ---
> > 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 > 27)
> > + 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...
--
Ville Syrjälä
Intel
Re: [PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation
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
> ---
> 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 > 27)
> + 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
[PATCH 2/3] drm/i915/psr: Improve fast and IO wake lines calculation
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
---
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,
+ struct intel_crtc_state *crtc_state)
+{
+ struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+
+ if (DISPLAY_VER(i915) < 20 || crtc_state->port_clock > 27)
+ return 10;
+ else if (crtc_state->port_clock > 162000)
+ return 11;
+ else
+ return 15;
+}
+
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);
+ 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;
--
2.34.1
