Hi, On Sat, Oct 15, 2022 at 05:04:50PM +0200, Noralf Trønnes wrote: > Den 13.10.2022 10.36, skrev Maxime Ripard: > > 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. I'd rather not, tbh. We've collectively spent weeks figuring this out, reviewing it and so on, I very much want to avoid doing this all over again if it's going to be useful at some point. Jani also wanted to expose a function and not a raw mode, so this patch also addresses that: https://lore.kernel.org/dri-devel/8735eeg31e.fsf@xxxxxxxxx/ Maxime
