[PATCH 0/3] drm/panthor: Fix 3 issues reported by the kernel test bot

[PATCH 0/3] drm/panthor: Fix 3 issues reported by the kernel test bot

[Boris Brezillon]

Hello,

Here are three trivial fixes for bugs reported by the intel kernel test
bot.

Regards,

Boris

Boris Brezillon (3):
  drm/panthor: Fix panthor_devfreq kerneldoc
  drm/panthor: Explicitly include page.h for the {virt,__phys)_to_pfn()
    defs
  drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

 drivers/gpu/drm/panthor/Kconfig           | 1 +
 drivers/gpu/drm/panthor/panthor_devfreq.c | 2 +-
 drivers/gpu/drm/panthor/panthor_device.c  | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH 1/3] drm/panthor: Fix panthor_devfreq kerneldoc

[Boris Brezillon]

Missing '*' to have a valid kerneldoc prefix.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202403031019.6jvrOqGT-lkp@intel.com/
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
index 7ac4fa290f27..c6d3c327cc24 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.c
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
@@ -34,7 +34,7 @@ struct panthor_devfreq {
 	/** @last_busy_state: True if the GPU was busy last time we updated the state. */
 	bool last_busy_state;
 
-	/*
+	/**
 	 * @lock: Lock used to protect busy_time, idle_time, time_last_update and
 	 * last_busy_state.
 	 *
-- 
2.43.0

[PATCH 2/3] drm/panthor: Explicitly include page.h for the {virt, __phys)_to_pfn() defs

[Boris Brezillon]

Something on arm[64] must be including <asm/page.h>, but things fail
to compile on sparc64. Make sure this header is included explicitly
so this driver can be compile-tested on all supported architectures.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202403031142.Vl4pW7X6-lkp@intel.com/
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index bfe8da4a6e4c..68e467ee458a 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -3,6 +3,8 @@
 /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
 /* Copyright 2023 Collabora ltd. */
 
+#include <asm/page.h>
+
 #include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
-- 
2.43.0

[PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Boris Brezillon]

panthor_device_resume/suspend() are only compiled when CONFIG_PM is
enabled but panthro_drv.c doesn't use the pm_ptr() macro to conditionally
discard resume/suspend assignments, which causes undefined symbol
errors at link time when !PM.

We could fix that by using pm_ptr(), but supporting the !PM case makes
little sense (the whole point of these embedded GPUs is to be low power,
so proper PM is a basic requirement in that case). So let's just enforce
the presence of CONFIG_PM with a Kconfig dependency instead.

If someone needs to relax this dependency, it can be done in a follow-up.

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>
---
 drivers/gpu/drm/panthor/Kconfig          | 1 +
 drivers/gpu/drm/panthor/panthor_device.c | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig
index 55b40ad07f3b..fdce7c1b2310 100644
--- a/drivers/gpu/drm/panthor/Kconfig
+++ b/drivers/gpu/drm/panthor/Kconfig
@@ -6,6 +6,7 @@ config DRM_PANTHOR
 	depends on ARM || ARM64 || COMPILE_TEST
 	depends on !GENERIC_ATOMIC64  # for IOMMU_IO_PGTABLE_LPAE
 	depends on MMU
+	depends on PM
 	select DEVFREQ_GOV_SIMPLE_ONDEMAND
 	select DRM_EXEC
 	select DRM_GEM_SHMEM_HELPER
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 68e467ee458a..efea29143a54 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -403,7 +403,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);
@@ -548,4 +547,3 @@ int panthor_device_suspend(struct device *dev)
 	mutex_unlock(&ptdev->pm.mmio_lock);
 	return ret;
 }
-#endif
-- 
2.43.0

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Liviu Dudau]

On Mon, Mar 04, 2024 at 10:08:12AM +0100, Boris Brezillon wrote:
> panthor_device_resume/suspend() are only compiled when CONFIG_PM is
> enabled but panthro_drv.c doesn't use the pm_ptr() macro to conditionally
> discard resume/suspend assignments, which causes undefined symbol
> errors at link time when !PM.
> 
> We could fix that by using pm_ptr(), but supporting the !PM case makes
> little sense (the whole point of these embedded GPUs is to be low power,
> so proper PM is a basic requirement in that case). So let's just enforce
> the presence of CONFIG_PM with a Kconfig dependency instead.
> 
> If someone needs to relax this dependency, it can be done in a follow-up.
> 
> 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: Liviu Dudau <liviu.dudau@arm.com>

Agree on this, there is no point on running on kernels without CONFIG_PM enabled.

Best regards,
Liviu


> ---
>  drivers/gpu/drm/panthor/Kconfig          | 1 +
>  drivers/gpu/drm/panthor/panthor_device.c | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig
> index 55b40ad07f3b..fdce7c1b2310 100644
> --- a/drivers/gpu/drm/panthor/Kconfig
> +++ b/drivers/gpu/drm/panthor/Kconfig
> @@ -6,6 +6,7 @@ config DRM_PANTHOR
>  	depends on ARM || ARM64 || COMPILE_TEST
>  	depends on !GENERIC_ATOMIC64  # for IOMMU_IO_PGTABLE_LPAE
>  	depends on MMU
> +	depends on PM
>  	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>  	select DRM_EXEC
>  	select DRM_GEM_SHMEM_HELPER
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 68e467ee458a..efea29143a54 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -403,7 +403,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);
> @@ -548,4 +547,3 @@ int panthor_device_suspend(struct device *dev)
>  	mutex_unlock(&ptdev->pm.mmio_lock);
>  	return ret;
>  }
> -#endif
> -- 
> 2.43.0
> 

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

Re: [PATCH 2/3] drm/panthor: Explicitly include page.h for the {virt,__phys)_to_pfn() defs

[Liviu Dudau]

On Mon, Mar 04, 2024 at 10:08:11AM +0100, Boris Brezillon wrote:
> Something on arm[64] must be including <asm/page.h>, but things fail
> to compile on sparc64. Make sure this header is included explicitly
> so this driver can be compile-tested on all supported architectures.

Is compilation on sparc64 possible because of 'depends on COMPILE_TEST'?
Otherwise it doesn't make sense to try to build this for any arch other
than arm[64].

Regardless, patch looks harmless, so

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

Best regards,
Liviu

> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202403031142.Vl4pW7X6-lkp@intel.com/
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panthor/panthor_device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index bfe8da4a6e4c..68e467ee458a 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -3,6 +3,8 @@
>  /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
>  /* Copyright 2023 Collabora ltd. */
>  
> +#include <asm/page.h>
> +
>  #include <linux/clk.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
> -- 
> 2.43.0
> 

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

Re: [PATCH 1/3] drm/panthor: Fix panthor_devfreq kerneldoc

[Liviu Dudau]

On Mon, Mar 04, 2024 at 10:08:10AM +0100, Boris Brezillon wrote:
> Missing '*' to have a valid kerneldoc prefix.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202403031019.6jvrOqGT-lkp@intel.com/
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

LGTM :)

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> index 7ac4fa290f27..c6d3c327cc24 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> @@ -34,7 +34,7 @@ struct panthor_devfreq {
>  	/** @last_busy_state: True if the GPU was busy last time we updated the state. */
>  	bool last_busy_state;
>  
> -	/*
> +	/**
>  	 * @lock: Lock used to protect busy_time, idle_time, time_last_update and
>  	 * last_busy_state.
>  	 *
> -- 
> 2.43.0
> 

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

Re: [PATCH 1/3] drm/panthor: Fix panthor_devfreq kerneldoc

[Steven Price]

On 04/03/2024 09:08, Boris Brezillon wrote:
> Missing '*' to have a valid kerneldoc prefix.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202403031019.6jvrOqGT-lkp@intel.com/
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

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

> ---
>  drivers/gpu/drm/panthor/panthor_devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> index 7ac4fa290f27..c6d3c327cc24 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> @@ -34,7 +34,7 @@ struct panthor_devfreq {
>  	/** @last_busy_state: True if the GPU was busy last time we updated the state. */
>  	bool last_busy_state;
>  
> -	/*
> +	/**
>  	 * @lock: Lock used to protect busy_time, idle_time, time_last_update and
>  	 * last_busy_state.
>  	 *

Re: [PATCH 2/3] drm/panthor: Explicitly include page.h for the {virt,__phys)_to_pfn() defs

[Steven Price]

On 04/03/2024 09:08, Boris Brezillon wrote:
> Something on arm[64] must be including <asm/page.h>, but things fail
> to compile on sparc64. Make sure this header is included explicitly
> so this driver can be compile-tested on all supported architectures.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202403031142.Vl4pW7X6-lkp@intel.com/
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Seems reasonable, although I do wonder if it's right to include a "asm"
header here or if we should pull in something like "linux/mm.h" which
includes asm/page.h. I can find examples of both. Either way:

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

> ---
>  drivers/gpu/drm/panthor/panthor_device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index bfe8da4a6e4c..68e467ee458a 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -3,6 +3,8 @@
>  /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
>  /* Copyright 2023 Collabora ltd. */
>  
> +#include <asm/page.h>
> +
>  #include <linux/clk.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Steven Price]

On 04/03/2024 09:08, Boris Brezillon wrote:
> panthor_device_resume/suspend() are only compiled when CONFIG_PM is
> enabled but panthro_drv.c doesn't use the pm_ptr() macro to conditionally
> discard resume/suspend assignments, which causes undefined symbol
> errors at link time when !PM.
> 
> We could fix that by using pm_ptr(), but supporting the !PM case makes
> little sense (the whole point of these embedded GPUs is to be low power,
> so proper PM is a basic requirement in that case). So let's just enforce
> the presence of CONFIG_PM with a Kconfig dependency instead.
> 
> If someone needs to relax this dependency, it can be done in a follow-up.
> 
> 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>

Seems reasonable, it should be easy to relax the CONFIG_PM condition in
the future if anyone has an actual need.

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

> ---
>  drivers/gpu/drm/panthor/Kconfig          | 1 +
>  drivers/gpu/drm/panthor/panthor_device.c | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig
> index 55b40ad07f3b..fdce7c1b2310 100644
> --- a/drivers/gpu/drm/panthor/Kconfig
> +++ b/drivers/gpu/drm/panthor/Kconfig
> @@ -6,6 +6,7 @@ config DRM_PANTHOR
>  	depends on ARM || ARM64 || COMPILE_TEST
>  	depends on !GENERIC_ATOMIC64  # for IOMMU_IO_PGTABLE_LPAE
>  	depends on MMU
> +	depends on PM
>  	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>  	select DRM_EXEC
>  	select DRM_GEM_SHMEM_HELPER
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 68e467ee458a..efea29143a54 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -403,7 +403,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);
> @@ -548,4 +547,3 @@ int panthor_device_suspend(struct device *dev)
>  	mutex_unlock(&ptdev->pm.mmio_lock);
>  	return ret;
>  }
> -#endif

Re: [PATCH 2/3] drm/panthor: Explicitly include page.h for the {virt,__phys)_to_pfn() defs

[Boris Brezillon]

On Mon, 4 Mar 2024 12:31:23 +0000
Steven Price <steven.price@arm.com> wrote:

> On 04/03/2024 09:08, Boris Brezillon wrote:
> > Something on arm[64] must be including <asm/page.h>, but things fail
> > to compile on sparc64. Make sure this header is included explicitly
> > so this driver can be compile-tested on all supported architectures.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202403031142.Vl4pW7X6-lkp@intel.com/
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> Seems reasonable, although I do wonder if it's right to include a "asm"
> header here or if we should pull in something like "linux/mm.h" which
> includes asm/page.h. I can find examples of both. Either way:

