[PATCH] drm/panthor: Fix the CONFIG_PM=n case

[PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Boris Brezillon]

Putting a hard dependency on CONFIG_PM is not possible because of a
circular dependency issue, and it's actually not desirable either. In
order to support this use case, we forcibly resume at init time, and
suspend at unplug time.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Tested by faking CONFIG_PM=n in the driver (basically commenting
all pm_runtime calls, and making the panthor_device_suspend/resume()
calls unconditional in the panthor_device_unplug/init() path) since
CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
can't be 100% sure this will work correctly on a platform that has
CONFIG_PM=n.
---
 drivers/gpu/drm/panthor/panthor_device.c | 13 +++++++++++--
 drivers/gpu/drm/panthor/panthor_drv.c    |  4 +++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 69deb8e17778..ba7aedbb4931 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -87,6 +87,10 @@ void panthor_device_unplug(struct panthor_device *ptdev)
 	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
 	pm_runtime_put_sync_suspend(ptdev->base.dev);
 
+	/* If PM is disabled, we need to call the suspend handler manually. */
+	if (!IS_ENABLED(CONFIG_PM))
+		panthor_device_suspend(ptdev->base.dev);
+
 	/* Report the unplug operation as done to unblock concurrent
 	 * panthor_device_unplug() callers.
 	 */
@@ -218,6 +222,13 @@ int panthor_device_init(struct panthor_device *ptdev)
 	if (ret)
 		return ret;
 
+	/* If PM is disabled, we need to call panthor_device_resume() manually. */
+	if (!IS_ENABLED(CONFIG_PM)) {
+		ret = panthor_device_resume(ptdev->base.dev);
+		if (ret)
+			return ret;
+	}
+
 	ret = panthor_gpu_init(ptdev);
 	if (ret)
 		goto err_rpm_put;
@@ -402,7 +413,6 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
 	return 0;
 }
 
-#ifdef CONFIG_PM
 int panthor_device_resume(struct device *dev)
 {
 	struct panthor_device *ptdev = dev_get_drvdata(dev);
@@ -547,4 +557,3 @@ int panthor_device_suspend(struct device *dev)
 	mutex_unlock(&ptdev->pm.mmio_lock);
 	return ret;
 }
-#endif
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index ff484506229f..2ea6a9f436db 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -1407,17 +1407,19 @@ static const struct of_device_id dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, dt_match);
 
+#ifdef CONFIG_PM
 static DEFINE_RUNTIME_DEV_PM_OPS(panthor_pm_ops,
 				 panthor_device_suspend,
 				 panthor_device_resume,
 				 NULL);
+#endif
 
 static struct platform_driver panthor_driver = {
 	.probe = panthor_probe,
 	.remove_new = panthor_remove,
 	.driver = {
 		.name = "panthor",
-		.pm = &panthor_pm_ops,
+		.pm = pm_ptr(&panthor_pm_ops),
 		.of_match_table = dt_match,
 	},
 };
-- 
2.43.0

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Liviu Dudau]

On Mon, Mar 18, 2024 at 09:58:55AM +0100, Boris Brezillon wrote:
> Putting a hard dependency on CONFIG_PM is not possible because of a
> circular dependency issue, and it's actually not desirable either. In
> order to support this use case, we forcibly resume at init time, and
> suspend at unplug time.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Tested by faking CONFIG_PM=n in the driver (basically commenting
> all pm_runtime calls, and making the panthor_device_suspend/resume()
> calls unconditional in the panthor_device_unplug/init() path) since
> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
> can't be 100% sure this will work correctly on a platform that has
> CONFIG_PM=n.

Yes, there are no realistic platforms to test where you can enable panthor
but disable runtime power management. All our emulator configuration are
based on Juno (ARCH_VEXPRESS) which select CONFIG_PM.

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_device.c | 13 +++++++++++--
>  drivers/gpu/drm/panthor/panthor_drv.c    |  4 +++-
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 69deb8e17778..ba7aedbb4931 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -87,6 +87,10 @@ void panthor_device_unplug(struct panthor_device *ptdev)
>  	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
>  	pm_runtime_put_sync_suspend(ptdev->base.dev);
>  
> +	/* If PM is disabled, we need to call the suspend handler manually. */
> +	if (!IS_ENABLED(CONFIG_PM))
> +		panthor_device_suspend(ptdev->base.dev);
> +
>  	/* Report the unplug operation as done to unblock concurrent
>  	 * panthor_device_unplug() callers.
>  	 */
> @@ -218,6 +222,13 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	if (ret)
>  		return ret;
>  
> +	/* If PM is disabled, we need to call panthor_device_resume() manually. */
> +	if (!IS_ENABLED(CONFIG_PM)) {
> +		ret = panthor_device_resume(ptdev->base.dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = panthor_gpu_init(ptdev);
>  	if (ret)
>  		goto err_rpm_put;
> @@ -402,7 +413,6 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM
>  int panthor_device_resume(struct device *dev)
>  {
>  	struct panthor_device *ptdev = dev_get_drvdata(dev);
> @@ -547,4 +557,3 @@ int panthor_device_suspend(struct device *dev)
>  	mutex_unlock(&ptdev->pm.mmio_lock);
>  	return ret;
>  }
> -#endif
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index ff484506229f..2ea6a9f436db 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1407,17 +1407,19 @@ static const struct of_device_id dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dt_match);
>  
> +#ifdef CONFIG_PM
>  static DEFINE_RUNTIME_DEV_PM_OPS(panthor_pm_ops,
>  				 panthor_device_suspend,
>  				 panthor_device_resume,
>  				 NULL);
> +#endif
>  
>  static struct platform_driver panthor_driver = {
>  	.probe = panthor_probe,
>  	.remove_new = panthor_remove,
>  	.driver = {
>  		.name = "panthor",
> -		.pm = &panthor_pm_ops,
> +		.pm = pm_ptr(&panthor_pm_ops),
>  		.of_match_table = dt_match,
>  	},
>  };
> -- 
> 2.43.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Steven Price]

