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

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

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 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
index f84f1b0cd23f..9fe80c4e555c 100644
--- a/drivers/gpu/drm/display/Kconfig
+++ b/drivers/gpu/drm/display/Kconfig
@@ -32,6 +32,7 @@ config DRM_DISPLAY_HDMI_HELPER
 config DRM_DP_AUX_CHARDEV
 	bool "DRM DP AUX Interface"
 	depends on DRM
+	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
@@ -40,6 +41,7 @@ config DRM_DP_AUX_CHARDEV
 config DRM_DP_CEC
 	bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
 	depends on DRM
+	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 v2] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3397 bytes --]

Hi

Am 27.04.22 um 23:55 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
> 
> 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 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 | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
> index f84f1b0cd23f..9fe80c4e555c 100644
> --- a/drivers/gpu/drm/display/Kconfig
> +++ b/drivers/gpu/drm/display/Kconfig
> @@ -32,6 +32,7 @@ config DRM_DISPLAY_HDMI_HELPER
>   config DRM_DP_AUX_CHARDEV
>   	bool "DRM DP AUX Interface"
>   	depends on DRM
> +	select DRM_DISPLAY_DP_HELPER

You cannot select DISPLAY_DP_HELPER without DISPLAY_HELPER.

Can't you simply make it depend on DISPLAY_DP_HELPER.  The menu entry 
will show up as soon as there's a driver that selcets DISPLAY_DP_HELPER.

Best regards
Thomas

>   	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
> @@ -40,6 +41,7 @@ config DRM_DP_AUX_CHARDEV
>   config DRM_DP_CEC
>   	bool "Enable DisplayPort CEC-Tunneling-over-AUX HDMI support"
>   	depends on DRM
> +	select DRM_DISPLAY_DP_HELPER
>   	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 v2] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[Javier Martinez Canillas]

Hello Thomas,

Thanks for your feedback.

On 4/28/22 09:02, Thomas Zimmermann wrote:

[snip]

>> 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 | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
>> index f84f1b0cd23f..9fe80c4e555c 100644
>> --- a/drivers/gpu/drm/display/Kconfig
>> +++ b/drivers/gpu/drm/display/Kconfig
>> @@ -32,6 +32,7 @@ config DRM_DISPLAY_HDMI_HELPER
>>   config DRM_DP_AUX_CHARDEV
>>   	bool "DRM DP AUX Interface"
>>   	depends on DRM
>> +	select DRM_DISPLAY_DP_HELPER
> 
> You cannot select DISPLAY_DP_HELPER without DISPLAY_HELPER.
>

That was my original thought as well and what did in v1, but then I noticed
that doing that it would force DRM_DISPLAY_HELPER to be set as built-in and
not allow to be built as a module.
 
> Can't you simply make it depend on DISPLAY_DP_HELPER.  The menu entry 
> will show up as soon as there's a driver that selcets DISPLAY_DP_HELPER.
>

I could but then that means that once won't be able to select these two config
options unless some enable symbol selects DRM_DISPLAY_DP_HELPER.

In my opinion, DRM_DP_AUX_CHARDEV and DRM_DP_CEC are different than all other
options that select DRM_DISPLAY_DP_HELPER, since those are drivers and want to
have both DRM_DISPLAY_DP_HELPER and DRM_DISPLAY_HELPER set.

But DRM_DP_AUX_CHARDEV and DRM_DP_CEC are just included in drm_display_helper.o
if enabled, and depend on symbols that are present if CONFIG_DRM_DISPLAY_DP_HELPER
is enabled. So just need the latter, if DRM_DISPLAY_HELPER is not enabled then it
will just be a no-op.

Having written that though I noticed that a "depends on DRM_DISPLAY_HELPER" makes
sense. If you agree I can add it and post a v3.

Now, pondering more about this issue, probably the most correct thing to do is for
the drivers that make use of the symbols exported by DRM_DP_{AUX_CHARDEV,CEC} to
select these. What do you think ?
 -- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3220 bytes --]

Hi