Actually, I considered including linux/mm.h too, so I'm happy to go for
this option (will fix when applying.

> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_device.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> > index bfe8da4a6e4c..68e467ee458a 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.c
> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
> > @@ -3,6 +3,8 @@
> >  /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
> >  /* Copyright 2023 Collabora ltd. */
> >  
> > +#include <asm/page.h>
> > +
> >  #include <linux/clk.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_domain.h>  
> 

Re: [PATCH 2/3] drm/panthor: Explicitly include page.h for the {virt,__phys)_to_pfn() defs

[Boris Brezillon]

On Mon, 4 Mar 2024 11:16:48 +0000
Liviu Dudau <liviu.dudau@arm.com> wrote:

> On Mon, Mar 04, 2024 at 10:08:11AM +0100, Boris Brezillon wrote:
> > Something on arm[64] must be including <asm/page.h>, but things fail
> > to compile on sparc64. Make sure this header is included explicitly
> > so this driver can be compile-tested on all supported architectures.  
> 
> Is compilation on sparc64 possible because of 'depends on COMPILE_TEST'?

Yes.

> Otherwise it doesn't make sense to try to build this for any arch other
> than arm[64].
> 
> Regardless, patch looks harmless, so
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Best regards,
> Liviu
> 
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202403031142.Vl4pW7X6-lkp@intel.com/
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_device.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> > index bfe8da4a6e4c..68e467ee458a 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.c
> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
> > @@ -3,6 +3,8 @@
> >  /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
> >  /* Copyright 2023 Collabora ltd. */
> >  
> > +#include <asm/page.h>
> > +
> >  #include <linux/clk.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_domain.h>
> > -- 
> > 2.43.0
> >   
> 

Re: [PATCH 0/3] drm/panthor: Fix 3 issues reported by the kernel test bot

[Boris Brezillon]

On Mon,  4 Mar 2024 10:08:09 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Hello,
> 
> Here are three trivial fixes for bugs reported by the intel kernel test
> bot.
> 
> Regards,
> 
> Boris
> 
> Boris Brezillon (3):
>   drm/panthor: Fix panthor_devfreq kerneldoc
>   drm/panthor: Explicitly include page.h for the {virt,__phys)_to_pfn()
>     defs
>   drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

Queued to drm-misc-next.

> 
>  drivers/gpu/drm/panthor/Kconfig           | 1 +
>  drivers/gpu/drm/panthor/panthor_devfreq.c | 2 +-
>  drivers/gpu/drm/panthor/panthor_device.c  | 4 ++--
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Jani Nikula]

On Mon, 04 Mar 2024, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> panthor_device_resume/suspend() are only compiled when CONFIG_PM is
> enabled but panthro_drv.c doesn't use the pm_ptr() macro to conditionally
> discard resume/suspend assignments, which causes undefined symbol
> errors at link time when !PM.
>
> We could fix that by using pm_ptr(), but supporting the !PM case makes
> little sense (the whole point of these embedded GPUs is to be low power,
> so proper PM is a basic requirement in that case). So let's just enforce
> the presence of CONFIG_PM with a Kconfig dependency instead.
>
> If someone needs to relax this dependency, it can be done in a follow-up.

This breaks the config for me:

  SYNC    include/config/auto.conf.cmd
  GEN     Makefile
drivers/iommu/Kconfig:14:error: recursive dependency detected!
drivers/iommu/Kconfig:14:	symbol IOMMU_SUPPORT is selected by DRM_PANTHOR
drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on PM
kernel/power/Kconfig:183:	symbol PM is selected by PM_SLEEP
kernel/power/Kconfig:117:	symbol PM_SLEEP depends on HIBERNATE_CALLBACKS
kernel/power/Kconfig:35:	symbol HIBERNATE_CALLBACKS is selected by XEN_SAVE_RESTORE
arch/x86/xen/Kconfig:67:	symbol XEN_SAVE_RESTORE depends on XEN
arch/x86/xen/Kconfig:6:	symbol XEN depends on PARAVIRT
arch/x86/Kconfig:781:	symbol PARAVIRT is selected by HYPERV
drivers/hv/Kconfig:5:	symbol HYPERV depends on X86_LOCAL_APIC
arch/x86/Kconfig:1106:	symbol X86_LOCAL_APIC depends on X86_UP_APIC
arch/x86/Kconfig:1081:	symbol X86_UP_APIC prompt is visible depending on PCI_MSI
drivers/pci/Kconfig:39:	symbol PCI_MSI is selected by AMD_IOMMU
drivers/iommu/amd/Kconfig:3:	symbol AMD_IOMMU depends on IOMMU_SUPPORT
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"


BR,
Jani.


>
> 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>
> ---
>  drivers/gpu/drm/panthor/Kconfig          | 1 +
>  drivers/gpu/drm/panthor/panthor_device.c | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig
> index 55b40ad07f3b..fdce7c1b2310 100644
> --- a/drivers/gpu/drm/panthor/Kconfig
> +++ b/drivers/gpu/drm/panthor/Kconfig
> @@ -6,6 +6,7 @@ config DRM_PANTHOR
>  	depends on ARM || ARM64 || COMPILE_TEST
>  	depends on !GENERIC_ATOMIC64  # for IOMMU_IO_PGTABLE_LPAE
>  	depends on MMU
> +	depends on PM
>  	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>  	select DRM_EXEC
>  	select DRM_GEM_SHMEM_HELPER
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 68e467ee458a..efea29143a54 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -403,7 +403,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);
> @@ -548,4 +547,3 @@ int panthor_device_suspend(struct device *dev)
>  	mutex_unlock(&ptdev->pm.mmio_lock);
>  	return ret;
>  }
> -#endif

-- 
Jani Nikula, Intel

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Boris Brezillon]

On Mon, 11 Mar 2024 13:05:01 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> This breaks the config for me:
> 
>   SYNC    include/config/auto.conf.cmd
>   GEN     Makefile
> drivers/iommu/Kconfig:14:error: recursive dependency detected!
> drivers/iommu/Kconfig:14:	symbol IOMMU_SUPPORT is selected by DRM_PANTHOR
> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on PM
> kernel/power/Kconfig:183:	symbol PM is selected by PM_SLEEP
> kernel/power/Kconfig:117:	symbol PM_SLEEP depends on HIBERNATE_CALLBACKS
> kernel/power/Kconfig:35:	symbol HIBERNATE_CALLBACKS is selected by XEN_SAVE_RESTORE
> arch/x86/xen/Kconfig:67:	symbol XEN_SAVE_RESTORE depends on XEN
> arch/x86/xen/Kconfig:6:	symbol XEN depends on PARAVIRT
> arch/x86/Kconfig:781:	symbol PARAVIRT is selected by HYPERV
> drivers/hv/Kconfig:5:	symbol HYPERV depends on X86_LOCAL_APIC
> arch/x86/Kconfig:1106:	symbol X86_LOCAL_APIC depends on X86_UP_APIC
> arch/x86/Kconfig:1081:	symbol X86_UP_APIC prompt is visible depending on PCI_MSI
> drivers/pci/Kconfig:39:	symbol PCI_MSI is selected by AMD_IOMMU
> drivers/iommu/amd/Kconfig:3:	symbol AMD_IOMMU depends on IOMMU_SUPPORT

Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
IOMMU_SUPPORT" in panthor then.