On 18/03/2024 08:58, Boris Brezillon wrote:
> Putting a hard dependency on CONFIG_PM is not possible because of a
> circular dependency issue, and it's actually not desirable either. In
> order to support this use case, we forcibly resume at init time, and
> suspend at unplug time.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
> Tested by faking CONFIG_PM=n in the driver (basically commenting
> all pm_runtime calls, and making the panthor_device_suspend/resume()
> calls unconditional in the panthor_device_unplug/init() path) since
> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
> can't be 100% sure this will work correctly on a platform that has
> CONFIG_PM=n.

The same - I can't test this properly :(

Note that the other option (which AFAICT doesn't cause any problems) is
to "select PM" rather than depend on it - AIUI the 'select' dependency
is considered in the opposite direction by kconfig so won't cause the
dependency loop. Of course if there is actually anyone who has a
platform which can be built !CONFIG_PM then that won't help. But the
inability of anyone to actually properly test this configuration does
worry me a little.

Steve

> ---
>  drivers/gpu/drm/panthor/panthor_device.c | 13 +++++++++++--
>  drivers/gpu/drm/panthor/panthor_drv.c    |  4 +++-
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 69deb8e17778..ba7aedbb4931 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -87,6 +87,10 @@ void panthor_device_unplug(struct panthor_device *ptdev)
>  	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
>  	pm_runtime_put_sync_suspend(ptdev->base.dev);
>  
> +	/* If PM is disabled, we need to call the suspend handler manually. */
> +	if (!IS_ENABLED(CONFIG_PM))
> +		panthor_device_suspend(ptdev->base.dev);
> +
>  	/* Report the unplug operation as done to unblock concurrent
>  	 * panthor_device_unplug() callers.
>  	 */
> @@ -218,6 +222,13 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	if (ret)
>  		return ret;
>  
> +	/* If PM is disabled, we need to call panthor_device_resume() manually. */
> +	if (!IS_ENABLED(CONFIG_PM)) {
> +		ret = panthor_device_resume(ptdev->base.dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = panthor_gpu_init(ptdev);
>  	if (ret)
>  		goto err_rpm_put;
> @@ -402,7 +413,6 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM
>  int panthor_device_resume(struct device *dev)
>  {
>  	struct panthor_device *ptdev = dev_get_drvdata(dev);
> @@ -547,4 +557,3 @@ int panthor_device_suspend(struct device *dev)
>  	mutex_unlock(&ptdev->pm.mmio_lock);
>  	return ret;
>  }
> -#endif
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index ff484506229f..2ea6a9f436db 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1407,17 +1407,19 @@ static const struct of_device_id dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dt_match);
>  
> +#ifdef CONFIG_PM
>  static DEFINE_RUNTIME_DEV_PM_OPS(panthor_pm_ops,
>  				 panthor_device_suspend,
>  				 panthor_device_resume,
>  				 NULL);
> +#endif
>  
>  static struct platform_driver panthor_driver = {
>  	.probe = panthor_probe,
>  	.remove_new = panthor_remove,
>  	.driver = {
>  		.name = "panthor",
> -		.pm = &panthor_pm_ops,
> +		.pm = pm_ptr(&panthor_pm_ops),
>  		.of_match_table = dt_match,
>  	},
>  };

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Boris Brezillon]

On Mon, 18 Mar 2024 11:31:05 +0000
Steven Price <steven.price@arm.com> wrote:

> On 18/03/2024 08:58, Boris Brezillon wrote:
> > Putting a hard dependency on CONFIG_PM is not possible because of a
> > circular dependency issue, and it's actually not desirable either. In
> > order to support this use case, we forcibly resume at init time, and
> > suspend at unplug time.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> > ---
> > Tested by faking CONFIG_PM=n in the driver (basically commenting
> > all pm_runtime calls, and making the panthor_device_suspend/resume()
> > calls unconditional in the panthor_device_unplug/init() path) since
> > CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
> > can't be 100% sure this will work correctly on a platform that has
> > CONFIG_PM=n.  
> 
> The same - I can't test this properly :(
> 
> Note that the other option (which AFAICT doesn't cause any problems) is
> to "select PM" rather than depend on it - AIUI the 'select' dependency
> is considered in the opposite direction by kconfig so won't cause the
> dependency loop.

Doesn't seem to work with COMPILE_TEST though? I mean, we need
something like

	depends on ARM || ARM64 || (COMPILE_TEST && PM)
	...
	select PM

but kconfig doesn't like that

drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected!
drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on
PM kernel/power/Kconfig:183:	symbol PM is selected by DRM_PANTHOR

which id why I initially when for a depends on PM


> Of course if there is actually anyone who has a
> platform which can be built !CONFIG_PM then that won't help. But the
> inability of anyone to actually properly test this configuration does
> worry me a little.

Well, as long as it doesn't regress the PM behavior, I think I'm happy
to take the risk. Worst case scenario, someone complains that this is
not working properly when they do the !PM bringup :-).

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Steven Price]

