* [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC
@ 2022-04-28 8:22 Javier Martinez Canillas
2022-04-28 8:42 ` Thomas Zimmermann
2022-04-28 18:07 ` Lyude Paul
0 siblings, 2 replies; 5+ messages in thread
From: Javier Martinez Canillas @ 2022-04-28 8:22 UTC (permalink / raw)
To: linux-kernel
Cc: Lyude Paul, Jani Nikula, Thomas Zimmermann,
Javier Martinez Canillas, Daniel Vetter, David Airlie, dri-devel
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
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC
2022-04-28 8:22 [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC Javier Martinez Canillas
@ 2022-04-28 8:42 ` Thomas Zimmermann
2022-04-28 9:11 ` Javier Martinez Canillas
2022-04-28 18:07 ` Lyude Paul
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Zimmermann @ 2022-04-28 8:42 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel, Jani Nikula, Lyude Paul
Cc: David Airlie, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 3792 bytes --]
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
--
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] 5+ messages in thread
* Re: [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC
2022-04-28 8:42 ` Thomas Zimmermann
@ 2022-04-28 9:11 ` Javier Martinez Canillas
2022-04-28 10:08 ` Thomas Zimmermann
0 siblings, 1 reply; 5+ messages in thread
From: Javier Martinez Canillas @ 2022-04-28 9:11 UTC (permalink / raw)
To: Thomas Zimmermann, linux-kernel, Jani Nikula, Lyude Paul
Cc: David Airlie, dri-devel
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.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC
2022-04-28 9:11 ` Javier Martinez Canillas
@ 2022-04-28 10:08 ` Thomas Zimmermann
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Zimmermann @ 2022-04-28 10:08 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel, Jani Nikula, Lyude Paul
Cc: David Airlie, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 2053 bytes --]
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.
>
--
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] 5+ messages in thread
* Re: [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC
2022-04-28 8:22 [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC Javier Martinez Canillas
2022-04-28 8:42 ` Thomas Zimmermann
@ 2022-04-28 18:07 ` Lyude Paul
1 sibling, 0 replies; 5+ messages in thread
From: Lyude Paul @ 2022-04-28 18:07 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: Jani Nikula, Thomas Zimmermann, Daniel Vetter, David Airlie, dri-devel
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
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-28 18:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-28 8:22 [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC Javier Martinez Canillas
2022-04-28 8:42 ` Thomas Zimmermann
2022-04-28 9:11 ` Javier Martinez Canillas
2022-04-28 10:08 ` Thomas Zimmermann
2022-04-28 18:07 ` Lyude Paul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).