> For a resolution refer to Documentation/kbuild/kconfig-language.rst
> subsection "Kconfig recursive dependency limitations"

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Jani Nikula]

On Mon, 11 Mar 2024, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> On Mon, 11 Mar 2024 13:05:01 +0200
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> This breaks the config for me:
>> 
>>   SYNC    include/config/auto.conf.cmd
>>   GEN     Makefile
>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
>> drivers/iommu/Kconfig:14:	symbol IOMMU_SUPPORT is selected by DRM_PANTHOR
>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on PM
>> kernel/power/Kconfig:183:	symbol PM is selected by PM_SLEEP
>> kernel/power/Kconfig:117:	symbol PM_SLEEP depends on HIBERNATE_CALLBACKS
>> kernel/power/Kconfig:35:	symbol HIBERNATE_CALLBACKS is selected by XEN_SAVE_RESTORE
>> arch/x86/xen/Kconfig:67:	symbol XEN_SAVE_RESTORE depends on XEN
>> arch/x86/xen/Kconfig:6:	symbol XEN depends on PARAVIRT
>> arch/x86/Kconfig:781:	symbol PARAVIRT is selected by HYPERV
>> drivers/hv/Kconfig:5:	symbol HYPERV depends on X86_LOCAL_APIC
>> arch/x86/Kconfig:1106:	symbol X86_LOCAL_APIC depends on X86_UP_APIC
>> arch/x86/Kconfig:1081:	symbol X86_UP_APIC prompt is visible depending on PCI_MSI
>> drivers/pci/Kconfig:39:	symbol PCI_MSI is selected by AMD_IOMMU
>> drivers/iommu/amd/Kconfig:3:	symbol AMD_IOMMU depends on IOMMU_SUPPORT
>
> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
> IOMMU_SUPPORT" in panthor then.

That works for me.

>
>> For a resolution refer to Documentation/kbuild/kconfig-language.rst
>> subsection "Kconfig recursive dependency limitations"

-- 
Jani Nikula, Intel

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Boris Brezillon]

On Mon, 11 Mar 2024 13:49:56 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Mon, 11 Mar 2024, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > On Mon, 11 Mar 2024 13:05:01 +0200
> > Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >  
> >> This breaks the config for me:
> >> 
> >>   SYNC    include/config/auto.conf.cmd
> >>   GEN     Makefile
> >> drivers/iommu/Kconfig:14:error: recursive dependency detected!
> >> drivers/iommu/Kconfig:14:	symbol IOMMU_SUPPORT is selected by DRM_PANTHOR
> >> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on PM
> >> kernel/power/Kconfig:183:	symbol PM is selected by PM_SLEEP
> >> kernel/power/Kconfig:117:	symbol PM_SLEEP depends on HIBERNATE_CALLBACKS
> >> kernel/power/Kconfig:35:	symbol HIBERNATE_CALLBACKS is selected by XEN_SAVE_RESTORE
> >> arch/x86/xen/Kconfig:67:	symbol XEN_SAVE_RESTORE depends on XEN
> >> arch/x86/xen/Kconfig:6:	symbol XEN depends on PARAVIRT
> >> arch/x86/Kconfig:781:	symbol PARAVIRT is selected by HYPERV
> >> drivers/hv/Kconfig:5:	symbol HYPERV depends on X86_LOCAL_APIC
> >> arch/x86/Kconfig:1106:	symbol X86_LOCAL_APIC depends on X86_UP_APIC
> >> arch/x86/Kconfig:1081:	symbol X86_UP_APIC prompt is visible depending on PCI_MSI
> >> drivers/pci/Kconfig:39:	symbol PCI_MSI is selected by AMD_IOMMU
> >> drivers/iommu/amd/Kconfig:3:	symbol AMD_IOMMU depends on IOMMU_SUPPORT  
> >
> > Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
> > IOMMU_SUPPORT" in panthor then.  
> 
> That works for me.

Let's revert the faulty commit first. We'll see if Steve has a
different solution for the original issue.

> 
> >  
> >> For a resolution refer to Documentation/kbuild/kconfig-language.rst
> >> subsection "Kconfig recursive dependency limitations"  
> 

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Robin Murphy]

On 2024-03-11 11:52 am, Boris Brezillon wrote:
> On Mon, 11 Mar 2024 13:49:56 +0200
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> 
>> On Mon, 11 Mar 2024, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>> On Mon, 11 Mar 2024 13:05:01 +0200
>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>   
>>>> This breaks the config for me:
>>>>
>>>>    SYNC    include/config/auto.conf.cmd
>>>>    GEN     Makefile
>>>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
>>>> drivers/iommu/Kconfig:14:	symbol IOMMU_SUPPORT is selected by DRM_PANTHOR
>>>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on PM
>>>> kernel/power/Kconfig:183:	symbol PM is selected by PM_SLEEP
>>>> kernel/power/Kconfig:117:	symbol PM_SLEEP depends on HIBERNATE_CALLBACKS
>>>> kernel/power/Kconfig:35:	symbol HIBERNATE_CALLBACKS is selected by XEN_SAVE_RESTORE
>>>> arch/x86/xen/Kconfig:67:	symbol XEN_SAVE_RESTORE depends on XEN
>>>> arch/x86/xen/Kconfig:6:	symbol XEN depends on PARAVIRT
>>>> arch/x86/Kconfig:781:	symbol PARAVIRT is selected by HYPERV
>>>> drivers/hv/Kconfig:5:	symbol HYPERV depends on X86_LOCAL_APIC
>>>> arch/x86/Kconfig:1106:	symbol X86_LOCAL_APIC depends on X86_UP_APIC
>>>> arch/x86/Kconfig:1081:	symbol X86_UP_APIC prompt is visible depending on PCI_MSI
>>>> drivers/pci/Kconfig:39:	symbol PCI_MSI is selected by AMD_IOMMU
>>>> drivers/iommu/amd/Kconfig:3:	symbol AMD_IOMMU depends on IOMMU_SUPPORT
>>>
>>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
>>> IOMMU_SUPPORT" in panthor then.
>>
>> That works for me.
> 
> Let's revert the faulty commit first. We'll see if Steve has a
> different solution for the original issue.