Am 28.04.22 um 09:26 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> Thanks for your feedback.
> 
> On 4/28/22 09:02, Thomas Zimmermann wrote:
> 
> [snip]
> 
>>> 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 | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig
>>> index f84f1b0cd23f..9fe80c4e555c 100644
>>> --- a/drivers/gpu/drm/display/Kconfig
>>> +++ b/drivers/gpu/drm/display/Kconfig
>>> @@ -32,6 +32,7 @@ config DRM_DISPLAY_HDMI_HELPER
>>>    config DRM_DP_AUX_CHARDEV
>>>    	bool "DRM DP AUX Interface"
>>>    	depends on DRM
>>> +	select DRM_DISPLAY_DP_HELPER
>>
>> You cannot select DISPLAY_DP_HELPER without DISPLAY_HELPER.
>>
> 
> That was my original thought as well and what did in v1, but then I noticed
> that doing that it would force DRM_DISPLAY_HELPER to be set as built-in and
> not allow to be built as a module.

It was a rhetorical only. I didn't mean to actually set DISPLAY_HELPER.

>   
>> Can't you simply make it depend on DISPLAY_DP_HELPER.  The menu entry
>> will show up as soon as there's a driver that selcets DISPLAY_DP_HELPER.
>>
> 
> I could but then that means that once won't be able to select these two config
> options unless some enable symbol selects DRM_DISPLAY_DP_HELPER.
> 
> In my opinion, DRM_DP_AUX_CHARDEV and DRM_DP_CEC are different than all other
> options that select DRM_DISPLAY_DP_HELPER, since those are drivers and want to
> have both DRM_DISPLAY_DP_HELPER and DRM_DISPLAY_HELPER set.
> 
> But DRM_DP_AUX_CHARDEV and DRM_DP_CEC are just included in drm_display_helper.o
> if enabled, and depend on symbols that are present if CONFIG_DRM_DISPLAY_DP_HELPER
> is enabled. So just need the latter, if DRM_DISPLAY_HELPER is not enabled then it
> will just be a no-op.
> 
> Having written that though I noticed that a "depends on DRM_DISPLAY_HELPER" makes
> sense. If you agree I can add it and post a v3.

Yes please.  These options enable features of the DP code. If there's no 
driver with DP, it doesn't make sense to allow them.

I know that there could be an odd situation where userspace might not 
have DP, but still wants the chardev file of aux bus.  But that 
situation existed already when the code was located within KMS helpers.

> 
> Now, pondering more about this issue, probably the most correct thing to do is for
> the drivers that make use of the symbols exported by DRM_DP_{AUX_CHARDEV,CEC} to
> select these. What do you think ?

That's not considered good style. Select should not be used for anything 
that is user-configurable. [1]

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.17.4/source/Documentation/kbuild/kconfig-language.rst#L148

>   --
> Best regards,
> 
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
> 

-- 
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 v2] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[Javier Martinez Canillas]

On 4/28/22 09:45, Thomas Zimmermann wrote:

[snip]

>>> You cannot select DISPLAY_DP_HELPER without DISPLAY_HELPER.
>>>
>>
>> That was my original thought as well and what did in v1, but then I noticed
>> that doing that it would force DRM_DISPLAY_HELPER to be set as built-in and
>> not allow to be built as a module.
> 
> It was a rhetorical only. I didn't mean to actually set DISPLAY_HELPER.
> 

Ah, sorry for misunderstanding.

>>   
>>> Can't you simply make it depend on DISPLAY_DP_HELPER.  The menu entry
>>> will show up as soon as there's a driver that selcets DISPLAY_DP_HELPER.
>>>
>>
>> I could but then that means that once won't be able to select these two config
>> options unless some enable symbol selects DRM_DISPLAY_DP_HELPER.
>>
>> In my opinion, DRM_DP_AUX_CHARDEV and DRM_DP_CEC are different than all other
>> options that select DRM_DISPLAY_DP_HELPER, since those are drivers and want to
>> have both DRM_DISPLAY_DP_HELPER and DRM_DISPLAY_HELPER set.
>>
>> But DRM_DP_AUX_CHARDEV and DRM_DP_CEC are just included in drm_display_helper.o
>> if enabled, and depend on symbols that are present if CONFIG_DRM_DISPLAY_DP_HELPER
>> is enabled. So just need the latter, if DRM_DISPLAY_HELPER is not enabled then it
>> will just be a no-op.
>>
>> Having written that though I noticed that a "depends on DRM_DISPLAY_HELPER" makes
>> sense. If you agree I can add it and post a v3.
> 
> Yes please.  These options enable features of the DP code. If there's no 
> driver with DP, it doesn't make sense to allow them.
> 
> I know that there could be an odd situation where userspace might not 
> have DP, but still wants the chardev file of aux bus.  But that 
> situation existed already when the code was located within KMS helpers.
> 

