linux-kernel - Re: [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 18 Oct 2022 10:27:13 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     kfyatek+publicgit@...il.com
Cc:     Karol Herbst <kherbst@...hat.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        David Airlie <airlied@...ux.ie>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Lyude Paul <lyude@...hat.com>, Emma Anholt <emma@...olt.net>,
        Chen-Yu Tsai <wens@...e.org>,
        Samuel Holland <samuel@...lland.org>,
        Ben Skeggs <bskeggs@...hat.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Dom Cobley <dom@...pberrypi.com>, linux-sunxi@...ts.linux.dev,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Noralf Trønnes <noralf@...nnes.org>,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        nouveau@...ts.freedesktop.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-arm-kernel@...ts.infradead.org,
        dri-devel@...ts.freedesktop.org,
        Hans de Goede <hdegoede@...hat.com>,
        Phil Elwell <phil@...pberrypi.com>
Subject: 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 <kfyatek+publicgit@...il.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@...nnes.org>
> > Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@...il.com>
> > Signed-off-by: Maxime Ripard <maxime@...no.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.

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)