FWIW, the reasoning in the offending commit seems incredibly tenuous. 
There are far more practical reasons for building an arm/arm64 kernel 
without PM - for debugging or whatever, and where one may even still 
want a usable GPU, let alone just a non-broken build - than there are 
for building this driver for x86. Using pm_ptr() is trivial, and if you 
want to support COMPILE_TEST then there's really no justifiable excuse 
not to.

Thanks,
Robin.

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Boris Brezillon]

On Mon, 11 Mar 2024 13:11:28 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 2024-03-11 11:52 am, Boris Brezillon wrote:
> > On Mon, 11 Mar 2024 13:49:56 +0200
> > Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >   
> >> On Mon, 11 Mar 2024, Boris Brezillon <boris.brezillon@collabora.com> wrote:  
> >>> On Mon, 11 Mar 2024 13:05:01 +0200
> >>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>     
> >>>> This breaks the config for me:
> >>>>
> >>>>    SYNC    include/config/auto.conf.cmd
> >>>>    GEN     Makefile
> >>>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
> >>>> drivers/iommu/Kconfig:14:	symbol IOMMU_SUPPORT is selected by DRM_PANTHOR
> >>>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on PM
> >>>> kernel/power/Kconfig:183:	symbol PM is selected by PM_SLEEP
> >>>> kernel/power/Kconfig:117:	symbol PM_SLEEP depends on HIBERNATE_CALLBACKS
> >>>> kernel/power/Kconfig:35:	symbol HIBERNATE_CALLBACKS is selected by XEN_SAVE_RESTORE
> >>>> arch/x86/xen/Kconfig:67:	symbol XEN_SAVE_RESTORE depends on XEN
> >>>> arch/x86/xen/Kconfig:6:	symbol XEN depends on PARAVIRT
> >>>> arch/x86/Kconfig:781:	symbol PARAVIRT is selected by HYPERV
> >>>> drivers/hv/Kconfig:5:	symbol HYPERV depends on X86_LOCAL_APIC
> >>>> arch/x86/Kconfig:1106:	symbol X86_LOCAL_APIC depends on X86_UP_APIC
> >>>> arch/x86/Kconfig:1081:	symbol X86_UP_APIC prompt is visible depending on PCI_MSI
> >>>> drivers/pci/Kconfig:39:	symbol PCI_MSI is selected by AMD_IOMMU
> >>>> drivers/iommu/amd/Kconfig:3:	symbol AMD_IOMMU depends on IOMMU_SUPPORT  
> >>>
> >>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
> >>> IOMMU_SUPPORT" in panthor then.  
> >>
> >> That works for me.  
> > 
> > Let's revert the faulty commit first. We'll see if Steve has a
> > different solution for the original issue.  
> 
> FWIW, the reasoning in the offending commit seems incredibly tenuous. 
> There are far more practical reasons for building an arm/arm64 kernel 
> without PM - for debugging or whatever, and where one may even still 
> want a usable GPU, let alone just a non-broken build - than there are 
> for building this driver for x86. Using pm_ptr() is trivial, and if you 
> want to support COMPILE_TEST then there's really no justifiable excuse 
> not to.

The problem is not just about using pm_ptr(), but also making sure
panthor_device_resume/suspend() are called called in the init/unplug
path when !PM, as I don't think the PM helpers automate that for us. I
was just aiming for a simple fix that wouldn't force me to test the !PM
case...

> 
> Thanks,
> Robin.

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Robin Murphy]

On 2024-03-11 1:22 pm, Boris Brezillon wrote:
> On Mon, 11 Mar 2024 13:11:28 +0000
> Robin Murphy <robin.murphy@arm.com> wrote:
> 
>> On 2024-03-11 11:52 am, Boris Brezillon wrote:
>>> On Mon, 11 Mar 2024 13:49:56 +0200
>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>    
>>>> On Mon, 11 Mar 2024, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>>>> On Mon, 11 Mar 2024 13:05:01 +0200
>>>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>>>      
>>>>>> This breaks the config for me:
>>>>>>
>>>>>>     SYNC    include/config/auto.conf.cmd
>>>>>>     GEN     Makefile
>>>>>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
>>>>>> drivers/iommu/Kconfig:14:	symbol IOMMU_SUPPORT is selected by DRM_PANTHOR
>>>>>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on PM
>>>>>> kernel/power/Kconfig:183:	symbol PM is selected by PM_SLEEP
>>>>>> kernel/power/Kconfig:117:	symbol PM_SLEEP depends on HIBERNATE_CALLBACKS
>>>>>> kernel/power/Kconfig:35:	symbol HIBERNATE_CALLBACKS is selected by XEN_SAVE_RESTORE
>>>>>> arch/x86/xen/Kconfig:67:	symbol XEN_SAVE_RESTORE depends on XEN
>>>>>> arch/x86/xen/Kconfig:6:	symbol XEN depends on PARAVIRT
>>>>>> arch/x86/Kconfig:781:	symbol PARAVIRT is selected by HYPERV
>>>>>> drivers/hv/Kconfig:5:	symbol HYPERV depends on X86_LOCAL_APIC
>>>>>> arch/x86/Kconfig:1106:	symbol X86_LOCAL_APIC depends on X86_UP_APIC
>>>>>> arch/x86/Kconfig:1081:	symbol X86_UP_APIC prompt is visible depending on PCI_MSI
>>>>>> drivers/pci/Kconfig:39:	symbol PCI_MSI is selected by AMD_IOMMU
>>>>>> drivers/iommu/amd/Kconfig:3:	symbol AMD_IOMMU depends on IOMMU_SUPPORT
>>>>>
>>>>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
>>>>> IOMMU_SUPPORT" in panthor then.
>>>>
>>>> That works for me.
>>>
>>> Let's revert the faulty commit first. We'll see if Steve has a
>>> different solution for the original issue.
>>
>> FWIW, the reasoning in the offending commit seems incredibly tenuous.
>> There are far more practical reasons for building an arm/arm64 kernel
>> without PM - for debugging or whatever, and where one may even still
>> want a usable GPU, let alone just a non-broken build - than there are
>> for building this driver for x86. Using pm_ptr() is trivial, and if you
>> want to support COMPILE_TEST then there's really no justifiable excuse
>> not to.
> 
> The problem is not just about using pm_ptr(), but also making sure
> panthor_device_resume/suspend() are called called in the init/unplug
> path when !PM, as I don't think the PM helpers automate that for us. I
> was just aiming for a simple fix that wouldn't force me to test the !PM
> case...
Fair enough, at worst we could always have a runtime check and refuse to 
probe in conditions we don't think are worth the bother of implementing 
fully-functional support for. However if we want to make an argument for 
only supporting "realistic" configs at build time then that is an 
argument for dropping COMPILE_TEST as well.

