Re: [PATCH] media: venus: dec: Fix capture formats enumeration order

[Enric Balletbo i Serra]

Hi all,

On Tue, Mar 7, 2023 at 9:13 AM Javier Martinez Canillas
<javierm@xxxxxxxxxx> wrote:
>
> Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx> writes:
>
> Hello Dikshita,
>
> > On 3/6/2023 3:38 PM, Javier Martinez Canillas wrote:
> >> Jordan Crouse <jorcrous@xxxxxxxxxx> writes:
> >>
> >> Hello Jordan,
> >>
> >>> On Fri, Feb 10, 2023 at 09:18:35AM +0100, Javier Martinez Canillas wrote:
> >>>> Commit 9593126dae3e ("media: venus: Add a handling of QC08C compressed
> >>>> format") and commit cef92b14e653 ("media: venus: Add a handling of QC10C
> >>>> compressed format") added support for the QC08C and QC10C compressed
> >>>> formats respectively.
> >>>>
> >>>> But these also caused a regression, because the new formats where added
> >>>> at the beginning of the vdec_formats[] array and the vdec_inst_init()
> >>>> function sets the default format output and capture using fixed indexes
> >>>> of that array:
> >>>>
> >>>> static void vdec_inst_init(struct venus_inst *inst)
> >>>> {
> >>>> ...
> >>>>    inst->fmt_out = &vdec_formats[8];
> >>>>    inst->fmt_cap = &vdec_formats[0];
> >>>> ...
> >>>> }
> >>>>
> >>>> Since now V4L2_PIX_FMT_NV12 is not the first entry in the array anymore,
> >>>> the default capture format is not set to that as it was done before.
> >>>>
> >>>> Both commits changed the first index to keep inst->fmt_out default format
> >>>> set to V4L2_PIX_FMT_H264, but did not update the latter to keep .fmt_out
> >>>> default format set to V4L2_PIX_FMT_NV12.
> >>>>
> >>>> Rather than updating the index to the current V4L2_PIX_FMT_NV12 position,
> >>>> let's reorder the entries so that this format is the first entry again.
> >>>>
> >>>> This would also make VIDIOC_ENUM_FMT report the V4L2_PIX_FMT_NV12 format
> >>>> with an index 0 as it did before the QC08C and QC10C formats were added.
> >>>>
> >>>> Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
> >>>> Fixes: cef92b14e653 ("media: venus: Add a handling of QC10C compressed format")
> >>>> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> >>> I just came across this issue independently and can confirm this patch fixes
> >>> the GStreamer V4L2 decoder on QRB5165.
> >>>
> >>> Tested-by: Jordan Crouse <jorcrous@xxxxxxxxxx>
> >>>

This patch also fixes an issue running a V4L2 based decoder on Acer
Chromebook Spin 513 which is very similar to the HP X2 Chromebook, not
surprising as both platforms are basically the same, but anyway:

Tested-by: Enric Balletbo i Serra <eballetbo@xxxxxxxxxx>

>
> >> Thanks for testing it!
> >>
> >> Stanimir, can we please get this for v6.3 as well?
> >
> > Hi Javier, Jordan
> >
> > Could you please explain what regression/issue you see with patch?
> >
> > venus hardware supports QC08C which provides better performance hence
> > driver is publishing it as preferred color format.
> >
> > if client doesn't support this or want to use any other format, they can
> > set the desired format with s_fmt.
> >

I guess general clients are unlikely to support this format as it is
an opaque intermediate format used by Qualcomm platforms, and the
purpose of that format is to be used for other Qualcomm hardware
blocks that know about this format. So I'd say that returning by
default a more common format is more reliable. Using your argument if
someone wants to use QC08C (because he knows it can use it) set with
s_fmt will do the trick too.

In any case, the problem here seems to be that s_fmt is not working,
so it would be nice to have a solution for that first and meanwhile do
not change the old behaviour. Just my two cents.

Best regards,
 Enric Balletbo

>
> VIDIOC_S_FMT is currently broken for venus, at least on the HP X2
> Chromebook and only the default works. I'm still investigating why
> vdec_s_fmt() is not working.
>
> But basically, if VIDIOC_S_FMT is called for the capture queue,
> then later the VIDIOC_G_FMT ioctl fails with -EINVAL. This is due
> the following condition checked in vdec_check_src_change():
>
> static int vdec_check_src_change(struct venus_inst *inst)
> {
> ...
>         if (inst->subscriptions & V4L2_EVENT_SOURCE_CHANGE &&
>             inst->codec_state == VENUS_DEC_STATE_INIT &&
>             !inst->reconfig)
>                 return -EINVAL;
> ...
> }
>
> But regardless, I think that it would be better for a driver to
> not change the order of advertised VIDIOC_ENUM_FMT pixel formats.
>
> Because what happens now is that a decoding that was previously
> working by default is not working anymore due a combination of
> the default being changed and S_FMT not working as expected.
>
> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>