| Message ID | 4e1e00422a14db4e2a80870afb704405da16fd1b.1718655077.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Mark simple_ondemand governor as softdep | expand |
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? 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");
On 03/07/2024 13:42, Dragan Simic 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> Reviewed-by: Steven Price <steven.price@arm.com> > > Just checking, could this patch be accepted, please? The Lima counterpart > has already been accepted. [6] Thanks for the prod - I have to admit I saw there was discussion about the Lima patch and so just put this on my list to look again later after the discussion had reached a conclusion. > 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] I agree - it's not perfect, but it's the best we have for now. I hope sometime we'll have a cleaner solution to express dependencies like this (good luck! ;) ). Thanks, Steve > 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");
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");
On 03/07/2024 14:20, Boris Brezillon wrote: > 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: <snip> >> >> 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? Nope, our messages crossed in the post ;) I'll push to drm-misc-next. Thanks, Steve
On 2024-07-03 15:20, Boris Brezillon wrote: > On Wed, 03 Jul 2024 14:42:37 +0200 > Dragan Simic <dsimic@manjaro.org> wrote: >> 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> No worries, and thanks! > 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");
On 2024-07-03 15:20, Steven Price wrote: > On 03/07/2024 13:42, Dragan Simic wrote: >> 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> > > Reviewed-by: Steven Price <steven.price@arm.com> Thanks! >> Just checking, could this patch be accepted, please? The Lima >> counterpart >> has already been accepted. [6] > > Thanks for the prod - I have to admit I saw there was discussion about > the Lima patch and so just put this on my list to look again later > after > the discussion had reached a conclusion. > >> 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] > > I agree - it's not perfect, but it's the best we have for now. I hope > sometime we'll have a cleaner solution to express dependencies like > this > (good luck! ;) ). Thanks. :) Implementing harddeps is _relatively_ straightforward, but getting full support for harddeps reach various Linux distributions is going to be an uphill battle without doubt. :) >> 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");
Hello Steven and Boris, On 2024-07-03 16:52, Dragan Simic wrote: > On 2024-07-03 15:20, Steven Price wrote: >> On 03/07/2024 13:42, Dragan Simic wrote: >>> 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> >> >> Reviewed-by: Steven Price <steven.price@arm.com> > > Thanks! > >>> Just checking, could this patch be accepted, please? The Lima >>> counterpart >>> has already been accepted. [6] >> >> Thanks for the prod - I have to admit I saw there was discussion about >> the Lima patch and so just put this on my list to look again later >> after >> the discussion had reached a conclusion. >> >>> 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] >> >> I agree - it's not perfect, but it's the best we have for now. I hope >> sometime we'll have a cleaner solution to express dependencies like >> this >> (good luck! ;) ). > > Thanks. :) Implementing harddeps is _relatively_ straightforward, but > getting full support for harddeps reach various Linux distributions is > going to be an uphill battle without doubt. :) Another option has become available for expressing additional module dependencies, weakdeps. [1][2] Long story short, weakdeps are similar to softdeps, in the sense of telling the initial ramdisk utilities to include additional kernel modules, but weakdeps result in no module loading being performed by userspace. Maybe "weak" isn't the best possible word choice (arguably, "soft" also wasn't the best word choice), but weakdeps should be a better choice for use with Panfrost and governor_simpleondemand, because weakdeps provide the required information to the utilities used to generate initial ramdisks, while the actual module loading is left to the kernel. The recent addition of weakdeps renders the previously mentioned harddeps obsolete, because weakdeps actually do what we need. Obviously, "weak" doesn't go along very well with the actual nature of the dependency between Panfrost and governor_simpleondemand, but it's pretty much just the somewhat unfortunate word choice. The support for weakdeps has been already added to the kmod [3][4] and Dracut [5] userspace utilities. I'll hopefully add support for weakdeps to mkinitcpio [6] rather soon. Maybe we could actually add MODULE_HARDDEP() as some kind of syntactic sugar, which would currently be an alias for MODULE_WEAKDEP(), so the actual hard module dependencies could be expressed properly, and possibly handled differently in the future, with no need to go back and track all such instances of hard module dependencies. With all this in mind, here's what I'm going to do: 1) Submit a patch that adds MODULE_HARDDEP() as syntactic sugar 2) Implement support for weakdeps in Arch Linux's mkinitcpio [6] 3) Depending on what kind of feedback the MODULE_HARDDEP() patch receives, I'll submit follow-up patches for Lima and Panfrost, which will swap uses of MODULE_SOFTDEP() with MODULE_HARDDEP() or MODULE_WEAKDEP() Looking forward to your thoughts. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/module.h?id=61842868de13aa7fd7391c626e889f4d6f1450bf [2] https://lore.kernel.org/linux-kernel/20240724102349.430078-1-jtornosm@redhat.com/T/#u [3] https://github.com/kmod-project/kmod/commit/05828b4a6e9327a63ef94df544a042b5e9ce4fe7 [4] https://github.com/kmod-project/kmod/commit/d06712b51404061eef92cb275b8303814fca86ec [5] https://github.com/dracut-ng/dracut-ng/commit/8517a6be5e20f4a6d87e55fce35ee3e29e2a1150 [6] https://gitlab.archlinux.org/archlinux/mkinitcpio/mkinitcpio >>> 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");
Hi Dragan, On 25/07/2024 09:24, Dragan Simic wrote: > Hello Steven and Boris, <snip> > Another option has become available for expressing additional module > dependencies, weakdeps. [1][2] Long story short, weakdeps are similar > to softdeps, in the sense of telling the initial ramdisk utilities to > include additional kernel modules, but weakdeps result in no module > loading being performed by userspace. > > Maybe "weak" isn't the best possible word choice (arguably, "soft" also > wasn't the best word choice), but weakdeps should be a better choice for > use with Panfrost and governor_simpleondemand, because weakdeps provide > the required information to the utilities used to generate initial > ramdisks, > while the actual module loading is left to the kernel. > > The recent addition of weakdeps renders the previously mentioned harddeps > obsolete, because weakdeps actually do what we need. Obviously, "weak" > doesn't go along very well with the actual nature of the dependency between > Panfrost and governor_simpleondemand, but it's pretty much just the > somewhat > unfortunate word choice. > > The support for weakdeps has been already added to the kmod [3][4] and > Dracut [5] userspace utilities. I'll hopefully add support for weakdeps > to mkinitcpio [6] rather soon. That sounds much closer to the dependency we want to advertise for Panfrost so that's great. > Maybe we could actually add MODULE_HARDDEP() as some kind of syntactic > sugar, which would currently be an alias for MODULE_WEAKDEP(), so the > actual hard module dependencies could be expressed properly, and possibly > handled differently in the future, with no need to go back and track all > such instances of hard module dependencies. Please do! While "weak" dependencies tell the initramfs tools what to put in, it would be good to be able to actually express that this module actually requires the governor. I can see the potential utility in initramfs tools wanting to put a module in without "weak" dependencies if initramfs size was limited[1] and "limited support" was appropriate, and that's not what Panfrost gives. So having a way of fixing this in the future without churn in driver would be good. > With all this in mind, here's what I'm going to do: > > 1) Submit a patch that adds MODULE_HARDDEP() as syntactic sugar > 2) Implement support for weakdeps in Arch Linux's mkinitcpio [6] > 3) Depending on what kind of feedback the MODULE_HARDDEP() patch receives, > I'll submit follow-up patches for Lima and Panfrost, which will swap > uses of MODULE_SOFTDEP() with MODULE_HARDDEP() or MODULE_WEAKDEP() It sounds good from my perspective. It will be interesting to see what feedback comes from people more familiar with initramfs tools. Thanks, Steve [1] Although from my understanding it's firmware which is the real cause of bloat in initramfs size. I guess I need to start paying attention to this for panthor which adds GPU firmware - although currently tiny in comparison to others. > Looking forward to your thoughts. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/module.h?id=61842868de13aa7fd7391c626e889f4d6f1450bf > [2] > https://lore.kernel.org/linux-kernel/20240724102349.430078-1-jtornosm@redhat.com/T/#u > [3] > https://github.com/kmod-project/kmod/commit/05828b4a6e9327a63ef94df544a042b5e9ce4fe7 > [4] > https://github.com/kmod-project/kmod/commit/d06712b51404061eef92cb275b8303814fca86ec > [5] > https://github.com/dracut-ng/dracut-ng/commit/8517a6be5e20f4a6d87e55fce35ee3e29e2a1150 > [6] https://gitlab.archlinux.org/archlinux/mkinitcpio/mkinitcpio > >>>> 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");
On Thursday, 25 July 2024 11:20:22 CEST Steven Price wrote: > [1] Although from my understanding it's firmware which is the real cause > of bloat in initramfs size. I guess I need to start paying attention to > this for panthor which adds GPU firmware - although currently tiny in > comparison to others. Can confirm that's the case. When using (f.e.?) plymouth, the gpu drivers and corresponding firmware gets added to initramfs. If you then have much more and much larger firmware files (nvidia f.e. added 2 fw files of 23 and 38 MB respectively ...) then you get a corresponding much larger initramfs. If you add a (fixed) bug in initramfs-tools where symlinks to directories weren't preserved, but full copies were made, you could end up with a single initramfs of 240MB ...
Hello Steven, On 2024-07-25 11:20, Steven Price wrote: > On 25/07/2024 09:24, Dragan Simic wrote: >> Hello Steven and Boris, > > <snip> > >> Another option has become available for expressing additional module >> dependencies, weakdeps. [1][2] Long story short, weakdeps are similar >> to softdeps, in the sense of telling the initial ramdisk utilities to >> include additional kernel modules, but weakdeps result in no module >> loading being performed by userspace. >> >> Maybe "weak" isn't the best possible word choice (arguably, "soft" >> also >> wasn't the best word choice), but weakdeps should be a better choice >> for >> use with Panfrost and governor_simpleondemand, because weakdeps >> provide >> the required information to the utilities used to generate initial >> ramdisks, >> while the actual module loading is left to the kernel. >> >> The recent addition of weakdeps renders the previously mentioned >> harddeps >> obsolete, because weakdeps actually do what we need. Obviously, >> "weak" >> doesn't go along very well with the actual nature of the dependency >> between >> Panfrost and governor_simpleondemand, but it's pretty much just the >> somewhat >> unfortunate word choice. >> >> The support for weakdeps has been already added to the kmod [3][4] and >> Dracut [5] userspace utilities. I'll hopefully add support for >> weakdeps >> to mkinitcpio [6] rather soon. > > That sounds much closer to the dependency we want to advertise for > Panfrost so that's great. > >> Maybe we could actually add MODULE_HARDDEP() as some kind of syntactic >> sugar, which would currently be an alias for MODULE_WEAKDEP(), so the >> actual hard module dependencies could be expressed properly, and >> possibly >> handled differently in the future, with no need to go back and track >> all >> such instances of hard module dependencies. > > Please do! While "weak" dependencies tell the initramfs tools what to > put in, it would be good to be able to actually express that this > module > actually requires the governor. Great, I'm glad that you agree. Here's the MODULE_HARDDEP() patch on the linux-modules mailing list, and we'll see will it be accepted: https://lore.kernel.org/linux-modules/04e0676b0e77c5eb69df6972f41d77cdf061265a.1721906745.git.dsimic@manjaro.org/T/#u > I can see the potential utility in > initramfs tools wanting to put a module in without "weak" dependencies > if initramfs size was limited[1] and "limited support" was appropriate, > and that's not what Panfrost gives. So having a way of fixing this in > the future without churn in driver would be good. Sure, that's a good example, but unfortunately, omitting weakdep modules that way from the initial ramdisk, and keeping only the harddep modules, wouldn't be that simple. :( In fact, it's unknown which one(s) of the weakdep modules is/are actually needed on some platform or device, so pruning the weakdep modules would require some additional information, to end up with a fully functional device after booting it up. Of course, the distinction between the harddeps and the weakdeps opens up a path towards using such additional "pruning information" in a safe and robust way, by ensuring that the absolutely required harddep modules aren't pruned away. This is just another example of how "weak" was a somewhat unfortunate word choice, but we've got to live with it. :) >> With all this in mind, here's what I'm going to do: >> >> 1) Submit a patch that adds MODULE_HARDDEP() as syntactic sugar >> 2) Implement support for weakdeps in Arch Linux's mkinitcpio [6] >> 3) Depending on what kind of feedback the MODULE_HARDDEP() patch >> receives, >> I'll submit follow-up patches for Lima and Panfrost, which will >> swap >> uses of MODULE_SOFTDEP() with MODULE_HARDDEP() or MODULE_WEAKDEP() > > It sounds good from my perspective. It will be interesting to see what > feedback comes from people more familiar with initramfs tools. Great, thanks once again! > [1] Although from my understanding it's firmware which is the real > cause > of bloat in initramfs size. I guess I need to start paying attention to > this for panthor which adds GPU firmware - although currently tiny in > comparison to others. We might have a solution for the initramfs bloat induced by the firmware blobs, which I'm going to fight for, one way or another. :) Though, only time will tell will the related patches be accepted. [7] [7] https://lore.kernel.org/linux-rockchip/9b7a9e9b88ad8c7489ee1b4c70b8751eeb5cf6f9.1720049413.git.dsimic@manjaro.org/T/#u >> Looking forward to your thoughts. >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/module.h?id=61842868de13aa7fd7391c626e889f4d6f1450bf >> [2] >> https://lore.kernel.org/linux-kernel/20240724102349.430078-1-jtornosm@redhat.com/T/#u >> [3] >> https://github.com/kmod-project/kmod/commit/05828b4a6e9327a63ef94df544a042b5e9ce4fe7 >> [4] >> https://github.com/kmod-project/kmod/commit/d06712b51404061eef92cb275b8303814fca86ec >> [5] >> https://github.com/dracut-ng/dracut-ng/commit/8517a6be5e20f4a6d87e55fce35ee3e29e2a1150 >> [6] https://gitlab.archlinux.org/archlinux/mkinitcpio/mkinitcpio >> >>>>> 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");
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");
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> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 1 + 1 file changed, 1 insertion(+)