The DRM_DP_AUX_CHARDEV and DRM_DP_CEC Kconfig symbols enable code that use DP helper functions, that are only present if CONFIG_DRM_DISPLAY_DP_HELPER is also enabled.
But these don't select the DRM_DISPLAY_DP_HELPER symbol, meaning that it is possible to enable any of them without CONFIG_DRM_DISPLAY_DP_HELPER.
That will lead to the following linking errors with the mentioned config:
LD vmlinux.o MODPOST vmlinux.symvers MODINFO modules.builtin.modinfo GEN modules.builtin LD .tmp_vmlinux.kallsyms1 KSYMS .tmp_vmlinux.kallsyms1.S AS .tmp_vmlinux.kallsyms1.S LD .tmp_vmlinux.kallsyms2 KSYMS .tmp_vmlinux.kallsyms2.S AS .tmp_vmlinux.kallsyms2.S LD vmlinux SYSMAP System.map SORTTAB vmlinux OBJCOPY arch/arm64/boot/Image MODPOST modules-only.symvers ERROR: modpost: "drm_dp_dpcd_write" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! ERROR: modpost: "drm_dp_read_desc" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! ERROR: modpost: "drm_dp_dpcd_read" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! make[1]: *** [scripts/Makefile.modpost:134: modules-only.symvers] Error 1 make[1]: *** Deleting file 'modules-only.symvers' make: *** [Makefile:1749: modules] Error 2
Besides making these symbols to select CONFIG_DRM_DISPLAY_DP_HELPER, make them to depend on DRM_DISPLAY_HELPER, since can't be enabled without it.
Note: It seems this has been an issue for a long time but was made easier to reproduce after the commit 1e0f66420b13 ("drm/display: Introduce a DRM display-helper module"). Adding a Fixes: tag just to make sure that this fix will be picked for stable once the mentioned change also lands there.
Fixes: 1e0f66420b13 ("drm/display: Introduce a DRM display-helper module") Signed-off-by: Javier Martinez Canillas javierm@redhat.com ---
Changes in v3: - Also make DRM_DP_AUX_CHARDEV and DRM_DP_CEC depend on DRM_DISPLAY_HELPER (Thomas Zimmermann).
Changes in v2: - Explain better the issue in the change description. - Only select DRM_DISPLAY_DP_HELPER and not DRM_DISPLAY_HELPER.
drivers/gpu/drm/display/Kconfig | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig index f84f1b0cd23f..1b6e6af37546 100644 --- a/drivers/gpu/drm/display/Kconfig +++ b/drivers/gpu/drm/display/Kconfig @@ -31,7 +31,8 @@ config DRM_DISPLAY_HDMI_HELPER
config DRM_DP_AUX_CHARDEV bool "DRM DP AUX Interface" - depends on DRM + depends on DRM && DRM_DISPLAY_HELPER + select DRM_DISPLAY_DP_HELPER help Choose this option to enable a /dev/drm_dp_auxN node that allows to read and write values to arbitrary DPCD registers on the DP aux @@ -39,7 +40,8 @@ config DRM_DP_AUX_CHARDEV
config DRM_DP_CEC bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support" - depends on DRM + depends on DRM && DRM_DISPLAY_HELPER + select DRM_DISPLAY_DP_HELPER select CEC_CORE help Choose this option if you want to enable HDMI CEC support for
Hi
Am 28.04.22 um 10:22 schrieb Javier Martinez Canillas:
The DRM_DP_AUX_CHARDEV and DRM_DP_CEC Kconfig symbols enable code that use DP helper functions, that are only present if CONFIG_DRM_DISPLAY_DP_HELPER is also enabled.
But these don't select the DRM_DISPLAY_DP_HELPER symbol, meaning that it is possible to enable any of them without CONFIG_DRM_DISPLAY_DP_HELPER.
That will lead to the following linking errors with the mentioned config:
LD vmlinux.o MODPOST vmlinux.symvers MODINFO modules.builtin.modinfo GEN modules.builtin LD .tmp_vmlinux.kallsyms1 KSYMS .tmp_vmlinux.kallsyms1.S AS .tmp_vmlinux.kallsyms1.S LD .tmp_vmlinux.kallsyms2 KSYMS .tmp_vmlinux.kallsyms2.S AS .tmp_vmlinux.kallsyms2.S LD vmlinux SYSMAP System.map SORTTAB vmlinux OBJCOPY arch/arm64/boot/Image MODPOST modules-only.symvers ERROR: modpost: "drm_dp_dpcd_write" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! ERROR: modpost: "drm_dp_read_desc" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! ERROR: modpost: "drm_dp_dpcd_read" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! make[1]: *** [scripts/Makefile.modpost:134: modules-only.symvers] Error 1 make[1]: *** Deleting file 'modules-only.symvers' make: *** [Makefile:1749: modules] Error 2
Besides making these symbols to select CONFIG_DRM_DISPLAY_DP_HELPER, make them to depend on DRM_DISPLAY_HELPER, since can't be enabled without it.
Note: It seems this has been an issue for a long time but was made easier to reproduce after the commit 1e0f66420b13 ("drm/display: Introduce a DRM display-helper module"). Adding a Fixes: tag just to make sure that this fix will be picked for stable once the mentioned change also lands there.
Fixes: 1e0f66420b13 ("drm/display: Introduce a DRM display-helper module") Signed-off-by: Javier Martinez Canillas javierm@redhat.com
Changes in v3:
- Also make DRM_DP_AUX_CHARDEV and DRM_DP_CEC depend on DRM_DISPLAY_HELPER (Thomas Zimmermann).
Changes in v2:
Explain better the issue in the change description.
Only select DRM_DISPLAY_DP_HELPER and not DRM_DISPLAY_HELPER.
drivers/gpu/drm/display/Kconfig | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig index f84f1b0cd23f..1b6e6af37546 100644 --- a/drivers/gpu/drm/display/Kconfig +++ b/drivers/gpu/drm/display/Kconfig @@ -31,7 +31,8 @@ config DRM_DISPLAY_HDMI_HELPER
config DRM_DP_AUX_CHARDEV bool "DRM DP AUX Interface"
- depends on DRM
- depends on DRM && DRM_DISPLAY_HELPER
- select DRM_DISPLAY_DP_HELPER
I'd be OK with that, but I'm still wondering why you're not making it depend on DRM_DISPLAY_DP_HELPER. If a config only enables HDMI (without DP), these options would still show up.
help Choose this option to enable a /dev/drm_dp_auxN node that allows to read and write values to arbitrary DPCD registers on the DP aux @@ -39,7 +40,8 @@ config DRM_DP_AUX_CHARDEV
config DRM_DP_CEC bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
- depends on DRM
- depends on DRM && DRM_DISPLAY_HELPER
- select DRM_DISPLAY_DP_HELPER
Same here.
Best regards Thomas
select CEC_CORE help Choose this option if you want to enable HDMI CEC support for
On 4/28/22 10:42, Thomas Zimmermann wrote:
Hi
[snip]
drivers/gpu/drm/display/Kconfig | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig index f84f1b0cd23f..1b6e6af37546 100644 --- a/drivers/gpu/drm/display/Kconfig +++ b/drivers/gpu/drm/display/Kconfig @@ -31,7 +31,8 @@ config DRM_DISPLAY_HDMI_HELPER
config DRM_DP_AUX_CHARDEV bool "DRM DP AUX Interface"
- depends on DRM
- depends on DRM && DRM_DISPLAY_HELPER
- select DRM_DISPLAY_DP_HELPER
My rationale was that since DRM_DISPLAY_DP_HELPER is not a user-visible symbol but DRM_DP_{AUX_CHARDEV,CEC} are, the latter should be able to be enabled by the user without relying on other drivers to select the required symbols (I would argue that even should select DP_HELPER but that can't be done or otherwise the helper couldn't be built as module).
In other words, either DRM_DP_{AUX_CHARDEV,CEC} can be user selectable as a standalone symbol or can't and will only be visible if other driver selects their required symbols.
In which case, why not just do this non-user visible and just make the drivers using their helpers to select it directly? That is one of the options I proposed before.
So I believe this is the less intrusive change that will preserve the current behaviour as much as possible.
I don't have a strong opinion though and if you prefer I can change to be a depends instead.
Hi
Am 28.04.22 um 11:11 schrieb Javier Martinez Canillas:
On 4/28/22 10:42, Thomas Zimmermann wrote:
Hi
[snip]
drivers/gpu/drm/display/Kconfig | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig index f84f1b0cd23f..1b6e6af37546 100644 --- a/drivers/gpu/drm/display/Kconfig +++ b/drivers/gpu/drm/display/Kconfig @@ -31,7 +31,8 @@ config DRM_DISPLAY_HDMI_HELPER
config DRM_DP_AUX_CHARDEV bool "DRM DP AUX Interface"
- depends on DRM
- depends on DRM && DRM_DISPLAY_HELPER
- select DRM_DISPLAY_DP_HELPER
My rationale was that since DRM_DISPLAY_DP_HELPER is not a user-visible symbol but DRM_DP_{AUX_CHARDEV,CEC} are, the latter should be able to be enabled by the user without relying on other drivers to select the required symbols (I would argue that even should select DP_HELPER but that can't be done or otherwise the helper couldn't be built as module).
In other words, either DRM_DP_{AUX_CHARDEV,CEC} can be user selectable as a standalone symbol or can't and will only be visible if other driver selects their required symbols.
In which case, why not just do this non-user visible and just make the drivers using their helpers to select it directly? That is one of the options I proposed before.
So I believe this is the less intrusive change that will preserve the current behaviour as much as possible.
Makes sense.
Acked-by: Thomas Zimmermann tzimmermann@suse.de
Maybe wait a bit before landing, so that the actual maintainers have a chance to comment.
Best regards Thomas
I don't have a strong opinion though and if you prefer I can change to be a depends instead.
Reviewed-by: Lyude Paul lyude@redhat.com
On Thu, 2022-04-28 at 10:22 +0200, Javier Martinez Canillas wrote:
The DRM_DP_AUX_CHARDEV and DRM_DP_CEC Kconfig symbols enable code that use DP helper functions, that are only present if CONFIG_DRM_DISPLAY_DP_HELPER is also enabled.
But these don't select the DRM_DISPLAY_DP_HELPER symbol, meaning that it is possible to enable any of them without CONFIG_DRM_DISPLAY_DP_HELPER.
That will lead to the following linking errors with the mentioned config:
LD vmlinux.o MODPOST vmlinux.symvers MODINFO modules.builtin.modinfo GEN modules.builtin LD .tmp_vmlinux.kallsyms1 KSYMS .tmp_vmlinux.kallsyms1.S AS .tmp_vmlinux.kallsyms1.S LD .tmp_vmlinux.kallsyms2 KSYMS .tmp_vmlinux.kallsyms2.S AS .tmp_vmlinux.kallsyms2.S LD vmlinux SYSMAP System.map SORTTAB vmlinux OBJCOPY arch/arm64/boot/Image MODPOST modules-only.symvers ERROR: modpost: "drm_dp_dpcd_write" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! ERROR: modpost: "drm_dp_read_desc" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! ERROR: modpost: "drm_dp_dpcd_read" [drivers/gpu/drm/display/drm_display_helper.ko] undefined! make[1]: *** [scripts/Makefile.modpost:134: modules-only.symvers] Error 1 make[1]: *** Deleting file 'modules-only.symvers' make: *** [Makefile:1749: modules] Error 2
Besides making these symbols to select CONFIG_DRM_DISPLAY_DP_HELPER, make them to depend on DRM_DISPLAY_HELPER, since can't be enabled without it.
Note: It seems this has been an issue for a long time but was made easier to reproduce after the commit 1e0f66420b13 ("drm/display: Introduce a DRM display-helper module"). Adding a Fixes: tag just to make sure that this fix will be picked for stable once the mentioned change also lands there.
Fixes: 1e0f66420b13 ("drm/display: Introduce a DRM display-helper module") Signed-off-by: Javier Martinez Canillas javierm@redhat.com
Changes in v3:
- Also make DRM_DP_AUX_CHARDEV and DRM_DP_CEC depend on DRM_DISPLAY_HELPER
(Thomas Zimmermann).
Changes in v2:
- Explain better the issue in the change description.
- Only select DRM_DISPLAY_DP_HELPER and not DRM_DISPLAY_HELPER.
drivers/gpu/drm/display/Kconfig | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig index f84f1b0cd23f..1b6e6af37546 100644 --- a/drivers/gpu/drm/display/Kconfig +++ b/drivers/gpu/drm/display/Kconfig @@ -31,7 +31,8 @@ config DRM_DISPLAY_HDMI_HELPER config DRM_DP_AUX_CHARDEV bool "DRM DP AUX Interface" - depends on DRM + depends on DRM && DRM_DISPLAY_HELPER + select DRM_DISPLAY_DP_HELPER help Choose this option to enable a /dev/drm_dp_auxN node that allows to read and write values to arbitrary DPCD registers on the DP aux @@ -39,7 +40,8 @@ config DRM_DP_AUX_CHARDEV config DRM_DP_CEC bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support" - depends on DRM + depends on DRM && DRM_DISPLAY_HELPER + select DRM_DISPLAY_DP_HELPER select CEC_CORE help Choose this option if you want to enable HDMI CEC support for
dri-devel@lists.freedesktop.org
-
Javier Martinez Canillas -
Lyude Paul -
Thomas Zimmermann