On 18/03/2024 13:08, Boris Brezillon wrote:
> On Mon, 18 Mar 2024 11:31:05 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 18/03/2024 08:58, Boris Brezillon wrote:
>>> Putting a hard dependency on CONFIG_PM is not possible because of a
>>> circular dependency issue, and it's actually not desirable either. In
>>> order to support this use case, we forcibly resume at init time, and
>>> suspend at unplug time.
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
>>
>> Reviewed-by: Steven Price <steven.price@arm.com>
>>
>>> ---
>>> Tested by faking CONFIG_PM=n in the driver (basically commenting
>>> all pm_runtime calls, and making the panthor_device_suspend/resume()
>>> calls unconditional in the panthor_device_unplug/init() path) since
>>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
>>> can't be 100% sure this will work correctly on a platform that has
>>> CONFIG_PM=n.  
>>
>> The same - I can't test this properly :(
>>
>> Note that the other option (which AFAICT doesn't cause any problems) is
>> to "select PM" rather than depend on it - AIUI the 'select' dependency
>> is considered in the opposite direction by kconfig so won't cause the
>> dependency loop.
> 
> Doesn't seem to work with COMPILE_TEST though? I mean, we need
> something like
> 
> 	depends on ARM || ARM64 || (COMPILE_TEST && PM)
> 	...
> 	select PM
> 
> but kconfig doesn't like that

Why do we need the "&& PM" part? Just:

	depends on ARM || ARM64 || COMPILE_TEST
	...
	select PM

Or at least that appears to work for me.

> drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected!
> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on
> PM kernel/power/Kconfig:183:	symbol PM is selected by DRM_PANTHOR
> 
> which id why I initially when for a depends on PM
> 
> 
>> Of course if there is actually anyone who has a
>> platform which can be built !CONFIG_PM then that won't help. But the
>> inability of anyone to actually properly test this configuration does
>> worry me a little.
> 
> Well, as long as it doesn't regress the PM behavior, I think I'm happy
> to take the risk. Worst case scenario, someone complains that this is
> not working properly when they do the !PM bringup :-).

Indeed, I've no objection to this patch - although I really should have
compiled tested it as Robin pointed out ;)

But one other thing I've noticed when compile testing it - we don't
appear to have fully fixed the virt_to_pfn() problem. On x86 with
COMPILE_TEST I still get an error. Looking at the code it appears that
virt_to_pfn() isn't available on x86... it overrides asm/page.h and
doesn't provide a definition. The definition on x86 is hiding in
asm/xen/page.h.

Outside of arch code it's only drivers/xen that currently uses that
function. So I guess it's probably best to do a
PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :)

Steve

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Robin Murphy]

On 18/03/2024 1:49 pm, Steven Price wrote:
> On 18/03/2024 13:08, Boris Brezillon wrote:
>> On Mon, 18 Mar 2024 11:31:05 +0000
>> Steven Price <steven.price@arm.com> wrote:
>>
>>> On 18/03/2024 08:58, Boris Brezillon wrote:
>>>> Putting a hard dependency on CONFIG_PM is not possible because of a
>>>> circular dependency issue, and it's actually not desirable either. In
>>>> order to support this use case, we forcibly resume at init time, and
>>>> suspend at unplug time.
>>>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
>>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>>>
>>> Reviewed-by: Steven Price <steven.price@arm.com>
>>>
>>>> ---
>>>> Tested by faking CONFIG_PM=n in the driver (basically commenting
>>>> all pm_runtime calls, and making the panthor_device_suspend/resume()
>>>> calls unconditional in the panthor_device_unplug/init() path) since
>>>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
>>>> can't be 100% sure this will work correctly on a platform that has
>>>> CONFIG_PM=n.
>>>
>>> The same - I can't test this properly :(
>>>
>>> Note that the other option (which AFAICT doesn't cause any problems) is
>>> to "select PM" rather than depend on it - AIUI the 'select' dependency
>>> is considered in the opposite direction by kconfig so won't cause the
>>> dependency loop.
>>
>> Doesn't seem to work with COMPILE_TEST though? I mean, we need
>> something like
>>
>> 	depends on ARM || ARM64 || (COMPILE_TEST && PM)
>> 	...
>> 	select PM
>>
>> but kconfig doesn't like that
> 
> Why do we need the "&& PM" part? Just:
> 
> 	depends on ARM || ARM64 || COMPILE_TEST
> 	...
> 	select PM
> 
> Or at least that appears to work for me.
> 
>> drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected!
>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on
>> PM kernel/power/Kconfig:183:	symbol PM is selected by DRM_PANTHOR
>>
>> which id why I initially when for a depends on PM
>>
>>
>>> Of course if there is actually anyone who has a
>>> platform which can be built !CONFIG_PM then that won't help. But the
>>> inability of anyone to actually properly test this configuration does
>>> worry me a little.
>>
>> Well, as long as it doesn't regress the PM behavior, I think I'm happy
>> to take the risk. Worst case scenario, someone complains that this is
>> not working properly when they do the !PM bringup :-).
> 
> Indeed, I've no objection to this patch - although I really should have
> compiled tested it as Robin pointed out ;)
> 
> But one other thing I've noticed when compile testing it - we don't
> appear to have fully fixed the virt_to_pfn() problem. On x86 with
> COMPILE_TEST I still get an error. Looking at the code it appears that
> virt_to_pfn() isn't available on x86... it overrides asm/page.h and
> doesn't provide a definition. The definition on x86 is hiding in
> asm/xen/page.h.
> 
> Outside of arch code it's only drivers/xen that currently uses that
> function. So I guess it's probably best to do a
> PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :)

