| Message ID | cover.1713780345.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: Restore helper usability | expand |
On Mon, 22 Apr 2024, Geert Uytterhoeven <geert+renesas@glider.be> 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. I still disagree with this, because fundamentally the source symbol really should not have to care about the dependencies of the target symbol. That said, I'm not going to keep arguing against this. Whatever. BR, Jani. > > Thanks for applying! > > [1] https://lore.kernel.org/r/20240327-kms-kconfig-helpers-v3-0-eafee11b84b3@kernel.org/ > > Geert Uytterhoeven (11): > Revert "drm: fix DRM_DISPLAY_DP_HELPER dependencies, part 2" > Revert "drm/display: Select DRM_KMS_HELPER for DP helpers" > Revert "drm/bridge: dw-hdmi: Make DRM_DW_HDMI selectable" > Revert "drm: fix DRM_DISPLAY_DP_HELPER dependencies" > Revert "drm: Switch DRM_DISPLAY_HDMI_HELPER to depends on" > Revert "drm: Switch DRM_DISPLAY_HDCP_HELPER to depends on" > Revert "drm: Switch DRM_DISPLAY_DP_HELPER to depends on" > Revert "drm: Switch DRM_DISPLAY_DP_AUX_BUS to depends on" > Revert "drm: Switch DRM_DISPLAY_HELPER to depends on" > Revert "drm: Make drivers depends on DRM_DW_HDMI" > Revert "drm/display: Make all helpers visible and switch to depends > on" > > drivers/gpu/drm/Kconfig | 8 +++---- > drivers/gpu/drm/amd/amdgpu/Kconfig | 12 ++++------ > drivers/gpu/drm/bridge/Kconfig | 28 +++++++++++----------- > drivers/gpu/drm/bridge/analogix/Kconfig | 18 +++++++------- > drivers/gpu/drm/bridge/cadence/Kconfig | 8 +++---- > drivers/gpu/drm/bridge/imx/Kconfig | 4 ++-- > drivers/gpu/drm/bridge/synopsys/Kconfig | 6 ++--- > drivers/gpu/drm/display/Kconfig | 32 ++++++++++--------------- > drivers/gpu/drm/exynos/Kconfig | 4 ++-- > drivers/gpu/drm/i915/Kconfig | 8 +++---- > drivers/gpu/drm/imx/ipuv3/Kconfig | 5 ++-- > drivers/gpu/drm/ingenic/Kconfig | 2 +- > drivers/gpu/drm/mediatek/Kconfig | 6 ++--- > drivers/gpu/drm/meson/Kconfig | 2 +- > drivers/gpu/drm/msm/Kconfig | 8 +++---- > drivers/gpu/drm/nouveau/Kconfig | 10 ++++---- > drivers/gpu/drm/panel/Kconfig | 32 ++++++++++++------------- > drivers/gpu/drm/radeon/Kconfig | 8 +++---- > drivers/gpu/drm/renesas/rcar-du/Kconfig | 2 +- > drivers/gpu/drm/rockchip/Kconfig | 10 ++++---- > drivers/gpu/drm/sun4i/Kconfig | 2 +- > drivers/gpu/drm/tegra/Kconfig | 8 +++---- > drivers/gpu/drm/vc4/Kconfig | 10 ++++---- > drivers/gpu/drm/xe/Kconfig | 13 ++++------ > drivers/gpu/drm/xlnx/Kconfig | 8 +++---- > 25 files changed, 116 insertions(+), 138 deletions(-)
On Mon, Apr 22, 2024 at 02:50:09PM +0300, Jani Nikula wrote: > On Mon, 22 Apr 2024, Geert Uytterhoeven <geert+renesas@glider.be> 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. > > I still disagree with this, because fundamentally the source symbol > really should not have to care about the dependencies of the target > symbol. I'd agree with you, but if the driver depends on helpers it becomes ridiculous. So, it seems, we need a different solution for the original problem. > That said, I'm not going to keep arguing against this. Whatever. > > > BR, > Jani. > > > > > > Thanks for applying! > > > > [1] https://lore.kernel.org/r/20240327-kms-kconfig-helpers-v3-0-eafee11b84b3@kernel.org/ > > > > Geert Uytterhoeven (11): > > Revert "drm: fix DRM_DISPLAY_DP_HELPER dependencies, part 2" > > Revert "drm/display: Select DRM_KMS_HELPER for DP helpers" > > Revert "drm/bridge: dw-hdmi: Make DRM_DW_HDMI selectable" > > Revert "drm: fix DRM_DISPLAY_DP_HELPER dependencies" > > Revert "drm: Switch DRM_DISPLAY_HDMI_HELPER to depends on" > > Revert "drm: Switch DRM_DISPLAY_HDCP_HELPER to depends on" > > Revert "drm: Switch DRM_DISPLAY_DP_HELPER to depends on" > > Revert "drm: Switch DRM_DISPLAY_DP_AUX_BUS to depends on" > > Revert "drm: Switch DRM_DISPLAY_HELPER to depends on" > > Revert "drm: Make drivers depends on DRM_DW_HDMI" > > Revert "drm/display: Make all helpers visible and switch to depends > > on" > > > > drivers/gpu/drm/Kconfig | 8 +++---- > > drivers/gpu/drm/amd/amdgpu/Kconfig | 12 ++++------ > > drivers/gpu/drm/bridge/Kconfig | 28 +++++++++++----------- > > drivers/gpu/drm/bridge/analogix/Kconfig | 18 +++++++------- > > drivers/gpu/drm/bridge/cadence/Kconfig | 8 +++---- > > drivers/gpu/drm/bridge/imx/Kconfig | 4 ++-- > > drivers/gpu/drm/bridge/synopsys/Kconfig | 6 ++--- > > drivers/gpu/drm/display/Kconfig | 32 ++++++++++--------------- > > drivers/gpu/drm/exynos/Kconfig | 4 ++-- > > drivers/gpu/drm/i915/Kconfig | 8 +++---- > > drivers/gpu/drm/imx/ipuv3/Kconfig | 5 ++-- > > drivers/gpu/drm/ingenic/Kconfig | 2 +- > > drivers/gpu/drm/mediatek/Kconfig | 6 ++--- > > drivers/gpu/drm/meson/Kconfig | 2 +- > > drivers/gpu/drm/msm/Kconfig | 8 +++---- > > drivers/gpu/drm/nouveau/Kconfig | 10 ++++---- > > drivers/gpu/drm/panel/Kconfig | 32 ++++++++++++------------- > > drivers/gpu/drm/radeon/Kconfig | 8 +++---- > > drivers/gpu/drm/renesas/rcar-du/Kconfig | 2 +- > > drivers/gpu/drm/rockchip/Kconfig | 10 ++++---- > > drivers/gpu/drm/sun4i/Kconfig | 2 +- > > drivers/gpu/drm/tegra/Kconfig | 8 +++---- > > drivers/gpu/drm/vc4/Kconfig | 10 ++++---- > > drivers/gpu/drm/xe/Kconfig | 13 ++++------ > > drivers/gpu/drm/xlnx/Kconfig | 8 +++---- > > 25 files changed, 116 insertions(+), 138 deletions(-) > > -- > Jani Nikula, Intel
On Mon, Apr 22, 2024, at 13:50, Jani Nikula wrote: > On Mon, 22 Apr 2024, Geert Uytterhoeven <geert+renesas@glider.be> 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@arndb.de> > 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. 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. The display helpers however don't have this problem because they do not have any dependencies outside of drivers/gpu/ Arnd
On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@arndb.de> wrote: > On Mon, Apr 22, 2024, at 13:50, Jani Nikula wrote: >> On Mon, 22 Apr 2024, Geert Uytterhoeven <geert+renesas@glider.be> 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@arndb.de> > >> 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.
On Mon, Apr 22, 2024, at 15:28, Jani Nikula wrote: > On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@arndb.de> 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
Hi Arnd, CC kbuild On Mon, Apr 22, 2024 at 3:55 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Mon, Apr 22, 2024, at 15:28, Jani Nikula wrote: > > On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@arndb.de> wrote: > >> 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 So the DRIVER section should gain a "depends on I2C" statement. Yamada-san: would it be difficult to modify Kconfig to ignore symbols like DRIVER that select other symbols with unmet dependencies? Currently it already warns about that. Handling this implicitly (instead of the current explict "depends on") would have the disadvantage though: a user who is not aware of the implicit dependency may wonder why DRIVER is invisible in his config interface. > > > 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. Gr{oetje,eeting}s, Geert
On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@arndb.de> wrote: > On Mon, Apr 22, 2024, at 15:28, Jani Nikula wrote: >> On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@arndb.de> 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 This case is really what I was referring to, although I was sloppy with words there. I understand that select does work recursively for selects. >> 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. So bool is kind of problematic for depends on and select even when it's not really used for describing builtin vs. no, but rather yes vs. no? >> - 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. I think there will still be some things that select and some other things that depends on DRM_DISPLAY_HELPER, for example. Usually that's a recipe for kconfig failures down the line. (Is it ever a good idea?) For example, after this series, we'll (again) have: config DRM_DISPLAY_DP_AUX_CEC bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support" depends on DRM && DRM_DISPLAY_HELPER select DRM_DISPLAY_DP_HELPER select CEC_CORE Should we now also drop the help from DRM_DISPLAY_HELPER, and select it everywhere, including here, and in DRM_DISPLAY_HDCP_HELPER and DRM_DISPLAY_HDMI_HELPER? > 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. Yeah, these seem to be the really problematic ones. I admit I may have been misguided in insisting the same approach to the helpers that should be used here; at least the helpers should be easier to fix without selecting the target symbol dependencies in the source symbols. BR, Jani.
On Mon, 22 Apr 2024, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Arnd, > > CC kbuild > > On Mon, Apr 22, 2024 at 3:55 PM Arnd Bergmann <arnd@arndb.de> wrote: >> 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 > > So the DRIVER section should gain a "depends on I2C" statement. Why should DRIVER have to care that HELPER_B needs either FEATURE_A or I2C? It should only have to care about HELPER_B. And if the dependencies of FEATURE_A or HELPER_B later change, that's their business, not DRIVER's. BR, Jani. > > Yamada-san: would it be difficult to modify Kconfig to ignore symbols > like DRIVER that select other symbols with unmet dependencies? > Currently it already warns about that. > > Handling this implicitly (instead of the current explict "depends > on") would have the disadvantage though: a user who is not aware of > the implicit dependency may wonder why DRIVER is invisible in his > config interface.
Hi Jani, On Mon, Apr 22, 2024 at 7:15 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 22 Apr 2024, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Apr 22, 2024 at 3:55 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> 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 > > > > So the DRIVER section should gain a "depends on I2C" statement. > > Why should DRIVER have to care that HELPER_B needs either FEATURE_A or > I2C? It should only have to care about HELPER_B. And if the dependencies > of FEATURE_A or HELPER_B later change, that's their business, not > DRIVER's. That's correct. But currently the dependency on I2C is not handled automatically. > > Yamada-san: would it be difficult to modify Kconfig to ignore symbols > > like DRIVER that select other symbols with unmet dependencies? > > Currently it already warns about that. > > > > Handling this implicitly (instead of the current explict "depends > > on") would have the disadvantage though: a user who is not aware of > > the implicit dependency may wonder why DRIVER is invisible in his > > config interface. Gr{oetje,eeting}s, Geert
Hi Jani, CC kbuild On Mon, Apr 22, 2024 at 7:00 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@arndb.de> wrote: > > 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 > > This case is really what I was referring to, although I was sloppy with > words there. I understand that select does work recursively for selects. > > >> 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. > > So bool is kind of problematic for depends on and select even when it's > not really used for describing builtin vs. no, but rather yes vs. no? Yes, the underlying issue is that bool is used for two different things: A. To enable a driver module that can be only built-in, B. To enable an option or feature of a driver or subsystem. Without this distinction, dependencies cannot be auto-propagated 100% correctly. Fixing that would require introducing a third type (and possibly renaming the existing ones to end up with 3 good names). Actually two types could work: 1. driver, 2. option, as case A is just a driver that can only be built-in (i.e. "depends on y", which is similar to the behavior with CONFIG_MODULES=n). Gr{oetje,eeting}s, Geert
On Mon, Apr 22, 2024, at 18:58, Geert Uytterhoeven wrote: > On Mon, Apr 22, 2024 at 3:55 PM Arnd Bergmann <arnd@arndb.de> wrote: >> On Mon, Apr 22, 2024, at 15:28, Jani Nikula wrote: >> 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 > > So the DRIVER section should gain a "depends on I2C" statement. That is of course the common workaround, but my point was that nothing should ever 'select I2C' or any of the other subsystems that are user visible. > Yamada-san: would it be difficult to modify Kconfig to ignore symbols > like DRIVER that select other symbols with unmet dependencies? > Currently it already warns about that. > > Handling this implicitly (instead of the current explict "depends > on") would have the disadvantage though: a user who is not aware of > the implicit dependency may wonder why DRIVER is invisible in his > config interface. I think hiding this would make it much harder to get anything right. The symbols in question are almost all ones that should be enabled in normal configs, and the 'make menuconfig' help doesn't make it too hard to figure things out normally, we just have to find a way to avoid regressions when converting things to 'depends on' that used an incorrect 'select'. Arnd
On Tue, Apr 23, 2024 at 3:24 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Mon, Apr 22, 2024, at 18:58, Geert Uytterhoeven wrote: > > On Mon, Apr 22, 2024 at 3:55 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> On Mon, Apr 22, 2024, at 15:28, Jani Nikula wrote: > >> 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 > > > > So the DRIVER section should gain a "depends on I2C" statement. > > That is of course the common workaround, but my point was > that nothing should ever 'select I2C' or any of the other > subsystems that are user visible. > > > Yamada-san: would it be difficult to modify Kconfig to ignore symbols > > like DRIVER that select other symbols with unmet dependencies? > > Currently it already warns about that. > > > > Handling this implicitly (instead of the current explict "depends > > on") would have the disadvantage though: a user who is not aware of > > the implicit dependency may wonder why DRIVER is invisible in his > > config interface. > > I think hiding this would make it much harder to get anything > right. The symbols in question are almost all ones that should > be enabled in normal configs, and the 'make menuconfig' help > doesn't make it too hard to figure things out normally, we just > have to find a way to avoid regressions when converting things > to 'depends on' that used an incorrect 'select'. > > Arnd I am confused because you repeatedly discussed the missing I2C dependency. Are you talking about DRM drivers, or is it just "an example" in general? DRM selects I2C. https://github.com/torvalds/linux/blob/v6.9-rc4/drivers/gpu/drm/Kconfig#L16 If you make sure individual DRM drivers depend on DRM, none of them can be enabled without I2C. Currently, this is not guaranteed just because DRM folks do not know how to use the "menuconfig" syntax. The "menuconfig" makes sense only when it is followed by "if". diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 5a0c476361c3..6984b3fea271 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -29,6 +29,8 @@ menuconfig DRM details. You should also select and configure AGP (/dev/agpgart) support if it is available for your platform. +if DRM + config DRM_MIPI_DBI tristate depends on DRM @@ -414,3 +416,5 @@ config DRM_LIB_RANDOM config DRM_PRIVACY_SCREEN bool default n + +endif -- Best Regards Masahiro Yamada
On Mon, Apr 22, 2024, at 21:42, Masahiro Yamada wrote: > On Tue, Apr 23, 2024 at 3:24 AM Arnd Bergmann <arnd@arndb.de> wrote: >> On Mon, Apr 22, 2024, at 18:58, Geert Uytterhoeven wrote: >> > On Mon, Apr 22, 2024 at 3:55 PM Arnd Bergmann <arnd@arndb.de> wrote: >> >> On Mon, Apr 22, 2024, at 15:28, Jani Nikula wrote: >> >> I think hiding this would make it much harder to get anything >> right. The symbols in question are almost all ones that should >> be enabled in normal configs, and the 'make menuconfig' help >> doesn't make it too hard to figure things out normally, we just >> have to find a way to avoid regressions when converting things >> to 'depends on' that used an incorrect 'select'. > > I am confused because you repeatedly discussed > the missing I2C dependency. > > > Are you talking about DRM drivers, > or is it just "an example" in general? > > > > DRM selects I2C. It's a prominent example because I2C ends up showing up in most circular dependencies. I forgot that CONFIG_DRM itself selects this one, but this is clearly part of the overall problem: $ git grep -w 'select I2C' | wc -l 35 $ git grep -w 'depends on I2C' | wc -l 1068 Attempting to clean up some of the incorrect 'select' statements, such as changing DRM_NOUVEAU to 'depends on ACPI_VIDEO' instead of 'select' tends to run into another 'select I2C' such as this one: drivers/i2c/Kconfig:8: symbol I2C is selected by DRM_NOUVEAU drivers/gpu/drm/nouveau/Kconfig:2: symbol DRM_NOUVEAU depends on ACPI_VIDEO drivers/acpi/Kconfig:214: symbol ACPI_VIDEO depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:136: symbol BACKLIGHT_CLASS_DEVICE is selected by FB_BACKLIGHT drivers/video/fbdev/core/Kconfig:184: symbol FB_BACKLIGHT is selected by HT16K33 drivers/auxdisplay/Kconfig:490: symbol HT16K33 depends on I2C Again, I2C was probably not the best example for an urgent problem as it ends up being selected unconditionally anyway, but I think ACPI_VIDEO and BACKLIGHT_CLASS_DEVICE are the ones that we should stop selecting. > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 5a0c476361c3..6984b3fea271 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -29,6 +29,8 @@ menuconfig DRM > details. You should also select and configure AGP > (/dev/agpgart) support if it is available for your platform. > > +if DRM > + > config DRM_MIPI_DBI > tristate > depends on DRM > @@ -414,3 +416,5 @@ config DRM_LIB_RANDOM > config DRM_PRIVACY_SCREEN > bool > default n > + > +endif This is a probably good idea (aside from DRM_PANEL_ORIENTATION_QUIRKS, which needs to be moved out of the section), but seems completely unrelated to the issue of selecting too many symbols. Arnd
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 applying! [1] https://lore.kernel.org/r/20240327-kms-kconfig-helpers-v3-0-eafee11b84b3@kernel.org/ Geert Uytterhoeven (11): Revert "drm: fix DRM_DISPLAY_DP_HELPER dependencies, part 2" Revert "drm/display: Select DRM_KMS_HELPER for DP helpers" Revert "drm/bridge: dw-hdmi: Make DRM_DW_HDMI selectable" Revert "drm: fix DRM_DISPLAY_DP_HELPER dependencies" Revert "drm: Switch DRM_DISPLAY_HDMI_HELPER to depends on" Revert "drm: Switch DRM_DISPLAY_HDCP_HELPER to depends on" Revert "drm: Switch DRM_DISPLAY_DP_HELPER to depends on" Revert "drm: Switch DRM_DISPLAY_DP_AUX_BUS to depends on" Revert "drm: Switch DRM_DISPLAY_HELPER to depends on" Revert "drm: Make drivers depends on DRM_DW_HDMI" Revert "drm/display: Make all helpers visible and switch to depends on" drivers/gpu/drm/Kconfig | 8 +++---- drivers/gpu/drm/amd/amdgpu/Kconfig | 12 ++++------ drivers/gpu/drm/bridge/Kconfig | 28 +++++++++++----------- drivers/gpu/drm/bridge/analogix/Kconfig | 18 +++++++------- drivers/gpu/drm/bridge/cadence/Kconfig | 8 +++---- drivers/gpu/drm/bridge/imx/Kconfig | 4 ++-- drivers/gpu/drm/bridge/synopsys/Kconfig | 6 ++--- drivers/gpu/drm/display/Kconfig | 32 ++++++++++--------------- drivers/gpu/drm/exynos/Kconfig | 4 ++-- drivers/gpu/drm/i915/Kconfig | 8 +++---- drivers/gpu/drm/imx/ipuv3/Kconfig | 5 ++-- drivers/gpu/drm/ingenic/Kconfig | 2 +- drivers/gpu/drm/mediatek/Kconfig | 6 ++--- drivers/gpu/drm/meson/Kconfig | 2 +- drivers/gpu/drm/msm/Kconfig | 8 +++---- drivers/gpu/drm/nouveau/Kconfig | 10 ++++---- drivers/gpu/drm/panel/Kconfig | 32 ++++++++++++------------- drivers/gpu/drm/radeon/Kconfig | 8 +++---- drivers/gpu/drm/renesas/rcar-du/Kconfig | 2 +- drivers/gpu/drm/rockchip/Kconfig | 10 ++++---- drivers/gpu/drm/sun4i/Kconfig | 2 +- drivers/gpu/drm/tegra/Kconfig | 8 +++---- drivers/gpu/drm/vc4/Kconfig | 10 ++++---- drivers/gpu/drm/xe/Kconfig | 13 ++++------ drivers/gpu/drm/xlnx/Kconfig | 8 +++---- 25 files changed, 116 insertions(+), 138 deletions(-)