| Message ID | 20211025071317.3393329-3-tejaskumarx.surendrakumar.upadhyay@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable PXP support | expand |
On 10/25/2021 12:13 AM, Tejas Upadhyay wrote: > selftest --r live shows failure in suspend tests when > RPM wakelock is not acquired during suspend. > > This changes addresses below error : > <4> [154.177535] RPM wakelock ref not held during HW access > <4> [154.177575] WARNING: CPU: 4 PID: 5772 at > drivers/gpu/drm/i915/intel_runtime_pm.h:113 > fwtable_write32+0x240/0x320 [i915] > <4> [154.177974] Modules linked in: i915(+) vgem drm_shmem_helper > fuse snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic > ledtrig_audio mei_hdcp mei_pxp x86_pkg_temp_thermal coretemp > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_intel_dspcfg > snd_hda_codec snd_hwdep igc snd_hda_core ttm mei_me ptp > snd_pcm prime_numbers mei i2c_i801 pps_core i2c_smbus intel_lpss_pci > btusb btrtl btbcm btintel bluetooth ecdh_generic ecc [last unloaded: i915] > <4> [154.178143] CPU: 4 PID: 5772 Comm: i915_selftest Tainted: G > U 5.15.0-rc6-CI-Patchwork_21432+ #1 > <4> [154.178154] Hardware name: ASUS System Product Name/TUF GAMING > Z590-PLUS WIFI, BIOS 0811 04/06/2021 > <4> [154.178160] RIP: 0010:fwtable_write32+0x240/0x320 [i915] > <4> [154.178604] Code: 15 7b e1 0f 0b e9 34 fe ff ff 80 3d a9 89 31 > 00 00 0f 85 31 fe ff ff 48 c7 c7 88 9e 4f a0 c6 05 95 89 31 00 01 e8 > c0 15 7b e1 <0f> 0b e9 17 fe ff ff 8b 05 0f 83 58 e2 85 c0 0f 85 8d > 00 00 00 48 > <4> [154.178614] RSP: 0018:ffffc900016279f0 EFLAGS: 00010286 > <4> [154.178626] RAX: 0000000000000000 RBX: ffff888204fe0ee0 > RCX: 0000000000000001 > <4> [154.178634] RDX: 0000000080000001 RSI: ffffffff823142b5 > RDI: 00000000ffffffff > <4> [154.178641] RBP: 00000000000320f0 R08: 0000000000000000 > R09: c0000000ffffcd5a > <4> [154.178647] R10: 00000000000f8c90 R11: ffffc90001627808 > R12: 0000000000000000 > <4> [154.178654] R13: 0000000040000000 R14: ffffffffa04d12e0 > R15: 0000000000000000 > <4> [154.178660] FS: 00007f7390aa4c00(0000) GS:ffff88844f000000(0000) > knlGS:0000000000000000 > <4> [154.178669] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > <4> [154.178675] CR2: 000055bc40595028 CR3: 0000000204474005 > CR4: 0000000000770ee0 > <4> [154.178682] PKRU: 55555554 > <4> [154.178687] Call Trace: > <4> [154.178706] intel_pxp_fini_hw+0x23/0x30 [i915] > <4> [154.179284] intel_pxp_suspend+0x1f/0x30 [i915] > <4> [154.179807] live_gt_resume+0x5b/0x90 [i915] > > Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > index 524eaf678790..65535f459f9b 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > @@ -298,10 +298,14 @@ static void wait_for_suspend(struct intel_gt *gt) > > void intel_gt_suspend_prepare(struct intel_gt *gt) > { > + intel_wakeref_t wakeref; > + > user_forcewake(gt, true); > wait_for_suspend(gt); > > - intel_pxp_suspend(>->pxp, false); > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) { > + intel_pxp_suspend(>->pxp, false); > + } Chris had spotted this as well, but he suggested to go with the same stile we have for other components where we split the HW access parts in gt_suspend_late, something like the below (compile-tested only), which IMO is a better approach. Daniele diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index b4a8594bc46c..4f68299d1fd9 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -303,7 +303,7 @@ void intel_gt_suspend_prepare(struct intel_gt *gt) user_forcewake(gt, true); wait_for_suspend(gt); - intel_pxp_suspend(>->pxp, false); + intel_pxp_suspend_prepare(>->pxp, false); } static suspend_state_t pm_suspend_target(void) @@ -343,6 +343,8 @@ void intel_gt_suspend_late(struct intel_gt *gt) return; with_intel_runtime_pm(gt->uncore->rpm, wakeref) { + intel_pxp_suspend(>->pxp); + intel_rps_disable(>->rps); intel_rc6_disable(>->rc6); intel_llc_disable(>->llc); @@ -355,7 +357,7 @@ void intel_gt_suspend_late(struct intel_gt *gt) void intel_gt_runtime_suspend(struct intel_gt *gt) { - intel_pxp_suspend(>->pxp, true); + intel_pxp_runtime_suspend(>->pxp); intel_uc_runtime_suspend(>->uc); GT_TRACE(gt, "\n"); @@ -373,7 +375,7 @@ int intel_gt_runtime_resume(struct intel_gt *gt) if (ret) return ret; - intel_pxp_resume(>->pxp); + intel_pxp_runtime_resume(>->pxp); return 0; } diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c index 23fd86de5a24..7ffdc92e5325 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c @@ -8,7 +8,7 @@ #include "intel_pxp_pm.h" #include "intel_pxp_session.h" -void intel_pxp_suspend(struct intel_pxp *pxp, bool runtime) +void intel_pxp_suspend_prepare(struct intel_pxp *pxp, bool runtime) { if (!intel_pxp_is_enabled(pxp)) return; @@ -23,6 +23,12 @@ void intel_pxp_suspend(struct intel_pxp *pxp, bool runtime) */ if (!runtime) intel_pxp_invalidate(pxp); +} + +void intel_pxp_suspend(struct intel_pxp *pxp) +{ + if (!intel_pxp_is_enabled(pxp)) + return; intel_pxp_fini_hw(pxp); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h index c89e97a0c3d0..f1121865dc9d 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h @@ -9,10 +9,15 @@ #include "intel_pxp_types.h" #ifdef CONFIG_DRM_I915_PXP -void intel_pxp_suspend(struct intel_pxp *pxp, bool runtime); +void intel_pxp_suspend_prepare(struct intel_pxp *pxp, bool runtime); +void intel_pxp_suspend(struct intel_pxp *pxp); void intel_pxp_resume(struct intel_pxp *pxp); #else -static inline void intel_pxp_suspend(struct intel_pxp *pxp, bool runtime) +static inline void intel_pxp_suspend_prepare(struct intel_pxp *pxp, bool runtime) +{ +} + +static inline void intel_pxp_suspend(struct intel_pxp *pxp) { } @@ -21,4 +26,15 @@ static inline void intel_pxp_resume(struct intel_pxp *pxp) } #endif +static inline void intel_pxp_runtime_suspend(struct intel_pxp *pxp) +{ + intel_pxp_suspend_prepare(pxp, true); + intel_pxp_suspend(pxp); +} + +static inline void intel_pxp_runtime_resume(struct intel_pxp *pxp) +{ + intel_pxp_resume(pxp); +} + #endif /* __INTEL_PXP_PM_H__ */ > } > > static suspend_state_t pm_suspend_target(void)
> -----Original Message----- > From: Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com> > Sent: 29 October 2021 05:41 > To: Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadhyay@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH V2 2/2] drm/i915/gt: Hold RPM wakelock > during PXP suspend > > > > On 10/25/2021 12:13 AM, Tejas Upadhyay wrote: > > selftest --r live shows failure in suspend tests when RPM wakelock is > > not acquired during suspend. > > > > This changes addresses below error : > > <4> [154.177535] RPM wakelock ref not held during HW access <4> > > [154.177575] WARNING: CPU: 4 PID: 5772 at > > drivers/gpu/drm/i915/intel_runtime_pm.h:113 > > fwtable_write32+0x240/0x320 [i915] > > <4> [154.177974] Modules linked in: i915(+) vgem drm_shmem_helper fuse > > snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic > > ledtrig_audio mei_hdcp mei_pxp x86_pkg_temp_thermal coretemp > > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_intel_dspcfg > > snd_hda_codec snd_hwdep igc snd_hda_core ttm mei_me ptp snd_pcm > > prime_numbers mei i2c_i801 pps_core i2c_smbus intel_lpss_pci btusb > > btrtl btbcm btintel bluetooth ecdh_generic ecc [last unloaded: i915] > > <4> [154.178143] CPU: 4 PID: 5772 Comm: i915_selftest Tainted: G > > U 5.15.0-rc6-CI-Patchwork_21432+ #1 > > <4> [154.178154] Hardware name: ASUS System Product Name/TUF > GAMING > > Z590-PLUS WIFI, BIOS 0811 04/06/2021 <4> [154.178160] RIP: > > 0010:fwtable_write32+0x240/0x320 [i915] <4> [154.178604] Code: 15 7b > > e1 0f 0b e9 34 fe ff ff 80 3d a9 89 31 > > 00 00 0f 85 31 fe ff ff 48 c7 c7 88 9e 4f a0 c6 05 95 89 31 00 01 e8 > > c0 15 7b e1 <0f> 0b e9 17 fe ff ff 8b 05 0f 83 58 e2 85 c0 0f 85 8d > > 00 00 00 48 > > <4> [154.178614] RSP: 0018:ffffc900016279f0 EFLAGS: 00010286 <4> > > [154.178626] RAX: 0000000000000000 RBX: ffff888204fe0ee0 > > RCX: 0000000000000001 > > <4> [154.178634] RDX: 0000000080000001 RSI: ffffffff823142b5 > > RDI: 00000000ffffffff > > <4> [154.178641] RBP: 00000000000320f0 R08: 0000000000000000 > > R09: c0000000ffffcd5a > > <4> [154.178647] R10: 00000000000f8c90 R11: ffffc90001627808 > > R12: 0000000000000000 > > <4> [154.178654] R13: 0000000040000000 R14: ffffffffa04d12e0 > > R15: 0000000000000000 > > <4> [154.178660] FS: 00007f7390aa4c00(0000) GS:ffff88844f000000(0000) > > knlGS:0000000000000000 > > <4> [154.178669] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> > > [154.178675] CR2: 000055bc40595028 CR3: 0000000204474005 > > CR4: 0000000000770ee0 > > <4> [154.178682] PKRU: 55555554 > > <4> [154.178687] Call Trace: > > <4> [154.178706] intel_pxp_fini_hw+0x23/0x30 [i915] <4> [154.179284] > > intel_pxp_suspend+0x1f/0x30 [i915] <4> [154.179807] > > live_gt_resume+0x5b/0x90 [i915] > > > > Signed-off-by: Tejas Upadhyay > > <tejaskumarx.surendrakumar.upadhyay@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > > b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > > index 524eaf678790..65535f459f9b 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > > @@ -298,10 +298,14 @@ static void wait_for_suspend(struct intel_gt > > *gt) > > > > void intel_gt_suspend_prepare(struct intel_gt *gt) > > { > > + intel_wakeref_t wakeref; > > + > > user_forcewake(gt, true); > > wait_for_suspend(gt); > > > > - intel_pxp_suspend(>->pxp, false); > > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) { > > + intel_pxp_suspend(>->pxp, false); > > + } > > Chris had spotted this as well, but he suggested to go with the same stile we > have for other components where we split the HW access parts in > gt_suspend_late, something like the below (compile-tested only), which IMO > is a better approach. > > Daniele Ok, looks good as well. Let me try and test below approach. - Tejas > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > index b4a8594bc46c..4f68299d1fd9 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > @@ -303,7 +303,7 @@ void intel_gt_suspend_prepare(struct intel_gt *gt) > user_forcewake(gt, true); > wait_for_suspend(gt); > > - intel_pxp_suspend(>->pxp, false); > + intel_pxp_suspend_prepare(>->pxp, false); > } > > static suspend_state_t pm_suspend_target(void) @@ -343,6 +343,8 @@ > void intel_gt_suspend_late(struct intel_gt *gt) > return; > > with_intel_runtime_pm(gt->uncore->rpm, wakeref) { > + intel_pxp_suspend(>->pxp); > + > intel_rps_disable(>->rps); > intel_rc6_disable(>->rc6); > intel_llc_disable(>->llc); > @@ -355,7 +357,7 @@ void intel_gt_suspend_late(struct intel_gt *gt) > > void intel_gt_runtime_suspend(struct intel_gt *gt) > { > - intel_pxp_suspend(>->pxp, true); > + intel_pxp_runtime_suspend(>->pxp); > intel_uc_runtime_suspend(>->uc); > > GT_TRACE(gt, "\n"); > @@ -373,7 +375,7 @@ int intel_gt_runtime_resume(struct intel_gt *gt) > if (ret) > return ret; > > - intel_pxp_resume(>->pxp); > + intel_pxp_runtime_resume(>->pxp); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > index 23fd86de5a24..7ffdc92e5325 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > @@ -8,7 +8,7 @@ > #include "intel_pxp_pm.h" > #include "intel_pxp_session.h" > > -void intel_pxp_suspend(struct intel_pxp *pxp, bool runtime) > +void intel_pxp_suspend_prepare(struct intel_pxp *pxp, bool runtime) > { > if (!intel_pxp_is_enabled(pxp)) > return; > @@ -23,6 +23,12 @@ void intel_pxp_suspend(struct intel_pxp *pxp, bool > runtime) > */ > if (!runtime) > intel_pxp_invalidate(pxp); > +} > + > +void intel_pxp_suspend(struct intel_pxp *pxp) { > + if (!intel_pxp_is_enabled(pxp)) > + return; > > intel_pxp_fini_hw(pxp); > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h > b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h > index c89e97a0c3d0..f1121865dc9d 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h > @@ -9,10 +9,15 @@ > #include "intel_pxp_types.h" > > #ifdef CONFIG_DRM_I915_PXP > -void intel_pxp_suspend(struct intel_pxp *pxp, bool runtime); > +void intel_pxp_suspend_prepare(struct intel_pxp *pxp, bool runtime); > +void intel_pxp_suspend(struct intel_pxp *pxp); > void intel_pxp_resume(struct intel_pxp *pxp); > #else > -static inline void intel_pxp_suspend(struct intel_pxp *pxp, bool runtime) > +static inline void intel_pxp_suspend_prepare(struct intel_pxp *pxp, > bool runtime) > +{ > +} > + > +static inline void intel_pxp_suspend(struct intel_pxp *pxp) > { > } > > @@ -21,4 +26,15 @@ static inline void intel_pxp_resume(struct intel_pxp > *pxp) > } > #endif > > +static inline void intel_pxp_runtime_suspend(struct intel_pxp *pxp) { > + intel_pxp_suspend_prepare(pxp, true); > + intel_pxp_suspend(pxp); > +} > + > +static inline void intel_pxp_runtime_resume(struct intel_pxp *pxp) { > + intel_pxp_resume(pxp); > +} > + > #endif /* __INTEL_PXP_PM_H__ */ > > > } > > > > static suspend_state_t pm_suspend_target(void)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index 524eaf678790..65535f459f9b 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -298,10 +298,14 @@ static void wait_for_suspend(struct intel_gt *gt) void intel_gt_suspend_prepare(struct intel_gt *gt) { + intel_wakeref_t wakeref; + user_forcewake(gt, true); wait_for_suspend(gt); - intel_pxp_suspend(>->pxp, false); + with_intel_runtime_pm(gt->uncore->rpm, wakeref) { + intel_pxp_suspend(>->pxp, false); + } } static suspend_state_t pm_suspend_target(void)
selftest --r live shows failure in suspend tests when RPM wakelock is not acquired during suspend. This changes addresses below error : <4> [154.177535] RPM wakelock ref not held during HW access <4> [154.177575] WARNING: CPU: 4 PID: 5772 at drivers/gpu/drm/i915/intel_runtime_pm.h:113 fwtable_write32+0x240/0x320 [i915] <4> [154.177974] Modules linked in: i915(+) vgem drm_shmem_helper fuse snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio mei_hdcp mei_pxp x86_pkg_temp_thermal coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_intel_dspcfg snd_hda_codec snd_hwdep igc snd_hda_core ttm mei_me ptp snd_pcm prime_numbers mei i2c_i801 pps_core i2c_smbus intel_lpss_pci btusb btrtl btbcm btintel bluetooth ecdh_generic ecc [last unloaded: i915] <4> [154.178143] CPU: 4 PID: 5772 Comm: i915_selftest Tainted: G U 5.15.0-rc6-CI-Patchwork_21432+ #1 <4> [154.178154] Hardware name: ASUS System Product Name/TUF GAMING Z590-PLUS WIFI, BIOS 0811 04/06/2021 <4> [154.178160] RIP: 0010:fwtable_write32+0x240/0x320 [i915] <4> [154.178604] Code: 15 7b e1 0f 0b e9 34 fe ff ff 80 3d a9 89 31 00 00 0f 85 31 fe ff ff 48 c7 c7 88 9e 4f a0 c6 05 95 89 31 00 01 e8 c0 15 7b e1 <0f> 0b e9 17 fe ff ff 8b 05 0f 83 58 e2 85 c0 0f 85 8d 00 00 00 48 <4> [154.178614] RSP: 0018:ffffc900016279f0 EFLAGS: 00010286 <4> [154.178626] RAX: 0000000000000000 RBX: ffff888204fe0ee0 RCX: 0000000000000001 <4> [154.178634] RDX: 0000000080000001 RSI: ffffffff823142b5 RDI: 00000000ffffffff <4> [154.178641] RBP: 00000000000320f0 R08: 0000000000000000 R09: c0000000ffffcd5a <4> [154.178647] R10: 00000000000f8c90 R11: ffffc90001627808 R12: 0000000000000000 <4> [154.178654] R13: 0000000040000000 R14: ffffffffa04d12e0 R15: 0000000000000000 <4> [154.178660] FS: 00007f7390aa4c00(0000) GS:ffff88844f000000(0000) knlGS:0000000000000000 <4> [154.178669] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> [154.178675] CR2: 000055bc40595028 CR3: 0000000204474005 CR4: 0000000000770ee0 <4> [154.178682] PKRU: 55555554 <4> [154.178687] Call Trace: <4> [154.178706] intel_pxp_fini_hw+0x23/0x30 [i915] <4> [154.179284] intel_pxp_suspend+0x1f/0x30 [i915] <4> [154.179807] live_gt_resume+0x5b/0x90 [i915] Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)