Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
On 18/03/2024 14:49, Boris Brezillon wrote: > On Mon, 18 Mar 2024 14:34:07 + > Steven Price wrote: > >> On 18/03/2024 14:18, Boris Brezillon wrote: >>> On Mon, 18 Mar 2024 13:49:52 + >>> Steven Price wrote: >>> On 18/03/2024 13:08, Boris Brezillon wrote: > On Mon, 18 Mar 2024 11:31:05 + > Steven Price 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 >>> Closes: >>> https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/ >>> Signed-off-by: Boris Brezillon >> >> Reviewed-by: Steven Price >> >>> --- >>> 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 >
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
On Mon, 18 Mar 2024 14:34:07 + Steven Price wrote: > On 18/03/2024 14:18, Boris Brezillon wrote: > > On Mon, 18 Mar 2024 13:49:52 + > > Steven Price wrote: > > > >> On 18/03/2024 13:08, Boris Brezillon wrote: > >>> On Mon, 18 Mar 2024 11:31:05 + > >>> Steven Price 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 > > Closes: > > https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/ > > Signed-off-by: Boris Brezillon > > Reviewed-by: Steven Price > > > --- > > Tested by faking CONFIG_PM=n in the driver (basically commenting > > all pm_runtime calls, and making the panthor_device_suspend/resume() > > calls unconditional in the panthor_device_unplug/init() path) since > > CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I > > can't be 100% sure this will work correctly on a platform that has > > CONFIG_PM=n. > > The same - I can't test this properly :( > > Note that the other option (which AFAICT doesn't cause any problems) is > to "select PM" rather than depend on it - AIUI the 'select' dependency > is considered in the opposite direction by kconfig so won't cause the > dependency loop. > >>> > >>> Doesn't seem to work with COMPILE_TEST though? I mean, we need > >>> something like > >>> > >>> depends on ARM || ARM64 || (COMPILE_TEST && PM) > >>> ... > >>> select PM > >>> > >>> but kconfig doesn't like that > >> > >> Why do we need the "&& PM" part? Just: > >> > >>depends on ARM || ARM64 || COMPILE_TEST > >>... > >>select PM > >> > >> Or at least that appears to work for me. > > > > Uh, you're right, sorry for the brain fart. This is being said, I > > see no other driver selecting the PM option directly (if you grep for > > 'select PM' in drivers/, you'll find occurrences in drivers/soc, but > > those are under ARCH_/SOC_ options, which means they are indirectly > > arch/platform dependent, not driver dependent). I'm really not sure > > selecting PM here from a driver is right to be honest. > > Yeah, I'm not very convinced about that either. It's just a pain that > the code is going to go untested. If that's really bothering you, let's just return an error in the probe path when CONFIG_PM=n as you were suggesting last week. > > >> > >>> drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected! > >>> drivers/gpu/drm/panthor/Kconfig:3:symbol DRM_PANTHOR depends on > >>> PM kernel/power/Kconfig:183: symbol PM is selected by DRM_PANTHOR > >>> > >>> which id why I initially when for a depends on PM > >>> > >>> > Of course if there is actually anyone who has a > platform which can be built !CONFIG_PM then that won't help. But the > inability of anyone to actually properly test this configuration does > worry me a little. > >>> > >>> Well, as long as it doesn't regress the PM behavior, I think I'm happy > >>> to take the risk. Worst case scenario, someone complains that this is > >>> not working properly when they do the !PM bringup :-). > >> > >> Indeed, I've no objection to this patch - although I really should have > >> compiled tested it as Robin pointed out ;) > >> > >> But one other thing I've noticed when compile testing it - we don't > >> appear to have fully fixed the virt_to_pfn() problem. On x86 with > >> COMPILE_TEST I still get an error. Looking at the code it appears that > >> virt_to_pfn() isn't available on x86... it overrides asm/page.h and > >> doesn't provide a definition. The definition on x86 is hiding in > >> asm/xen/page.h. > > > > Looks like the kbuild bot didn't catch that yet :-). > > > >> > >> Outside of arch code it's only drivers/xen that currently uses that > >> function. So I guess it's probably best to do a > >> PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :) > > > > Mind sending a fix for that? > > Yeah, I'll have a go at Robin's suggestion of storing the struct page > instead. If that's not too much to ask, could you also send a variant of this patch returning an error when CONFIG_PM is disabled (and fixing the other mistake I made, of course)? Thanks, Boris
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
On 18/03/2024 14:18, Boris Brezillon wrote: > On Mon, 18 Mar 2024 13:49:52 + > Steven Price wrote: > >> On 18/03/2024 13:08, Boris Brezillon wrote: >>> On Mon, 18 Mar 2024 11:31:05 + >>> Steven Price 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 > Closes: > https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/ > Signed-off-by: Boris Brezillon Reviewed-by: Steven Price > --- > Tested by faking CONFIG_PM=n in the driver (basically commenting > all pm_runtime calls, and making the panthor_device_suspend/resume() > calls unconditional in the panthor_device_unplug/init() path) since > CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I > can't be 100% sure this will work correctly on a platform that has > CONFIG_PM=n. The same - I can't test this properly :( Note that the other option (which AFAICT doesn't cause any problems) is to "select PM" rather than depend on it - AIUI the 'select' dependency is considered in the opposite direction by kconfig so won't cause the dependency loop. >>> >>> Doesn't seem to work with COMPILE_TEST though? I mean, we need >>> something like >>> >>> depends on ARM || ARM64 || (COMPILE_TEST && PM) >>> ... >>> select PM >>> >>> but kconfig doesn't like that >> >> Why do we need the "&& PM" part? Just: >> >> depends on ARM || ARM64 || COMPILE_TEST >> ... >> select PM >> >> Or at least that appears to work for me. > > Uh, you're right, sorry for the brain fart. This is being said, I > see no other driver selecting the PM option directly (if you grep for > 'select PM' in drivers/, you'll find occurrences in drivers/soc, but > those are under ARCH_/SOC_ options, which means they are indirectly > arch/platform dependent, not driver dependent). I'm really not sure > selecting PM here from a driver is right to be honest. Yeah, I'm not very convinced about that either. It's just a pain that the code is going to go untested. >> >>> drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected! >>> drivers/gpu/drm/panthor/Kconfig:3: symbol DRM_PANTHOR depends on >>> PM kernel/power/Kconfig:183:symbol PM is selected by DRM_PANTHOR >>> >>> which id why I initially when for a depends on PM >>> >>> Of course if there is actually anyone who has a platform which can be built !CONFIG_PM then that won't help. But the inability of anyone to actually properly test this configuration does worry me a little. >>> >>> Well, as long as it doesn't regress the PM behavior, I think I'm happy >>> to take the risk. Worst case scenario, someone complains that this is >>> not working properly when they do the !PM bringup :-). >> >> Indeed, I've no objection to this patch - although I really should have >> compiled tested it as Robin pointed out ;) >> >> But one other thing I've noticed when compile testing it - we don't >> appear to have fully fixed the virt_to_pfn() problem. On x86 with >> COMPILE_TEST I still get an error. Looking at the code it appears that >> virt_to_pfn() isn't available on x86... it overrides asm/page.h and >> doesn't provide a definition. The definition on x86 is hiding in >> asm/xen/page.h. > > Looks like the kbuild bot didn't catch that yet :-). > >> >> Outside of arch code it's only drivers/xen that currently uses that >> function. So I guess it's probably best to do a >> PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :) > > Mind sending a fix for that? Yeah, I'll have a go at Robin's suggestion of storing the struct page instead. Steve
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
On Mon, 18 Mar 2024 13:49:52 + Steven Price wrote: > On 18/03/2024 13:08, Boris Brezillon wrote: > > On Mon, 18 Mar 2024 11:31:05 + > > Steven Price 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 > >>> Closes: > >>> https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/ > >>> Signed-off-by: Boris Brezillon > >> > >> Reviewed-by: Steven Price > >> > >>> --- > >>> Tested by faking CONFIG_PM=n in the driver (basically commenting > >>> all pm_runtime calls, and making the panthor_device_suspend/resume() > >>> calls unconditional in the panthor_device_unplug/init() path) since > >>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I > >>> can't be 100% sure this will work correctly on a platform that has > >>> CONFIG_PM=n. > >> > >> The same - I can't test this properly :( > >> > >> Note that the other option (which AFAICT doesn't cause any problems) is > >> to "select PM" rather than depend on it - AIUI the 'select' dependency > >> is considered in the opposite direction by kconfig so won't cause the > >> dependency loop. > > > > Doesn't seem to work with COMPILE_TEST though? I mean, we need > > something like > > > > depends on ARM || ARM64 || (COMPILE_TEST && PM) > > ... > > select PM > > > > but kconfig doesn't like that > > Why do we need the "&& PM" part? Just: > > depends on ARM || ARM64 || COMPILE_TEST > ... > select PM > > Or at least that appears to work for me. Uh, you're right, sorry for the brain fart. This is being said, I see no other driver selecting the PM option directly (if you grep for 'select PM' in drivers/, you'll find occurrences in drivers/soc, but those are under ARCH_/SOC_ options, which means they are indirectly arch/platform dependent, not driver dependent). I'm really not sure selecting PM here from a driver is right to be honest. > > > drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected! > > drivers/gpu/drm/panthor/Kconfig:3: symbol DRM_PANTHOR depends on > > PM kernel/power/Kconfig:183:symbol PM is selected by DRM_PANTHOR > > > > which id why I initially when for a depends on PM > > > > > >> Of course if there is actually anyone who has a > >> platform which can be built !CONFIG_PM then that won't help. But the > >> inability of anyone to actually properly test this configuration does > >> worry me a little. > > > > Well, as long as it doesn't regress the PM behavior, I think I'm happy > > to take the risk. Worst case scenario, someone complains that this is > > not working properly when they do the !PM bringup :-). > > Indeed, I've no objection to this patch - although I really should have > compiled tested it as Robin pointed out ;) > > But one other thing I've noticed when compile testing it - we don't > appear to have fully fixed the virt_to_pfn() problem. On x86 with > COMPILE_TEST I still get an error. Looking at the code it appears that > virt_to_pfn() isn't available on x86... it overrides asm/page.h and > doesn't provide a definition. The definition on x86 is hiding in > asm/xen/page.h. Looks like the kbuild bot didn't catch that yet :-). > > Outside of arch code it's only drivers/xen that currently uses that > function. So I guess it's probably best to do a > PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :) Mind sending a fix for that?
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
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 + Steven Price 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 Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/ Signed-off-by: Boris Brezillon Reviewed-by: Steven Price --- Tested by faking CONFIG_PM=n in the driver (basically commenting all pm_runtime calls, and making the panthor_device_suspend/resume() calls unconditional in the panthor_device_unplug/init() path) since CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I can't be 100% sure this will work correctly on a platform that has CONFIG_PM=n. The same - I can't test this properly :( Note that the other option (which AFAICT doesn't cause any problems) is to "select PM" rather than depend on it - AIUI the 'select' dependency is considered in the opposite direction by kconfig so won't cause the dependency loop. Doesn't seem to work with COMPILE_TEST though? I mean, we need something like depends on ARM || ARM64 || (COMPILE_TEST && PM) ... select PM but kconfig doesn't like that Why do we need the "&& PM" part? Just: depends on ARM || ARM64 || COMPILE_TEST ... select PM Or at least that appears to work for me. drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected! drivers/gpu/drm/panthor/Kconfig:3: symbol DRM_PANTHOR depends on PM kernel/power/Kconfig:183:symbol PM is selected by DRM_PANTHOR which id why I initially when for a depends on PM Of course if there is actually anyone who has a platform which can be built !CONFIG_PM then that won't help. But the inability of anyone to actually properly test this configuration does worry me a little. Well, as long as it doesn't regress the PM behavior, I think I'm happy to take the risk. Worst case scenario, someone complains that this is not working properly when they do the !PM bringup :-). Indeed, I've no objection to this patch - although I really should have compiled tested it as Robin pointed out ;) But one other thing I've noticed when compile testing it - we don't appear to have fully fixed the virt_to_pfn() problem. On x86 with COMPILE_TEST I still get an error. Looking at the code it appears that virt_to_pfn() isn't available on x86... it overrides asm/page.h and doesn't provide a definition. The definition on x86 is hiding in asm/xen/page.h. Outside of arch code it's only drivers/xen that currently uses that function. So I guess it's probably best to do a PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :) FWIW from a quick look it might be cleaner to store the struct page pointer for the dummy page - especially since the VA only seems to be used once in panthor_device_init() anyway - then use page_to_pfn() at the business end. Cheers, Robin.
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
On 18/03/2024 13:08, Boris Brezillon wrote: > On Mon, 18 Mar 2024 11:31:05 + > Steven Price 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 >>> Closes: >>> https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/ >>> Signed-off-by: Boris Brezillon >> >> Reviewed-by: Steven Price >> >>> --- >>> Tested by faking CONFIG_PM=n in the driver (basically commenting >>> all pm_runtime calls, and making the panthor_device_suspend/resume() >>> calls unconditional in the panthor_device_unplug/init() path) since >>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I >>> can't be 100% sure this will work correctly on a platform that has >>> CONFIG_PM=n. >> >> The same - I can't test this properly :( >> >> Note that the other option (which AFAICT doesn't cause any problems) is >> to "select PM" rather than depend on it - AIUI the 'select' dependency >> is considered in the opposite direction by kconfig so won't cause the >> dependency loop. > > Doesn't seem to work with COMPILE_TEST though? I mean, we need > something like > > depends on ARM || ARM64 || (COMPILE_TEST && PM) > ... > select PM > > but kconfig doesn't like that Why do we need the "&& PM" part? Just: depends on ARM || ARM64 || COMPILE_TEST ... select PM Or at least that appears to work for me. > drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected! > drivers/gpu/drm/panthor/Kconfig:3:symbol DRM_PANTHOR depends on > PM kernel/power/Kconfig:183: symbol PM is selected by DRM_PANTHOR > > which id why I initially when for a depends on PM > > >> Of course if there is actually anyone who has a >> platform which can be built !CONFIG_PM then that won't help. But the >> inability of anyone to actually properly test this configuration does >> worry me a little. > > Well, as long as it doesn't regress the PM behavior, I think I'm happy > to take the risk. Worst case scenario, someone complains that this is > not working properly when they do the !PM bringup :-). Indeed, I've no objection to this patch - although I really should have compiled tested it as Robin pointed out ;) But one other thing I've noticed when compile testing it - we don't appear to have fully fixed the virt_to_pfn() problem. On x86 with COMPILE_TEST I still get an error. Looking at the code it appears that virt_to_pfn() isn't available on x86... it overrides asm/page.h and doesn't provide a definition. The definition on x86 is hiding in asm/xen/page.h. Outside of arch code it's only drivers/xen that currently uses that function. So I guess it's probably best to do a PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :) Steve
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
On Mon, 18 Mar 2024 11:31:05 + Steven Price 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 > > Closes: > > https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/ > > Signed-off-by: Boris Brezillon > > Reviewed-by: Steven Price > > > --- > > Tested by faking CONFIG_PM=n in the driver (basically commenting > > all pm_runtime calls, and making the panthor_device_suspend/resume() > > calls unconditional in the panthor_device_unplug/init() path) since > > CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I > > can't be 100% sure this will work correctly on a platform that has > > CONFIG_PM=n. > > The same - I can't test this properly :( > > Note that the other option (which AFAICT doesn't cause any problems) is > to "select PM" rather than depend on it - AIUI the 'select' dependency > is considered in the opposite direction by kconfig so won't cause the > dependency loop. Doesn't seem to work with COMPILE_TEST though? I mean, we need something like depends on ARM || ARM64 || (COMPILE_TEST && PM) ... select PM but kconfig doesn't like that drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected! drivers/gpu/drm/panthor/Kconfig:3: symbol DRM_PANTHOR depends on PM kernel/power/Kconfig:183:symbol PM is selected by DRM_PANTHOR which id why I initially when for a depends on PM > Of course if there is actually anyone who has a > platform which can be built !CONFIG_PM then that won't help. But the > inability of anyone to actually properly test this configuration does > worry me a little. Well, as long as it doesn't regress the PM behavior, I think I'm happy to take the risk. Worst case scenario, someone complains that this is not working properly when they do the !PM bringup :-).
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
On Mon, 18 Mar 2024 12:18:53 +
Robin Murphy 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
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
> > Signed-off-by: Boris Brezillon
> > ---
> > 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(>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 = _pm_ops,
> > + .pm = pm_ptr(_pm_ops),
> > .of_match_table = dt_match,
> > },
> > };
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
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
Closes:
https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
Signed-off-by: Boris Brezillon
---
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(>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 = _pm_ops,
+ .pm = pm_ptr(_pm_ops),
.of_match_table = dt_match,
},
};
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
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
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
> Signed-off-by: Boris Brezillon
Reviewed-by: Steven Price
> ---
> 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(>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 = _pm_ops,
> + .pm = pm_ptr(_pm_ops),
> .of_match_table = dt_match,
> },
> };
Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
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
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
> Signed-off-by: Boris Brezillon
> ---
> 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
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(>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 = _pm_ops,
> + .pm = pm_ptr(_pm_ops),
> .of_match_table = dt_match,
> },
> };
> --
> 2.43.0
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
[PATCH] drm/panthor: Fix the CONFIG_PM=n case
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
Closes:
https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
Signed-off-by: Boris Brezillon
---
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(>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 = _pm_ops,
+ .pm = pm_ptr(_pm_ops),
.of_match_table = dt_match,
},
};
--
2.43.0
