[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <M5XZKR.3DW6QGFX6KVP1@crapouillou.net>
Date: Mon, 07 Nov 2022 21:03:22 +0000
From: Paul Cercueil <paul@...pouillou.net>
To: Lucas Stach <l.stach@...gutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Russell King <linux+etnaviv@...linux.org.uk>,
Christian Gmeiner <christian.gmeiner@...il.com>,
etnaviv@...ts.freedesktop.org
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@...gutronix.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@...pouillou.net>
>> ---
>> Cc: Lucas Stach <l.stach@...gutronix.de>
>> Cc: Russell King <linux+etnaviv@...linux.org.uk>
>> Cc: Christian Gmeiner <christian.gmeiner@...il.com>
>> Cc: etnaviv@...ts.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,
>
>
Powered by blists - more mailing lists