Nouveau - Oct 2022 - [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes

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 at cerno.tech>
> > 
> > ---
> > 
> > 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
at mail.gmail.com/

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20221013/eda72592/attachment.sig>

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 at cerno.tech>
>>>
>>> ---
>>>
>>> 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
at mail.gmail.com/
> 
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.