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:27:13 +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 19/22] drm/vc4: vec: Check for VEC output constraints
Message-ID: <20221018082713.kaecdaf6izunpa7t@houat>
References: <20220728-rpi-analog-tv-properties-v5-0-d841cc64fe4b@cerno.tech>
 <20220728-rpi-analog-tv-properties-v5-19-d841cc64fe4b@cerno.tech>
 <270c4a51-0a03-f048-46da-ab37900f8b91@gmail.com>
 <31759a06-ddd0-6b78-0e10-8e8754f394f9@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="khr24myzeaxu6pdr"
Content-Disposition: inline
In-Reply-To: <31759a06-ddd0-6b78-0e10-8e8754f394f9@gmail.com>
Precedence: bulk


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

Hi,

On Sun, Oct 16, 2022 at 08:16:32PM +0200, Mateusz Kwiatkowski wrote:
> W dniu 13.10.2022 o 15:19, Maxime Ripard pisze:
> > From: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> >
> > The VEC can accept pretty much any relatively reasonable mode, but still
> > has a bunch of constraints to meet.
> >
> > Let's create an atomic_check() implementation that will make sure we
> > don't end up accepting a non-functional mode.
> >
> > Acked-by: Noralf Tr=F8nnes <noralf@tronnes.org>
> > Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >=A0 drivers/gpu/drm/vc4/vc4_vec.c | 48 +++++++++++++++++++++++++++++++++=
++++++++++
> >=A0 1 file changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_ve=
c.c
> > index 90e375a8a8f9..1fcb7baf874e 100644
> > --- a/drivers/gpu/drm/vc4/vc4_vec.c
> > +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> > @@ -453,6 +453,7 @@ static int vc4_vec_encoder_atomic_check(struct drm_=
encoder *encoder,
> > =A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0struct drm_crtc_st=
ate *crtc_state,
> > =A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0struct drm_connect=
or_state *conn_state)
> >=A0 {
> > +=A0=A0 =A0const struct drm_display_mode *mode =3D &crtc_state->adjuste=
d_mode;
> > =A0=A0=A0 =A0const struct vc4_vec_tv_mode *vec_mode;
> > =A0
> > =A0=A0=A0 =A0vec_mode =3D &vc4_vec_tv_modes[conn_state->tv.legacy_mode];
> > @@ -461,6 +462,53 @@ static int vc4_vec_encoder_atomic_check(struct drm=
_encoder *encoder,
> > =A0=A0=A0 =A0=A0=A0=A0 !drm_mode_equal(vec_mode->mode, &crtc_state->adj=
usted_mode))
> > =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > =A0
> > +=A0=A0 =A0if (mode->crtc_hdisplay % 4)
> > +=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0if (!(mode->crtc_hsync_end - mode->crtc_hsync_start))
> > +=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0switch (mode->vtotal) {
> > +=A0=A0 =A0case 525:
> > +=A0=A0 =A0=A0=A0 =A0if (mode->crtc_vtotal > 262)
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay=
 > 253)
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0if (!(mode->crtc_vsync_start - mode->crtc_vdisplay=
))
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0if ((mode->crtc_vsync_end - mode->crtc_vsync_start=
) !=3D 3)
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 4)
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0break;
> > +
> > +=A0=A0 =A0case 625:
> > +=A0=A0 =A0=A0=A0 =A0if (mode->crtc_vtotal > 312)
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay=
 > 305)
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0if (!(mode->crtc_vsync_start - mode->crtc_vdisplay=
))
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0if ((mode->crtc_vsync_end - mode->crtc_vsync_start=
) !=3D 3)
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 2)
> > +=A0=A0 =A0=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +
> > +=A0=A0 =A0=A0=A0 =A0break;
> > +
> > +=A0=A0 =A0default:
> > +=A0=A0 =A0=A0=A0 =A0return -EINVAL;
> > +=A0=A0 =A0}
> > +
> > =A0=A0=A0 =A0return 0;
> >=A0 }
> > =A0
> >
>=20
> In my original version of this function
> (https://github.com/raspberrypi/linux/pull/4406/files) the switch is over
> reference_mode->vtotal, not mode->vtotal. This was intended to explicitly=
 allow
> a different value of mode->vtotal, to support non-standard modes, such as=
 "fake"
> 525 lines with SECAM encoding, or the progressive modes.
>
> You're switching over mode->vtotal, which makes specifying those impossib=
le.
> I don't think we should limit the users like that.
>
> We're removing reference_mode in patch 20/22, so adding a switch over
> reference_mode->vtotal is probably not a good idea in that case I'd switch
> over mode->htotal instead: 858 for "NTSC" and 864 for "PAL". This may see=
m a bit
> weird, but any other value of htotal causes the VEC to output garbage any=
way.

Ack, I'll change it.

If it ever causes an issue, we can always switch back to a reference
mode anyway. We'd just have to call drm_mode_analog_tv at each
atomic_check so I'd rather avoid the overhead if we can

Maxime

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

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

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY05jYQAKCRDj7w1vZxhR
xWoTAP0eC45ml3mXGk5QHe/4VqLwsotPDOcBr4EdsQUxCduL0QD9GehziPurHbq2
XDYQ9YzMcem3HRvtKSZYxu6b5/2RiAw=
=6OJV
-----END PGP SIGNATURE-----

--khr24myzeaxu6pdr--