[PATCH 3/3] drm/panthor: Fix undefined panthor_device_suspend/resume symbol issue

[Boris Brezillon]

On Mon, 11 Mar 2024 14:59:39 +0100
Boris Brezillon <boris.brezillon at collabora.com> wrote:

> On Mon, 11 Mar 2024 14:58:37 +0100
> Boris Brezillon <boris.brezillon at collabora.com> wrote:
> 
> > On Mon, 11 Mar 2024 13:46:23 +0000
> > Steven Price <steven.price at arm.com> 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 at arm.com> wrote:
> > > >>      
> > > >>> On 2024-03-11 11:52 am, Boris Brezillon wrote:      
> > > >>>> On Mon, 11 Mar 2024 13:49:56 +0200
> > > >>>> Jani Nikula <jani.nikula at linux.intel.com> wrote:
> > > >>>>         
> > > >>>>> On Mon, 11 Mar 2024, Boris Brezillon
> > > >>>>> <boris.brezillon at collabora.com> wrote:      
> > > >>>>>> On Mon, 11 Mar 2024 13:05:01 +0200
> > > >>>>>> Jani Nikula <jani.nikula at linux.intel.com> 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,
>         },
>  };