| Message ID | 20200229221649.90813-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/stm: repair runtime power management | expand |
Hello Marek,
Thank for your patch. Pm_runtime_put_sync is also done into function ltdc_crtc_mode_fixup.
To avoid several call of Pm_runtime_put_sync, it could be better to check pm_runtime activity:
+ int ret;
DRM_DEBUG_DRIVER("\n");
+ if (!pm_runtime_active(ddev->dev)) {
+ ret = pm_runtime_get_sync(ddev->dev);
+ if (ret) {
+ DRM_ERROR("Failed to enable crtc, cannot get sync\n");
+ return;
+ }
+ }
+
Best regards
Yannick Fertré
-----Original Message-----
From: Marek Vasut <marex@denx.de>
Sent: samedi 29 février 2020 23:17
To: dri-devel@lists.freedesktop.org
Cc: Marek Vasut <marex@denx.de>; Yannick FERTRE <yannick.fertre@st.com>; Philippe CORNU <philippe.cornu@st.com>; Benjamin Gaignard <benjamin.gaignard@linaro.org>; Vincent ABRIOU <vincent.abriou@st.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre TORGUE <alexandre.torgue@st.com>; linux-stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org
Subject: [PATCH] drm/stm: repair runtime power management
Add missing pm_runtime_get_sync() into ltdc_crtc_atomic_enable() to match pm_runtime_put_sync() in ltdc_crtc_atomic_disable(), otherwise the LTDC might suspend via runtime PM, disable clock, and then fail to resume later on.
The test which triggers it is roughly -- run qt5 application which uses eglfs platform and etnaviv, stop the application, sleep for 15 minutes, run the application again. This leads to a timeout waiting for vsync, because the LTDC has suspended, but did not resume.
Fixes: 35ab6cfbf211 ("drm/stm: support runtime power management")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Yannick Fertré <yannick.fertre@st.com>
Cc: Philippe Cornu <philippe.cornu@st.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
To: dri-devel@lists.freedesktop.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
---
------------[ cut here ]------------
WARNING: CPU: 0 PID: 297 at drivers/gpu/drm/drm_atomic_helper.c:1494 drm_atomic_helper_wait_for_vblanks+0x1dc/0x200
[CRTC:35:crtc-0] vblank wait timed out
Modules linked in:
CPU: 0 PID: 297 Comm: QSGRenderThread Not tainted 5.6.0-rc3-next-20200228-00010-g318bf0fc08ef #2 Hardware name: STM32 (Device Tree Support) [<c010f18c>] (unwind_backtrace) from [<c010afb8>] (show_stack+0x10/0x14) [<c010afb8>] (show_stack) from [<c07b1d3c>] (dump_stack+0xb4/0xd0) [<c07b1d3c>] (dump_stack) from [<c011d8b8>] (__warn+0xd4/0xf0) [<c011d8b8>] (__warn) from [<c011dc4c>] (warn_slowpath_fmt+0x78/0xa8) [<c011dc4c>] (warn_slowpath_fmt) from [<c04a266c>] (drm_atomic_helper_wait_for_vblanks+0x1dc/0x200)
[<c04a266c>] (drm_atomic_helper_wait_for_vblanks) from [<c04a510c>] (drm_atomic_helper_commit_tail+0
x50/0x60)
[<c04a510c>] (drm_atomic_helper_commit_tail) from [<c04a52a8>] (commit_tail+0x12c/0x13c) [<c04a52a8>] (commit_tail) from [<c04a53b4>] (drm_atomic_helper_commit+0xf4/0x100)
[<c04a53b4>] (drm_atomic_helper_commit) from [<c04a2d38>] (drm_atomic_helper_set_config+0x58/0x6c)
[<c04a2d38>] (drm_atomic_helper_set_config) from [<c04b1994>] (drm_mode_setcrtc+0x450/0x550) [<c04b1994>] (drm_mode_setcrtc) from [<c04ad570>] (drm_ioctl_kernel+0x90/0xe8) [<c04ad570>] (drm_ioctl_kernel) from [<c04ad8ac>] (drm_ioctl+0x2e4/0x32c) [<c04ad8ac>] (drm_ioctl) from [<c0246784>] (vfs_ioctl+0x20/0x38) [<c0246784>] (vfs_ioctl) from [<c02470f0>] (ksys_ioctl+0xbc/0x7b0) [<c02470f0>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54) Exception stack(0xee8f3fa8 to 0xee8f3ff0)
3fa0: 00000005 adcbeb18 00000005 c06864a2 adcbeb18 00000001
3fc0: 00000005 adcbeb18 c06864a2 00000036 00000029 00000023 00000023 00000007
3fe0: b113b098 adcbeafc b1125413 b6155cf8 ---[ end trace 2ad5ba954ceb767a ]---
---
drivers/gpu/drm/stm/ltdc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 99bf93e8b36f..301de0498078 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -425,9 +425,12 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc,
struct drm_crtc_state *old_state) {
struct ltdc_device *ldev = crtc_to_ltdc(crtc);
+ struct drm_device *ddev = crtc->dev;
DRM_DEBUG_DRIVER("\n");
+ pm_runtime_get_sync(ddev->dev);
+
/* Sets the background color value */
reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK);
--
2.25.0
On 3/9/20 11:35 AM, Yannick FERTRE wrote: > Hello Marek, Hi, (please stop top-posting) > Thank for your patch. Pm_runtime_put_sync is also done into function ltdc_crtc_mode_fixup. > To avoid several call of Pm_runtime_put_sync, it could be better to check pm_runtime activity: > > + int ret; > > DRM_DEBUG_DRIVER("\n"); > > + if (!pm_runtime_active(ddev->dev)) { > + ret = pm_runtime_get_sync(ddev->dev); > + if (ret) { > + DRM_ERROR("Failed to enable crtc, cannot get sync\n"); > + return; > + } > + } > + Where should this go ? And wouldn't that only hide nastier PM imbalance issues ? > Best regards > > Yannick Fertré > > > -----Original Message----- > From: Marek Vasut <marex@denx.de> > Sent: samedi 29 février 2020 23:17 > To: dri-devel@lists.freedesktop.org > Cc: Marek Vasut <marex@denx.de>; Yannick FERTRE <yannick.fertre@st.com>; Philippe CORNU <philippe.cornu@st.com>; Benjamin Gaignard <benjamin.gaignard@linaro.org>; Vincent ABRIOU <vincent.abriou@st.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre TORGUE <alexandre.torgue@st.com>; linux-stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org > Subject: [PATCH] drm/stm: repair runtime power management > > Add missing pm_runtime_get_sync() into ltdc_crtc_atomic_enable() to match pm_runtime_put_sync() in ltdc_crtc_atomic_disable(), otherwise the LTDC might suspend via runtime PM, disable clock, and then fail to resume later on. > > The test which triggers it is roughly -- run qt5 application which uses eglfs platform and etnaviv, stop the application, sleep for 15 minutes, run the application again. This leads to a timeout waiting for vsync, because the LTDC has suspended, but did not resume. > > Fixes: 35ab6cfbf211 ("drm/stm: support runtime power management") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Yannick Fertré <yannick.fertre@st.com> > Cc: Philippe Cornu <philippe.cornu@st.com> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Cc: Vincent Abriou <vincent.abriou@st.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > To: dri-devel@lists.freedesktop.org > Cc: linux-stm32@st-md-mailman.stormreply.com > Cc: linux-arm-kernel@lists.infradead.org > --- > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 297 at drivers/gpu/drm/drm_atomic_helper.c:1494 drm_atomic_helper_wait_for_vblanks+0x1dc/0x200 > [CRTC:35:crtc-0] vblank wait timed out > Modules linked in: > CPU: 0 PID: 297 Comm: QSGRenderThread Not tainted 5.6.0-rc3-next-20200228-00010-g318bf0fc08ef #2 Hardware name: STM32 (Device Tree Support) [<c010f18c>] (unwind_backtrace) from [<c010afb8>] (show_stack+0x10/0x14) [<c010afb8>] (show_stack) from [<c07b1d3c>] (dump_stack+0xb4/0xd0) [<c07b1d3c>] (dump_stack) from [<c011d8b8>] (__warn+0xd4/0xf0) [<c011d8b8>] (__warn) from [<c011dc4c>] (warn_slowpath_fmt+0x78/0xa8) [<c011dc4c>] (warn_slowpath_fmt) from [<c04a266c>] (drm_atomic_helper_wait_for_vblanks+0x1dc/0x200) > [<c04a266c>] (drm_atomic_helper_wait_for_vblanks) from [<c04a510c>] (drm_atomic_helper_commit_tail+0 > x50/0x60) > [<c04a510c>] (drm_atomic_helper_commit_tail) from [<c04a52a8>] (commit_tail+0x12c/0x13c) [<c04a52a8>] (commit_tail) from [<c04a53b4>] (drm_atomic_helper_commit+0xf4/0x100) > [<c04a53b4>] (drm_atomic_helper_commit) from [<c04a2d38>] (drm_atomic_helper_set_config+0x58/0x6c) > [<c04a2d38>] (drm_atomic_helper_set_config) from [<c04b1994>] (drm_mode_setcrtc+0x450/0x550) [<c04b1994>] (drm_mode_setcrtc) from [<c04ad570>] (drm_ioctl_kernel+0x90/0xe8) [<c04ad570>] (drm_ioctl_kernel) from [<c04ad8ac>] (drm_ioctl+0x2e4/0x32c) [<c04ad8ac>] (drm_ioctl) from [<c0246784>] (vfs_ioctl+0x20/0x38) [<c0246784>] (vfs_ioctl) from [<c02470f0>] (ksys_ioctl+0xbc/0x7b0) [<c02470f0>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54) Exception stack(0xee8f3fa8 to 0xee8f3ff0) > 3fa0: 00000005 adcbeb18 00000005 c06864a2 adcbeb18 00000001 > 3fc0: 00000005 adcbeb18 c06864a2 00000036 00000029 00000023 00000023 00000007 > 3fe0: b113b098 adcbeafc b1125413 b6155cf8 ---[ end trace 2ad5ba954ceb767a ]--- > --- > drivers/gpu/drm/stm/ltdc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 99bf93e8b36f..301de0498078 100644 > --- a/drivers/gpu/drm/stm/ltdc.c > +++ b/drivers/gpu/drm/stm/ltdc.c > @@ -425,9 +425,12 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc, > struct drm_crtc_state *old_state) { > struct ltdc_device *ldev = crtc_to_ltdc(crtc); > + struct drm_device *ddev = crtc->dev; > > DRM_DEBUG_DRIVER("\n"); > > + pm_runtime_get_sync(ddev->dev); > + > /* Sets the background color value */ > reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK); > > -- > 2.25.0 >
On 3/9/20 12:57 PM, Marek Vasut wrote: > On 3/9/20 11:35 AM, Yannick FERTRE wrote: >> Hello Marek, > > Hi, > > (please stop top-posting) > >> Thank for your patch. Pm_runtime_put_sync is also done into function ltdc_crtc_mode_fixup. >> To avoid several call of Pm_runtime_put_sync, it could be better to check pm_runtime activity: >> >> + int ret; >> >> DRM_DEBUG_DRIVER("\n"); >> >> + if (!pm_runtime_active(ddev->dev)) { >> + ret = pm_runtime_get_sync(ddev->dev); >> + if (ret) { >> + DRM_ERROR("Failed to enable crtc, cannot get sync\n"); >> + return; >> + } >> + } >> + > > Where should this go ? And wouldn't that only hide nastier PM imbalance > issues ? Hi Marek, I tested the patch & it generate an error when I try wake up / sleep the board STM32MP1 DK2 with weston application. It need an additional patch drm-stm-ltdc-remove-call-of-pm-runtime-functions. Thanks for the patch. Tested-by: Yannick Fertre <yannick.fertre@st.com> > >> Best regards >> >> Yannick Fertré >> >> >> -----Original Message----- >> From: Marek Vasut <marex@denx.de> >> Sent: samedi 29 février 2020 23:17 >> To: dri-devel@lists.freedesktop.org >> Cc: Marek Vasut <marex@denx.de>; Yannick FERTRE <yannick.fertre@st.com>; Philippe CORNU <philippe.cornu@st.com>; Benjamin Gaignard <benjamin.gaignard@linaro.org>; Vincent ABRIOU <vincent.abriou@st.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre TORGUE <alexandre.torgue@st.com>; linux-stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org >> Subject: [PATCH] drm/stm: repair runtime power management >> >> Add missing pm_runtime_get_sync() into ltdc_crtc_atomic_enable() to match pm_runtime_put_sync() in ltdc_crtc_atomic_disable(), otherwise the LTDC might suspend via runtime PM, disable clock, and then fail to resume later on. >> >> The test which triggers it is roughly -- run qt5 application which uses eglfs platform and etnaviv, stop the application, sleep for 15 minutes, run the application again. This leads to a timeout waiting for vsync, because the LTDC has suspended, but did not resume. >> >> Fixes: 35ab6cfbf211 ("drm/stm: support runtime power management") >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Yannick Fertré <yannick.fertre@st.com> >> Cc: Philippe Cornu <philippe.cornu@st.com> >> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> >> Cc: Vincent Abriou <vincent.abriou@st.com> >> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Cc: Alexandre Torgue <alexandre.torgue@st.com> >> To: dri-devel@lists.freedesktop.org >> Cc: linux-stm32@st-md-mailman.stormreply.com >> Cc: linux-arm-kernel@lists.infradead.org >> --- >> ------------[ cut here ]------------ >> WARNING: CPU: 0 PID: 297 at drivers/gpu/drm/drm_atomic_helper.c:1494 drm_atomic_helper_wait_for_vblanks+0x1dc/0x200 >> [CRTC:35:crtc-0] vblank wait timed out >> Modules linked in: >> CPU: 0 PID: 297 Comm: QSGRenderThread Not tainted 5.6.0-rc3-next-20200228-00010-g318bf0fc08ef #2 Hardware name: STM32 (Device Tree Support) [<c010f18c>] (unwind_backtrace) from [<c010afb8>] (show_stack+0x10/0x14) [<c010afb8>] (show_stack) from [<c07b1d3c>] (dump_stack+0xb4/0xd0) [<c07b1d3c>] (dump_stack) from [<c011d8b8>] (__warn+0xd4/0xf0) [<c011d8b8>] (__warn) from [<c011dc4c>] (warn_slowpath_fmt+0x78/0xa8) [<c011dc4c>] (warn_slowpath_fmt) from [<c04a266c>] (drm_atomic_helper_wait_for_vblanks+0x1dc/0x200) >> [<c04a266c>] (drm_atomic_helper_wait_for_vblanks) from [<c04a510c>] (drm_atomic_helper_commit_tail+0 >> x50/0x60) >> [<c04a510c>] (drm_atomic_helper_commit_tail) from [<c04a52a8>] (commit_tail+0x12c/0x13c) [<c04a52a8>] (commit_tail) from [<c04a53b4>] (drm_atomic_helper_commit+0xf4/0x100) >> [<c04a53b4>] (drm_atomic_helper_commit) from [<c04a2d38>] (drm_atomic_helper_set_config+0x58/0x6c) >> [<c04a2d38>] (drm_atomic_helper_set_config) from [<c04b1994>] (drm_mode_setcrtc+0x450/0x550) [<c04b1994>] (drm_mode_setcrtc) from [<c04ad570>] (drm_ioctl_kernel+0x90/0xe8) [<c04ad570>] (drm_ioctl_kernel) from [<c04ad8ac>] (drm_ioctl+0x2e4/0x32c) [<c04ad8ac>] (drm_ioctl) from [<c0246784>] (vfs_ioctl+0x20/0x38) [<c0246784>] (vfs_ioctl) from [<c02470f0>] (ksys_ioctl+0xbc/0x7b0) [<c02470f0>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54) Exception stack(0xee8f3fa8 to 0xee8f3ff0) >> 3fa0: 00000005 adcbeb18 00000005 c06864a2 adcbeb18 00000001 >> 3fc0: 00000005 adcbeb18 c06864a2 00000036 00000029 00000023 00000023 00000007 >> 3fe0: b113b098 adcbeafc b1125413 b6155cf8 ---[ end trace 2ad5ba954ceb767a ]--- >> --- >> drivers/gpu/drm/stm/ltdc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 99bf93e8b36f..301de0498078 100644 >> --- a/drivers/gpu/drm/stm/ltdc.c >> +++ b/drivers/gpu/drm/stm/ltdc.c >> @@ -425,9 +425,12 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc, >> struct drm_crtc_state *old_state) { >> struct ltdc_device *ldev = crtc_to_ltdc(crtc); >> + struct drm_device *ddev = crtc->dev; >> >> DRM_DEBUG_DRIVER("\n"); >> >> + pm_runtime_get_sync(ddev->dev); >> + >> /* Sets the background color value */ >> reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK); >> >> -- >> 2.25.0 >> > >
On 7/1/20 2:14 PM, Yannick FERTRE wrote: > > > On 3/9/20 12:57 PM, Marek Vasut wrote: >> On 3/9/20 11:35 AM, Yannick FERTRE wrote: >>> Hello Marek, >> >> Hi, >> >> (please stop top-posting) >> >>> Thank for your patch. Pm_runtime_put_sync is also done into function ltdc_crtc_mode_fixup. >>> To avoid several call of Pm_runtime_put_sync, it could be better to check pm_runtime activity: >>> >>> + int ret; >>> >>> DRM_DEBUG_DRIVER("\n"); >>> >>> + if (!pm_runtime_active(ddev->dev)) { >>> + ret = pm_runtime_get_sync(ddev->dev); >>> + if (ret) { >>> + DRM_ERROR("Failed to enable crtc, cannot get sync\n"); >>> + return; >>> + } >>> + } >>> + >> >> Where should this go ? And wouldn't that only hide nastier PM imbalance >> issues ? > Hi Marek, > I tested the patch & it generate an error when I try wake up / sleep > the board STM32MP1 DK2 with weston application. > It need an additional patch > drm-stm-ltdc-remove-call-of-pm-runtime-functions. > > Thanks for the patch. > > Tested-by: Yannick Fertre <yannick.fertre@st.com> > Hi Marek, before merging the 2 patches, I would like to be sure that Yannick's patch does not "break" your use case (Qt I think)? May I ask you please to give it a try? Note: If you think there is no need to do extra checks, simply tell me of course Philippe :-) > >> >>> Best regards >>> >>> Yannick Fertré >>> >>> >>> -----Original Message----- >>> From: Marek Vasut <marex@denx.de> >>> Sent: samedi 29 février 2020 23:17 >>> To: dri-devel@lists.freedesktop.org >>> Cc: Marek Vasut <marex@denx.de>; Yannick FERTRE <yannick.fertre@st.com>; Philippe CORNU <philippe.cornu@st.com>; Benjamin Gaignard <benjamin.gaignard@linaro.org>; Vincent ABRIOU <vincent.abriou@st.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; Alexandre TORGUE <alexandre.torgue@st.com>; linux-stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org >>> Subject: [PATCH] drm/stm: repair runtime power management >>> >>> Add missing pm_runtime_get_sync() into ltdc_crtc_atomic_enable() to match pm_runtime_put_sync() in ltdc_crtc_atomic_disable(), otherwise the LTDC might suspend via runtime PM, disable clock, and then fail to resume later on. >>> >>> The test which triggers it is roughly -- run qt5 application which uses eglfs platform and etnaviv, stop the application, sleep for 15 minutes, run the application again. This leads to a timeout waiting for vsync, because the LTDC has suspended, but did not resume. >>> >>> Fixes: 35ab6cfbf211 ("drm/stm: support runtime power management") >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Yannick Fertré <yannick.fertre@st.com> >>> Cc: Philippe Cornu <philippe.cornu@st.com> >>> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> >>> Cc: Vincent Abriou <vincent.abriou@st.com> >>> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >>> Cc: Alexandre Torgue <alexandre.torgue@st.com> >>> To: dri-devel@lists.freedesktop.org >>> Cc: linux-stm32@st-md-mailman.stormreply.com >>> Cc: linux-arm-kernel@lists.infradead.org >>> --- >>> ------------[ cut here ]------------ >>> WARNING: CPU: 0 PID: 297 at drivers/gpu/drm/drm_atomic_helper.c:1494 drm_atomic_helper_wait_for_vblanks+0x1dc/0x200 >>> [CRTC:35:crtc-0] vblank wait timed out >>> Modules linked in: >>> CPU: 0 PID: 297 Comm: QSGRenderThread Not tainted 5.6.0-rc3-next-20200228-00010-g318bf0fc08ef #2 Hardware name: STM32 (Device Tree Support) [<c010f18c>] (unwind_backtrace) from [<c010afb8>] (show_stack+0x10/0x14) [<c010afb8>] (show_stack) from [<c07b1d3c>] (dump_stack+0xb4/0xd0) [<c07b1d3c>] (dump_stack) from [<c011d8b8>] (__warn+0xd4/0xf0) [<c011d8b8>] (__warn) from [<c011dc4c>] (warn_slowpath_fmt+0x78/0xa8) [<c011dc4c>] (warn_slowpath_fmt) from [<c04a266c>] (drm_atomic_helper_wait_for_vblanks+0x1dc/0x200) >>> [<c04a266c>] (drm_atomic_helper_wait_for_vblanks) from [<c04a510c>] (drm_atomic_helper_commit_tail+0 >>> x50/0x60) >>> [<c04a510c>] (drm_atomic_helper_commit_tail) from [<c04a52a8>] (commit_tail+0x12c/0x13c) [<c04a52a8>] (commit_tail) from [<c04a53b4>] (drm_atomic_helper_commit+0xf4/0x100) >>> [<c04a53b4>] (drm_atomic_helper_commit) from [<c04a2d38>] (drm_atomic_helper_set_config+0x58/0x6c) >>> [<c04a2d38>] (drm_atomic_helper_set_config) from [<c04b1994>] (drm_mode_setcrtc+0x450/0x550) [<c04b1994>] (drm_mode_setcrtc) from [<c04ad570>] (drm_ioctl_kernel+0x90/0xe8) [<c04ad570>] (drm_ioctl_kernel) from [<c04ad8ac>] (drm_ioctl+0x2e4/0x32c) [<c04ad8ac>] (drm_ioctl) from [<c0246784>] (vfs_ioctl+0x20/0x38) [<c0246784>] (vfs_ioctl) from [<c02470f0>] (ksys_ioctl+0xbc/0x7b0) [<c02470f0>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54) Exception stack(0xee8f3fa8 to 0xee8f3ff0) >>> 3fa0: 00000005 adcbeb18 00000005 c06864a2 adcbeb18 00000001 >>> 3fc0: 00000005 adcbeb18 c06864a2 00000036 00000029 00000023 00000023 00000007 >>> 3fe0: b113b098 adcbeafc b1125413 b6155cf8 ---[ end trace 2ad5ba954ceb767a ]--- >>> --- >>> drivers/gpu/drm/stm/ltdc.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 99bf93e8b36f..301de0498078 100644 >>> --- a/drivers/gpu/drm/stm/ltdc.c >>> +++ b/drivers/gpu/drm/stm/ltdc.c >>> @@ -425,9 +425,12 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc, >>> struct drm_crtc_state *old_state) { >>> struct ltdc_device *ldev = crtc_to_ltdc(crtc); >>> + struct drm_device *ddev = crtc->dev; >>> >>> DRM_DEBUG_DRIVER("\n"); >>> >>> + pm_runtime_get_sync(ddev->dev); >>> + >>> /* Sets the background color value */ >>> reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK); >>> >>> -- >>> 2.25.0 >>> >>
On 7/2/20 12:07 PM, Philippe CORNU wrote: Hi, [...] >>>> Thank for your patch. Pm_runtime_put_sync is also done into function ltdc_crtc_mode_fixup. >>>> To avoid several call of Pm_runtime_put_sync, it could be better to check pm_runtime activity: >>>> >>>> + int ret; >>>> >>>> DRM_DEBUG_DRIVER("\n"); >>>> >>>> + if (!pm_runtime_active(ddev->dev)) { >>>> + ret = pm_runtime_get_sync(ddev->dev); >>>> + if (ret) { >>>> + DRM_ERROR("Failed to enable crtc, cannot get sync\n"); >>>> + return; >>>> + } >>>> + } >>>> + >>> >>> Where should this go ? And wouldn't that only hide nastier PM imbalance >>> issues ? >> Hi Marek, >> I tested the patch & it generate an error when I try wake up / sleep >> the board STM32MP1 DK2 with weston application. >> It need an additional patch >> drm-stm-ltdc-remove-call-of-pm-runtime-functions. >> >> Thanks for the patch. >> >> Tested-by: Yannick Fertre <yannick.fertre@st.com> >> > > Hi Marek, > before merging the 2 patches, I would like to be sure that Yannick's > patch does not "break" your use case (Qt I think)? > May I ask you please to give it a try? > Note: If you think there is no need to do extra checks, simply tell me > of course It's fine, thanks !
On 2/29/20 11:16 PM, Marek Vasut wrote: > Add missing pm_runtime_get_sync() into ltdc_crtc_atomic_enable() to > match pm_runtime_put_sync() in ltdc_crtc_atomic_disable(), otherwise > the LTDC might suspend via runtime PM, disable clock, and then fail > to resume later on. > > The test which triggers it is roughly -- run qt5 application which > uses eglfs platform and etnaviv, stop the application, sleep for 15 > minutes, run the application again. This leads to a timeout waiting > for vsync, because the LTDC has suspended, but did not resume. > > Fixes: 35ab6cfbf211 ("drm/stm: support runtime power management") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Yannick Fertré <yannick.fertre@st.com> > Cc: Philippe Cornu <philippe.cornu@st.com> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > Cc: Vincent Abriou <vincent.abriou@st.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Alexandre Torgue <alexandre.torgue@st.com> > To: dri-devel@lists.freedesktop.org > Cc: linux-stm32@st-md-mailman.stormreply.com > Cc: linux-arm-kernel@lists.infradead.org > --- > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 297 at drivers/gpu/drm/drm_atomic_helper.c:1494 drm_atomic_helper_wait_for_vblanks+0x1dc/0x200 > [CRTC:35:crtc-0] vblank wait timed out > Modules linked in: > CPU: 0 PID: 297 Comm: QSGRenderThread Not tainted 5.6.0-rc3-next-20200228-00010-g318bf0fc08ef #2 > Hardware name: STM32 (Device Tree Support) > [<c010f18c>] (unwind_backtrace) from [<c010afb8>] (show_stack+0x10/0x14) > [<c010afb8>] (show_stack) from [<c07b1d3c>] (dump_stack+0xb4/0xd0) > [<c07b1d3c>] (dump_stack) from [<c011d8b8>] (__warn+0xd4/0xf0) > [<c011d8b8>] (__warn) from [<c011dc4c>] (warn_slowpath_fmt+0x78/0xa8) > [<c011dc4c>] (warn_slowpath_fmt) from [<c04a266c>] (drm_atomic_helper_wait_for_vblanks+0x1dc/0x200) > [<c04a266c>] (drm_atomic_helper_wait_for_vblanks) from [<c04a510c>] (drm_atomic_helper_commit_tail+0 > x50/0x60) > [<c04a510c>] (drm_atomic_helper_commit_tail) from [<c04a52a8>] (commit_tail+0x12c/0x13c) > [<c04a52a8>] (commit_tail) from [<c04a53b4>] (drm_atomic_helper_commit+0xf4/0x100) > [<c04a53b4>] (drm_atomic_helper_commit) from [<c04a2d38>] (drm_atomic_helper_set_config+0x58/0x6c) > [<c04a2d38>] (drm_atomic_helper_set_config) from [<c04b1994>] (drm_mode_setcrtc+0x450/0x550) > [<c04b1994>] (drm_mode_setcrtc) from [<c04ad570>] (drm_ioctl_kernel+0x90/0xe8) > [<c04ad570>] (drm_ioctl_kernel) from [<c04ad8ac>] (drm_ioctl+0x2e4/0x32c) > [<c04ad8ac>] (drm_ioctl) from [<c0246784>] (vfs_ioctl+0x20/0x38) > [<c0246784>] (vfs_ioctl) from [<c02470f0>] (ksys_ioctl+0xbc/0x7b0) > [<c02470f0>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54) > Exception stack(0xee8f3fa8 to 0xee8f3ff0) > 3fa0: 00000005 adcbeb18 00000005 c06864a2 adcbeb18 00000001 > 3fc0: 00000005 adcbeb18 c06864a2 00000036 00000029 00000023 00000023 00000007 > 3fe0: b113b098 adcbeafc b1125413 b6155cf8 > ---[ end trace 2ad5ba954ceb767a ]--- > --- > drivers/gpu/drm/stm/ltdc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c > index 99bf93e8b36f..301de0498078 100644 > --- a/drivers/gpu/drm/stm/ltdc.c > +++ b/drivers/gpu/drm/stm/ltdc.c > @@ -425,9 +425,12 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc, > struct drm_crtc_state *old_state) > { > struct ltdc_device *ldev = crtc_to_ltdc(crtc); > + struct drm_device *ddev = crtc->dev; > > DRM_DEBUG_DRIVER("\n"); > > + pm_runtime_get_sync(ddev->dev); > + > /* Sets the background color value */ > reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK); > > Hi Marek, Many thanks for your patch, Acked-by: Philippe Cornu <philippe.cornu@st.com> Tested-by: Yannick Fertre <yannick.fertre@st.com> Hi Benjamin, Could you please apply "drm/stm: ltdc: remove call of pm-runtime functions" **before** this one. Thank you. Philippe :-)
Le jeu. 2 juil. 2020 à 14:31, Philippe CORNU <philippe.cornu@st.com> a écrit : > > > > On 2/29/20 11:16 PM, Marek Vasut wrote: > > Add missing pm_runtime_get_sync() into ltdc_crtc_atomic_enable() to > > match pm_runtime_put_sync() in ltdc_crtc_atomic_disable(), otherwise > > the LTDC might suspend via runtime PM, disable clock, and then fail > > to resume later on. > > > > The test which triggers it is roughly -- run qt5 application which > > uses eglfs platform and etnaviv, stop the application, sleep for 15 > > minutes, run the application again. This leads to a timeout waiting > > for vsync, because the LTDC has suspended, but did not resume. > > > > Fixes: 35ab6cfbf211 ("drm/stm: support runtime power management") > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Yannick Fertré <yannick.fertre@st.com> > > Cc: Philippe Cornu <philippe.cornu@st.com> > > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> > > Cc: Vincent Abriou <vincent.abriou@st.com> > > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > > Cc: Alexandre Torgue <alexandre.torgue@st.com> > > To: dri-devel@lists.freedesktop.org > > Cc: linux-stm32@st-md-mailman.stormreply.com > > Cc: linux-arm-kernel@lists.infradead.org > > --- > > ------------[ cut here ]------------ > > WARNING: CPU: 0 PID: 297 at drivers/gpu/drm/drm_atomic_helper.c:1494 drm_atomic_helper_wait_for_vblanks+0x1dc/0x200 > > [CRTC:35:crtc-0] vblank wait timed out > > Modules linked in: > > CPU: 0 PID: 297 Comm: QSGRenderThread Not tainted 5.6.0-rc3-next-20200228-00010-g318bf0fc08ef #2 > > Hardware name: STM32 (Device Tree Support) > > [<c010f18c>] (unwind_backtrace) from [<c010afb8>] (show_stack+0x10/0x14) > > [<c010afb8>] (show_stack) from [<c07b1d3c>] (dump_stack+0xb4/0xd0) > > [<c07b1d3c>] (dump_stack) from [<c011d8b8>] (__warn+0xd4/0xf0) > > [<c011d8b8>] (__warn) from [<c011dc4c>] (warn_slowpath_fmt+0x78/0xa8) > > [<c011dc4c>] (warn_slowpath_fmt) from [<c04a266c>] (drm_atomic_helper_wait_for_vblanks+0x1dc/0x200) > > [<c04a266c>] (drm_atomic_helper_wait_for_vblanks) from [<c04a510c>] (drm_atomic_helper_commit_tail+0 > > x50/0x60) > > [<c04a510c>] (drm_atomic_helper_commit_tail) from [<c04a52a8>] (commit_tail+0x12c/0x13c) > > [<c04a52a8>] (commit_tail) from [<c04a53b4>] (drm_atomic_helper_commit+0xf4/0x100) > > [<c04a53b4>] (drm_atomic_helper_commit) from [<c04a2d38>] (drm_atomic_helper_set_config+0x58/0x6c) > > [<c04a2d38>] (drm_atomic_helper_set_config) from [<c04b1994>] (drm_mode_setcrtc+0x450/0x550) > > [<c04b1994>] (drm_mode_setcrtc) from [<c04ad570>] (drm_ioctl_kernel+0x90/0xe8) > > [<c04ad570>] (drm_ioctl_kernel) from [<c04ad8ac>] (drm_ioctl+0x2e4/0x32c) > > [<c04ad8ac>] (drm_ioctl) from [<c0246784>] (vfs_ioctl+0x20/0x38) > > [<c0246784>] (vfs_ioctl) from [<c02470f0>] (ksys_ioctl+0xbc/0x7b0) > > [<c02470f0>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54) > > Exception stack(0xee8f3fa8 to 0xee8f3ff0) > > 3fa0: 00000005 adcbeb18 00000005 c06864a2 adcbeb18 00000001 > > 3fc0: 00000005 adcbeb18 c06864a2 00000036 00000029 00000023 00000023 00000007 > > 3fe0: b113b098 adcbeafc b1125413 b6155cf8 > > ---[ end trace 2ad5ba954ceb767a ]--- > > --- > > drivers/gpu/drm/stm/ltdc.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c > > index 99bf93e8b36f..301de0498078 100644 > > --- a/drivers/gpu/drm/stm/ltdc.c > > +++ b/drivers/gpu/drm/stm/ltdc.c > > @@ -425,9 +425,12 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc, > > struct drm_crtc_state *old_state) > > { > > struct ltdc_device *ldev = crtc_to_ltdc(crtc); > > + struct drm_device *ddev = crtc->dev; > > > > DRM_DEBUG_DRIVER("\n"); > > > > + pm_runtime_get_sync(ddev->dev); > > + > > /* Sets the background color value */ > > reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK); > > > > > Hi Marek, > Many thanks for your patch, > Acked-by: Philippe Cornu <philippe.cornu@st.com> > Tested-by: Yannick Fertre <yannick.fertre@st.com> > > > Hi Benjamin, > Could you please apply "drm/stm: ltdc: remove call of pm-runtime > functions" **before** this one. Thank you. Applied on drm-misc-next. Thanks, Benjamin > > Philippe :-)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c index 99bf93e8b36f..301de0498078 100644 --- a/drivers/gpu/drm/stm/ltdc.c +++ b/drivers/gpu/drm/stm/ltdc.c @@ -425,9 +425,12 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state) { struct ltdc_device *ldev = crtc_to_ltdc(crtc); + struct drm_device *ddev = crtc->dev; DRM_DEBUG_DRIVER("\n"); + pm_runtime_get_sync(ddev->dev); + /* Sets the background color value */ reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK);
Add missing pm_runtime_get_sync() into ltdc_crtc_atomic_enable() to match pm_runtime_put_sync() in ltdc_crtc_atomic_disable(), otherwise the LTDC might suspend via runtime PM, disable clock, and then fail to resume later on. The test which triggers it is roughly -- run qt5 application which uses eglfs platform and etnaviv, stop the application, sleep for 15 minutes, run the application again. This leads to a timeout waiting for vsync, because the LTDC has suspended, but did not resume. Fixes: 35ab6cfbf211 ("drm/stm: support runtime power management") Signed-off-by: Marek Vasut <marex@denx.de> Cc: Yannick Fertré <yannick.fertre@st.com> Cc: Philippe Cornu <philippe.cornu@st.com> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org> Cc: Vincent Abriou <vincent.abriou@st.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Alexandre Torgue <alexandre.torgue@st.com> To: dri-devel@lists.freedesktop.org Cc: linux-stm32@st-md-mailman.stormreply.com Cc: linux-arm-kernel@lists.infradead.org --- ------------[ cut here ]------------ WARNING: CPU: 0 PID: 297 at drivers/gpu/drm/drm_atomic_helper.c:1494 drm_atomic_helper_wait_for_vblanks+0x1dc/0x200 [CRTC:35:crtc-0] vblank wait timed out Modules linked in: CPU: 0 PID: 297 Comm: QSGRenderThread Not tainted 5.6.0-rc3-next-20200228-00010-g318bf0fc08ef #2 Hardware name: STM32 (Device Tree Support) [<c010f18c>] (unwind_backtrace) from [<c010afb8>] (show_stack+0x10/0x14) [<c010afb8>] (show_stack) from [<c07b1d3c>] (dump_stack+0xb4/0xd0) [<c07b1d3c>] (dump_stack) from [<c011d8b8>] (__warn+0xd4/0xf0) [<c011d8b8>] (__warn) from [<c011dc4c>] (warn_slowpath_fmt+0x78/0xa8) [<c011dc4c>] (warn_slowpath_fmt) from [<c04a266c>] (drm_atomic_helper_wait_for_vblanks+0x1dc/0x200) [<c04a266c>] (drm_atomic_helper_wait_for_vblanks) from [<c04a510c>] (drm_atomic_helper_commit_tail+0 x50/0x60) [<c04a510c>] (drm_atomic_helper_commit_tail) from [<c04a52a8>] (commit_tail+0x12c/0x13c) [<c04a52a8>] (commit_tail) from [<c04a53b4>] (drm_atomic_helper_commit+0xf4/0x100) [<c04a53b4>] (drm_atomic_helper_commit) from [<c04a2d38>] (drm_atomic_helper_set_config+0x58/0x6c) [<c04a2d38>] (drm_atomic_helper_set_config) from [<c04b1994>] (drm_mode_setcrtc+0x450/0x550) [<c04b1994>] (drm_mode_setcrtc) from [<c04ad570>] (drm_ioctl_kernel+0x90/0xe8) [<c04ad570>] (drm_ioctl_kernel) from [<c04ad8ac>] (drm_ioctl+0x2e4/0x32c) [<c04ad8ac>] (drm_ioctl) from [<c0246784>] (vfs_ioctl+0x20/0x38) [<c0246784>] (vfs_ioctl) from [<c02470f0>] (ksys_ioctl+0xbc/0x7b0) [<c02470f0>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54) Exception stack(0xee8f3fa8 to 0xee8f3ff0) 3fa0: 00000005 adcbeb18 00000005 c06864a2 adcbeb18 00000001 3fc0: 00000005 adcbeb18 c06864a2 00000036 00000029 00000023 00000023 00000007 3fe0: b113b098 adcbeafc b1125413 b6155cf8 ---[ end trace 2ad5ba954ceb767a ]--- --- drivers/gpu/drm/stm/ltdc.c | 3 +++ 1 file changed, 3 insertions(+)