On Mon, 11 Mar 2024 14:58:37 +0100
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
> On Mon, 11 Mar 2024 13:46:23 +0000
> Steven Price <steven.price@xxxxxxx> wrote:
>
> > On 11/03/2024 13:36, Robin Murphy wrote:
> > > On 2024-03-11 1:22 pm, Boris Brezillon wrote:
> > >> On Mon, 11 Mar 2024 13:11:28 +0000
> > >> Robin Murphy <robin.murphy@xxxxxxx> wrote:
> > >>
> > >>> On 2024-03-11 11:52 am, Boris Brezillon wrote:
> > >>>> On Mon, 11 Mar 2024 13:49:56 +0200
> > >>>> Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> > >>>>
> > >>>>> On Mon, 11 Mar 2024, Boris Brezillon
> > >>>>> <boris.brezillon@xxxxxxxxxxxxx> wrote:
> > >>>>>> On Mon, 11 Mar 2024 13:05:01 +0200
> > >>>>>> Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> > >>>>>>
> > >>>>>>> This breaks the config for me:
> > >>>>>>>
> > >>>>>>> SYNC include/config/auto.conf.cmd
> > >>>>>>> GEN Makefile
> > >>>>>>> drivers/iommu/Kconfig:14:error: recursive dependency detected!
> > >>>>>>> drivers/iommu/Kconfig:14: symbol IOMMU_SUPPORT is selected by
> > >>>>>>> DRM_PANTHOR
> > >>>>>>> drivers/gpu/drm/panthor/Kconfig:3: symbol DRM_PANTHOR depends
> > >>>>>>> on PM
> > >>>>>>> kernel/power/Kconfig:183: symbol PM is selected by PM_SLEEP
> > >>>>>>> kernel/power/Kconfig:117: symbol PM_SLEEP depends on
> > >>>>>>> HIBERNATE_CALLBACKS
> > >>>>>>> kernel/power/Kconfig:35: symbol HIBERNATE_CALLBACKS is
> > >>>>>>> selected by XEN_SAVE_RESTORE
> > >>>>>>> arch/x86/xen/Kconfig:67: symbol XEN_SAVE_RESTORE depends on XEN
> > >>>>>>> arch/x86/xen/Kconfig:6: symbol XEN depends on PARAVIRT
> > >>>>>>> arch/x86/Kconfig:781: symbol PARAVIRT is selected by HYPERV
> > >>>>>>> drivers/hv/Kconfig:5: symbol HYPERV depends on X86_LOCAL_APIC
> > >>>>>>> arch/x86/Kconfig:1106: symbol X86_LOCAL_APIC depends on
> > >>>>>>> X86_UP_APIC
> > >>>>>>> arch/x86/Kconfig:1081: symbol X86_UP_APIC prompt is visible
> > >>>>>>> depending on PCI_MSI
> > >>>>>>> drivers/pci/Kconfig:39: symbol PCI_MSI is selected by AMD_IOMMU
> > >>>>>>> drivers/iommu/amd/Kconfig:3: symbol AMD_IOMMU depends on
> > >>>>>>> IOMMU_SUPPORT
> > >>>>>>
> > >>>>>> Uh, I guess we want a "depends on IOMMU_SUPPORT" instead of "select
> > >>>>>> IOMMU_SUPPORT" in panthor then.
> > >>>>>
> > >>>>> That works for me.
> > >>>>
> > >>>> Let's revert the faulty commit first. We'll see if Steve has a
> > >>>> different solution for the original issue.
> > >>>
> > >>> FWIW, the reasoning in the offending commit seems incredibly tenuous.
> > >>> There are far more practical reasons for building an arm/arm64 kernel
> > >>> without PM - for debugging or whatever, and where one may even still
> > >>> want a usable GPU, let alone just a non-broken build - than there are
> > >>> for building this driver for x86. Using pm_ptr() is trivial, and if you
> > >>> want to support COMPILE_TEST then there's really no justifiable excuse
> > >>> not to.
> > >>
> > >> The problem is not just about using pm_ptr(), but also making sure
> > >> panthor_device_resume/suspend() are called called in the init/unplug
> > >> path when !PM, as I don't think the PM helpers automate that for us. I
> > >> was just aiming for a simple fix that wouldn't force me to test the !PM
> > >> case...
> > > Fair enough, at worst we could always have a runtime check and refuse to
> > > probe in conditions we don't think are worth the bother of implementing
> > > fully-functional support for. However if we want to make an argument for
> > > only supporting "realistic" configs at build time then that is an
> > > argument for dropping COMPILE_TEST as well.
> >
> > Can we just replace the "depends on PM" with "select PM"? In my
> > (admittedly very limited) testing this works. Otherwise I think we need
> > to bite the bullet and support !PM in some way (maybe just as Robin
> > suggests with a runtime bail out).
>
> I won't have time to test it this week, but if someone is interested,
> here's a diff implementing manual resume/suspend in the init/unplug
> path:
>
> --->8---
This time with the diff :-)
--->8---
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 69deb8e17778..3d05e7358f0e 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -87,6 +87,10 @@ void panthor_device_unplug(struct panthor_device *ptdev)
pm_runtime_dont_use_autosuspend(ptdev->base.dev);
pm_runtime_put_sync_suspend(ptdev->base.dev);
+ /* If PM is disabled, we need to call the suspend handler manually. */
+ if (!IS_ENABLED(CONFIG_PM))
+ panthor_device_suspend(ptdev->base.dev);
+
/* Report the unplug operation as done to unblock concurrent
* panthor_device_unplug() callers.
*/
@@ -218,6 +222,13 @@ int panthor_device_init(struct panthor_device *ptdev)
if (ret)
return ret;
+ /* If PM is disabled, we need to call panthor_device_resume() manually. */
+ if (!IS_ENABLED(CONFIG_PM)) {
+ ret = panthor_device_resume(ptdev->base.dev);
+ if (ret)
+ return ret;
+ }
+
ret = panthor_gpu_init(ptdev);
if (ret)
goto err_rpm_put;
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index ff484506229f..2ea6a9f436db 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -1407,17 +1407,19 @@ static const struct of_device_id dt_match[] = {
};
MODULE_DEVICE_TABLE(of, dt_match);
+#ifdef CONFIG_PM
static DEFINE_RUNTIME_DEV_PM_OPS(panthor_pm_ops,
panthor_device_suspend,
panthor_device_resume,
NULL);
+#endif
static struct platform_driver panthor_driver = {
.probe = panthor_probe,
.remove_new = panthor_remove,
.driver = {
.name = "panthor",
- .pm = &panthor_pm_ops,
+ .pm = pm_ptr(&panthor_pm_ops),
.of_match_table = dt_match,
},
};
