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

[Jani Nikula]

On Thu, 07 Apr 2022, Thomas Zimmermann <tzimmermann at suse.de> wrote:
> 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 at 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.

Documentation/kbuild/kconfig-language.rst:

  Note:
	select should be used with care. select will force
	a symbol to a value without visiting the dependencies.
	By abusing select you are able to select a symbol FOO even
	if FOO depends on BAR that is not set.
-->	In general use select only for non-visible symbols
-->	(no prompts anywhere) and for symbols with no dependencies.
	That will limit the usefulness but on the other hand avoid
	the illegal configurations all over.

Most of the difficult Kconfig issues I've encountered over the years
come from not following the above two rules. People break those rules
for "convenience", causing a lot of inconvenience down the line.


BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Graphics Center