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:   Tue, 18 Oct 2022 10:08:06 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     kfyatek+publicgit@gmail.com
Cc:     Karol Herbst <kherbst@redhat.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        David Airlie <airlied@linux.ie>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Lyude Paul <lyude@redhat.com>, Emma Anholt <emma@anholt.net>,
        Chen-Yu Tsai <wens@csie.org>,
        Samuel Holland <samuel@sholland.org>,
        Ben Skeggs <bskeggs@redhat.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Dom Cobley <dom@raspberrypi.com>, linux-sunxi@lists.linux.dev,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        Noralf =?utf-8?Q?Tr=C3=B8nnes?= <noralf@tronnes.org>,
        intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        nouveau@lists.freedesktop.org,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        linux-arm-kernel@lists.infradead.org,
        dri-devel@lists.freedesktop.org,
        Hans de Goede <hdegoede@redhat.com>,
        Phil Elwell <phil@raspberrypi.com>
Subject: Re: [PATCH v5 06/22] drm/modes: Add a function to generate analog
 display modes
Message-ID: <20221018080806.mkw4zbzchlatxgwq@houat>
References: <20220728-rpi-analog-tv-properties-v5-0-d841cc64fe4b@cerno.tech>
 <20220728-rpi-analog-tv-properties-v5-6-d841cc64fe4b@cerno.tech>
 <30dbbcc8-1d14-0fc8-ed7c-0c3f7d094ea3@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="bz2m37ekv7hmvzdb"
Content-Disposition: inline
In-Reply-To: <30dbbcc8-1d14-0fc8-ed7c-0c3f7d094ea3@gmail.com>
Precedence: bulk


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

Hi,

