Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i8771445c:Fastmail
Date:   Thu, 27 Oct 2022 13:58:22 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Mateusz Kwiatkowski <kfyatek@gmail.com>
Cc:     Karol Herbst <kherbst@redhat.com>, Emma Anholt <emma@anholt.net>,
        Ben Skeggs <bskeggs@redhat.com>, Chen-Yu Tsai <wens@csie.org>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Samuel Holland <samuel@sholland.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Lyude Paul <lyude@redhat.com>, linux-sunxi@lists.linux.dev,
        intel-gfx@lists.freedesktop.org,
        Phil Elwell <phil@raspberrypi.com>,
        linux-arm-kernel@lists.infradead.org,
        nouveau@lists.freedesktop.org, Hans de Goede <hdegoede@redhat.com>,
        Dom Cobley <dom@raspberrypi.com>,
        dri-devel@lists.freedesktop.org,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        linux-kernel@vger.kernel.org,
        Noralf =?utf-8?Q?Tr=C3=B8nnes?= <noralf@tronnes.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>
Subject: Re: [PATCH v6 22/23] drm/vc4: vec: Add support for more analog TV
 standards
Message-ID: <20221027115822.5vd3fqlcpy4gfq5v@houat>
References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech>
 <20220728-rpi-analog-tv-properties-v6-22-e7792734108f@cerno.tech>
 <e9b8ebf6-8781-0c55-5dd5-9f5077bd6b93@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="ysfbxxbxsmqctfb3"
Content-Disposition: inline
In-Reply-To: <e9b8ebf6-8781-0c55-5dd5-9f5077bd6b93@gmail.com>
Precedence: bulk


--ysfbxxbxsmqctfb3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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!
>=20
> I still yet need to test your v6 changes, but looking at this code with j=
ust my
> mental static analysis, it seems to me that the vc4_vec_encoder_atomic_ch=
eck()
> should have the tv_mode validation. I should've added it to the PAL60 pat=
ch,
> but it somehow slipped my mind then.
>=20
> Anyway, I mentioned it previously here:
> https://lore.kernel.org/dri-devel/0f2beec2-ae8e-5579-f0b6-a73d9dae1af4@gm=
ail.com/
>=20
> It would look something like this, inside vc4_vec_encoder_atomic_check():
>=20
> +	const struct vc4_vec_tv_mode *tv_mode =3D
> +		vc4_vec_tv_mode_lookup(conn_state->tv.mode);
> +
> +	if (!tv_mode)
> +		return -EINVAL;
>=20
> Without this, it's possible to set e.g. 480i mode and SECAM, which will f=
ail -
> 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 l=
engthy
> 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:
>=20
> @@ -325,12 +325,28 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_mode=
s[] =3D {
>  		.config0 =3D VEC_CONFIG0_PAL_M_STD,
>  		.config1 =3D VEC_CONFIG1_C_CVBS_CVBS,
>  	},
> +	{
> +		/* PAL-M-50 */
> +		.mode =3D DRM_MODE_TV_MODE_PAL,
> +		.expected_htotal =3D 864,
> +		.config0 =3D VEC_CONFIG0_PAL_BDGHI_STD,
> +		.config1 =3D VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
> +		.custom_freq =3D 0x21e6efe3,
> +	},
>  	{
>  		.mode =3D DRM_MODE_TV_MODE_PAL_N,
>  		.expected_htotal =3D 864,
>  		.config0 =3D VEC_CONFIG0_PAL_N_STD,
>  		.config1 =3D VEC_CONFIG1_C_CVBS_CVBS,
>  	},
> +	{
> +		/* PAL-N-60 */
> +		.mode =3D DRM_MODE_TV_MODE_PAL_N,
> +		.expected_htotal =3D 858,
> +		.config0 =3D VEC_CONFIG0_PAL_M_STD,
> +		.config1 =3D VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
> +		.custom_freq =3D 0x21f69446,
> +	},
>  	{
>  		.mode =3D DRM_MODE_TV_MODE_SECAM,
>  		.expected_htotal =3D 864,
>=20
> 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, w=
hich
> 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 neverthele=
ss
> 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.
>=20
> 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 prove=
rbial
> two ;) So it's your call, I'm just leaving those settings here just in ca=
se.

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

--ysfbxxbxsmqctfb3
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY1pyXgAKCRDj7w1vZxhR
xUhMAP0XpvpKwMyQUT0qv5zm8wU1Oj1FqnHhCXeuVVO8jcNQEQEA3oyfkhXsxwGB
0XwSRrhGDf0kHcwS9XXqBCQFEgMYgww=
=04yJ
-----END PGP SIGNATURE-----

--ysfbxxbxsmqctfb3--