Re: [PATCH 00/11] drm: Restore helper usability

[Arnd Bergmann]

On Mon, Apr 22, 2024, at 15:28, Jani Nikula wrote:
> On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@xxxxxxxx> wrote:
>> On Mon, Apr 22, 2024, at 13:50, Jani Nikula wrote:
>>
>>> I still disagree with this, because fundamentally the source symbol
>>> really should not have to care about the dependencies of the target
>>> symbol.
>>
>> Sorry you missed the IRC discussion on #armlinux, we should have
>> included you as well since you applied the original patch.
>>
>> I think the reason for this revert is a bit more nuanced than
>> just the usability problem. Sorry if I'm dragging this out too
>> much, but I want to be sure that two points come across:
>>
>> 1. There is a semantic problem that is mostly subjective, but
>>    with the naming as "helper", I generally expect it as a hidden
>>    symbol that gets selected by its users, while calling same module
>>    "feature" would be something that is user-enabled and that
>>    other modules depend on. Both ways are commonly used in the
>>    kernel and are not mistakes on their own.
>
> Fair enough. I believe for (optional) "feature" the common pattern would
> then be depends on FEATURE || FEATURE=n.
>
>> 2. Using "select" on user visible symbols that have dependencies
>>    is a common source for bugs, and this is is a problem in
>>    drivers/gpu/drm more than elsewhere in the kernel, as these
>>    drivers traditionally select entire subsystems or drivers
>>    (I2C, VIRTIO, INPUT, ACPI_WMI, BACKLIGHT_CLASS_DEVICE,
>>    POWER_SUPPLY, SND_PCM, INTERCONNECT, ...). This regularly
>>    leads to circular dependencies and we should fix all of them.
>
> What annoys me is that the fixes tend to fall in two categories:
>
> - Play catch with selecting the dependencies of the selected
>   symbols. "depends on" handles this recursively, while select does
>   not.

I'm not sure where this misunderstanding comes from, as you
seem to be repeating the same incorrect assumption about
how select works that Maxime wrote in his changelog. To clarify,
this works exactly as one would expect:

config HELPER_A
       tristate

config HELPER_B
       tristate
       select HELPER_A

config DRIVER
       tristate "Turn on the driver and the helpers it uses"
       select HELPER_B # this recursively selects HELPER_A

Whereas this one is broken:

config FEATURE_A
       tristate "user visible if I2C is enabled"
       depends on I2C

config HELPER_B
       tristate # hidden
       select FEATURE_A

config DRIVER
       tristate "This driver is broken if I2C is disabled"
       select HELPER_B

>   There is no end to this, it just goes on and on, as the
>   dependencies of the selected symbols change over time. Often the
>   selects require unintuitive if patterns that are about the
>   implementation details of the symbol being selected.

Agreed, that is the problem I frequently face with drivers/gpu/drm,
and most of the time it can only be solved by rewriting the whole
system to not select user-visible symbol at all.

Using 'depends on' by itself is unfortunately not enough to
avoid /all/ the problems. See e.g. today's failure

config DRM_DISPLAY_HELPER
       tristate "DRM Display Helpers"
       default y

config DRM_DISPLAY_DP_HELPER
       bool "DRM DisplayPort Helpers"
       depends on DRM_DISPLAY_HELPER

config DRM_PANEL_LG_SW43408
       tristate "LG SW43408 panel"
       depends on DRM_DISPLAY_DP_HELPER

This version is still broken for DRM_DISPLAY_HELPER=m,
DRM_DISPLAY_DP_HELPER=m, DRM_PANEL_LG_SW43408=y because
the dependency on the bool symbol is not enough to
ensure that DRM_DISPLAY_HELPER is also built-in, so you
still need explicit dependencies on both
DRM_DISPLAY_HELPER and DRM_DISPLAY_DP_HELPER in the users.

This can be solved by making DRM_DISPLAY_DP_HELPER a
tristate symbol and adjusting the #ifdef checks and
Makefile logic accordingly, which is exactly what you'd
need to do to make it work with 'select' as well.

> - Brush the invalid configs under the rug by using IS_REACHABLE(),
>   switching from a loud link time failure to a silent runtime
>   failure. (I regularly reject patches adding IS_REACHABLE() to i915,
>   because from my pov e.g. i915=y backlight=m is an invalid
>   configuration that the user shouldn't have to debug at runtime. But I
>   can't express that in kconfig while everyone selects backlight.)

Thanks a lot for rejecting the IS_REACHABLE() patches, it is
indeed the worst way to handle those (I know, as I introduced
IS_REACHABLE() only to replace open-coded versions of the same,
not to have it as a feature or to use it in new code).

> If you have other ideas how these should be fixed, I'm all ears.
>
>>    The display helpers however don't have this problem because
>>    they do not have any dependencies outside of drivers/gpu/
>
> Fair enough, though I think they still suffer from some of them having
> dependencies. (Wasn't this how the original patches and the debate all
> got started?)

I believe that Maxime said on IRC that he only did the patches
originally because he expected problems with them based on his
understanding on how Kconfig works. I'm not aware of any
particular problem here.

Let me know if you see a problem with any of the symbols that
Geert has proposed for reverting, and I'll try to find a solution.
In my randconfig test environment, I have several patches that
I sent in the past to clean up the ACPI_VIDEO, I2C, BACKLIGHT and
LED dependencies to stop using 'select' as I could not otherwise
get nouveau, i915 and xe to build reliably, but that means I
may be missing some of the other problems.

     Arnd