Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Mateusz Kwiatkowski <kfyatek@gmail.com>
Message-ID: <31759a06-ddd0-6b78-0e10-8e8754f394f9@gmail.com>
Date:   Sun, 16 Oct 2022 20:16:32 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.3
Subject: Re: [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints
Content-Language: pl
To:     Maxime Ripard <maxime@cerno.tech>,
        Karol Herbst <kherbst@redhat.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        David Airlie <airlied@linux.ie>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Lyude Paul <lyude@redhat.com>,
        Maxime Ripard <mripard@kernel.org>,
        Emma Anholt <emma@anholt.net>, Chen-Yu Tsai <wens@csie.org>,
        Samuel Holland <samuel@sholland.org>,
        Ben Skeggs <bskeggs@redhat.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Jernej Skrabec <jernej.skrabec@gmail.com>
Cc:     Dom Cobley <dom@raspberrypi.com>, linux-sunxi@lists.linux.dev,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= <noralf@tronnes.org>,
        intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        nouveau@lists.freedesktop.org,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        linux-arm-kernel@lists.infradead.org,
        dri-devel@lists.freedesktop.org,
        Hans de Goede <hdegoede@redhat.com>,
        Phil Elwell <phil@raspberrypi.com>
Reply-To: kfyatek+publicgit@gmail.com
References: <20220728-rpi-analog-tv-properties-v5-0-d841cc64fe4b@cerno.tech>
 <20220728-rpi-analog-tv-properties-v5-19-d841cc64fe4b@cerno.tech>
 <270c4a51-0a03-f048-46da-ab37900f8b91@gmail.com>
In-Reply-To: <270c4a51-0a03-f048-46da-ab37900f8b91@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@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@tronnes.org>
> Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@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