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:   Mon, 5 Sep 2022 15:37:55 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Mateusz Kwiatkowski <kfyatek@gmail.com>
Cc:     Ben Skeggs <bskeggs@redhat.com>, David Airlie <airlied@linux.ie>,
        Chen-Yu Tsai <wens@csie.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Lyude Paul <lyude@redhat.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Samuel Holland <samuel@sholland.org>,
        Karol Herbst <kherbst@redhat.com>,
        Noralf =?utf-8?Q?Tr=C3=B8nnes?= <noralf@tronnes.org>,
        Emma Anholt <emma@anholt.net>, Daniel Vetter <daniel@ffwll.ch>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Hans de Goede <hdegoede@redhat.com>,
        linux-arm-kernel@lists.infradead.org,
        Phil Elwell <phil@raspberrypi.com>,
        intel-gfx@lists.freedesktop.org,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        dri-devel@lists.freedesktop.org, Dom Cobley <dom@raspberrypi.com>,
        linux-kernel@vger.kernel.org, nouveau@lists.freedesktop.org,
        linux-sunxi@lists.linux.dev,
        Geert Uytterhoeven <geert@linux-m68k.org>
Subject: Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog
 display modes
Message-ID: <20220905133755.gcmmntg3wnecyqjq@houat>
References: <20220728-rpi-analog-tv-properties-v2-0-459522d653a7@cerno.tech>
 <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech>
 <242d272b-5b79-986c-9aaf-64e62f6b37ff@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="duymfdwojjs2xq73"
Content-Disposition: inline
In-Reply-To: <242d272b-5b79-986c-9aaf-64e62f6b37ff@gmail.com>
Precedence: bulk


--duymfdwojjs2xq73
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Aug 31, 2022 at 03:44:52AM +0200, Mateusz Kwiatkowski wrote:
> > +#define NTSC_HFP_DURATION_TYP_NS=A0=A0 =A01500
> > +#define NTSC_HFP_DURATION_MIN_NS=A0=A0 =A01270
> > +#define NTSC_HFP_DURATION_MAX_NS=A0=A0 =A02220
>=20
> You've defined those min/typ/max ranges, but you're not using the "typ" f=
ield
> for anything other than hslen.

Yeah... I've left most of them because it was so hard to find most of
them, it's useful at least for documentation purposes. And it's a define
so there's pretty much no downside to it as far as the final binary is
involved.

> The actual "typical" value is thus always the midpoint, which isn't
> necessarily the best choice.
>=20
> In particular, for the standard 720px wide modes at 13.5 MHz, hsync_start
> ends up being 735 for 480i and 734 for 576i, instead of 736 and 732 reque=
sted
> by BT.601. That's all obviously within tolerances, but the image ends up
> noticeably off-center (at least on modern TVs), especially in the 576i ca=
se.

I'll try to fix that up.

> > +=A0=A0 =A0htotal =3D params->line_duration_ns * pixel_clock_hz / NSEC_=
PER_SEC;
>=20
> You're multiplying an unsigned int and an unsigned long - both types are =
only
> required to be 32 bit, so this is likely to overflow. You need to use a c=
ast to
> unsigned long long, and then call do_div() for 64-bit division.
>=20
> This actually overflowed on me on my Pi running ARM32 kernel, resulting in
> negative horizontal porch lengths, and drm_helper_probe_add_cmdline_mode()
> taking over the mode generation (badly), and a horrible mess on screen.

Indeed, that's bad.

> > +=A0=A0 =A0vfp =3D vfp_min + (porches_rem / 2);
> > +=A0=A0 =A0vbp =3D porches - vfp;
>=20
> Relative position of the vertical sync within the VBI effectively moves t=
he
> image up and down. Adding that (porches_rem / 2) moves the image up off c=
enter
> by that many pixels. I'd keep the VFP always at minimum to keep the image
> centered.

And you would increase the back porch only then?

Maxime

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

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

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYxX7swAKCRDj7w1vZxhR
xYNaAP9smEnRh5n1ZGmEenQrzOJSe2qrC45ne/SGvv82vvsHOAD/S1/3mpdTYjRp
q9UQRTTfatktAcjCnUHE0eBICMwRtA4=
=w772
-----END PGP SIGNATURE-----

--duymfdwojjs2xq73--