Re: [PATCH 3/8] drm/display: Introduce a DRM display-helper module

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi Javier

Am 07.04.22 um 09:43 schrieb Javier Martinez Canillas:
> On 4/6/22 21:08, Thomas Zimmermann wrote:
>> Hi Javier
>>
>> Am 30.03.22 um 11:23 schrieb Javier Martinez Canillas:
>>> On 3/22/22 20:27, Thomas Zimmermann wrote:
>>>> Replace the DP-helper module with a display-helper module. Update
>>>> all related Kconfig and Makefile rules.
>>>>
>>>> Besides the existing code for DisplayPort, the new module will
>>>> contain helpers for other video-output standards, such as HDMI.
>>>> Drivers will still be able to select the required video-output
>>>> helpers. Linking all such code into a single module avoids the
>>>> proliferation of small kernel modules.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +config DRM_DISPLAY_HELPER
>>>> +	tristate
>>>> +	depends on DRM
>>>> +	help
>>>> +	  DRM helpers for display adapters.
>>>> +
>>>>    config DRM_DP_HELPER
>>>>    	tristate
>>>>    	depends on DRM
>>>> +	select DRM_DISPLAY_HELPER
>>>>    	help
>>>>    	  DRM helpers for DisplayPort.
>>>>
>>>
>>> I was about to ask why this would still be needed but then re-read the
>>> commit message that says drivers will still be able to select required
>>> video-output helpers.
>>>
>>> That makes sense since the fact that all helpers will be in the same module
>>> would be transparent to drivers.
>>
>> After some more testing, it turns out to be not so easy. For example, if
>> we have DP_HELPER=m and HDMI_HELPER=y, then DISPLAY_HELPER would be
>> auto-selected as 'y'. The code for DP_HELPER would not be linked correctly.
>>
>> I'm going to make drivers select DISPLAY_HELPER and the rsp helpers
>> explicitly. The individual helpers would be covered boolean options that
>> enable the feature in the display-helper library.
>>
>> If you know some Kconfig magic to enable the original design, let me know.
>>
> 
> I do not. But I wonder if the problem here is the usage of select rather than
> depends and if with the later the original design could still be achieved...

With 'depends DRM_DISPLAY_HELPER' users would need to explictly enable 
DRM_DISPLAY_HELPER, I think.

> 
> But yes, probably the only way to prevent that issue is to make the drivers
> to explicitly select both DRM_DISPLAY_HELPER and respective helpers symbol.

I'll remake the patches with the new style.

I think another idea that could work is to use an intermediate symbol. 
For DP, drivers would select the tristate DP_HELPER, which in turn 
selects tristate DISPLAY_HELPER and boolean DISPLAY_DP_HELPER.  But this 
would require a 'useless' symbol DP_HELPER only for convenience.  It's 
an even less optimal solution, it seems.

Best regards
Thomas

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