* [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
@ 2022-03-16 18:36 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-03-16 18:36 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Daniel Vetter, Thierry Reding
Cc: Arnd Bergmann, Sam Ravnborg, Alex Deucher, Kees Cook,
Lukas Bulwahn, Jani Nikula, Deepak Rawat,
Javier Martinez Canillas, Linus Walleij, Douglas Anderson,
Noralf Trønnes, AngeloGioacchino Del Regno, Dmitry Baryshkov,
Dillon Min, dri-devel, linux-kernel
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
--
2.29.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] drm/tegra: vic: fix unused-function warnings
2022-03-16 18:36 ` Arnd Bergmann
@ 2022-03-16 18:36 ` Arnd Bergmann
-1 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-03-16 18:36 UTC (permalink / raw)
To: Thierry Reding, David Airlie, Daniel Vetter, Jonathan Hunter,
Dmitry Osipenko, Ulf Hansson
Cc: Arnd Bergmann, Mikko Perttunen, Robin Murphy, dri-devel,
linux-tegra, linux-kernel
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 = {
--
2.29.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH] drm/tegra: vic: fix unused-function warnings
@ 2022-03-16 18:36 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-03-16 18:36 UTC (permalink / raw)
To: Thierry Reding, David Airlie, Daniel Vetter, Jonathan Hunter,
Dmitry Osipenko, Ulf Hansson
Cc: Arnd Bergmann, linux-kernel, dri-devel, Mikko Perttunen,
linux-tegra, Robin Murphy
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 = {
--
2.29.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] drm/tegra: vic: fix unused-function warnings
2022-03-16 18:36 ` Arnd Bergmann
@ 2022-04-06 13:21 ` Thierry Reding
-1 siblings, 0 replies; 18+ messages in thread
From: Thierry Reding @ 2022-04-06 13:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Airlie, Daniel Vetter, Jonathan Hunter, Dmitry Osipenko,
Ulf Hansson, Arnd Bergmann, Mikko Perttunen, Robin Murphy,
dri-devel, linux-tegra, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/tegra: vic: fix unused-function warnings
@ 2022-04-06 13:21 ` Thierry Reding
0 siblings, 0 replies; 18+ messages in thread
From: Thierry Reding @ 2022-04-06 13:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ulf Hansson, Arnd Bergmann, David Airlie, linux-kernel, dri-devel,
Jonathan Hunter, linux-tegra, Dmitry Osipenko, Mikko Perttunen,
Robin Murphy
[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/tegra: vic: fix unused-function warnings
2022-04-06 13:21 ` Thierry Reding
@ 2022-04-06 15:51 ` Arnd Bergmann
-1 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-04-06 15:51 UTC (permalink / raw)
To: Thierry Reding
Cc: David Airlie, Daniel Vetter, Jonathan Hunter, Dmitry Osipenko,
Ulf Hansson, Arnd Bergmann, Mikko Perttunen, Robin Murphy,
dri-devel, open list:TEGRA ARCHITECTURE SUPPORT,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/tegra: vic: fix unused-function warnings
@ 2022-04-06 15:51 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-04-06 15:51 UTC (permalink / raw)
To: Thierry Reding
Cc: Ulf Hansson, Arnd Bergmann, David Airlie,
Linux Kernel Mailing List, dri-devel, Jonathan Hunter,
open list:TEGRA ARCHITECTURE SUPPORT, Dmitry Osipenko,
Mikko Perttunen, Robin Murphy
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
2022-03-16 18:36 ` Arnd Bergmann
@ 2022-03-16 19:12 ` Thomas Zimmermann
-1 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-03-16 19:12 UTC (permalink / raw)
To: Arnd Bergmann, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Thierry Reding
Cc: Lukas Bulwahn, Kees Cook, Arnd Bergmann, Jani Nikula,
Sam Ravnborg, Javier Martinez Canillas, dri-devel,
Douglas Anderson, Deepak Rawat, Noralf Trønnes,
AngeloGioacchino Del Regno, Dmitry Baryshkov, Alex Deucher,
Dillon Min, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 4335 bytes --]
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
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
@ 2022-03-16 19:12 ` Thomas Zimmermann
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-03-16 19:12 UTC (permalink / raw)
To: Arnd Bergmann, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Thierry Reding
Cc: Kees Cook, Arnd Bergmann, Jani Nikula, Dillon Min,
Javier Martinez Canillas, dri-devel, Douglas Anderson,
Deepak Rawat, Noralf Trønnes, Dmitry Baryshkov,
AngeloGioacchino Del Regno, Alex Deucher, Lukas Bulwahn,
Sam Ravnborg, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 4335 bytes --]
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
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
2022-03-16 19:12 ` Thomas Zimmermann
@ 2022-03-16 19:31 ` Thomas Zimmermann
-1 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-03-16 19:31 UTC (permalink / raw)
To: Arnd Bergmann, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Thierry Reding
Cc: Alex Deucher, Kees Cook, Arnd Bergmann, Jani Nikula, Dillon Min,
Javier Martinez Canillas, dri-devel, Douglas Anderson,
Deepak Rawat, Noralf Trønnes, AngeloGioacchino Del Regno,
Dmitry Baryshkov, Lukas Bulwahn, Sam Ravnborg, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 4975 bytes --]
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
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
@ 2022-03-16 19:31 ` Thomas Zimmermann
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-03-16 19:31 UTC (permalink / raw)
To: Arnd Bergmann, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Thierry Reding
Cc: Lukas Bulwahn, Kees Cook, Arnd Bergmann, Jani Nikula,
Sam Ravnborg, Javier Martinez Canillas, dri-devel,
Douglas Anderson, Deepak Rawat, Noralf Trønnes,
AngeloGioacchino Del Regno, Dmitry Baryshkov, Alex Deucher,
Dillon Min, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 4975 bytes --]
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
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
2022-03-16 19:31 ` Thomas Zimmermann
@ 2022-03-16 20:59 ` Arnd Bergmann
-1 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-03-16 20:59 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Alex Deucher, Noralf Trønnes, Kees Cook, Arnd Bergmann,
Deepak Rawat, David Airlie, Dillon Min, Javier Martinez Canillas,
Douglas Anderson, Jani Nikula, Thierry Reding, dri-devel,
AngeloGioacchino Del Regno, Dmitry Baryshkov, Lukas Bulwahn,
Sam Ravnborg, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
@ 2022-03-16 20:59 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-03-16 20:59 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Maarten Lankhorst, Maxime Ripard, David Airlie, Daniel Vetter,
Thierry Reding, Lukas Bulwahn, Kees Cook, Arnd Bergmann,
Jani Nikula, Sam Ravnborg, Javier Martinez Canillas, dri-devel,
Douglas Anderson, Deepak Rawat, Noralf Trønnes,
AngeloGioacchino Del Regno, Dmitry Baryshkov, Alex Deucher,
Dillon Min, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
2022-03-16 20:59 ` Arnd Bergmann
@ 2022-03-17 19:15 ` Thomas Zimmermann
-1 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-03-17 19:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Alex Deucher, Noralf Trønnes, Kees Cook, Arnd Bergmann,
Deepak Rawat, David Airlie, Dillon Min, Javier Martinez Canillas,
Douglas Anderson, Jani Nikula, Thierry Reding, dri-devel,
AngeloGioacchino Del Regno, Dmitry Baryshkov, Lukas Bulwahn,
Sam Ravnborg, Linux Kernel Mailing List
[-- Attachment #1.1: Type: text/plain, Size: 4105 bytes --]
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
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
@ 2022-03-17 19:15 ` Thomas Zimmermann
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-03-17 19:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Maarten Lankhorst, Maxime Ripard, David Airlie, Daniel Vetter,
Thierry Reding, Lukas Bulwahn, Kees Cook, Arnd Bergmann,
Jani Nikula, Sam Ravnborg, Javier Martinez Canillas, dri-devel,
Douglas Anderson, Deepak Rawat, Noralf Trønnes,
AngeloGioacchino Del Regno, Dmitry Baryshkov, Alex Deucher,
Dillon Min, Linux Kernel Mailing List
[-- Attachment #1.1: Type: text/plain, Size: 4105 bytes --]
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
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
2022-03-17 19:15 ` Thomas Zimmermann
@ 2022-03-17 20:54 ` Arnd Bergmann
-1 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-03-17 20:54 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Alex Deucher, Noralf Trønnes, Kees Cook, Arnd Bergmann,
Deepak Rawat, David Airlie, Dillon Min, Javier Martinez Canillas,
Douglas Anderson, Jani Nikula, Thierry Reding, dri-devel,
AngeloGioacchino Del Regno, Dmitry Baryshkov, Lukas Bulwahn,
Sam Ravnborg, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies
@ 2022-03-17 20:54 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2022-03-17 20:54 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Maarten Lankhorst, Maxime Ripard, David Airlie, Daniel Vetter,
Thierry Reding, Lukas Bulwahn, Kees Cook, Arnd Bergmann,
Jani Nikula, Sam Ravnborg, Javier Martinez Canillas, dri-devel,
Douglas Anderson, Deepak Rawat, Noralf Trønnes,
AngeloGioacchino Del Regno, Dmitry Baryshkov, Alex Deucher,
Dillon Min, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 18+ messages in thread