linux-kernel - Re: [PATCH v3 16/19] media: rkvdec: Drop unneeded per

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: <b97351fc-1238-e3ee-e7ec-6e74b19725fb@kwiboo.se>
Date:   Tue, 18 Aug 2020 22:25:10 +0000 (UTC)
From:   Jonas Karlman <jonas@...boo.se>
To:     Ezequiel Garcia <ezequiel@...labora.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Tomasz Figa <tfiga@...omium.org>, kernel@...labora.com,
        Hans Verkuil <hverkuil@...all.nl>,
        Alexandre Courbot <acourbot@...omium.org>,
        Jeffrey Kardatzke <jkardatzke@...omium.org>,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Maxime Ripard <mripard@...nel.org>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Jernej Skrabec <jernej.skrabec@...l.net>
Subject: Re: [PATCH v3 16/19] media: rkvdec: Drop unneeded per_request
 driver-specific control flag

On 2020-08-18 23:38, Ezequiel Garcia wrote:
> On Tue, 2020-08-18 at 20:17 +0000, Jonas Karlman wrote:
>> Hi Ezequiel,
>>
>> On 2020-08-14 15:36, Ezequiel Garcia wrote:
>>> Currently, the drivers makes no distinction between per_request
>>> and mandatory, as both are used in the same request validate check.
>>>
>>> The driver only cares to know if a given control is
>>> required to be part of a request, so only one flag is needed.
>>
>> This patch cause decoding issues with ffmpeg.
>>
>> The removal of per_request makes DECODE_MODE and START_CODE ctrls
>> mandatory to be included in the request.
>>
> 
> Ugh, I just failed boolean logic 101.
> 
> Yeah, we those controls shouldn't be mandatory.

Yep, removing mandatory flag makes rkvdec decoding work again :-)

> 
> I'll send a fix for that. Other than this, can I add your tested-by to the series?

Yes, with above fix this series is

Tested-by: Jonas Karlman <jonas@...boo.se>

using ffmpeg [1] on rk3288 (hantro) and rk3399 (rkvdec).


I have also done limited testing of field decoding on H.264 conformance
video samples and rkvdec manage to generate matching checksums.
On hantro the output is slightly different for fld and picaff samples
and match for frm and mbaff samples.

Because field decoding works correctly with rkvdec I am confident that
uapi contains everything needed to support field decoding.


[1] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3.1

Best regards,
Jonas

> 
> Thanks,
> Ezequiel
> 
>> Best regards,
>> Jonas
>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@...labora.com>
>>> ---
>>>  drivers/staging/media/rkvdec/rkvdec.c | 6 +-----
>>>  drivers/staging/media/rkvdec/rkvdec.h | 1 -
>>>  2 files changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>> index 7c5129593921..cd720d726d7f 100644
>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>> @@ -55,23 +55,19 @@ static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>>>  
>>>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>>  	{
>>> -		.per_request = true,
>>>  		.mandatory = true,
>>>  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS,
>>>  	},
>>>  	{
>>> -		.per_request = true,
>>>  		.mandatory = true,
>>>  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_SPS,
>>>  		.cfg.ops = &rkvdec_ctrl_ops,
>>>  	},
>>>  	{
>>> -		.per_request = true,
>>>  		.mandatory = true,
>>>  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PPS,
>>>  	},
>>>  	{
>>> -		.per_request = true,
>>>  		.mandatory = true,
>>>  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX,
>>>  	},
>>> @@ -615,7 +611,7 @@ static int rkvdec_request_validate(struct media_request *req)
>>>  		u32 id = ctrls->ctrls[i].cfg.id;
>>>  		struct v4l2_ctrl *ctrl;
>>>  
>>> -		if (!ctrls->ctrls[i].per_request || !ctrls->ctrls[i].mandatory)
>>> +		if (!ctrls->ctrls[i].mandatory)
>>>  			continue;
>>>  
>>>  		ctrl = v4l2_ctrl_request_hdl_ctrl_find(hdl, id);
>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>> index 2fc9f46b6910..77a137cca88e 100644
>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>> @@ -25,7 +25,6 @@
>>>  struct rkvdec_ctx;
>>>  
>>>  struct rkvdec_ctrl_desc {
>>> -	u32 per_request : 1;
>>>  	u32 mandatory : 1;
>>>  	struct v4l2_ctrl_config cfg;
>>>  };
>>>
> 
>