linux-kernel - Re: [PATCH v6 22/23] drm/vc4: vec: Add support for more analog TV standards

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 for Android: free password hash cracker in your pocket

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

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

Hi,

On Thu, Oct 27, 2022 at 12:32:50AM +0200, Mateusz Kwiatkowski wrote:
> I've seen that you've incorporated my PAL60 patch. Thanks!
> 
> I still yet need to test your v6 changes, but looking at this code with just my
> mental static analysis, it seems to me that the vc4_vec_encoder_atomic_check()
> should have the tv_mode validation. I should've added it to the PAL60 patch,
> but it somehow slipped my mind then.
> 
> Anyway, I mentioned it previously here:
> https://lore.kernel.org/dri-devel/0f2beec2-ae8e-5579-f0b6-a73d9dae1af4@gmail.com/
> 
> It would look something like this, inside vc4_vec_encoder_atomic_check():
> 
> +	const struct vc4_vec_tv_mode *tv_mode =
> +		vc4_vec_tv_mode_lookup(conn_state->tv.mode);
> +
> +	if (!tv_mode)
> +		return -EINVAL;
> 
> Without this, it's possible to set e.g. 480i mode and SECAM, which will fail -
> but with the current version it will only fail in vc4_vec_encoder_enable(),
> which cannot return an error, and in my experience that causes a rather lengthy
> lockup.

ACK, I'll add it.

> But, like I said, I still need to actually test that with this version.
>
> Anyway, I was also thinking about adding support for the more "exotic"
> non-standard modes. NTSC-50 is, unfortunately, impossible with VEC, but
> PAL-N-60 and PAL-M-50 should work. The necessary vc4_vec_tv_modes entries would
> look something like:
> 
> @@ -325,12 +325,28 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>  		.config0 = VEC_CONFIG0_PAL_M_STD,
>  		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
>  	},
> +	{
> +		/* PAL-M-50 */
> +		.mode = DRM_MODE_TV_MODE_PAL,
> +		.expected_htotal = 864,
> +		.config0 = VEC_CONFIG0_PAL_BDGHI_STD,
> +		.config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
> +		.custom_freq = 0x21e6efe3,
> +	},
>  	{
>  		.mode = DRM_MODE_TV_MODE_PAL_N,
>  		.expected_htotal = 864,
>  		.config0 = VEC_CONFIG0_PAL_N_STD,
>  		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
>  	},
> +	{
> +		/* PAL-N-60 */
> +		.mode = DRM_MODE_TV_MODE_PAL_N,
> +		.expected_htotal = 858,
> +		.config0 = VEC_CONFIG0_PAL_M_STD,
> +		.config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
> +		.custom_freq = 0x21f69446,
> +	},
>  	{
>  		.mode = DRM_MODE_TV_MODE_SECAM,
>  		.expected_htotal = 864,
> 
> I'm not sure if we actually want to add that. The two arguments for doing so
> I can think of is 1. it should work, so "why not", 2. it means that more modes
> will result in _some_ kind of a valid signal, rather than erroring out, which
> is always a plus in my book. I can also think of a hypothetical use case, like
> someone in South America with an old PAL-N-only set that would nevertheless
> still sync at 60 Hz (perhaps with the help of messing with vertical hold knob),
> who would like to play retro games at 60 Hz in color.
> 
> But on the other hand, I admit that this scenario is likely a stretch and the
> number of people who would actually use it is probably close to the proverbial
> two ;) So it's your call, I'm just leaving those settings here just in case.

This series is already pretty massive and is difficult to merge, so I'd
rather avoid to add new stuff in at every version. The changes look easy
enough to be a follow-up patch, so I'd prefer to do it that way.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)