FWIW from a quick look it might be cleaner to store the struct page 
pointer for the dummy page - especially since the VA only seems to be 
used once in panthor_device_init() anyway - then use page_to_pfn() at 
the business end.

Cheers,
Robin.

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Boris Brezillon]

On Mon, 18 Mar 2024 13:49:52 +0000
Steven Price <steven.price@arm.com> wrote:

> On 18/03/2024 13:08, Boris Brezillon wrote:
> > On Mon, 18 Mar 2024 11:31:05 +0000
> > Steven Price <steven.price@arm.com> wrote:
> >   
> >> On 18/03/2024 08:58, Boris Brezillon wrote:  
> >>> Putting a hard dependency on CONFIG_PM is not possible because of a
> >>> circular dependency issue, and it's actually not desirable either. In
> >>> order to support this use case, we forcibly resume at init time, and
> >>> suspend at unplug time.
> >>>
> >>> Reported-by: kernel test robot <lkp@intel.com>
> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>    
> >>
> >> Reviewed-by: Steven Price <steven.price@arm.com>
> >>  
> >>> ---
> >>> Tested by faking CONFIG_PM=n in the driver (basically commenting
> >>> all pm_runtime calls, and making the panthor_device_suspend/resume()
> >>> calls unconditional in the panthor_device_unplug/init() path) since
> >>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
> >>> can't be 100% sure this will work correctly on a platform that has
> >>> CONFIG_PM=n.    
> >>
> >> The same - I can't test this properly :(
> >>
> >> Note that the other option (which AFAICT doesn't cause any problems) is
> >> to "select PM" rather than depend on it - AIUI the 'select' dependency
> >> is considered in the opposite direction by kconfig so won't cause the
> >> dependency loop.  
> > 
> > Doesn't seem to work with COMPILE_TEST though? I mean, we need
> > something like
> > 
> > 	depends on ARM || ARM64 || (COMPILE_TEST && PM)
> > 	...
> > 	select PM
> > 
> > but kconfig doesn't like that  
> 
> Why do we need the "&& PM" part? Just:
> 
> 	depends on ARM || ARM64 || COMPILE_TEST
> 	...
> 	select PM
> 
> Or at least that appears to work for me.

Uh, you're right, sorry for the brain fart. This is being said, I
see no other driver selecting the PM option directly (if you grep for
'select PM' in drivers/, you'll find occurrences in drivers/soc, but
those are under ARCH_/SOC_ options, which means they are indirectly
arch/platform dependent, not driver dependent). I'm really not sure
selecting PM here from a driver is right to be honest.

> 
> > drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected!
> > drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on
> > PM kernel/power/Kconfig:183:	symbol PM is selected by DRM_PANTHOR
> > 
> > which id why I initially when for a depends on PM
> > 
> >   
> >> Of course if there is actually anyone who has a
> >> platform which can be built !CONFIG_PM then that won't help. But the
> >> inability of anyone to actually properly test this configuration does
> >> worry me a little.  
> > 
> > Well, as long as it doesn't regress the PM behavior, I think I'm happy
> > to take the risk. Worst case scenario, someone complains that this is
> > not working properly when they do the !PM bringup :-).  
> 
> Indeed, I've no objection to this patch - although I really should have
> compiled tested it as Robin pointed out ;)
> 
> But one other thing I've noticed when compile testing it - we don't
> appear to have fully fixed the virt_to_pfn() problem. On x86 with
> COMPILE_TEST I still get an error. Looking at the code it appears that
> virt_to_pfn() isn't available on x86... it overrides asm/page.h and
> doesn't provide a definition. The definition on x86 is hiding in
> asm/xen/page.h.

Looks like the kbuild bot didn't catch that yet :-).

> 
> Outside of arch code it's only drivers/xen that currently uses that
> function. So I guess it's probably best to do a
> PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :)

Mind sending a fix for that?

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Steven Price]

