| Message ID | 20240318085855.994179-1-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: Fix the CONFIG_PM=n case | expand |
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 >
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, > }, > };
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, > }, > };
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, > > }, > > };
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 :-).
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
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.
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?
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
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
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
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, }, };
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(-)