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

[Jordan Crouse]

On Tue, Mar 07, 2023 at 05:20:18PM +0100, Enric Balletbo i Serra wrote:
> 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.

For my part, I was using the gstreamer v4l2 decoder which for some reason tries
to verify it can support whatever format it gets with G_FMT *before*
trying a S_FMT. I can't confirm or deny if S_FMT currently works or not.

That said, I entirely agree with Javier. While it might be more
bandwidth efficient, QC08C is a obscure format. It is far more likely that the
average open source user would rather use a well known output format and, as
has been mentioned, once S_FMT is fixed those in the know can use the other
formats if they are working with other Qualcomm hardware blocks.

Jordan