| Message ID | 20220428082244.390859-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v3] drm/display: Select DP helper for DRM_DP_AUX_CHARDEV and DRM_DP_CEC | expand |
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
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
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(-)