On 18/03/2024 14:18, Boris Brezillon wrote:
> On Mon, 18 Mar 2024 13:49:52 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 18/03/2024 13:08, Boris Brezillon wrote:
>>> On Mon, 18 Mar 2024 11:31:05 +0000
>>> Steven Price <steven.price@arm.com> wrote:
>>>   
>>>> On 18/03/2024 08:58, Boris Brezillon wrote:  
>>>>> Putting a hard dependency on CONFIG_PM is not possible because of a
>>>>> circular dependency issue, and it's actually not desirable either. In
>>>>> order to support this use case, we forcibly resume at init time, and
>>>>> suspend at unplug time.
>>>>>
>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
>>>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>    
>>>>
>>>> Reviewed-by: Steven Price <steven.price@arm.com>
>>>>  
>>>>> ---
>>>>> Tested by faking CONFIG_PM=n in the driver (basically commenting
>>>>> all pm_runtime calls, and making the panthor_device_suspend/resume()
>>>>> calls unconditional in the panthor_device_unplug/init() path) since
>>>>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
>>>>> can't be 100% sure this will work correctly on a platform that has
>>>>> CONFIG_PM=n.    
>>>>
>>>> The same - I can't test this properly :(
>>>>
>>>> Note that the other option (which AFAICT doesn't cause any problems) is
>>>> to "select PM" rather than depend on it - AIUI the 'select' dependency
>>>> is considered in the opposite direction by kconfig so won't cause the
>>>> dependency loop.  
>>>
>>> Doesn't seem to work with COMPILE_TEST though? I mean, we need
>>> something like
>>>
>>> 	depends on ARM || ARM64 || (COMPILE_TEST && PM)
>>> 	...
>>> 	select PM
>>>
>>> but kconfig doesn't like that  
>>
>> Why do we need the "&& PM" part? Just:
>>
>> 	depends on ARM || ARM64 || COMPILE_TEST
>> 	...
>> 	select PM
>>
>> Or at least that appears to work for me.
> 
> Uh, you're right, sorry for the brain fart. This is being said, I
> see no other driver selecting the PM option directly (if you grep for
> 'select PM' in drivers/, you'll find occurrences in drivers/soc, but
> those are under ARCH_/SOC_ options, which means they are indirectly
> arch/platform dependent, not driver dependent). I'm really not sure
> selecting PM here from a driver is right to be honest.

Yeah, I'm not very convinced about that either. It's just a pain that
the code is going to go untested.

>>
>>> drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected!
>>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on
>>> PM kernel/power/Kconfig:183:	symbol PM is selected by DRM_PANTHOR
>>>
>>> which id why I initially when for a depends on PM
>>>
>>>   
>>>> Of course if there is actually anyone who has a
>>>> platform which can be built !CONFIG_PM then that won't help. But the
>>>> inability of anyone to actually properly test this configuration does
>>>> worry me a little.  
>>>
>>> Well, as long as it doesn't regress the PM behavior, I think I'm happy
>>> to take the risk. Worst case scenario, someone complains that this is
>>> not working properly when they do the !PM bringup :-).  
>>
>> Indeed, I've no objection to this patch - although I really should have
>> compiled tested it as Robin pointed out ;)
>>
>> But one other thing I've noticed when compile testing it - we don't
>> appear to have fully fixed the virt_to_pfn() problem. On x86 with
>> COMPILE_TEST I still get an error. Looking at the code it appears that
>> virt_to_pfn() isn't available on x86... it overrides asm/page.h and
>> doesn't provide a definition. The definition on x86 is hiding in
>> asm/xen/page.h.
> 
> Looks like the kbuild bot didn't catch that yet :-).
> 
>>
>> Outside of arch code it's only drivers/xen that currently uses that
>> function. So I guess it's probably best to do a
>> PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :)
> 
> Mind sending a fix for that?

Yeah, I'll have a go at Robin's suggestion of storing the struct page
instead.

Steve

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Boris Brezillon]

On Mon, 18 Mar 2024 14:34:07 +0000
Steven Price <steven.price@arm.com> wrote:

> On 18/03/2024 14:18, Boris Brezillon wrote:
> > On Mon, 18 Mar 2024 13:49:52 +0000
> > Steven Price <steven.price@arm.com> wrote:
> >   
> >> On 18/03/2024 13:08, Boris Brezillon wrote:  
> >>> On Mon, 18 Mar 2024 11:31:05 +0000
> >>> Steven Price <steven.price@arm.com> wrote:
> >>>     
> >>>> On 18/03/2024 08:58, Boris Brezillon wrote:    
> >>>>> Putting a hard dependency on CONFIG_PM is not possible because of a
> >>>>> circular dependency issue, and it's actually not desirable either. In
> >>>>> order to support this use case, we forcibly resume at init time, and
> >>>>> suspend at unplug time.
> >>>>>
> >>>>> Reported-by: kernel test robot <lkp@intel.com>
> >>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
> >>>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>      
> >>>>
> >>>> Reviewed-by: Steven Price <steven.price@arm.com>
> >>>>    
> >>>>> ---
> >>>>> Tested by faking CONFIG_PM=n in the driver (basically commenting
> >>>>> all pm_runtime calls, and making the panthor_device_suspend/resume()
> >>>>> calls unconditional in the panthor_device_unplug/init() path) since
> >>>>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
> >>>>> can't be 100% sure this will work correctly on a platform that has
> >>>>> CONFIG_PM=n.      
> >>>>
> >>>> The same - I can't test this properly :(
> >>>>
> >>>> Note that the other option (which AFAICT doesn't cause any problems) is
> >>>> to "select PM" rather than depend on it - AIUI the 'select' dependency
> >>>> is considered in the opposite direction by kconfig so won't cause the
> >>>> dependency loop.    
> >>>
> >>> Doesn't seem to work with COMPILE_TEST though? I mean, we need
> >>> something like
> >>>
> >>> 	depends on ARM || ARM64 || (COMPILE_TEST && PM)
> >>> 	...
> >>> 	select PM
> >>>
> >>> but kconfig doesn't like that    
> >>
> >> Why do we need the "&& PM" part? Just:
> >>
> >> 	depends on ARM || ARM64 || COMPILE_TEST
> >> 	...
> >> 	select PM
> >>
> >> Or at least that appears to work for me.  
> > 
> > Uh, you're right, sorry for the brain fart. This is being said, I
> > see no other driver selecting the PM option directly (if you grep for
> > 'select PM' in drivers/, you'll find occurrences in drivers/soc, but
> > those are under ARCH_/SOC_ options, which means they are indirectly
> > arch/platform dependent, not driver dependent). I'm really not sure
> > selecting PM here from a driver is right to be honest.  
> 
> Yeah, I'm not very convinced about that either. It's just a pain that
> the code is going to go untested.

If that's really bothering you, let's just return an error in the
probe path when CONFIG_PM=n as you were suggesting last week.

> 
> >>  
> >>> drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected!
> >>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on
> >>> PM kernel/power/Kconfig:183:	symbol PM is selected by DRM_PANTHOR
> >>>
> >>> which id why I initially when for a depends on PM
> >>>
> >>>     
> >>>> Of course if there is actually anyone who has a
> >>>> platform which can be built !CONFIG_PM then that won't help. But the
> >>>> inability of anyone to actually properly test this configuration does
> >>>> worry me a little.    
> >>>
> >>> Well, as long as it doesn't regress the PM behavior, I think I'm happy
> >>> to take the risk. Worst case scenario, someone complains that this is
> >>> not working properly when they do the !PM bringup :-).    
> >>
> >> Indeed, I've no objection to this patch - although I really should have
> >> compiled tested it as Robin pointed out ;)
> >>
> >> But one other thing I've noticed when compile testing it - we don't
> >> appear to have fully fixed the virt_to_pfn() problem. On x86 with
> >> COMPILE_TEST I still get an error. Looking at the code it appears that
> >> virt_to_pfn() isn't available on x86... it overrides asm/page.h and
> >> doesn't provide a definition. The definition on x86 is hiding in
> >> asm/xen/page.h.  
> > 
> > Looks like the kbuild bot didn't catch that yet :-).
> >   
> >>
> >> Outside of arch code it's only drivers/xen that currently uses that
> >> function. So I guess it's probably best to do a
> >> PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :)  
> > 
> > Mind sending a fix for that?  
> 
> Yeah, I'll have a go at Robin's suggestion of storing the struct page
> instead.

If that's not too much to ask, could you also send a variant of this
patch returning an error when CONFIG_PM is disabled (and fixing the
other mistake I made, of course)?

Thanks,

Boris

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Steven Price]

On 18/03/2024 14:49, Boris Brezillon wrote:
> On Mon, 18 Mar 2024 14:34:07 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 18/03/2024 14:18, Boris Brezillon wrote:
>>> On Mon, 18 Mar 2024 13:49:52 +0000
>>> Steven Price <steven.price@arm.com> wrote:
>>>   
>>>> On 18/03/2024 13:08, Boris Brezillon wrote:  
>>>>> On Mon, 18 Mar 2024 11:31:05 +0000
>>>>> Steven Price <steven.price@arm.com> wrote:
>>>>>     
>>>>>> On 18/03/2024 08:58, Boris Brezillon wrote:    
>>>>>>> Putting a hard dependency on CONFIG_PM is not possible because of a
>>>>>>> circular dependency issue, and it's actually not desirable either. In
>>>>>>> order to support this use case, we forcibly resume at init time, and
>>>>>>> suspend at unplug time.
>>>>>>>
>>>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
>>>>>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>      
>>>>>>
>>>>>> Reviewed-by: Steven Price <steven.price@arm.com>
>>>>>>    
>>>>>>> ---
>>>>>>> Tested by faking CONFIG_PM=n in the driver (basically commenting
>>>>>>> all pm_runtime calls, and making the panthor_device_suspend/resume()
>>>>>>> calls unconditional in the panthor_device_unplug/init() path) since
>>>>>>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
>>>>>>> can't be 100% sure this will work correctly on a platform that has
>>>>>>> CONFIG_PM=n.      
>>>>>>
>>>>>> The same - I can't test this properly :(
>>>>>>
>>>>>> Note that the other option (which AFAICT doesn't cause any problems) is
>>>>>> to "select PM" rather than depend on it - AIUI the 'select' dependency
>>>>>> is considered in the opposite direction by kconfig so won't cause the
>>>>>> dependency loop.    
>>>>>
>>>>> Doesn't seem to work with COMPILE_TEST though? I mean, we need
>>>>> something like
>>>>>
>>>>> 	depends on ARM || ARM64 || (COMPILE_TEST && PM)
>>>>> 	...
>>>>> 	select PM
>>>>>
>>>>> but kconfig doesn't like that    
>>>>
>>>> Why do we need the "&& PM" part? Just:
>>>>
>>>> 	depends on ARM || ARM64 || COMPILE_TEST
>>>> 	...
>>>> 	select PM
>>>>
>>>> Or at least that appears to work for me.  
>>>
>>> Uh, you're right, sorry for the brain fart. This is being said, I
>>> see no other driver selecting the PM option directly (if you grep for
>>> 'select PM' in drivers/, you'll find occurrences in drivers/soc, but
>>> those are under ARCH_/SOC_ options, which means they are indirectly
>>> arch/platform dependent, not driver dependent). I'm really not sure
>>> selecting PM here from a driver is right to be honest.  
>>
>> Yeah, I'm not very convinced about that either. It's just a pain that
>> the code is going to go untested.
> 
> If that's really bothering you, let's just return an error in the
> probe path when CONFIG_PM=n as you were suggesting last week.

Honestly I'm fine with this patch (modulo it needs compile testing!).
I'd obviously just be happier if someone actually had a platform where
this could be tested. But really one of two things will happen:

 a) Nobody will ever execute the code. In which case it doesn't really
