Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue
On Mon, 11 Mar 2024 14:59:39 +0100 Boris Brezillon wrote: > On Mon, 11 Mar 2024 14:58:37 +0100 > Boris Brezillon wrote: > > > On Mon, 11 Mar 2024 13:46:23 + > > Steven Price 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 + > > > >> Robin Murphy wrote: > > > >> > > > >>> On 2024-03-11 11:52 am, Boris Brezillon wrote: > > > On Mon, 11 Mar 2024 13:49:56 +0200 > > > Jani Nikula wrote: > > > > > > > On Mon, 11 Mar 2024, Boris Brezillon > > > > wrote: > > > >> On Mon, 11 Mar 2024 13:05:01 +0200 > > > >> Jani Nikula 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()
Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue
On Mon, 11 Mar 2024 14:58:37 +0100
Boris Brezillon wrote:
> On Mon, 11 Mar 2024 13:46:23 +
> Steven Price 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 +
> > >> Robin Murphy wrote:
> > >>
> > >>> On 2024-03-11 11:52 am, Boris Brezillon wrote:
> > On Mon, 11 Mar 2024 13:49:56 +0200
> > Jani Nikula wrote:
> >
> > > On Mon, 11 Mar 2024, Boris Brezillon
> > > wrote:
> > >> On Mon, 11 Mar 2024 13:05:01 +0200
> > >> Jani Nikula 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 =
Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue
On Mon, 11 Mar 2024 13:46:23 + Steven Price 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 + > >> Robin Murphy wrote: > >> > >>> On 2024-03-11 11:52 am, Boris Brezillon wrote: > On Mon, 11 Mar 2024 13:49:56 +0200 > Jani Nikula wrote: > > > On Mon, 11 Mar 2024, Boris Brezillon > > wrote: > >> On Mon, 11 Mar 2024 13:05:01 +0200 > >> Jani Nikula 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
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 + >> Robin Murphy wrote: >> >>> On 2024-03-11 11:52 am, Boris Brezillon wrote: On Mon, 11 Mar 2024 13:49:56 +0200 Jani Nikula wrote: > On Mon, 11 Mar 2024, Boris Brezillon > wrote: >> On Mon, 11 Mar 2024 13:05:01 +0200 >> Jani Nikula 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
On 2024-03-11 1:22 pm, Boris Brezillon wrote: On Mon, 11 Mar 2024 13:11:28 + Robin Murphy wrote: On 2024-03-11 11:52 am, Boris Brezillon wrote: On Mon, 11 Mar 2024 13:49:56 +0200 Jani Nikula wrote: On Mon, 11 Mar 2024, Boris Brezillon wrote: On Mon, 11 Mar 2024 13:05:01 +0200 Jani Nikula wrote: This breaks the config for me: SYNCinclude/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
On Mon, 11 Mar 2024 13:11:28 + Robin Murphy wrote: > On 2024-03-11 11:52 am, Boris Brezillon wrote: > > On Mon, 11 Mar 2024 13:49:56 +0200 > > Jani Nikula wrote: > > > >> On Mon, 11 Mar 2024, Boris Brezillon > >> wrote: > >>> On Mon, 11 Mar 2024 13:05:01 +0200 > >>> Jani Nikula wrote: > >>> > This breaks the config for me: > > SYNCinclude/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
On 2024-03-11 11:52 am, Boris Brezillon wrote: On Mon, 11 Mar 2024 13:49:56 +0200 Jani Nikula wrote: On Mon, 11 Mar 2024, Boris Brezillon wrote: On Mon, 11 Mar 2024 13:05:01 +0200 Jani Nikula wrote: This breaks the config for me: SYNCinclude/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
On Mon, 11 Mar 2024 13:49:56 +0200 Jani Nikula wrote: > On Mon, 11 Mar 2024, Boris Brezillon wrote: > > On Mon, 11 Mar 2024 13:05:01 +0200 > > Jani Nikula wrote: > > > >> This breaks the config for me: > >> > >> SYNCinclude/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
On Mon, 11 Mar 2024, Boris Brezillon wrote: > On Mon, 11 Mar 2024 13:05:01 +0200 > Jani Nikula wrote: > >> This breaks the config for me: >> >> SYNCinclude/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
On Mon, 11 Mar 2024 13:05:01 +0200 Jani Nikula wrote: > This breaks the config for me: > > SYNCinclude/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
On Mon, 04 Mar 2024, 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.
This breaks the config for me:
SYNCinclude/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
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
> Signed-off-by: Boris Brezillon
> ---
> 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(>pm.mmio_lock);
> return ret;
> }
> -#endif
--
Jani Nikula, Intel
Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue
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
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
> Signed-off-by: Boris Brezillon
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
> ---
> 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(>pm.mmio_lock);
> return ret;
> }
> -#endif
Re: [PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue
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
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
> Signed-off-by: Boris Brezillon
Reviewed-by: Liviu Dudau
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(>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! /
---
¯\_(ツ)_/¯
[PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue
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
Closes:
https://lore.kernel.org/oe-kbuild-all/202403031944.eoimq8wk-...@intel.com/
Signed-off-by: Boris Brezillon
---
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(>pm.mmio_lock);
return ret;
}
-#endif
--
2.43.0
