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

[Dragan Simic <dsimic@manjaro.org>]

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