Thanks,
Robin.

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Steven Price]

On 11/03/2024 13:36, Robin Murphy wrote:
> On 2024-03-11 1:22 pm, Boris Brezillon wrote:
>> On Mon, 11 Mar 2024 13:11:28 +0000
>> Robin Murphy <robin.murphy@arm.com> wrote:
>>
>>> On 2024-03-11 11:52 am, Boris Brezillon wrote:
>>>> On Mon, 11 Mar 2024 13:49:56 +0200
>>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>>   
>>>>> On Mon, 11 Mar 2024, Boris Brezillon
>>>>> <boris.brezillon@collabora.com> wrote:
>>>>>> On Mon, 11 Mar 2024 13:05:01 +0200
>>>>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>>>>     
>>>>>>> This breaks the config for me:
>>>>>>>
>>>>>>>     SYNC    include/config/auto.conf.cmd
>>>>>>>     GEN     Makefile
>>>>>>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
>>>>>>> drivers/iommu/Kconfig:14:    symbol IOMMU_SUPPORT is selected by
>>>>>>> DRM_PANTHOR
>>>>>>> drivers/gpu/drm/panthor/Kconfig:3:    symbol DRM_PANTHOR depends
>>>>>>> on PM
>>>>>>> kernel/power/Kconfig:183:    symbol PM is selected by PM_SLEEP
>>>>>>> kernel/power/Kconfig:117:    symbol PM_SLEEP depends on
>>>>>>> HIBERNATE_CALLBACKS
>>>>>>> kernel/power/Kconfig:35:    symbol HIBERNATE_CALLBACKS is
>>>>>>> selected by XEN_SAVE_RESTORE
>>>>>>> arch/x86/xen/Kconfig:67:    symbol XEN_SAVE_RESTORE depends on XEN
>>>>>>> arch/x86/xen/Kconfig:6:    symbol XEN depends on PARAVIRT
>>>>>>> arch/x86/Kconfig:781:    symbol PARAVIRT is selected by HYPERV
>>>>>>> drivers/hv/Kconfig:5:    symbol HYPERV depends on X86_LOCAL_APIC
>>>>>>> arch/x86/Kconfig:1106:    symbol X86_LOCAL_APIC depends on
>>>>>>> X86_UP_APIC
>>>>>>> arch/x86/Kconfig:1081:    symbol X86_UP_APIC prompt is visible
>>>>>>> depending on PCI_MSI
>>>>>>> drivers/pci/Kconfig:39:    symbol PCI_MSI is selected by AMD_IOMMU
>>>>>>> drivers/iommu/amd/Kconfig:3:    symbol AMD_IOMMU depends on
>>>>>>> IOMMU_SUPPORT
>>>>>>
>>>>>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
>>>>>> IOMMU_SUPPORT" in panthor then.
>>>>>
>>>>> That works for me.
>>>>
>>>> Let's revert the faulty commit first. We'll see if Steve has a
>>>> different solution for the original issue.
>>>
>>> FWIW, the reasoning in the offending commit seems incredibly tenuous.
>>> There are far more practical reasons for building an arm/arm64 kernel
>>> without PM - for debugging or whatever, and where one may even still
>>> want a usable GPU, let alone just a non-broken build - than there are
>>> for building this driver for x86. Using pm_ptr() is trivial, and if you
>>> want to support COMPILE_TEST then there's really no justifiable excuse
>>> not to.
>>
>> The problem is not just about using pm_ptr(), but also making sure
>> panthor_device_resume/suspend() are called called in the init/unplug
>> path when !PM, as I don't think the PM helpers automate that for us. I
>> was just aiming for a simple fix that wouldn't force me to test the !PM
>> case...
> Fair enough, at worst we could always have a runtime check and refuse to
> probe in conditions we don't think are worth the bother of implementing
> fully-functional support for. However if we want to make an argument for
> only supporting "realistic" configs at build time then that is an
> argument for dropping COMPILE_TEST as well.

Can we just replace the "depends on PM" with "select PM"? In my
(admittedly very limited) testing this works. Otherwise I think we need
to bite the bullet and support !PM in some way (maybe just as Robin
suggests with a runtime bail out).

Steve

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Boris Brezillon]

On Mon, 11 Mar 2024 13:46:23 +0000
Steven Price <steven.price@arm.com> wrote:

