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

[Jani Nikula]

On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@xxxxxxxx> wrote:
> On Mon, Apr 22, 2024, at 13:50, Jani Nikula wrote:
>> On Mon, 22 Apr 2024, Geert Uytterhoeven <geert+renesas@xxxxxxxxx> wrote:
>>> 	Hi all,
>>>
>>> As discussed on IRC with Maxime and Arnd, this series reverts the
>>> conversion of select to depends for various DRM helpers in series
>>> "[PATCH v3 00/13] drm/display: Convert helpers Kconfig symbols to
>>> depends on"[1], and various fixes for it.  This conversion introduced a
>>> big usability issue when configuring a kernel and enabling DRM drivers
>>> that use DRM helper code: as drivers now depend on helpers, the user
>>> needs to know which helpers to enable, before the driver he is
>>> interested even becomes visible.  The user should not need to know that,
>>> and drivers should select the helpers they need.
>>>
>>> Hence revert back to what we had before, where drivers selected the
>>> helpers (and any of their dependencies, if they can be met) they need.
>>> In general, when a symbol selects another symbol, it should just make
>>> sure the dependencies of the target symbol are met, which may mean
>>> adding dependencies to the source symbol.
>
> Thanks for doing this.
>
> Acked-by: Arnd Bergmann <arnd@xxxxxxxx>
>
>> 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. 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.

- 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.)

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?)


BR,
Jani.


-- 
Jani Nikula, Intel