Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

From: Jonas Karlman <jonas@kwiboo.se>
To: Ezequiel Garcia <ezequiel@collabora.com>,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	linux-kernel@vger.kernel.org
Cc: Hans Verkuil <hans.verkuil@cisco.com>,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>,
	Tomasz Figa <tfiga@chromium.org>,
	Alexandre Courbot <acourbot@chromium.org>
Subject: Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation
Date: Fri, 03 Jul 2020 19:17:56 +0000 (UTC)	[thread overview]
Message-ID: <0353b993-907d-2dfe-993f-94b82aa27e00@kwiboo.se> (raw)
In-Reply-To: <7abec9992460dcd84a2c951fce55bc8e46f2a0ed.camel@collabora.com>

On 2020-07-03 16:58, Ezequiel Garcia wrote:
> On Fri, 2020-07-03 at 06:55 +0000, Jonas Karlman wrote:
>> On 2020-07-03 05:14, Ezequiel Garcia wrote:
>>> Hi Jonas,
>>>
>>> Thanks for working on this.
>>>
>>> On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
>>>> Add an optional validate_fmt operation that is used to validate the
>>>> pixelformat of CAPTURE buffers.
>>>>
>>>> This is used in next patch to ensure correct pixelformat is used for 10-bit
>>>> and 4:2:2 content.
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>>  drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
>>>>  drivers/staging/media/rkvdec/rkvdec.h | 1 +
>>>>  2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>> index b1de55aa6535..465444c58f13 100644
>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>> @@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
>>>>  	if (WARN_ON(!coded_desc))
>>>>  		return -EINVAL;
>>>>  
>>>> +	if (coded_desc->ops->validate_fmt) {
>>>> +		int ret;
>>>> +
>>>> +		ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> + 
>>>
>>> I don't think this approach will be enough. Unless I'm mistaken,
>>> it's perfectly legal as per the stateless spec to first
>>> call S_FMT on the OUTPUT queue (which is propagated to the CAPTURE side),
>>> and then set the SPS and other controls.
>>
>> I agree that this will not be enough to cover all use cases stated in the spec.
>>
>>> The application is not required to do a TRY_FMT after S_EXT_CTRLS.
>>
>> If I remember correctly we were required to implement a TRY_FMT loop in
>> ffmpeg due to cedrus defaulting to SUNXI_TILED_NV12 instead of linear NV12
>> on platforms where display controller did not support the tiled modifier.
>>
>> So having TRY_FMT as part of the init sequence has been my only test-case.
>>
>>> What I believe is needed is for the S_EXT_CTRLS to modify
>>> and restrict the CAPTURE format accordingly, so the application
>>> gets the correct format on G_FMT (and restrict future TRY_FMT).
>>
>> This sounds like a proper solution, I do belive we may have a chicken or
>> the egg problem depending on if application call S_EXT_CTRLS or S_FMT first.
>>
> 
> IIUC, the order is specified in the stateless spec [1].
> 
> 1) S_FMT on OUTPUT (to set the coded pixelformat). CAPTURE format
> format is propagated here and a default format is set.
> 
> 2) S_EXT_CTRLS, parameters are set. We don't do anything here,
> but here we'd validate the SPS and restrict the CAPTURE pixelformat
> (and perhaps reset the default CAPTURE pixelformat).
> 
> 3) G_FMT on CAPTURE.
> 
> 4) (optional) ENUM_FMT / S_FMT on CAPTURE, to negotiate
> something different from default.

There is also the following scenario that we may need to support:

1) S_FMT on OUTPUT, default CAPTURE format is set.

2) skip S_EXT_CTRLS, mandatory controls is only validated in req_validate.

3) G_FMT on CAPTURE, returns default CAPTURE format.

4) S_FMT on CAPTURE, CAPTURE format is changed from default to selected format.

5) STREAMON

From this point on I would expect S_EXT_CTRLS with a V4L2_CTRL_WHICH_REQUEST_VAL
flag to reject any SPS not matching the selected CAPTURE format. Effectively
allowing S_FMT to lock down a format instead of an initial S_EXT_CTRLS during init.

This means that we have to both allow and reject a SPS depending on the state.

Regards,
Jonas

> 
> Regards,
> Ezequiel 
> 
> [1] Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> 
>> I guess we may need to lock down on a format at whatever comes first,
>> S_FMT on CAPTURE or S_EXT_CTRLS with SPS ctrl.
>>
>> I have an idea on how this could be addressed, will explore and see
>> if I can come up with something new.
>>
>> Regards,
>> Jonas
>>
>>> Also, V4L2 spec asks drivers not to fail on S_FMT
>>> format mismatch, but instead to adjust and return a legal format
>>> back to the application [1].
>>>
>>> Let me know what you think and thanks again.
>>>
>>> Ezequiel
>>>
>>> [1] Documentation/userspace-api/media/v4l/vidioc-g-fmt.rst
>>>
>>>>  	for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
>>>>  		if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
>>>>  			break;
>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>> index 2fc9f46b6910..be4fc3645cde 100644
>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>> @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
>>>>  struct rkvdec_coded_fmt_ops {
>>>>  	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
>>>>  			  struct v4l2_format *f);
>>>> +	int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
>>>>  	int (*start)(struct rkvdec_ctx *ctx);
>>>>  	void (*stop)(struct rkvdec_ctx *ctx);
>>>>  	int (*run)(struct rkvdec_ctx *ctx);
> 
> 