matter as long as it builds.

 b) Someone will execute the code, and it'll either work which is great
(and so we won't hear), or it will break and hopefully then we'll have a
willing tester ;)

>>
>>>>  
>>>>> drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected!
>>>>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on
>>>>> PM kernel/power/Kconfig:183:	symbol PM is selected by DRM_PANTHOR
>>>>>
>>>>> which id why I initially when for a depends on PM
>>>>>
>>>>>     
>>>>>> Of course if there is actually anyone who has a
>>>>>> platform which can be built !CONFIG_PM then that won't help. But the
>>>>>> inability of anyone to actually properly test this configuration does
>>>>>> worry me a little.    
>>>>>
>>>>> Well, as long as it doesn't regress the PM behavior, I think I'm happy
>>>>> to take the risk. Worst case scenario, someone complains that this is
>>>>> not working properly when they do the !PM bringup :-).    
>>>>
>>>> Indeed, I've no objection to this patch - although I really should have
>>>> compiled tested it as Robin pointed out ;)
>>>>
>>>> But one other thing I've noticed when compile testing it - we don't
>>>> appear to have fully fixed the virt_to_pfn() problem. On x86 with
>>>> COMPILE_TEST I still get an error. Looking at the code it appears that
>>>> virt_to_pfn() isn't available on x86... it overrides asm/page.h and
>>>> doesn't provide a definition. The definition on x86 is hiding in
>>>> asm/xen/page.h.  
>>>
>>> Looks like the kbuild bot didn't catch that yet :-).
>>>   
>>>>
>>>> Outside of arch code it's only drivers/xen that currently uses that
>>>> function. So I guess it's probably best to do a
>>>> PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :)  
>>>
>>> Mind sending a fix for that?  
>>
>> Yeah, I'll have a go at Robin's suggestion of storing the struct page
>> instead.
> 
> If that's not too much to ask, could you also send a variant of this
> patch returning an error when CONFIG_PM is disabled (and fixing the
> other mistake I made, of course)?

Too late ;)

Are you happy to respin this with the #ifdef dropped?

Thanks,

Steve

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Robin Murphy]

On 18/03/2024 8:58 am, Boris Brezillon wrote:
> Putting a hard dependency on CONFIG_PM is not possible because of a
> circular dependency issue, and it's actually not desirable either. In
> order to support this use case, we forcibly resume at init time, and
> suspend at unplug time.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Tested by faking CONFIG_PM=n in the driver (basically commenting
> all pm_runtime calls, and making the panthor_device_suspend/resume()
> calls unconditional in the panthor_device_unplug/init() path) since
> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
> can't be 100% sure this will work correctly on a platform that has
> CONFIG_PM=n.
> ---
>   drivers/gpu/drm/panthor/panthor_device.c | 13 +++++++++++--
>   drivers/gpu/drm/panthor/panthor_drv.c    |  4 +++-
>   2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 69deb8e17778..ba7aedbb4931 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -87,6 +87,10 @@ void panthor_device_unplug(struct panthor_device *ptdev)
>   	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
>   	pm_runtime_put_sync_suspend(ptdev->base.dev);
>   
> +	/* If PM is disabled, we need to call the suspend handler manually. */
> +	if (!IS_ENABLED(CONFIG_PM))
> +		panthor_device_suspend(ptdev->base.dev);
> +
>   	/* Report the unplug operation as done to unblock concurrent
>   	 * panthor_device_unplug() callers.
>   	 */
> @@ -218,6 +222,13 @@ int panthor_device_init(struct panthor_device *ptdev)
>   	if (ret)
>   		return ret;
>   
> +	/* If PM is disabled, we need to call panthor_device_resume() manually. */
> +	if (!IS_ENABLED(CONFIG_PM)) {
> +		ret = panthor_device_resume(ptdev->base.dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	ret = panthor_gpu_init(ptdev);
>   	if (ret)
>   		goto err_rpm_put;
> @@ -402,7 +413,6 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
>   	return 0;
>   }
>   
> -#ifdef CONFIG_PM
>   int panthor_device_resume(struct device *dev)
>   {
>   	struct panthor_device *ptdev = dev_get_drvdata(dev);
> @@ -547,4 +557,3 @@ int panthor_device_suspend(struct device *dev)
>   	mutex_unlock(&ptdev->pm.mmio_lock);
>   	return ret;
>   }
> -#endif
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index ff484506229f..2ea6a9f436db 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1407,17 +1407,19 @@ static const struct of_device_id dt_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, dt_match);
>   
> +#ifdef CONFIG_PM

