Re: [PATCH 7/8] drm/bridge: fix dependency for lvds-encoder

[Arnd Bergmann]

On Mon, May 28, 2018 at 10:06 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Mon, May 28, 2018 at 10:02 AM, Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>> Hi Arnd,
>>
>> Thank you for the patch.
>>
>> On Friday, 25 May 2018 18:50:14 EEST Arnd Bergmann wrote:
>>> The DRM panel bridge code is built into the kms helpers module, so we
>>> get a link error when trying to use it from a built-in driver while the
>>> kms helper is a loadable module:
>>>
>>> drivers/gpu/drm/bridge/lvds-encoder.o: In function `lvds_encoder_probe':
>>> lvds-encoder.c:(.text+0x124): undefined reference to
>>> `devm_drm_panel_bridge_add'
>>>
>>> This adds a the same dependency in the lvds-encoder that we use for all
>>> the other users of the panel bridge. I did not bisect the problem, but
>>> from inspection it seems to date back to the patch that separated out
>>> the panel bridge from lvds encoder.
>>>
>>> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the
>>> lvds-encoder bridge.") Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index 6caa47834194..cf47bfa7a050 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -46,6 +46,7 @@ config DRM_DUMB_VGA_DAC
>>>  config DRM_LVDS_ENCODER
>>>       tristate "Transparent parallel to LVDS encoder support"
>>>       depends on OF
>>> +     select DRM_KMS_HELPER
>>>       select DRM_PANEL_BRIDGE
>>>       help
>>>         Support for transparent parallel to LVDS encoders that don't require
>>
>> Wouldn't it be better to apply the following ?
>>
>> config DRM_PANEL_BRIDGE
>>         def_bool y
>>         depends on DRM_BRIDGE
>> -       depends on DRM_KMS_HELPER
>> +       select DRM_KMS_HELPER
>>         select DRM_PANEL
>>         help
>>            DRM bridge wrapper of DRM panels
>>
>> Otherwise you'll potentially have to patch every user of DRM_PANEL_BRIDGE as
>> done in this patch.
>
> Select isn't recursive, so this won't work unfortunately :-/

The problem is a bit different: select *is* recursive, which is part of the
reason we normally try to avoid it (it gets hard to disable certain
symbols or turn them into modules when there are lots of things selecting
them).

However, DRM_PANEL_BRIDGE is a silent 'bool' symbol that is
always enabled when DRM_BRIDGE is enabled. Making it 'select
DRM_KMS_HELPER' would lead to DRM_KMS_HELPER always
being built-in even if all other DRM drivers are configured as
loadable modules! Note these Makefile line in drivers/gpu/drm:

drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o

The intention is definitely that drm_kms_helper can be a loadable module,
(it cannot be built-in when CONFIG_DRM=m) and the panel bridge
is simply a component that gets linked into it, as of commit 123387d5efa6
("drm/bridge: Build the panel wrapper in drm_kms_helper").

       Arnd