Re: [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes

[Noralf Trønnes]

Den 13.10.2022 10.36, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Sat, Oct 01, 2022 at 02:52:06PM +0200, Noralf Trønnes wrote:
>> Den 29.09.2022 18.31, skrev Maxime Ripard:
>>> Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
>>> 625-lines modes in their drivers.
>>>
>>> Since those modes are fairly standard, and that we'll need to use them
>>> in more places in the future, it makes sense to move their definition
>>> into the core framework.
>>>
>>> However, analog display usually have fairly loose timings requirements,
>>> the only discrete parameters being the total number of lines and pixel
>>> clock frequency. Thus, we created a function that will create a display
>>> mode from the standard, the pixel frequency and the active area.
>>>
>>> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
>>>
>>> ---
>>>
>>> Changes in v4:
>>> - Reworded the line length check comment
>>> - Switch to HZ_PER_KHZ in tests
>>> - Use previous timing to fill our mode
>>> - Move the number of lines check earlier
>>> ---
>>>  drivers/gpu/drm/drm_modes.c            | 474 +++++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/tests/Makefile         |   1 +
>>>  drivers/gpu/drm/tests/drm_modes_test.c | 144 ++++++++++
>>>  include/drm/drm_modes.h                |  17 ++
>>>  4 files changed, 636 insertions(+)
>>>
>>
>> I haven't followed the discussion on this patch, but it seems rather
>> excessive to add over 600 lines of code (including tests) to add 2 fixed
>> modes. And it's very difficult to see from the code what the actual
>> display mode timings really are, which would be useful for other
>> developers down the road wanting to use them.
>>
>> Why not just hardcode the modes?
> 
> Yeah, I have kind of the same feeling tbh, but it was asked back on the
> v1 to ease the transition of old fbdev drivers, since they will need
> such a function:
> https://lore.kernel.org/dri-devel/CAMuHMdUrwzPYjA0wdR7ADj5Ov6+m03JbnY8fBYzRYyWDuNm5=g@xxxxxxxxxxxxxx/
> 

If that's the case I suggest you just hardcode them for now and leave
the complexity to the developer doing the actual conversion of the fbdev
driver. Who knows when that will happen, but that person will have your
well documented and discussed work to fall back on.

Noralf.