> On 11/03/2024 13:36, Robin Murphy wrote:
> > On 2024-03-11 1:22 pm, Boris Brezillon wrote:  
> >> On Mon, 11 Mar 2024 13:11:28 +0000
> >> Robin Murphy <robin.murphy@arm.com> wrote:
> >>  
> >>> On 2024-03-11 11:52 am, Boris Brezillon wrote:  
> >>>> On Mon, 11 Mar 2024 13:49:56 +0200
> >>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>>     
> >>>>> On Mon, 11 Mar 2024, Boris Brezillon
> >>>>> <boris.brezillon@collabora.com> wrote:  
> >>>>>> On Mon, 11 Mar 2024 13:05:01 +0200
> >>>>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>>>>       
> >>>>>>> This breaks the config for me:
> >>>>>>>
> >>>>>>>     SYNC    include/config/auto.conf.cmd
> >>>>>>>     GEN     Makefile
> >>>>>>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
> >>>>>>> drivers/iommu/Kconfig:14:    symbol IOMMU_SUPPORT is selected by
> >>>>>>> DRM_PANTHOR
> >>>>>>> drivers/gpu/drm/panthor/Kconfig:3:    symbol DRM_PANTHOR depends
> >>>>>>> on PM
> >>>>>>> kernel/power/Kconfig:183:    symbol PM is selected by PM_SLEEP
> >>>>>>> kernel/power/Kconfig:117:    symbol PM_SLEEP depends on
> >>>>>>> HIBERNATE_CALLBACKS
> >>>>>>> kernel/power/Kconfig:35:    symbol HIBERNATE_CALLBACKS is
> >>>>>>> selected by XEN_SAVE_RESTORE
> >>>>>>> arch/x86/xen/Kconfig:67:    symbol XEN_SAVE_RESTORE depends on XEN
> >>>>>>> arch/x86/xen/Kconfig:6:    symbol XEN depends on PARAVIRT
> >>>>>>> arch/x86/Kconfig:781:    symbol PARAVIRT is selected by HYPERV
> >>>>>>> drivers/hv/Kconfig:5:    symbol HYPERV depends on X86_LOCAL_APIC
> >>>>>>> arch/x86/Kconfig:1106:    symbol X86_LOCAL_APIC depends on
> >>>>>>> X86_UP_APIC
> >>>>>>> arch/x86/Kconfig:1081:    symbol X86_UP_APIC prompt is visible
> >>>>>>> depending on PCI_MSI
> >>>>>>> drivers/pci/Kconfig:39:    symbol PCI_MSI is selected by AMD_IOMMU
> >>>>>>> drivers/iommu/amd/Kconfig:3:    symbol AMD_IOMMU depends on
> >>>>>>> IOMMU_SUPPORT  
> >>>>>>
> >>>>>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
> >>>>>> IOMMU_SUPPORT" in panthor then.  
> >>>>>
> >>>>> That works for me.  
> >>>>
> >>>> Let's revert the faulty commit first. We'll see if Steve has a
> >>>> different solution for the original issue.  
> >>>
> >>> FWIW, the reasoning in the offending commit seems incredibly tenuous.
> >>> There are far more practical reasons for building an arm/arm64 kernel
> >>> without PM - for debugging or whatever, and where one may even still
> >>> want a usable GPU, let alone just a non-broken build - than there are
> >>> for building this driver for x86. Using pm_ptr() is trivial, and if you
> >>> want to support COMPILE_TEST then there's really no justifiable excuse
> >>> not to.  
> >>
> >> The problem is not just about using pm_ptr(), but also making sure
> >> panthor_device_resume/suspend() are called called in the init/unplug
> >> path when !PM, as I don't think the PM helpers automate that for us. I
> >> was just aiming for a simple fix that wouldn't force me to test the !PM
> >> case...  
> > Fair enough, at worst we could always have a runtime check and refuse to
> > probe in conditions we don't think are worth the bother of implementing
> > fully-functional support for. However if we want to make an argument for
> > only supporting "realistic" configs at build time then that is an
> > argument for dropping COMPILE_TEST as well.  
> 
> Can we just replace the "depends on PM" with "select PM"? In my
> (admittedly very limited) testing this works. Otherwise I think we need
> to bite the bullet and support !PM in some way (maybe just as Robin
> suggests with a runtime bail out).

I won't have time to test it this week, but if someone is interested,
here's a diff implementing manual resume/suspend in the init/unplug
path:

--->8---

Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Boris Brezillon]

On Mon, 11 Mar 2024 14:58:37 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Mon, 11 Mar 2024 13:46:23 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
> > On 11/03/2024 13:36, Robin Murphy wrote:  
> > > On 2024-03-11 1:22 pm, Boris Brezillon wrote:    
> > >> On Mon, 11 Mar 2024 13:11:28 +0000
> > >> Robin Murphy <robin.murphy@arm.com> wrote:
> > >>    
> > >>> On 2024-03-11 11:52 am, Boris Brezillon wrote:    
> > >>>> On Mon, 11 Mar 2024 13:49:56 +0200
> > >>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >>>>       
> > >>>>> On Mon, 11 Mar 2024, Boris Brezillon
> > >>>>> <boris.brezillon@collabora.com> wrote:    
> > >>>>>> On Mon, 11 Mar 2024 13:05:01 +0200
> > >>>>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >>>>>>         
> > >>>>>>> This breaks the config for me:
> > >>>>>>>
> > >>>>>>>     SYNC    include/config/auto.conf.cmd
> > >>>>>>>     GEN     Makefile
> > >>>>>>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
> > >>>>>>> drivers/iommu/Kconfig:14:    symbol IOMMU_SUPPORT is selected by
> > >>>>>>> DRM_PANTHOR
> > >>>>>>> drivers/gpu/drm/panthor/Kconfig:3:    symbol DRM_PANTHOR depends
> > >>>>>>> on PM
> > >>>>>>> kernel/power/Kconfig:183:    symbol PM is selected by PM_SLEEP
> > >>>>>>> kernel/power/Kconfig:117:    symbol PM_SLEEP depends on
> > >>>>>>> HIBERNATE_CALLBACKS
> > >>>>>>> kernel/power/Kconfig:35:    symbol HIBERNATE_CALLBACKS is
> > >>>>>>> selected by XEN_SAVE_RESTORE
> > >>>>>>> arch/x86/xen/Kconfig:67:    symbol XEN_SAVE_RESTORE depends on XEN
> > >>>>>>> arch/x86/xen/Kconfig:6:    symbol XEN depends on PARAVIRT
> > >>>>>>> arch/x86/Kconfig:781:    symbol PARAVIRT is selected by HYPERV
> > >>>>>>> drivers/hv/Kconfig:5:    symbol HYPERV depends on X86_LOCAL_APIC
> > >>>>>>> arch/x86/Kconfig:1106:    symbol X86_LOCAL_APIC depends on
> > >>>>>>> X86_UP_APIC
> > >>>>>>> arch/x86/Kconfig:1081:    symbol X86_UP_APIC prompt is visible
> > >>>>>>> depending on PCI_MSI
> > >>>>>>> drivers/pci/Kconfig:39:    symbol PCI_MSI is selected by AMD_IOMMU
> > >>>>>>> drivers/iommu/amd/Kconfig:3:    symbol AMD_IOMMU depends on
> > >>>>>>> IOMMU_SUPPORT    
> > >>>>>>
> > >>>>>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
> > >>>>>> IOMMU_SUPPORT" in panthor then.    
> > >>>>>
> > >>>>> That works for me.    
> > >>>>
> > >>>> Let's revert the faulty commit first. We'll see if Steve has a
> > >>>> different solution for the original issue.    
> > >>>
> > >>> FWIW, the reasoning in the offending commit seems incredibly tenuous.
> > >>> There are far more practical reasons for building an arm/arm64 kernel
> > >>> without PM - for debugging or whatever, and where one may even still
> > >>> want a usable GPU, let alone just a non-broken build - than there are
> > >>> for building this driver for x86. Using pm_ptr() is trivial, and if you
> > >>> want to support COMPILE_TEST then there's really no justifiable excuse
> > >>> not to.    
> > >>
> > >> The problem is not just about using pm_ptr(), but also making sure
> > >> panthor_device_resume/suspend() are called called in the init/unplug
> > >> path when !PM, as I don't think the PM helpers automate that for us. I
> > >> was just aiming for a simple fix that wouldn't force me to test the !PM
> > >> case...    
> > > Fair enough, at worst we could always have a runtime check and refuse to
> > > probe in conditions we don't think are worth the bother of implementing
> > > fully-functional support for. However if we want to make an argument for
> > > only supporting "realistic" configs at build time then that is an
> > > argument for dropping COMPILE_TEST as well.    
> > 
> > Can we just replace the "depends on PM" with "select PM"? In my
> > (admittedly very limited) testing this works. Otherwise I think we need
> > to bite the bullet and support !PM in some way (maybe just as Robin
> > suggests with a runtime bail out).  
> 
> I won't have time to test it this week, but if someone is interested,
> here's a diff implementing manual resume/suspend in the init/unplug
> path:
> 
> --->8---  

