Re: [PATCH] drm/panfrost: Mark simple_ondemand governor as softdep

Boris Brezillon
2024-07-03T13:20:25Z

On Wed, 03 Jul 2024 14:42:37 +0200
Dragan Simic <dsimic@manjaro.org> wrote:

> Hello everyone,
>
> On 2024-06-17 22:17, Dragan Simic wrote:
> > Panfrost DRM driver uses devfreq to perform DVFS, while using
> > simple_ondemand
> > devfreq governor by default. This causes driver initialization to fail
> > on
> > boot when simple_ondemand governor isn't built into the kernel
> > statically,
> > as a result of the missing module dependency and, consequently, the
> > required
> > governor module not being included in the initial ramdisk. Thus, let's
> > mark
> > simple_ondemand governor as a softdep for Panfrost, to have its kernel
> > module
> > included in the initial ramdisk.
> >
> > This is a rather longstanding issue that has forced distributions to
> > build
> > devfreq governors statically into their kernels, [1][2] or has forced
> > users
> > to introduce some unnecessary workarounds. [3]
> >
> > For future reference, not having support for the simple_ondemand
> > governor in
> > the initial ramdisk produces errors in the kernel log similar to these
> > below,
> > which were taken from a Pine64 RockPro64:
> >
> > panfrost ff9a0000.gpu: [drm:panfrost_devfreq_init [panfrost]]
> > *ERROR* Couldn't initialize GPU devfreq
> > panfrost ff9a0000.gpu: Fatal error during GPU init
> > panfrost: probe of ff9a0000.gpu failed with error -22
> >
> > Having simple_ondemand marked as a softdep for Panfrost may not resolve
> > this
> > issue for all Linux distributions. In particular, it will remain
> > unresolved
> > for the distributions whose utilities for the initial ramdisk
> > generation do
> > not handle the available softdep information [4] properly yet.
> > However, some
> > Linux distributions already handle softdeps properly while generating
> > their
> > initial ramdisks, [5] and this is a prerequisite step in the right
> > direction
> > for the distributions that don't handle them properly yet.
> >
> > [1]
> > https://gitlab.manjaro.org/manjaro-arm/packages/core/linux/-/blob/linux61/config?ref_type=heads#L8180
> > [2] https://salsa.debian.org/kernel-team/linux/-/merge_requests/1066
> > [3] https://forum.pine64.org/showthread.php?tid=15458
> > [4]
> > https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/commit/?id=49d8e0b59052999de577ab732b719cfbeb89504d
> > [5]
> > https://github.com/archlinux/mkinitcpio/commit/97ac4d37aae084a050be512f6d8f4489054668ad
> >
> > Cc: Diederik de Haas <didi.debian@cknow.org>
> > Cc: Furkan Kardame <f.kardame@manjaro.org>
> > Cc: stable@vger.kernel.org
> > Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> > Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>
> Just checking, could this patch be accepted, please?

Yes, sorry for the delay. Here's my

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Steve, any objection?

> The Lima
> counterpart
> has already been accepted. [6]
>
> The approach in this patch is far from perfect, but it's still fine
> until
> there's a better solution, such as harddeps. I'll continue my research
> about the possibility for introducing harddeps, which would hopefully
> replace quite a few instances of the softdep (ab)use that already extend
> rather far. For example, have a look at the commit d5178578bcd4 (btrfs:
> directly call into crypto framework for checksumming) [7] and the lines
> containing MODULE_SOFTDEP() at the very end of fs/btrfs/super.c. [8]
>
> If a filesystem driver can rely on the (ab)use of softdeps, which may be
> fragile or seen as a bit wrong, I think we can follow the same approach,
> at least until a better solution is available.
>
> [6]
> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=0c94f58cef319ad054fd909b3bf4b7d09c03e11c
> [7]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d5178578bcd4
> [8]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/super.c#n2593
>
> > ---
> > drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index ef9f6c0716d5..149737d7a07e 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -828,3 +828,4 @@ module_platform_driver(panfrost_driver);
> > MODULE_AUTHOR("Panfrost Project Developers");
> > MODULE_DESCRIPTION("Panfrost DRM Driver");
> > MODULE_LICENSE("GPL v2");
> > +MODULE_SOFTDEP("pre: governor_simpleondemand");