Re: [PATCH] hantro: Fix JPEG encoder ENUM_FRAMESIZE on RK3399

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le mardi 24 janvier 2023 à 23:38 +0100, Michael Grzeschik a écrit :
> 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?

I was forgetting about this, sorry, ok if you have bandwidth, just update your
patch with ENOTTY, can you extend your comment with what was wrong with the ENUM
(like I did in my patch) ? Also don't forget to add a Fixes tag, this needs
backporting.

> 
> 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
>