On Mon, 11 Mar 2024 14:59:39 +0100
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
> 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;
And I forgot to remove the #ifdef CONFIG_PM in panthor_device.c.
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index ff484506229f..2ea6a9f436db 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1407,17 +1407,19 @@ static const struct of_device_id dt_match[] = {
> };
> MODULE_DEVICE_TABLE(of, dt_match);
>
> +#ifdef CONFIG_PM
> static DEFINE_RUNTIME_DEV_PM_OPS(panthor_pm_ops,
> panthor_device_suspend,
> panthor_device_resume,
> NULL);
> +#endif
>
> static struct platform_driver panthor_driver = {
> .probe = panthor_probe,
> .remove_new = panthor_remove,
> .driver = {
> .name = "panthor",
> - .pm = &panthor_pm_ops,
> + .pm = pm_ptr(&panthor_pm_ops),
> .of_match_table = dt_match,
> },
> };