This #ifdef isn't necessary, and in fact will break the !PM build - 
pm_ptr() already takes care of allowing the compiler to optimise out the 
ops structure itself without any further annotations.

Thanks,
Robin.

>   static DEFINE_RUNTIME_DEV_PM_OPS(panthor_pm_ops,
>   				 panthor_device_suspend,
>   				 panthor_device_resume,
>   				 NULL);
> +#endif
>   
>   static struct platform_driver panthor_driver = {
>   	.probe = panthor_probe,
>   	.remove_new = panthor_remove,
>   	.driver = {
>   		.name = "panthor",
> -		.pm = &panthor_pm_ops,
> +		.pm = pm_ptr(&panthor_pm_ops),
>   		.of_match_table = dt_match,
>   	},
>   };

Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Boris Brezillon]

On Mon, 18 Mar 2024 12:18:53 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 18/03/2024 8:58 am, Boris Brezillon wrote:
> > Putting a hard dependency on CONFIG_PM is not possible because of a
> > circular dependency issue, and it's actually not desirable either. In
> > order to support this use case, we forcibly resume at init time, and
> > suspend at unplug time.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > Tested by faking CONFIG_PM=n in the driver (basically commenting
> > all pm_runtime calls, and making the panthor_device_suspend/resume()
> > calls unconditional in the panthor_device_unplug/init() path) since
> > CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
> > can't be 100% sure this will work correctly on a platform that has
> > CONFIG_PM=n.
> > ---
> >   drivers/gpu/drm/panthor/panthor_device.c | 13 +++++++++++--
> >   drivers/gpu/drm/panthor/panthor_drv.c    |  4 +++-
> >   2 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> > index 69deb8e17778..ba7aedbb4931 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.c
> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
> > @@ -87,6 +87,10 @@ void panthor_device_unplug(struct panthor_device *ptdev)
> >   	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
> >   	pm_runtime_put_sync_suspend(ptdev->base.dev);
> >   
> > +	/* If PM is disabled, we need to call the suspend handler manually. */
> > +	if (!IS_ENABLED(CONFIG_PM))
> > +		panthor_device_suspend(ptdev->base.dev);
> > +
> >   	/* Report the unplug operation as done to unblock concurrent
> >   	 * panthor_device_unplug() callers.
> >   	 */
> > @@ -218,6 +222,13 @@ int panthor_device_init(struct panthor_device *ptdev)
> >   	if (ret)
> >   		return ret;
> >   
> > +	/* If PM is disabled, we need to call panthor_device_resume() manually. */
> > +	if (!IS_ENABLED(CONFIG_PM)) {
> > +		ret = panthor_device_resume(ptdev->base.dev);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >   	ret = panthor_gpu_init(ptdev);
> >   	if (ret)
> >   		goto err_rpm_put;
> > @@ -402,7 +413,6 @@ int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *
> >   	return 0;
> >   }
> >   
> > -#ifdef CONFIG_PM
> >   int panthor_device_resume(struct device *dev)
> >   {
> >   	struct panthor_device *ptdev = dev_get_drvdata(dev);
> > @@ -547,4 +557,3 @@ int panthor_device_suspend(struct device *dev)
> >   	mutex_unlock(&ptdev->pm.mmio_lock);
> >   	return ret;
> >   }
> > -#endif
> > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> > index ff484506229f..2ea6a9f436db 100644
> > --- a/drivers/gpu/drm/panthor/panthor_drv.c
> > +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> > @@ -1407,17 +1407,19 @@ static const struct of_device_id dt_match[] = {
> >   };
> >   MODULE_DEVICE_TABLE(of, dt_match);
> >   
> > +#ifdef CONFIG_PM  
> 
> This #ifdef isn't necessary, and in fact will break the !PM build - 
> pm_ptr() already takes care of allowing the compiler to optimise out the 
> ops structure itself without any further annotations.

Oops, I forgot how pm_ptr() was working (I thought it had 2
definitions, one for the PM case and another for the !PM one).

> 
> Thanks,
> Robin.
> 
> >   static DEFINE_RUNTIME_DEV_PM_OPS(panthor_pm_ops,
> >   				 panthor_device_suspend,
> >   				 panthor_device_resume,
> >   				 NULL);
> > +#endif
> >   
> >   static struct platform_driver panthor_driver = {
> >   	.probe = panthor_probe,
> >   	.remove_new = panthor_remove,
> >   	.driver = {
> >   		.name = "panthor",
> > -		.pm = &panthor_pm_ops,
> > +		.pm = pm_ptr(&panthor_pm_ops),
> >   		.of_match_table = dt_match,
> >   	},
> >   };