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
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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mateusz Kwiatkowski <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> 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
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Mateusz Kwiatkowski <[email protected]>
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> >? 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.
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