Nouveau - Oct 2022 - [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints

From: Mateusz Kwiatkowski <kfyatek+publicgit at 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?nnes <noralf at tronnes.org>
Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit at gmail.com>
Signed-off-by: Maxime Ripard <maxime at cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_vec.c | 48 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.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,
 					struct drm_crtc_state *crtc_state,
 					struct drm_connector_state *conn_state)
 {
+	const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
 	const struct vc4_vec_tv_mode *vec_mode;
 
 	vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode];
@@ -461,6 +462,53 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder
*encoder,
 	    !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode))
 		return -EINVAL;
 
+	if (mode->crtc_hdisplay % 4)
+		return -EINVAL;
+
+	if (!(mode->crtc_hsync_end - mode->crtc_hsync_start))
+		return -EINVAL;
+
+	switch (mode->vtotal) {
+	case 525:
+		if (mode->crtc_vtotal > 262)
+			return -EINVAL;
+
+		if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 253)
+			return -EINVAL;
+
+		if (!(mode->crtc_vsync_start - mode->crtc_vdisplay))
+			return -EINVAL;
+
+		if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3)
+			return -EINVAL;
+
+		if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 4)
+			return -EINVAL;
+
+		break;
+
+	case 625:
+		if (mode->crtc_vtotal > 312)
+			return -EINVAL;
+
+		if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 305)
+			return -EINVAL;
+
+		if (!(mode->crtc_vsync_start - mode->crtc_vdisplay))
+			return -EINVAL;
+
+		if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3)
+			return -EINVAL;
+
+		if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 2)
+			return -EINVAL;
+
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
 	return 0;
 }
 

-- 
b4 0.11.0-dev-7da52

Hi Maxime,

W dniu 13.10.2022 o 15:19, Maxime Ripard pisze:
> From: Mateusz Kwiatkowski > > 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?nnes > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by:
Maxime Ripard > --- > drivers/gpu/drm/vc4/vc4_vec.c | 48
+++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48
insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c
b/drivers/gpu/drm/vc4/vc4_vec.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, > struct
drm_crtc_state *crtc_state, > struct drm_connector_state *conn_state) > {
> + const struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> const struct vc4_vec_tv_mode *vec_mode; > > vec_mode
&vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > @@ -461,6 +462,53 @@
static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, >
!drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) >
return -EINVAL; > > + if (mode->crtc_hdisplay % 4) > + return
-EINVAL; > + > + if (!(mode->crtc_hsync_end -
mode->crtc_hsync_start)) > + return -EINVAL; > + > + switch
(mode->vtotal) { > + case 525: > + if (mode->crtc_vtotal > 262)
> + return -EINVAL; > + > + if (mode->crtc_vdisplay < 1 ||
mode->crtc_vdisplay > 253) > + return -EINVAL; > + > + if
(!(mode->crtc_vsync_start - mode->crtc_vdisplay)) > + return -EINVAL;
> + > + if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3)
> + return -EINVAL; > + > + if ((mode->crtc_vtotal -
mode->crtc_vsync_end) < 4) > + return -EINVAL; > + > + break;
> + > + case 625: > + if (mode->crtc_vtotal > 312) > + return
-EINVAL; > + > + if (mode->crtc_vdisplay < 1 ||
mode->crtc_vdisplay > 305) > + return -EINVAL; > + > + if
(!(mode->crtc_vsync_start - mode->crtc_vdisplay)) > + return -EINVAL;
> + > + if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3)
> + return -EINVAL; > + > + if ((mode->crtc_vtotal -
mode->crtc_vsync_end) < 2) > + return -EINVAL; > + > + break;
> + > + default: > + return -EINVAL; > + } > + > return 0;
> } 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
impossible. 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 seem a bit weird, but any other value of htotal causes the VEC to output
garbage anyway. Best regards, Mateusz Kwiatkowski