From: Arnd Bergmann arnd@arndb.de
The driver fails to build when the KMS helpers are disabled:
ld.lld: error: undefined symbol: drm_gem_fb_get_obj
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_fb_dirty) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_gem_fb_begin_cpu_access
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_swab
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_xrgb8888_to_rgb565
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_memcpy
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
This is fairly hard to hit in randconfig drivers, but it eventually did trigger for me in a configuration where all other DRM drivers are loadable modules, but DRM_PANEL_WIDECHIPS_WS2401 was built-in.
Adding a dependency in all drivers that select DRM_MIPI_DBI avoids the problem for now, adding the dependency in DRM_MIPI_DBI as well should help make it easier to figure out why it breaks if someone forgets the dependency the next time.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/gpu/drm/Kconfig | 2 +- drivers/gpu/drm/panel/Kconfig | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)
I see this warning on 5.17-rc8, but did not test it on linux-next, which may already have a fix.
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index b1f22e457fd0..d5ec0b77c010 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -30,7 +30,7 @@ menuconfig DRM
config DRM_MIPI_DBI tristate - depends on DRM + depends on DRM_KMS_HELPER
config DRM_MIPI_DSI bool diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 0aec5a10b064..96887d0efb9f 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -425,6 +425,7 @@ config DRM_PANEL_SAMSUNG_DB7430 tristate "Samsung DB7430-based DPI panels" depends on OF && SPI && GPIOLIB depends on BACKLIGHT_CLASS_DEVICE + depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Samsung @@ -440,6 +441,7 @@ config DRM_PANEL_SAMSUNG_S6D16D0 config DRM_PANEL_SAMSUNG_S6D27A1 tristate "Samsung S6D27A1 DPI panel driver" depends on OF && SPI && GPIOLIB + depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Samsung @@ -476,6 +478,7 @@ config DRM_PANEL_SAMSUNG_S6E63M0_SPI depends on SPI depends on DRM_PANEL_SAMSUNG_S6E63M0 default DRM_PANEL_SAMSUNG_S6E63M0 + depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to be able to access the Samsung @@ -677,6 +680,7 @@ config DRM_PANEL_WIDECHIPS_WS2401 tristate "Widechips WS2401 DPI panel driver" depends on SPI && GPIOLIB depends on BACKLIGHT_CLASS_DEVICE + depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Widechips WS2401 DPI
From: Arnd Bergmann arnd@arndb.de
The use of the old-style SET_RUNTIME_PM_OPS() and SET_SYSTEM_SLEEP_PM_OPS() macros requires function definitions to be hidden to avoid
drivers/gpu/drm/tegra/vic.c:326:12: error: 'vic_runtime_suspend' defined but not used [-Werror=unused-function] 326 | static int vic_runtime_suspend(struct device *dev) | ^~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/tegra/vic.c:292:12: error: 'vic_runtime_resume' defined but not used [-Werror=unused-function] 292 | static int vic_runtime_resume(struct device *dev) | ^~~~~~~~~~~~~~~~~~
Use the new-style SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() instead.
Fixes: 1e15f5b911d6 ("drm/tegra: vic: Stop channel on suspend") Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/gpu/drm/tegra/vic.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
I see this warning on 5.17-rc8, but did not test it on linux-next, which may already have a fix.
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index 1e342fa3d27b..f56f5921a8c2 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -513,9 +513,8 @@ static int vic_remove(struct platform_device *pdev) }
static const struct dev_pm_ops vic_pm_ops = { - SET_RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL) - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, - pm_runtime_force_resume) + RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL) + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) };
struct platform_driver tegra_vic_driver = {
On Wed, Mar 16, 2022 at 07:36:47PM +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The use of the old-style SET_RUNTIME_PM_OPS() and SET_SYSTEM_SLEEP_PM_OPS() macros requires function definitions to be hidden to avoid
drivers/gpu/drm/tegra/vic.c:326:12: error: 'vic_runtime_suspend' defined but not used [-Werror=unused-function] 326 | static int vic_runtime_suspend(struct device *dev) | ^~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/tegra/vic.c:292:12: error: 'vic_runtime_resume' defined but not used [-Werror=unused-function] 292 | static int vic_runtime_resume(struct device *dev) | ^~~~~~~~~~~~~~~~~~
Use the new-style SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() instead.
Fixes: 1e15f5b911d6 ("drm/tegra: vic: Stop channel on suspend") Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/tegra/vic.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
I see this warning on 5.17-rc8, but did not test it on linux-next, which may already have a fix.
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index 1e342fa3d27b..f56f5921a8c2 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -513,9 +513,8 @@ static int vic_remove(struct platform_device *pdev) }
static const struct dev_pm_ops vic_pm_ops = {
- SET_RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)
- SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
- RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)
- SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
};
struct platform_driver tegra_vic_driver = {
Hi Arnd,
is this a replacement for __maybe_unused annotations that we would typically use to address these? Is the ternary operator in PTR_IF enough to eliminate the warning? Does that work the same way for structure definitions as it does for conditionals where we use IS_ENABLED() to use the compiler's DCE for improved coverage?
It looks like it, but just making sure because there's another patch that fixes this warning by adding __maybe_unused.
Thierry
On Wed, Apr 6, 2022 at 3:21 PM Thierry Reding thierry.reding@gmail.com wrote:
On Wed, Mar 16, 2022 at 07:36:47PM +0100, Arnd Bergmann wrote:
static const struct dev_pm_ops vic_pm_ops = {
SET_RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,pm_runtime_force_resume)
RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)};
struct platform_driver tegra_vic_driver = {
Hi Arnd,
is this a replacement for __maybe_unused annotations that we would typically use to address these? Is the ternary operator in PTR_IF enough to eliminate the warning? Does that work the same way for structure definitions as it does for conditionals where we use IS_ENABLED() to use the compiler's DCE for improved coverage?
Yes to all three.
It looks like it, but just making sure because there's another patch that fixes this warning by adding __maybe_unused.
I sent a lot of patches in the past to add __maybe_unused, but this was mainly because we could never come up with a good replacement. Paul Cercueil has finally come up with a good solution, so this is how we should do it from now on.
Arnd
Hi
Am 16.03.22 um 19:36 schrieb Arnd Bergmann:
From: Arnd Bergmann arnd@arndb.de
The driver fails to build when the KMS helpers are disabled:
ld.lld: error: undefined symbol: drm_gem_fb_get_obj
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_fb_dirty) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_gem_fb_begin_cpu_access
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_swab
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_xrgb8888_to_rgb565
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_memcpy
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
This is fairly hard to hit in randconfig drivers, but it eventually did trigger for me in a configuration where all other DRM drivers are loadable modules, but DRM_PANEL_WIDECHIPS_WS2401 was built-in.
Adding a dependency in all drivers that select DRM_MIPI_DBI avoids the problem for now, adding the dependency in DRM_MIPI_DBI as well should help make it easier to figure out why it breaks if someone forgets the dependency the next time.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/Kconfig | 2 +- drivers/gpu/drm/panel/Kconfig | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)
I see this warning on 5.17-rc8, but did not test it on linux-next, which may already have a fix.
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index b1f22e457fd0..d5ec0b77c010 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -30,7 +30,7 @@ menuconfig DRM
config DRM_MIPI_DBI tristate
- depends on DRM
- depends on DRM_KMS_HELPER
This symbol cannot be selected by users, so it's maybe not a good idea to depend on it. In fact, I've had to remove such a statement because it created a cyclic dependency. [1]
Making the drivers depend on KMS helpers is the right thing though. If there's a better solution, please let me know.
Best regards Thomas
config DRM_MIPI_DSI bool diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 0aec5a10b064..96887d0efb9f 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -425,6 +425,7 @@ config DRM_PANEL_SAMSUNG_DB7430 tristate "Samsung DB7430-based DPI panels" depends on OF && SPI && GPIOLIB depends on BACKLIGHT_CLASS_DEVICE
- depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Samsung
@@ -440,6 +441,7 @@ config DRM_PANEL_SAMSUNG_S6D16D0 config DRM_PANEL_SAMSUNG_S6D27A1 tristate "Samsung S6D27A1 DPI panel driver" depends on OF && SPI && GPIOLIB
- depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Samsung
@@ -476,6 +478,7 @@ config DRM_PANEL_SAMSUNG_S6E63M0_SPI depends on SPI depends on DRM_PANEL_SAMSUNG_S6E63M0 default DRM_PANEL_SAMSUNG_S6E63M0
- depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to be able to access the Samsung
@@ -677,6 +680,7 @@ config DRM_PANEL_WIDECHIPS_WS2401 tristate "Widechips WS2401 DPI panel driver" depends on SPI && GPIOLIB depends on BACKLIGHT_CLASS_DEVICE
- depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Widechips WS2401 DPI
Am 16.03.22 um 20:12 schrieb Thomas Zimmermann:
Hi
Am 16.03.22 um 19:36 schrieb Arnd Bergmann:
From: Arnd Bergmann arnd@arndb.de
The driver fails to build when the KMS helpers are disabled:
ld.lld: error: undefined symbol: drm_gem_fb_get_obj
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_fb_dirty) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_gem_fb_begin_cpu_access
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_swab
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_xrgb8888_to_rgb565
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
ld.lld: error: undefined symbol: drm_fb_memcpy
referenced by drm_mipi_dbi.c gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
This is fairly hard to hit in randconfig drivers, but it eventually did trigger for me in a configuration where all other DRM drivers are loadable modules, but DRM_PANEL_WIDECHIPS_WS2401 was built-in.
Adding a dependency in all drivers that select DRM_MIPI_DBI avoids the problem for now, adding the dependency in DRM_MIPI_DBI as well should help make it easier to figure out why it breaks if someone forgets the dependency the next time.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/gpu/drm/Kconfig | 2 +- drivers/gpu/drm/panel/Kconfig | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)
I see this warning on 5.17-rc8, but did not test it on linux-next, which may already have a fix.
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index b1f22e457fd0..d5ec0b77c010 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -30,7 +30,7 @@ menuconfig DRM config DRM_MIPI_DBI tristate - depends on DRM + depends on DRM_KMS_HELPER
This symbol cannot be selected by users, so it's maybe not a good idea to depend on it. In fact, I've had to remove such a statement because it created a cyclic dependency. [1]
[1] https://lore.kernel.org/dri-devel/20220315084559.23510-1-tzimmermann@suse.de...
Making the drivers depend on KMS helpers is the right thing though. If there's a better solution, please let me know.
Best regards Thomas
config DRM_MIPI_DSI bool diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 0aec5a10b064..96887d0efb9f 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -425,6 +425,7 @@ config DRM_PANEL_SAMSUNG_DB7430 tristate "Samsung DB7430-based DPI panels" depends on OF && SPI && GPIOLIB depends on BACKLIGHT_CLASS_DEVICE + depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Samsung @@ -440,6 +441,7 @@ config DRM_PANEL_SAMSUNG_S6D16D0 config DRM_PANEL_SAMSUNG_S6D27A1 tristate "Samsung S6D27A1 DPI panel driver" depends on OF && SPI && GPIOLIB + depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Samsung @@ -476,6 +478,7 @@ config DRM_PANEL_SAMSUNG_S6E63M0_SPI depends on SPI depends on DRM_PANEL_SAMSUNG_S6E63M0 default DRM_PANEL_SAMSUNG_S6E63M0 + depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to be able to access the Samsung @@ -677,6 +680,7 @@ config DRM_PANEL_WIDECHIPS_WS2401 tristate "Widechips WS2401 DPI panel driver" depends on SPI && GPIOLIB depends on BACKLIGHT_CLASS_DEVICE + depends on DRM_KMS_HELPER select DRM_MIPI_DBI help Say Y here if you want to enable support for the Widechips WS2401 DPI
On Wed, Mar 16, 2022 at 8:31 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Am 16.03.22 um 20:12 schrieb Thomas Zimmermann:
Adding a dependency in all drivers that select DRM_MIPI_DBI avoids the problem for now, adding the dependency in DRM_MIPI_DBI as well should help make it easier to figure out why it breaks if someone forgets the dependency the next time.
tristate
- depends on DRM
- depends on DRM_KMS_HELPER
This symbol cannot be selected by users, so it's maybe not a good idea to depend on it. In fact, I've had to remove such a statement because it created a cyclic dependency. [1]
I tried to explain above what I was thinking here: the added dependency is both a correct statement (DRM_MIPI_DBI depends on DRM_KMS_HELPER because it cannot be built without DRM_KMS_HELPER) and helpful as an indication what went wrong if we run into the same problem with a new driver, instead of the cryptic link failure you get something like
WARNING: unmet direct dependencies detected for DRM_MIPI_DBI Depends on [m]: HAS_IOMEM [=y] && DRM_KMS_HELPER [=m] Selected by [y]: - DRM_PANEL_WIDECHIPS_WS2401 [=y] && HAS_IOMEM [=y] && DRM [=y] && DRM_PANEL [=y] && SPI [=y] && GPIOLIB [=y] && BACKLIGHT_CLASS_DEVICE [=y] Selected by [m]: - TINYDRM_ILI9225 [=m] && HAS_IOMEM [=y] && DRM [=y] && SPI [=y]
[1] https://lore.kernel.org/dri-devel/20220315084559.23510-1-tzimmermann@suse.de...
I was going for 'depends on' in the panel drivers because I saw the same being done for other panel drivers, and mixing the two methods causes dependency loops. I looked again now, and find that 'select DRM_KMS_HELPER' is more common for other drivers, and makes sense here because it is generally not user-selectable.
The easiest replacement for my patch would then be to just use 'select DRM_KMS_HELPER' from CONFIG_DRM_MIPI_DBI, which makes it safer and more consistent with your change. If you like, I'll send an updated version.
One thing I'm not sure about is whether there is still use for ever having CONFIG_DRM without CONFIG_DRM_KMS_HELPER if it gets selected by almost every driver anyway. Is this actually a configuration that users rely on, or should we just remove the symbol completely and build the KMS helpers unconditionally?
Arnd
Hi Arnd
Am 16.03.22 um 21:59 schrieb Arnd Bergmann:
On Wed, Mar 16, 2022 at 8:31 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Am 16.03.22 um 20:12 schrieb Thomas Zimmermann:
Adding a dependency in all drivers that select DRM_MIPI_DBI avoids the problem for now, adding the dependency in DRM_MIPI_DBI as well should help make it easier to figure out why it breaks if someone forgets the dependency the next time.
tristate
- depends on DRM
- depends on DRM_KMS_HELPER
This symbol cannot be selected by users, so it's maybe not a good idea to depend on it. In fact, I've had to remove such a statement because it created a cyclic dependency. [1]
I tried to explain above what I was thinking here: the added dependency is both a correct statement (DRM_MIPI_DBI depends on DRM_KMS_HELPER because it cannot be built without DRM_KMS_HELPER) and helpful as an indication what went wrong if we run into the same problem with a new driver, instead of the cryptic link failure you get something like
WARNING: unmet direct dependencies detected for DRM_MIPI_DBI Depends on [m]: HAS_IOMEM [=y] && DRM_KMS_HELPER [=m] Selected by [y]:
- DRM_PANEL_WIDECHIPS_WS2401 [=y] && HAS_IOMEM [=y] && DRM [=y] &&
DRM_PANEL [=y] && SPI [=y] && GPIOLIB [=y] && BACKLIGHT_CLASS_DEVICE [=y] Selected by [m]:
- TINYDRM_ILI9225 [=m] && HAS_IOMEM [=y] && DRM [=y] && SPI [=y]
[1] https://lore.kernel.org/dri-devel/20220315084559.23510-1-tzimmermann@suse.de...
I was going for 'depends on' in the panel drivers because I saw the same being done for other panel drivers, and mixing the two methods causes dependency loops. I looked again now, and find that 'select DRM_KMS_HELPER' is more common for other drivers, and makes sense here because it is generally not user-selectable.
The easiest replacement for my patch would then be to just use 'select DRM_KMS_HELPER' from CONFIG_DRM_MIPI_DBI, which makes it safer and more consistent with your change. If you like, I'll send an updated version.
MIPI DBI is another helper and select is not transitive IIRC. So drivers would still have to select KMS helpers as well. (?)
I just added my patch to drm-misc-fixes today and it should show up in upstream soon. Maybe just rebase your patch on top of it and if nothing breaks let's merge it as-is including the 'depends on'. You can add
Acked-by: Thomas Zimmermann tzimmermann@suse.de
in this case.
More generally, I think you're right about making DRM helper libraries using 'depends on' to link to other libraries. Drivers would at least know which config symbols to select. A number of config rules would have to be adapted to make that happen, I guess.
One issue is that different submodules of DRM seem to use different logic for expressing such config dependencies. That's been an endless source of problems so far.
One thing I'm not sure about is whether there is still use for ever having CONFIG_DRM without CONFIG_DRM_KMS_HELPER if it gets selected by almost every driver anyway. Is this actually a configuration that users rely on, or should we just remove the symbol completely and build the KMS helpers unconditionally?
Best leave it as it is. i915 doesn't use it. And since it's a helper, it should not be lumped together with core DRM code simply for reasons of design.
For DRM_KMS_HELPER itself, the mid-term plan is to move some of the code into other modules. KMS helpers used to contain all kind of helpers, but recently there's interest in reducing the minimum size of a built-in DRM with minimal driver support. So the non-essential stuff needs to go into modules for the more-sophisticated DRM drivers.
Best regards Thomas
Arnd
On Thu, Mar 17, 2022 at 8:15 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Am 16.03.22 um 21:59 schrieb Arnd Bergmann:
On Wed, Mar 16, 2022 at 8:31 PM Thomas Zimmermann tzimmermann@suse.de wrote:
I was going for 'depends on' in the panel drivers because I saw the same being done for other panel drivers, and mixing the two methods causes dependency loops. I looked again now, and find that 'select DRM_KMS_HELPER' is more common for other drivers, and makes sense here because it is generally not user-selectable.
The easiest replacement for my patch would then be to just use 'select DRM_KMS_HELPER' from CONFIG_DRM_MIPI_DBI, which makes it safer and more consistent with your change. If you like, I'll send an updated version.
MIPI DBI is another helper and select is not transitive IIRC. So drivers would still have to select KMS helpers as well. (?)
Not sure what you mean here: if a driver selects DRM_MIPI_DBI, and DRM_MIPI_DBI selects DRM_KMS_HELPER, the leaf driver does not need to select DRM_KMS_HELPER because it is already selected. This is one of the major problems of overusing 'select' because you end up unable to turn things off.
Maybe you are thinking of the case where DRM_MIPI_DBI depends on DRM_KMS_HELPER, and something selects DRM_MIPI_DBI. In this case, the dependency does /not/ get inherited by the leaf driver, it needs a copy of the dependency or it triggers a warning, which is what my patch intended.
More generally, I think you're right about making DRM helper libraries using 'depends on' to link to other libraries. Drivers would at least know which config symbols to select. A number of config rules would have to be adapted to make that happen, I guess.
Generally speaking, a problem with DRM is that it uses way too much 'select' to enforce other subsystems to be enabled, this is what causes DRM to have more problems with incorrect or circular dependencies, and the only way to avoid that is to be consistent about the dependencies: each symbol should only be referenced with either 'select' or 'depends on' but not both, and 'select' should ideally only be used on hidden symbols.
One thing I'm not sure about is whether there is still use for ever having CONFIG_DRM without CONFIG_DRM_KMS_HELPER if it gets selected by almost every driver anyway. Is this actually a configuration that users rely on, or should we just remove the symbol completely and build the KMS helpers unconditionally?
Best leave it as it is. i915 doesn't use it. And since it's a helper, it should not be lumped together with core DRM code simply for reasons of design.
Ok
For DRM_KMS_HELPER itself, the mid-term plan is to move some of the code into other modules. KMS helpers used to contain all kind of helpers, but recently there's interest in reducing the minimum size of a built-in DRM with minimal driver support. So the non-essential stuff needs to go into modules for the more-sophisticated DRM drivers.
Right, that makes sense.
Arnd
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Thierry Reding -
Thomas Zimmermann