Messages in this thread | | | Date | Mon, 07 Nov 2022 21:03:22 +0000 | From | Paul Cercueil <> | Subject | Re: [PATCH 12/26] drm: etnaviv: Remove #ifdef guards for PM related functions |
| |
Hi Lucas,
Le lun. 7 nov. 2022 à 19:07:32 +0100, Lucas Stach <l.stach@pengutronix.de> a écrit : > Am Montag, dem 07.11.2022 um 17:52 +0000 schrieb Paul Cercueil: >> Use the RUNTIME_PM_OPS() and pm_ptr() macros to handle the >> .runtime_suspend/.runtime_resume callbacks. >> >> These macros allow the suspend and resume functions to be >> automatically >> dropped by the compiler when CONFIG_PM is disabled, without having >> to use #ifdef guards. >> >> This has the advantage of always compiling these functions in, >> independently of any Kconfig option. Thanks to that, bugs and other >> regressions are subsequently easier to catch. >> >> Some #ifdef CONFIG_PM guards were protecting simple statements, and >> were >> also converted to "if (IS_ENABLED(CONFIG_PM))". >> > Reasoning and the change itself look good.
That's an ack? :)
>> Note that this driver should probably use the >> DEFINE_RUNTIME_DEV_PM_OPS() macro instead, which will provide >> .suspend/.resume callbacks, pointing to pm_runtime_force_suspend() >> and >> pm_runtime_force_resume() respectively; unless those callbacks >> really >> aren't needed. > > This however isn't true, specifically this driver can _not_ use > pm_runtime_force_suspend, as the GPU can't be forced into suspend by > calling the rpm callback. A real suspend implementation would first > need to make sure the GPU finished working on the current queued jobs, > only then it would be able to power it down via the rpm suspend > callback.
Understood. I'll remove this paragraph if I have to V2.
Cheers, -Paul
>> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Russell King <linux+etnaviv@armlinux.org.uk> >> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> >> Cc: etnaviv@lists.freedesktop.org >> --- >> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 30 >> +++++++++++---------------- >> 1 file changed, 12 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> index 37018bc55810..e9a5444ec1c7 100644 >> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c >> @@ -1605,7 +1605,6 @@ static int etnaviv_gpu_hw_suspend(struct >> etnaviv_gpu *gpu) >> return etnaviv_gpu_clk_disable(gpu); >> } >> >> -#ifdef CONFIG_PM >> static int etnaviv_gpu_hw_resume(struct etnaviv_gpu *gpu) >> { >> int ret; >> @@ -1621,7 +1620,6 @@ static int etnaviv_gpu_hw_resume(struct >> etnaviv_gpu *gpu) >> >> return 0; >> } >> -#endif >> >> static int >> etnaviv_gpu_cooling_get_max_state(struct thermal_cooling_device >> *cdev, >> @@ -1689,11 +1687,10 @@ static int etnaviv_gpu_bind(struct device >> *dev, struct device *master, >> if (ret) >> goto out_workqueue; >> >> -#ifdef CONFIG_PM >> - ret = pm_runtime_get_sync(gpu->dev); >> -#else >> - ret = etnaviv_gpu_clk_enable(gpu); >> -#endif >> + if (IS_ENABLED(CONFIG_PM)) >> + ret = pm_runtime_get_sync(gpu->dev); >> + else >> + ret = etnaviv_gpu_clk_enable(gpu); >> if (ret < 0) >> goto out_sched; >> >> @@ -1737,12 +1734,12 @@ static void etnaviv_gpu_unbind(struct >> device *dev, struct device *master, >> >> etnaviv_sched_fini(gpu); >> >> -#ifdef CONFIG_PM >> - pm_runtime_get_sync(gpu->dev); >> - pm_runtime_put_sync_suspend(gpu->dev); >> -#else >> - etnaviv_gpu_hw_suspend(gpu); >> -#endif >> + if (IS_ENABLED(CONFIG_PM)) { >> + pm_runtime_get_sync(gpu->dev); >> + pm_runtime_put_sync_suspend(gpu->dev); >> + } else { >> + etnaviv_gpu_hw_suspend(gpu); >> + } >> >> if (gpu->mmu_context) >> etnaviv_iommu_context_put(gpu->mmu_context); >> @@ -1856,7 +1853,6 @@ static int etnaviv_gpu_platform_remove(struct >> platform_device *pdev) >> return 0; >> } >> >> -#ifdef CONFIG_PM >> static int etnaviv_gpu_rpm_suspend(struct device *dev) >> { >> struct etnaviv_gpu *gpu = dev_get_drvdata(dev); >> @@ -1899,18 +1895,16 @@ static int etnaviv_gpu_rpm_resume(struct >> device *dev) >> >> return 0; >> } >> -#endif >> >> static const struct dev_pm_ops etnaviv_gpu_pm_ops = { >> - SET_RUNTIME_PM_OPS(etnaviv_gpu_rpm_suspend, >> etnaviv_gpu_rpm_resume, >> - NULL) >> + RUNTIME_PM_OPS(etnaviv_gpu_rpm_suspend, etnaviv_gpu_rpm_resume, >> NULL) >> }; >> >> struct platform_driver etnaviv_gpu_driver = { >> .driver = { >> .name = "etnaviv-gpu", >> .owner = THIS_MODULE, >> - .pm = &etnaviv_gpu_pm_ops, >> + .pm = pm_ptr(&etnaviv_gpu_pm_ops), >> .of_match_table = etnaviv_gpu_match, >> }, >> .probe = etnaviv_gpu_platform_probe, > >
| |