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

[Dragan Simic <dsimic@manjaro.org>]

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");