Messages in this thread | | | Date | Tue, 24 Jan 2023 23:38:55 +0100 | From | Michael Grzeschik <> | Subject | Re: [PATCH] hantro: Fix JPEG encoder ENUM_FRAMESIZE on RK3399 |
| |
On Fri, Dec 23, 2022 at 12:05:21PM -0500, Nicolas Dufresne wrote: >Le vendredi 23 décembre 2022 à 13:28 -0300, Ezequiel Garcia a écrit : >> 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 :-) > >I see, and gstreamer had a quirk for such a bogus response. Let me explain why >its bogus, for the general knowlege. A driver that supports ENUM_FRAMESIZE but >does not return any sizes, is in theory a driver that does not support anything. >Fortunaly, GStreamer considered not having a single framesize bogus, and would >fallback to the old school try_fmt() dance to find the supported sizes. > >So yes, it used to work in gstreamer, and its indeed >79c987de8b35421a2a975982929f150dd415f8f7 that broke it. I'll correct his in V2. > >> >> 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. > >So I'll respin along these line to we more or less "revert back" to working >state. Though having a framesize enumeration on encoder raw (OUTPUT queue) is >what makes most sense so that will have to be revisited with a corrected >mechanism, as whenever we add VP8 and H.264 encoding, we'll need different range >per codec. I'll check in January with my colleague, we might do that inside the >VP8 encoder branch (that is nearly ready and will be sent after the holidays), >or could be an intermediate set.
I just came across this discussion and found my very similar and somehow forgotten patch the other day.
https://lore.kernel.org/linux-media/66839e0c4b19eb4faba5fbed5cd0a4ec0c8415f8.camel@ndufresne.ca/
Should I just send a v2 with the ENOTTY for now?
Thanks, Michael
>> > 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! >> > > >_______________________________________________ >linux-arm-kernel mailing list >linux-arm-kernel@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [unhandled content-type:application/pgp-signature] | |