On Sun, Oct 16, 2022 at 07:34:12PM +0200, Mateusz Kwiatkowski wrote:
> Hi Maxime & everyone,
>=20
> Sorry for being inactive in the discussions about this patchset for the l=
ast
> couple of weeks.
>=20
> > +const static struct analog_parameters tv_modes_parameters[] =3D {
> > +	TV_MODE_PARAMETER(DRM_MODE_ANALOG_NTSC,
> > +			  NTSC_LINES_NUMBER,
> > +			  NTSC_LINE_DURATION_NS,
> > +			  PARAM_RANGE(NTSC_HACT_DURATION_MIN_NS,
> > +				      NTSC_HACT_DURATION_TYP_NS,
> > +				      NTSC_HACT_DURATION_MAX_NS),
> > +			  PARAM_RANGE(NTSC_HFP_DURATION_MIN_NS,
> > +				      NTSC_HFP_DURATION_TYP_NS,
> > +				      NTSC_HFP_DURATION_MAX_NS),
> > +			  PARAM_RANGE(NTSC_HSLEN_DURATION_MIN_NS,
> > +				      NTSC_HSLEN_DURATION_TYP_NS,
> > +				      NTSC_HSLEN_DURATION_MAX_NS),
> > +			  PARAM_RANGE(NTSC_HBP_DURATION_MIN_NS,
> > +				      NTSC_HBP_DURATION_TYP_NS,
> > +				      NTSC_HBP_DURATION_MAX_NS),
> > +			  PARAM_RANGE(NTSC_HBLK_DURATION_MIN_NS,
> > +				      NTSC_HBLK_DURATION_TYP_NS,
> > +				      NTSC_HBLK_DURATION_MAX_NS),
> > +			  16,
> > +			  PARAM_FIELD(3, 3),
> > +			  PARAM_FIELD(3, 3),
> > +			  PARAM_FIELD(16, 17)),
> > +	TV_MODE_PARAMETER(DRM_MODE_ANALOG_PAL,
> > +			  PAL_LINES_NUMBER,
> > +			  PAL_LINE_DURATION_NS,
> > +			  PARAM_RANGE(PAL_HACT_DURATION_MIN_NS,
> > +				      PAL_HACT_DURATION_TYP_NS,
> > +				      PAL_HACT_DURATION_MAX_NS),
> > +			  PARAM_RANGE(PAL_HFP_DURATION_MIN_NS,
> > +				      PAL_HFP_DURATION_TYP_NS,
> > +				      PAL_HFP_DURATION_MAX_NS),
> > +			  PARAM_RANGE(PAL_HSLEN_DURATION_MIN_NS,
> > +				      PAL_HSLEN_DURATION_TYP_NS,
> > +				      PAL_HSLEN_DURATION_MAX_NS),
> > +			  PARAM_RANGE(PAL_HBP_DURATION_MIN_NS,
> > +				      PAL_HBP_DURATION_TYP_NS,
> > +				      PAL_HBP_DURATION_MAX_NS),
> > +			  PARAM_RANGE(PAL_HBLK_DURATION_MIN_NS,
> > +				      PAL_HBLK_DURATION_TYP_NS,
> > +				      PAL_HBLK_DURATION_MAX_NS),
> > +			  12,
> > +
> > +			  /*
> > +			   * The front porch is actually 6 short sync
> > +			   * pulses for the even field, and 5 for the
> > +			   * odd field. Each sync takes half a life so
> > +			   * the odd field front porch is shorter by
> > +			   * half a line.
> > +			   *
> > +			   * In progressive, we're supposed to use 6
> > +			   * pulses, so we're fine there
> > +			   */
> > +			  PARAM_FIELD(3, 2),
> > +
> > +			  /*
> > +			   * The vsync length is 5 long sync pulses,
> > +			   * each field taking half a line. We're
> > +			   * shorter for both fields by half a line.
> > +			   *
> > +			   * In progressive, we're supposed to use 5
> > +			   * pulses, so we're off by half
> > +			   * a line.
> > +			   *
> > +			   * In interlace, we're now off by half a line
> > +			   * for the even field and one line for the odd
> > +			   * field.
> > +			   */
> > +			  PARAM_FIELD(3, 3),
> > +
> > +			  /*
> > +			   * The back porch starts with post-equalizing
> > +			   * pulses, consisting in 5 short sync pulses
> > +			   * for the even field, 4 for the odd field. In
> > +			   * progressive, it's 5 short syncs.
> > +			   *
> > +			   * In progressive, we thus have 2.5 lines,
> > +			   * plus the 0.5 line we were missing
> > +			   * previously, so we should use 3 lines.
> > +			   *
> > +			   * In interlace, the even field is in the
> > +			   * exact same case than progressive. For the
> > +			   * odd field, we should be using 2 lines but
> > +			   * we're one line short, so we'll make up for
> > +			   * it here by using 3.
> > +			   *
> > +			   * The entire blanking area is supposed to
> > +			   * take 25 lines, so we also need to account
> > +			   * for the rest of the blanking area that
> > +			   * can't be in either the front porch or sync
> > +			   * period.
> > +			   */
> > +			  PARAM_FIELD(19, 20)),
> > +};
>=20
> Nit: setting vbp limits like that makes it impossible to use
> drm_analog_tv_mode() to generate modes that include the VBI for e.g. emit=
ting
> teletext.
>=20
> This probably doesn't matter, as it can still be created as a custom mode=
 from
> userspace, hence I'm mentioning it as a nit.

Yeah, I think it's out of scope at least for now. Also, the compositor
should probably be aware of the margins being used to put the VBI data,
so expecting userspace to come up with the mode is probably best?

> > +		 * By convention, NSTC (aka 525/60) systems start with
>=20
> Typo: s/NSTC/NTSC/

Fixed, thanks
Maxime

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

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

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY05e5QAKCRDj7w1vZxhR
xR8VAQC+375DtcagkAytZVeOEe9aSmsByYFfuLp2Bt0aRSrhEAD/SoTKGvPkbrp3
DYtTf34PAYs3gXueCuNbfrKzWk9/DgM=
=IY6U
-----END PGP SIGNATURE-----

--bz2m37ekv7hmvzdb--