Re: [PATCH 12/26] drm: etnaviv: Remove #ifdef guards for PM related functions

[Paul Cercueil <paul@crapouillou.net>]

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,
> 
>