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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87sfeh0yjn.fsf@minerva.mail-host-address-is-not-set>
Date:   Tue, 07 Mar 2023 09:13:16 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Dikshita Agarwal <quic_dikshita@...cinc.com>,
        Jordan Crouse <jorcrous@...zon.com>
Cc:     linux-kernel@...r.kernel.org, Albert Esteve <aesteve@...hat.com>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Sergio Lopez <slp@...hat.com>,
        Enric Balletbo i Serra <eballetb@...hat.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Stanimir Varbanov <stanimir.k.varbanov@...il.com>,
        Vikash Garodia <quic_vgarodia@...cinc.com>,
        linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH] media: venus: dec: Fix capture formats enumeration order

Dikshita Agarwal <quic_dikshita@...cinc.com> writes:

Hello Dikshita,

> On 3/6/2023 3:38 PM, Javier Martinez Canillas wrote:
>> Jordan Crouse <jorcrous@...zon.com> 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@...hat.com>
>>> I just came across this issue independently and can confirm this patch fixes
>>> the GStreamer V4L2 decoder on QRB5165.
>>>
>>> Tested-by: Jordan Crouse <jorcrous@...zon.com>
>>>
>> 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.
>

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