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

[Paul Cercueil <paul@xxxxxxxxxxxxxxx>]

Hi Lucas,

Le lun. 7 nov. 2022 à 19:07:32 +0100, Lucas Stach 
<l.stach@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >  ---
> >  Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> >  Cc: Russell King <linux+etnaviv@xxxxxxxxxxxxxxx>
> >  Cc: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
> >  Cc: etnaviv@xxxxxxxxxxxxxxxxxxxxx
> >  ---
> >   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,