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

[Dragan Simic <dsimic@manjaro.org>]

Hello everyone,

Just checking, any further thoughts about this patch?

On 2024-06-18 21:22, Dragan Simic wrote:
> On 2024-06-18 12:33, Dragan Simic wrote:
>> On 2024-06-18 10:13, Maxime Ripard wrote:
>>> On Tue, Jun 18, 2024 at 04:01:26PM GMT, Qiang Yu wrote:
>>>> On Tue, Jun 18, 2024 at 12:33 PM Qiang Yu <yuq825@gmail.com> wrote:
>>>> >
>>>> > I see the problem that initramfs need to build a module dependency chain,
>>>> > but lima does not call any symbol from simpleondemand governor module.
>>>> > softdep module seems to be optional while our dependency is hard one,
>>>> > can we just add MODULE_INFO(depends, _depends), or create a new
>>>> > macro called MODULE_DEPENDS()?
>> 
>> I had the same thoughts, because softdeps are for optional module
>> dependencies, while in this case it's a hard dependency.  Though,
>> I went with adding a softdep, simply because I saw no better option
>> available.
>> 
>>>> This doesn't work on my side because depmod generates modules.dep
>>>> by symbol lookup instead of modinfo section. So softdep may be our 
>>>> only
>>>> choice to add module dependency manually. I can accept the softdep
>>>> first, then make PM optional later.
>> 
>> I also thought about making devfreq optional in the Lima driver,
>> which would make this additional softdep much more appropriate.
>> Though, I'm not really sure that's a good approach, because not
>> having working devfreq for Lima might actually cause issues on
>> some devices, such as increased power consumption.
>> 
>> In other words, it might be better to have Lima probing fail if
>> devfreq can't be initialized, rather than having probing succeed
>> with no working devfreq.  Basically, failed probing is obvious,
>> while a warning in the kernel log about no devfreq might easily
>> be overlooked, causing regressions on some devices.
>> 
>>> It's still super fragile, and depends on the user not changing the
>>> policy. It should be solved in some other, more robust way.
>> 
>> I see, but I'm not really sure how to make it more robust?  In
>> the end, some user can blacklist the simple_ondemand governor
>> module, and we can't do much about it.
>> 
>> Introducing harddeps alongside softdeps would make sense from
>> the design standpoint, but the amount of required changes wouldn't
>> be trivial at all, on various levels.
> 
> After further investigation, it seems that the softdeps have
> already seen a fair amount of abuse for what they actually aren't
> intended, i.e. resolving hard dependencies.  For example, have
> a look at the commit d5178578bcd4 (btrfs: directly call into
> crypto framework for checksumming) [1] and the lines containing
> MODULE_SOFTDEP() at the very end of fs/btrfs/super.c. [2]
> 
> If a filesystem driver can rely on the abuse of softdeps, which
> admittedly are a bit fragile, I think we can follow the same
> approach, at least for now.
> 
> With all that in mind, I think that accepting this patch, as well
> as the related Panfrost patch, [3] should be warranted.  I'd keep
> investigating the possibility of introducing harddeps in form
> of MODULE_HARDDEP() and the related support in kmod project,
> similar to the already existing softdep support, [4] but that
> will inevitably take a lot of time, both for implementing it
> and for reaching various Linux distributions, which is another
> reason why accepting these patches seems reasonable.
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d5178578bcd4
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/super.c#n2593
> [3] 
> https://lore.kernel.org/dri-devel/4e1e00422a14db4e2a80870afb704405da16fd1b.1718655077.git.dsimic@manjaro.org/
> [4] 
> https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/commit/?id=49d8e0b59052999de577ab732b719cfbeb89504d