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;
MIME-Version: 1.0
References: <20221223141644.703088-1-nicolas.dufresne@collabora.com>
In-Reply-To: <20221223141644.703088-1-nicolas.dufresne@collabora.com>
From:   Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Date:   Fri, 23 Dec 2022 13:28:03 -0300
Message-ID: <CAAEAJfATk+jOq3qt-m2CZEbCVumHHWDFfuEXKA7k0NZQXajCRg@mail.gmail.com>
Subject: Re: [PATCH] hantro: Fix JPEG encoder ENUM_FRAMESIZE on RK3399
To:     Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc:     Philipp Zabel <p.zabel@pengutronix.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>, kernel@collabora.com,
        Robert Mader <robert.mader@collabora.com>,
        Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
        linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi everyone,

On Fri, Dec 23, 2022 at 11:17 AM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> The frmsize structure was left initialize to 0, as side effect, the driver was
> reporting an invalid frmsize.
>
>   Size: Stepwise 0x0 - 0x0 with step 0/0
>
> Fix this by replicating the constraints in the raw formats too. This fixes
> taking picture in Gnome Cheese Software, or any software using GSteamer
> encodebin feature.
>
> Fixes: 775fec69008d30 ("media: add Rockchip VPU JPEG encoder driver")

The frmsize is only for bitstream formats (see comment in struct hantro_fmt).
If I can read code correctly, this was broken by this commit:

commit 79c987de8b35421a2a975982929f150dd415f8f7
Author: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Date:   Mon Apr 4 18:06:40 2022 +0200

    media: hantro: Use post processor scaling capacities

Before that commit we used to return EINVAL for enum_framesizes
in RAW formats. I guess we missed that :-)

To be completely honest, I'm not sure if we used to support encodebin,
and I'm not too sure how to approach this issue, but I would really
love to start with something super simple like:

diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
b/drivers/media/platform/verisilicon/hantro_v4l2.c
index 2c7a805289e7..0b28f86b7463 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -161,8 +161,11 @@ static int vidioc_enum_framesizes(struct file
*file, void *priv,
        }

        /* For non-coded formats check if postprocessing scaling is possible */
-       if (fmt->codec_mode == HANTRO_MODE_NONE &&
hantro_needs_postproc(ctx, fmt)) {
-               return hanto_postproc_enum_framesizes(ctx, fsize);
+       if (fmt->codec_mode == HANTRO_MODE_NONE)
+        if (hantro_needs_postproc(ctx, fmt))
+            return hanto_postproc_enum_framesizes(ctx, fsize);
+        else
+            return -ENOTTY;
        } else if (fsize->index != 0) {
                vpu_debug(0, "invalid frame size index (expected 0, got %d)\n",
                          fsize->index);

(ENOTTY was suggested by Nicolas on IRC)

Nicolas also pointed out that our current handling of frmsize is not correct,
as we cannot express different constraints on combinations of RAW
and bitstream formats.

This seems to call for a rework of enum_framesizes, so frmsize
is not static but somehow obtained per-codec.

Thanks,
Ezequiel

> Reported-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

And thanks a lot for the report and the patch!