[PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[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
 	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

Re: [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[Thomas Zimmermann]

[-- 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 --]

Re: [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[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.

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

Re: [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[Thomas Zimmermann]

[-- 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 --]

Re: [PATCH v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[Lyude Paul]

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