Agreed.

>>
>> Now, pondering more about this issue, probably the most correct thing to do is for
>> the drivers that make use of the symbols exported by DRM_DP_{AUX_CHARDEV,CEC} to
>> select these. What do you think ?
> 
> That's not considered good style. Select should not be used for anything 
> that is user-configurable. [1]
>

Right. So giving even more thought to this, now I think that we should just include
drm_dp_aux_dev.o, drm_dp_cec.o (and probably drm_dp_aux_bus.o?) unconditionally to
drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER).

After all, these are not big objects and drm_display_helper can now be built as module.

I don't see that much value to have separate user-configurable config options...

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3152 bytes --]

Hi

Am 28.04.22 um 09:52 schrieb Javier Martinez Canillas:
> On 4/28/22 09:45, Thomas Zimmermann wrote:
> 
> [snip]
> 
>>>> You cannot select DISPLAY_DP_HELPER without DISPLAY_HELPER.
>>>>
>>>
>>> That was my original thought as well and what did in v1, but then I noticed
>>> that doing that it would force DRM_DISPLAY_HELPER to be set as built-in and
>>> not allow to be built as a module.
>>
>> It was a rhetorical only. I didn't mean to actually set DISPLAY_HELPER.
>>
> 
> Ah, sorry for misunderstanding.
> 
>>>    
>>>> Can't you simply make it depend on DISPLAY_DP_HELPER.  The menu entry
>>>> will show up as soon as there's a driver that selcets DISPLAY_DP_HELPER.
>>>>
>>>
>>> I could but then that means that once won't be able to select these two config
>>> options unless some enable symbol selects DRM_DISPLAY_DP_HELPER.
>>>
>>> In my opinion, DRM_DP_AUX_CHARDEV and DRM_DP_CEC are different than all other
>>> options that select DRM_DISPLAY_DP_HELPER, since those are drivers and want to
>>> have both DRM_DISPLAY_DP_HELPER and DRM_DISPLAY_HELPER set.
>>>
>>> But DRM_DP_AUX_CHARDEV and DRM_DP_CEC are just included in drm_display_helper.o
>>> if enabled, and depend on symbols that are present if CONFIG_DRM_DISPLAY_DP_HELPER
>>> is enabled. So just need the latter, if DRM_DISPLAY_HELPER is not enabled then it
>>> will just be a no-op.
>>>
>>> Having written that though I noticed that a "depends on DRM_DISPLAY_HELPER" makes
>>> sense. If you agree I can add it and post a v3.
>>
>> Yes please.  These options enable features of the DP code. If there's no
>> driver with DP, it doesn't make sense to allow them.
>>
>> I know that there could be an odd situation where userspace might not
>> have DP, but still wants the chardev file of aux bus.  But that
>> situation existed already when the code was located within KMS helpers.
>>
> 
> Agreed.
> 
>>>
>>> Now, pondering more about this issue, probably the most correct thing to do is for
>>> the drivers that make use of the symbols exported by DRM_DP_{AUX_CHARDEV,CEC} to
>>> select these. What do you think ?
>>
>> That's not considered good style. Select should not be used for anything
>> that is user-configurable. [1]
>>
> 
> Right. So giving even more thought to this, now I think that we should just include
> drm_dp_aux_dev.o, drm_dp_cec.o (and probably drm_dp_aux_bus.o?) unconditionally to
> drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER).
> 
> After all, these are not big objects and drm_display_helper can now be built as module.
> 
> I don't see that much value to have separate user-configurable config options...
> 

