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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux