drm/msm: Some runtime PM fixes
cover drm/msm: Some runtime PM fixes
Runtime PM was added in 4.14. These patches fix a couple of issues reported by people trying out the new kernel on DB410c.
These should ideally go in one of the 4.14-rcs.
Archit Taneja (2):
drm/msm/dsi: Use correct pm_runtime_put variant during host_init drm/msm/mdp5: Remove extra pm_runtime_put call in
mdp5_crtc_cursor_set()
drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
PATCH 1/2 drm/msm/dsi: Use correct pm_runtime_put variant during host_init
The DSI runtime PM suspend/resume callbacks check whether msm_host->cfg_hnd is non-NULL before trying to enable the bus clocks. This is done to accommodate early calls to these functions that may happen before the bus clocks are even initialized.
Calling pm_runtime_put_autosuspend() in dsi_host_init() can result in racy behaviour since msm_host->cfg_hnd is set very soon after. If the suspend callback happens too late, we end up trying to disable clocks that were never enabled, resulting in a bunch of WARN_ON splats.
Use pm_runtime_put_sync() so that the suspend callback is called immediately.
--- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1
@@ -248,7 +248,7 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(clk_disable_unprepare (ahb_clk); disable_gdsc:regulator_disable (gdsc_reg); -pm_runtime_put_autosuspend (dev); +pm_runtime_put_sync (dev); put_clk:clk_put (ahb_clk); put_gdsc:
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 1/2] drm/msm/dsi: Use correct pm_runtime_put variant during host_init
Quoted 9 lines · Archit Taneja
The DSI runtime PM suspend/resume callbacks check whether
msm_host->cfg_hndis non-NULL before trying to enable the bus clocks. This is done to accommodate early calls to these functions that may happen before the bus clocks are even initialized.Calling
pm_runtime_put_autosuspend()indsi_host_init()can result in racy behaviour sincemsm_host->cfg_hndis set very soon after. If the suspend callback happens too late, we end up trying to disable clocks that were never enabled, resulting in a bunch ofWARN_ONsplats.
Sounds like the correct fix here is to block autosuspend until after everything is set up, including bus clocks. This patch just makes the race harder to hit in practice ...
-Daniel
Quoted 31 lines
Use
pm_runtime_put_sync()so that the suspend callback is called immediately.Reported-by Nicolas Dechesne <[email protected]>Signed-off-by Archit Taneja <[email protected]>--- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -248,7 +248,7 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(clk_disable_unprepare (ahb_clk); disable_gdsc:regulator_disable (gdsc_reg); -pm_runtime_put_autosuspend (dev); +pm_runtime_put_sync (dev); put_clk:clk_put (ahb_clk); put_gdsc:Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 1/2] drm/msm/dsi: Use correct pm_runtime_put variant during host_init
Quoted 6 lines · Daniel Vetter
Quoted 5 lines · Archit Taneja
The DSI runtime PM suspend/resume callbacks check whether
msm_host->cfg_hndis non-NULL before trying to enable the bus clocks. This is done to accommodate early calls to these functions that may happen before the bus clocks are even initialized.
Quoted 8 lines
Quoted 4 lines
Calling
pm_runtime_put_autosuspend()indsi_host_init()can result in racy behaviour sincemsm_host->cfg_hndis set very soon after. If the suspend callback happens too late, we end up trying to disable clocks that were never enabled, resulting in a bunch ofWARN_ONsplats.Sounds like the correct fix here is to block autosuspend until after everything is set up, including bus clocks. This patch just makes the race harder to hit in practice ...
Thanks for the review. pm_runtime_put_sync() ensures that the suspend handler is called in the same context as that of the caller, right? msm_host->cfg_hnd is set to a non-NULL value only when we return from dsi_get_config(). The race would never happen in this case.
This call is a one time thing during DSI probe, we do a pm_runtime_get_sync() just so that we can read the block revision number. Once we have the revision number, we look at an internal table which maintains IP version specific resources, like what bus clocks to get, etc. Having pm_runtime_put_autosuspend() here didn't help much anyway.
Archit
Quoted 34 lines
-Daniel
Quoted 31 lines
Use
pm_runtime_put_sync()so that the suspend callback is called immediately.Reported-by Nicolas Dechesne <[email protected]>Signed-off-by Archit Taneja <[email protected]>--- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -248,7 +248,7 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(clk_disable_unprepare (ahb_clk); disable_gdsc:regulator_disable (gdsc_reg); -pm_runtime_put_autosuspend (dev); +pm_runtime_put_sync (dev); put_clk:clk_put (ahb_clk); put_gdsc:Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 6 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/msm/dsi: Use correct pm_runtime_put variant during host_init
Quoted 23 lines · Archit Taneja
Quoted 6 lines · Daniel Vetter
Quoted 5 lines · Archit Taneja
The DSI runtime PM suspend/resume callbacks check whether
msm_host->cfg_hndis non-NULL before trying to enable the bus clocks. This is done to accommodate early calls to these functions that may happen before the bus clocks are even initialized.Quoted 8 lines
Quoted 4 lines
Calling
pm_runtime_put_autosuspend()indsi_host_init()can result in racy behaviour sincemsm_host->cfg_hndis set very soon after. If the suspend callback happens too late, we end up trying to disable clocks that were never enabled, resulting in a bunch ofWARN_ONsplats.Sounds like the correct fix here is to block autosuspend until after everything is set up, including bus clocks. This patch just makes the race harder to hit in practice ...
Thanks for the review.
pm_runtime_put_sync()ensures that the suspend handler is called in the same context as that of the caller, right?msm_host->cfg_hndis set to a non-NULL value only when we return fromdsi_get_config(). The race would never happen in this case.
Quoted 5 lines
This call is a one time thing during DSI probe, we do a
pm_runtime_get_sync()just so that we can read the block revision number. Once we have the revision number, we look at an internal table which maintains IP version specific resources, like what bus clocks to get, etc. Havingpm_runtime_put_autosuspend()here didn't help much anyway.
Maybe this stuff is different on arm than on pci, but on x86 you have to explicitly enable autosuspend. Before that, the device stays on.
What I mean with properly fixing this, is to delay enabling of autosuspend until you're fully set up. Which then allows you to drop the check for clocks and other stuff.
For i915, what we do is hold an artificial runtime pm reference that we grab first thing in ->probe and drop only once everything is fully set up (including asynchronous workers that load firmware). That way we make sure that our runtime pm code never sees a partially initiliazed driver.
Doing something similar sounds best too, i.e. instead of dropping the runtime pm reference here, only drop it once dsi_get_config has been called. Pronto, no race, no need for special functions. -Daniel
Quoted 42 lines
Archit
Quoted 34 lines
-Daniel
Quoted 31 lines
Use
pm_runtime_put_sync()so that the suspend callback is called immediately.Reported-by Nicolas Dechesne <[email protected]>Signed-off-by Archit Taneja <[email protected]>--- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -248,7 +248,7 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(clk_disable_unprepare (ahb_clk); disable_gdsc:regulator_disable (gdsc_reg); -pm_runtime_put_autosuspend (dev); +pm_runtime_put_sync (dev); put_clk:clk_put (ahb_clk); put_gdsc:Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-develSignature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 1/2] drm/msm/dsi: Use correct pm_runtime_put variant during host_init
Quoted 24 lines · Daniel Vetter
Quoted 23 lines · Archit Taneja
Quoted 6 lines · Daniel Vetter
Quoted 5 lines · Archit Taneja
The DSI runtime PM suspend/resume callbacks check whether
msm_host->cfg_hndis non-NULL before trying to enable the bus clocks. This is done to accommodate early calls to these functions that may happen before the bus clocks are even initialized.Quoted 8 lines
Quoted 4 lines
Calling
pm_runtime_put_autosuspend()indsi_host_init()can result in racy behaviour sincemsm_host->cfg_hndis set very soon after. If the suspend callback happens too late, we end up trying to disable clocks that were never enabled, resulting in a bunch ofWARN_ONsplats.Sounds like the correct fix here is to block autosuspend until after everything is set up, including bus clocks. This patch just makes the race harder to hit in practice ...
Thanks for the review.
pm_runtime_put_sync()ensures that the suspend handler is called in the same context as that of the caller, right?msm_host->cfg_hndis set to a non-NULL value only when we return fromdsi_get_config(). The race would never happen in this case.
Quoted 8 lines
Quoted 5 lines
This call is a one time thing during DSI probe, we do a
pm_runtime_get_sync()just so that we can read the block revision number. Once we have the revision number, we look at an internal table which maintains IP version specific resources, like what bus clocks to get, etc. Havingpm_runtime_put_autosuspend()here didn't help much anyway.Maybe this stuff is different on arm than on pci, but on x86 you have to explicitly enable autosuspend. Before that, the device stays on.
On arm, we cannot assume that clks are enabled ever unless we've enabled them. And unclocked register access is insta-reboot. Although we probably shouldn't be using _autosuspend() on the display side of things. It makes sense for the gpu, where "booting up" the gpu is a heavier process and we might want to keep things alive for a bit longer incase another submit comes in.
BR,
-R
Quoted 53 lines
What I mean with properly fixing this, is to delay enabling of autosuspend until you're fully set up. Which then allows you to drop the check for clocks and other stuff.
For i915, what we do is hold an artificial runtime pm reference that we grab first thing in ->probe and drop only once everything is fully set up (including asynchronous workers that load firmware). That way we make sure that our runtime pm code never sees a partially initiliazed driver.
Doing something similar sounds best too, i.e. instead of dropping the runtime pm reference here, only drop it once
dsi_get_confighas been called. Pronto, no race, no need for special functions. -DanielQuoted 39 lines
Archit
Quoted 34 lines
-Daniel
Quoted 31 lines
Use
pm_runtime_put_sync()so that the suspend callback is called immediately.Reported-by Nicolas Dechesne <[email protected]>Signed-off-by Archit Taneja <[email protected]>--- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/msm/dsi/dsi_host.c +1
-2@@ -248,7 +248,7 @@ static const struct msm_dsi_cfg_handler *dsi_get_config(clk_disable_unprepare (ahb_clk); disable_gdsc:regulator_disable (gdsc_reg); -pm_runtime_put_autosuspend (dev); +pm_runtime_put_sync (dev); put_clk:clk_put (ahb_clk); put_gdsc: --The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 8 lines
Quoted 3 lines
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 2/2 drm/msm/mdp5: Remove extra pm_runtime_put call in mdp5_crtc_cursor_set()
In reply to Archit TanejaWhile converting mdp5_enable/disable() calls to pm_runtime_get/put() API, an extra call to pm_runtime_put_autosuspend() crept in mdp5_crtc_cursor_set(). This results in calling the suspend handler twice, and therefore clk_disables twice, which isn't a nice thing to do.
--- drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 2 -- 1 file changed, 2 deletions(-)
drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +0 -2
@@ -804,8 +804,6 @@ static int mdp5_crtc_cursor_set(struct drm_crtc *crtc,spin_unlock_irqrestore (&mdp5_crtc->cursor.lock, flags); -pm_runtime_put_autosuspend (&pdev->dev); - set_cursor: ret =mdp5_ctl_set_cursor (ctl, pipeline,0 , cursor_enable);if (ret) {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Quoted 10 lines · Archit Taneja
Runtime PM was added in 4.14. These patches fix a couple of issues reported by people trying out the new kernel on DB410c.
These should ideally go in one of the 4.14-rcs.
Archit Taneja (2):
drm/msm/dsi: Use correctpm_runtime_putvariant duringhost_initdrm/msm/mdp5: Remove extrapm_runtime_putcall inmdp5_crtc_cursor_set()
thanks. tested both patches on db410c and it fixes the issues we discussed yesterday. i can run glmark2 now with openbox. as such,
Quoted 12 lines
drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html