This time with the diff :-)

--->8---
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 69deb8e17778..3d05e7358f0e 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;
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 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Boris Brezillon]

On Mon, 11 Mar 2024 14:59:39 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Mon, 11 Mar 2024 14:58:37 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Mon, 11 Mar 2024 13:46:23 +0000
> > Steven Price <steven.price@arm.com> wrote:
> >   
> > > On 11/03/2024 13:36, Robin Murphy wrote:    
> > > > On 2024-03-11 1:22 pm, Boris Brezillon wrote:      
> > > >> On Mon, 11 Mar 2024 13:11:28 +0000
> > > >> Robin Murphy <robin.murphy@arm.com> wrote:
> > > >>      
> > > >>> On 2024-03-11 11:52 am, Boris Brezillon wrote:      
> > > >>>> On Mon, 11 Mar 2024 13:49:56 +0200
> > > >>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > >>>>         
> > > >>>>> On Mon, 11 Mar 2024, Boris Brezillon
> > > >>>>> <boris.brezillon@collabora.com> wrote:      
> > > >>>>>> On Mon, 11 Mar 2024 13:05:01 +0200
> > > >>>>>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > >>>>>>           
> > > >>>>>>> This breaks the config for me:
> > > >>>>>>>
> > > >>>>>>>     SYNC    include/config/auto.conf.cmd
> > > >>>>>>>     GEN     Makefile
> > > >>>>>>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
> > > >>>>>>> drivers/iommu/Kconfig:14:    symbol IOMMU_SUPPORT is selected by
> > > >>>>>>> DRM_PANTHOR
> > > >>>>>>> drivers/gpu/drm/panthor/Kconfig:3:    symbol DRM_PANTHOR depends
> > > >>>>>>> on PM
> > > >>>>>>> kernel/power/Kconfig:183:    symbol PM is selected by PM_SLEEP
> > > >>>>>>> kernel/power/Kconfig:117:    symbol PM_SLEEP depends on
> > > >>>>>>> HIBERNATE_CALLBACKS
> > > >>>>>>> kernel/power/Kconfig:35:    symbol HIBERNATE_CALLBACKS is
> > > >>>>>>> selected by XEN_SAVE_RESTORE
> > > >>>>>>> arch/x86/xen/Kconfig:67:    symbol XEN_SAVE_RESTORE depends on XEN
> > > >>>>>>> arch/x86/xen/Kconfig:6:    symbol XEN depends on PARAVIRT
> > > >>>>>>> arch/x86/Kconfig:781:    symbol PARAVIRT is selected by HYPERV
> > > >>>>>>> drivers/hv/Kconfig:5:    symbol HYPERV depends on X86_LOCAL_APIC
> > > >>>>>>> arch/x86/Kconfig:1106:    symbol X86_LOCAL_APIC depends on
> > > >>>>>>> X86_UP_APIC
> > > >>>>>>> arch/x86/Kconfig:1081:    symbol X86_UP_APIC prompt is visible
> > > >>>>>>> depending on PCI_MSI
> > > >>>>>>> drivers/pci/Kconfig:39:    symbol PCI_MSI is selected by AMD_IOMMU
> > > >>>>>>> drivers/iommu/amd/Kconfig:3:    symbol AMD_IOMMU depends on
> > > >>>>>>> IOMMU_SUPPORT      
> > > >>>>>>
> > > >>>>>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
> > > >>>>>> IOMMU_SUPPORT" in panthor then.      
> > > >>>>>
> > > >>>>> That works for me.      
> > > >>>>
> > > >>>> Let's revert the faulty commit first. We'll see if Steve has a
> > > >>>> different solution for the original issue.      
> > > >>>
> > > >>> FWIW, the reasoning in the offending commit seems incredibly tenuous.
> > > >>> There are far more practical reasons for building an arm/arm64 kernel
> > > >>> without PM - for debugging or whatever, and where one may even still
> > > >>> want a usable GPU, let alone just a non-broken build - than there are
> > > >>> for building this driver for x86. Using pm_ptr() is trivial, and if you
> > > >>> want to support COMPILE_TEST then there's really no justifiable excuse
> > > >>> not to.      
> > > >>
> > > >> The problem is not just about using pm_ptr(), but also making sure
> > > >> panthor_device_resume/suspend() are called called in the init/unplug
> > > >> path when !PM, as I don't think the PM helpers automate that for us. I
> > > >> was just aiming for a simple fix that wouldn't force me to test the !PM
> > > >> case...      
> > > > Fair enough, at worst we could always have a runtime check and refuse to
> > > > probe in conditions we don't think are worth the bother of implementing
> > > > fully-functional support for. However if we want to make an argument for
> > > > only supporting "realistic" configs at build time then that is an
> > > > argument for dropping COMPILE_TEST as well.      
> > > 
> > > Can we just replace the "depends on PM" with "select PM"? In my
> > > (admittedly very limited) testing this works. Otherwise I think we need
> > > to bite the bullet and support !PM in some way (maybe just as Robin
> > > suggests with a runtime bail out).    
> > 
> > I won't have time to test it this week, but if someone is interested,
> > here's a diff implementing manual resume/suspend in the init/unplug
> > path:
> >   
> > --->8---    
> 
> This time with the diff :-)
> 
> --->8---  
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 69deb8e17778..3d05e7358f0e 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;

And I forgot to remove the #ifdef CONFIG_PM in panthor_device.c.

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