I don't know the side effects of this. We're exporting another device 
file after all.

For know I'd make them depend on DRM_DISPLAY_DP_HELPER. If someone 
complains we can revert and fix the linker error by adding stub 
functions for the missing symbols.

Best regards
Thomas

-- 
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 v2] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2881 bytes --]

(cc Jani and Lyude)

Am 28.04.22 um 09:52 schrieb Javier Martinez Canillas:
> On 4/28/22 09:45, Thomas Zimmermann wrote:
> 
> [snip]
> 
>>>> You cannot select DISPLAY_DP_HELPER without DISPLAY_HELPER.
>>>>
>>>
>>> That was my original thought as well and what did in v1, but then I noticed
>>> that doing that it would force DRM_DISPLAY_HELPER to be set as built-in and
>>> not allow to be built as a module.
>>
>> It was a rhetorical only. I didn't mean to actually set DISPLAY_HELPER.
>>
> 
> Ah, sorry for misunderstanding.
> 
>>>    
>>>> Can't you simply make it depend on DISPLAY_DP_HELPER.  The menu entry
>>>> will show up as soon as there's a driver that selcets DISPLAY_DP_HELPER.
>>>>
>>>
>>> I could but then that means that once won't be able to select these two config
>>> options unless some enable symbol selects DRM_DISPLAY_DP_HELPER.
>>>
>>> In my opinion, DRM_DP_AUX_CHARDEV and DRM_DP_CEC are different than all other
>>> options that select DRM_DISPLAY_DP_HELPER, since those are drivers and want to
>>> have both DRM_DISPLAY_DP_HELPER and DRM_DISPLAY_HELPER set.
>>>
>>> But DRM_DP_AUX_CHARDEV and DRM_DP_CEC are just included in drm_display_helper.o
>>> if enabled, and depend on symbols that are present if CONFIG_DRM_DISPLAY_DP_HELPER
>>> is enabled. So just need the latter, if DRM_DISPLAY_HELPER is not enabled then it
>>> will just be a no-op.
>>>
>>> Having written that though I noticed that a "depends on DRM_DISPLAY_HELPER" makes
>>> sense. If you agree I can add it and post a v3.
>>
>> Yes please.  These options enable features of the DP code. If there's no
>> driver with DP, it doesn't make sense to allow them.
>>
>> I know that there could be an odd situation where userspace might not
>> have DP, but still wants the chardev file of aux bus.  But that
>> situation existed already when the code was located within KMS helpers.
>>
> 
> Agreed.
> 
>>>
>>> Now, pondering more about this issue, probably the most correct thing to do is for
>>> the drivers that make use of the symbols exported by DRM_DP_{AUX_CHARDEV,CEC} to
>>> select these. What do you think ?
>>
>> That's not considered good style. Select should not be used for anything
>> that is user-configurable. [1]
>>
> 
> Right. So giving even more thought to this, now I think that we should just include
> drm_dp_aux_dev.o, drm_dp_cec.o (and probably drm_dp_aux_bus.o?) unconditionally to
> drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER).
> 
> After all, these are not big objects and drm_display_helper can now be built as module.
> 
> I don't see that much value to have separate user-configurable config options...
> 

-- 
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 v2] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC

[Javier Martinez Canillas]

On 4/28/22 10:04, Thomas Zimmermann wrote:

[snip]

>>
>> Right. So giving even more thought to this, now I think that we should just include
>> drm_dp_aux_dev.o, drm_dp_cec.o (and probably drm_dp_aux_bus.o?) unconditionally to
>> drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER).
>>
>> After all, these are not big objects and drm_display_helper can now be built as module.
>>
>> I don't see that much value to have separate user-configurable config options...
>>
> 
> I don't know the side effects of this. We're exporting another device 
> file after all.
> 
> For know I'd make them depend on DRM_DISPLAY_DP_HELPER. If someone 
> complains we can revert and fix the linker error by adding stub 
> functions for the missing symbols.
>

Ok, I'll just add the depends on then to fix the linking errors and then either
adding the stubs when disabled or just making it part of the DP_HELPER can be
done as a follow-up.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat