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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20221018073416.7tttm6mnbt2pdrpk@houat>
Date:   Tue, 18 Oct 2022 09:34:16 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Noralf Trønnes <noralf@...nnes.org>
Cc:     Jernej Skrabec <jernej.skrabec@...il.com>,
        Chen-Yu Tsai <wens@...e.org>,
        Karol Herbst <kherbst@...hat.com>,
        Samuel Holland <samuel@...lland.org>,
        Lyude Paul <lyude@...hat.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Emma Anholt <emma@...olt.net>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Ben Skeggs <bskeggs@...hat.com>,
        David Airlie <airlied@...ux.ie>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        linux-arm-kernel@...ts.infradead.org,
        dri-devel@...ts.freedesktop.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        intel-gfx@...ts.freedesktop.org, linux-sunxi@...ts.linux.dev,
        Hans de Goede <hdegoede@...hat.com>,
        nouveau@...ts.freedesktop.org,
        Mateusz Kwiatkowski <kfyatek+publicgit@...il.com>,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        linux-kernel@...r.kernel.org, Dom Cobley <dom@...pberrypi.com>,
        Phil Elwell <phil@...pberrypi.com>
Subject: Re: [PATCH v4 11/30] drm/modes: Add a function to generate analog
 display modes

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@...no.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@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.

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@intel.com/

Maxime