Re: [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints
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
> >
> > 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.
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
signature.asc
Description: PGP signature
Re: [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
>
> 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
Re: [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
[PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints
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;
}
--
b4 0.11.0-dev-7da52
