| Message ID | 20220316183708.1505846-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: add CONFIG_DRM_KMS_HELPER dependencies | expand |
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
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