Mateusz Kwiatkowski
2022-Oct-16 18:12 UTC
[Nouveau] [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints
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
Mateusz Kwiatkowski
2022-Oct-16 18:16 UTC
[Nouveau] [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints
Hi Maxime, Sorry about the mess that happened to the previous message. I hope this one will be delivered more cleanly. W dniu 13.10.2022 o 15:19, Maxime Ripard pisze:> 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; >? } > ? >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