hantro: Fix JPEG encoder ENUM_FRAMESIZE on RK3399

  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")
Reported-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../platform/verisilicon/rockchip_vpu_hw.c    | 22 ++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)
  

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!
  

Hi,

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.

regards,
Nicolas

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

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
  

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
>