Re: [PATCH v3 16/19] media: rkvdec: Drop unneeded per_request driver-specific control flag

[Jonas Karlman]

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@xxxxxxxxx>

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@xxxxxxxxxxxxx>
>>> ---
>>>  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;
>>>  };
>>>
> 
>