[RFC PATCH 0/2] Add v4l2_ctrl_request_create()

[RFC PATCH 0/2] Add v4l2_ctrl_request_create()

[Hans Verkuil]

The Request API is missing a feature: if userspace did not set any controls,
then the request object will not contain a control object (that's created
only if the user sets a control in the request).

This is fine for e.g. stateless codecs since they require that each request
contains controls, so this is always done. And this is also the reason that
this hasn't been a problem before, since the Request API is almost exclusively
used by stateless codecs.

But for e.g. vivid this means that the completed request does not contain
any controls in the request containing the control values at the time that
the frame was captured (or output).

In addition, if a driver needs to set a status control, then that control
won't be part of the request either.

This RFC series adds a v4l2_ctrl_request_create() function that can be
called in the req_validate() callback of the request. If the request
doesn't contain a control object, then it will add a new one.

This is an RFC for now. Hopefully this will help Yunfei Dong with the
development of Read-Only Requests, since that should be a lot simpler with
this new function.

Also, I am not entirely happy with the name of the function and I also
think that we might need a helper in v4l2-common.c that combines with
function with vb2_request_validate(), thus avoiding that drivers need
to remember to call both v4l2_ctrl_request_create() and vb2_request_validate().

Regards,

	Hans

Hans Verkuil (2):
  v4l2-ctrls.c: add v4l2_ctrl_request_create
  vivid: call v4l2_ctrl_request_create()

 drivers/media/mc/mc-request.c                 |  3 +-
 drivers/media/test-drivers/vivid/vivid-core.c |  4 +++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 35 +++++++++++++++++++
 include/media/v4l2-ctrls.h                    | 16 +++++++++
 4 files changed, 57 insertions(+), 1 deletion(-)

-- 
2.27.0

[RFC PATCH 1/2] v4l2-ctrls.c: add v4l2_ctrl_request_create

[Hans Verkuil]

Add a new v4l2_ctrl_request_create() function that can be called in
req_validate() to create a control request object if needed.

This is needed if the driver needs to set controls in the request,

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-request.c        |  3 ++-
 drivers/media/v4l2-core/v4l2-ctrls.c | 35 ++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h           | 16 +++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
index c0782fd96c59..64df83c6f5e5 100644
--- a/drivers/media/mc/mc-request.c
+++ b/drivers/media/mc/mc-request.c
@@ -414,7 +414,8 @@ int media_request_object_bind(struct media_request *req,
 
 	spin_lock_irqsave(&req->lock, flags);
 
-	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_UPDATING))
+	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_UPDATING &&
+		    req->state != MEDIA_REQUEST_STATE_VALIDATING))
 		goto unlock;
 
 	obj->req = req;
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3f3fbcd60cc6..0d4c8551ba2a 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -4345,6 +4345,41 @@ void v4l2_ctrl_request_complete(struct media_request *req,
 }
 EXPORT_SYMBOL(v4l2_ctrl_request_complete);
 
+int v4l2_ctrl_request_create(struct media_request *req,
+			     struct v4l2_ctrl_handler *main_hdl)
+{
+	struct media_request_object *obj;
+	struct v4l2_ctrl_handler *new_hdl;
+	int ret = 0;
+
+	if (!req || !main_hdl)
+		return 0;
+
+	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_VALIDATING))
+		return -EBUSY;
+
+	/* If a request object is found, then do nothing. */
+	obj = media_request_object_find(req, &req_ops, main_hdl);
+	if (obj) {
+		media_request_object_put(obj);
+		return 0;
+	}
+
+	/* Create a new request so the driver can return controls */
+	new_hdl = kzalloc(sizeof(*new_hdl), GFP_KERNEL);
+	if (!new_hdl)
+		return -ENOMEM;
+
+	obj = &new_hdl->req_obj;
+	ret = v4l2_ctrl_handler_init(new_hdl, (main_hdl->nr_of_buckets - 1) * 8);
+	if (!ret)
+		ret = v4l2_ctrl_request_bind(req, new_hdl, main_hdl);
+	if (ret)
+		kfree(new_hdl);
+	return ret;
+}
+EXPORT_SYMBOL(v4l2_ctrl_request_create);
+
 int v4l2_ctrl_request_setup(struct media_request *req,
 			     struct v4l2_ctrl_handler *main_hdl)
 {
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f40e2cbb21d3..2703baa170fa 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -1254,6 +1254,22 @@ int v4l2_ctrl_request_setup(struct media_request *req,
 void v4l2_ctrl_request_complete(struct media_request *req,
 				struct v4l2_ctrl_handler *parent);
 
+/**
+ * v4l2_ctrl_request_create - Create a control handler request object
+ *
+ * @req: The request
+ * @parent: The parent control handler ('priv' in media_request_object_find())
+ *
+ * If the user created a request without controls, but the driver wants to
+ * set controls for the request, then this function can be called in the
+ * request's req_validate function. If there is no control object in the
+ * request, then this will create one. Now the driver can set controls
+ * and when v4l2_ctrl_request_complete() is called they will be automatically
+ * copied into the request.
+ */
+int v4l2_ctrl_request_create(struct media_request *req,
+			     struct v4l2_ctrl_handler *parent);
+
 /**
  * v4l2_ctrl_request_hdl_find - Find the control handler in the request
  *
-- 
2.27.0

[RFC PATCH 2/2] vivid: call v4l2_ctrl_request_create()

[Hans Verkuil]

Call v4l2_ctrl_request_create() from req_validate() to create the
control request object if needed.

Without this the returned request object would not have a copy of the
controls used for the captured frame.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/test-drivers/vivid/vivid-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index f7ee37e9508d..59e155a8765e 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.c
+++ b/drivers/media/test-drivers/vivid/vivid-core.c
@@ -783,7 +783,11 @@ static void vivid_dev_release(struct v4l2_device *v4l2_dev)
 static int vivid_req_validate(struct media_request *req)
 {
 	struct vivid_dev *dev = container_of(req->mdev, struct vivid_dev, mdev);
+	int ret;
 
+	ret = v4l2_ctrl_request_create(req, &dev->ctrl_hdl_vid_cap);
+	if (ret)
+		return ret;
 	if (dev->req_validate_error) {
 		dev->req_validate_error = false;
 		return -EINVAL;
-- 
2.27.0

Re: [RFC PATCH 1/2] v4l2-ctrls.c: add v4l2_ctrl_request_create

[Stanimir Varbanov]

Hi Hans,

On 7/28/20 12:48 PM, Hans Verkuil wrote:
> Add a new v4l2_ctrl_request_create() function that can be called in
> req_validate() to create a control request object if needed.
> 
> This is needed if the driver needs to set controls in the request,
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/mc/mc-request.c        |  3 ++-
>  drivers/media/v4l2-core/v4l2-ctrls.c | 35 ++++++++++++++++++++++++++++
>  include/media/v4l2-ctrls.h           | 16 +++++++++++++
>  3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> index c0782fd96c59..64df83c6f5e5 100644
> --- a/drivers/media/mc/mc-request.c
> +++ b/drivers/media/mc/mc-request.c
> @@ -414,7 +414,8 @@ int media_request_object_bind(struct media_request *req,
>  
>  	spin_lock_irqsave(&req->lock, flags);
>  
> -	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_UPDATING))
> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_UPDATING &&
> +		    req->state != MEDIA_REQUEST_STATE_VALIDATING))
>  		goto unlock;
>  
>  	obj->req = req;
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 3f3fbcd60cc6..0d4c8551ba2a 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -4345,6 +4345,41 @@ void v4l2_ctrl_request_complete(struct media_request *req,
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_request_complete);
>  
> +int v4l2_ctrl_request_create(struct media_request *req,
> +			     struct v4l2_ctrl_handler *main_hdl)
> +{
> +	struct media_request_object *obj;
> +	struct v4l2_ctrl_handler *new_hdl;
> +	int ret = 0;
> +
> +	if (!req || !main_hdl)
> +		return 0;
> +
> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_VALIDATING))
> +		return -EBUSY;
> +
> +	/* If a request object is found, then do nothing. */
> +	obj = media_request_object_find(req, &req_ops, main_hdl);
> +	if (obj) {
> +		media_request_object_put(obj);
> +		return 0;
> +	}
> +
> +	/* Create a new request so the driver can return controls */
> +	new_hdl = kzalloc(sizeof(*new_hdl), GFP_KERNEL);
> +	if (!new_hdl)
> +		return -ENOMEM;
> +
> +	obj = &new_hdl->req_obj;

Why initialize 'obj' and then not use it? Did you forget something

> +	ret = v4l2_ctrl_handler_init(new_hdl, (main_hdl->nr_of_buckets - 1) * 8);
> +	if (!ret)
> +		ret = v4l2_ctrl_request_bind(req, new_hdl, main_hdl);
> +	if (ret)
> +		kfree(new_hdl);
> +	return ret;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_request_create);
> +
>  int v4l2_ctrl_request_setup(struct media_request *req,
>  			     struct v4l2_ctrl_handler *main_hdl)
>  {
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index f40e2cbb21d3..2703baa170fa 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -1254,6 +1254,22 @@ int v4l2_ctrl_request_setup(struct media_request *req,
>  void v4l2_ctrl_request_complete(struct media_request *req,
>  				struct v4l2_ctrl_handler *parent);
>  
> +/**
> + * v4l2_ctrl_request_create - Create a control handler request object
> + *
> + * @req: The request
> + * @parent: The parent control handler ('priv' in media_request_object_find())
> + *
> + * If the user created a request without controls, but the driver wants to
> + * set controls for the request, then this function can be called in the
> + * request's req_validate function. If there is no control object in the
> + * request, then this will create one. Now the driver can set controls
> + * and when v4l2_ctrl_request_complete() is called they will be automatically
> + * copied into the request.
> + */
> +int v4l2_ctrl_request_create(struct media_request *req,
> +			     struct v4l2_ctrl_handler *parent);
> +
>  /**
>   * v4l2_ctrl_request_hdl_find - Find the control handler in the request
>   *
> 

-- 
regards,
Stan

Re: [RFC PATCH 1/2] v4l2-ctrls.c: add v4l2_ctrl_request_create

[Hans Verkuil]

On 18/08/2020 13:30, Stanimir Varbanov wrote:
> Hi Hans,
> 
> On 7/28/20 12:48 PM, Hans Verkuil wrote:
>> Add a new v4l2_ctrl_request_create() function that can be called in
>> req_validate() to create a control request object if needed.
>>
>> This is needed if the driver needs to set controls in the request,
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/mc/mc-request.c        |  3 ++-
>>  drivers/media/v4l2-core/v4l2-ctrls.c | 35 ++++++++++++++++++++++++++++
>>  include/media/v4l2-ctrls.h           | 16 +++++++++++++
>>  3 files changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
>> index c0782fd96c59..64df83c6f5e5 100644
>> --- a/drivers/media/mc/mc-request.c
>> +++ b/drivers/media/mc/mc-request.c
>> @@ -414,7 +414,8 @@ int media_request_object_bind(struct media_request *req,
>>  
>>  	spin_lock_irqsave(&req->lock, flags);
>>  
>> -	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_UPDATING))
>> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_UPDATING &&
>> +		    req->state != MEDIA_REQUEST_STATE_VALIDATING))
>>  		goto unlock;
>>  
>>  	obj->req = req;
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 3f3fbcd60cc6..0d4c8551ba2a 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -4345,6 +4345,41 @@ void v4l2_ctrl_request_complete(struct media_request *req,
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_request_complete);
>>  
>> +int v4l2_ctrl_request_create(struct media_request *req,
>> +			     struct v4l2_ctrl_handler *main_hdl)
>> +{
>> +	struct media_request_object *obj;
>> +	struct v4l2_ctrl_handler *new_hdl;
>> +	int ret = 0;
>> +
>> +	if (!req || !main_hdl)
>> +		return 0;
>> +
>> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_VALIDATING))
>> +		return -EBUSY;
>> +
>> +	/* If a request object is found, then do nothing. */
>> +	obj = media_request_object_find(req, &req_ops, main_hdl);
>> +	if (obj) {
>> +		media_request_object_put(obj);
>> +		return 0;
>> +	}
>> +
>> +	/* Create a new request so the driver can return controls */
>> +	new_hdl = kzalloc(sizeof(*new_hdl), GFP_KERNEL);
>> +	if (!new_hdl)
>> +		return -ENOMEM;
>> +
>> +	obj = &new_hdl->req_obj;
> 
> Why initialize 'obj' and then not use it? Did you forget something

Oops, spurious line, should be removed.

Thanks for noticing!

	Hans

> 
>> +	ret = v4l2_ctrl_handler_init(new_hdl, (main_hdl->nr_of_buckets - 1) * 8);
>> +	if (!ret)
>> +		ret = v4l2_ctrl_request_bind(req, new_hdl, main_hdl);
>> +	if (ret)
>> +		kfree(new_hdl);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(v4l2_ctrl_request_create);
>> +
>>  int v4l2_ctrl_request_setup(struct media_request *req,
>>  			     struct v4l2_ctrl_handler *main_hdl)
>>  {
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index f40e2cbb21d3..2703baa170fa 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -1254,6 +1254,22 @@ int v4l2_ctrl_request_setup(struct media_request *req,
>>  void v4l2_ctrl_request_complete(struct media_request *req,
>>  				struct v4l2_ctrl_handler *parent);
>>  
>> +/**
>> + * v4l2_ctrl_request_create - Create a control handler request object
>> + *
>> + * @req: The request
>> + * @parent: The parent control handler ('priv' in media_request_object_find())
>> + *
>> + * If the user created a request without controls, but the driver wants to
>> + * set controls for the request, then this function can be called in the
>> + * request's req_validate function. If there is no control object in the
>> + * request, then this will create one. Now the driver can set controls
>> + * and when v4l2_ctrl_request_complete() is called they will be automatically
>> + * copied into the request.
>> + */
>> +int v4l2_ctrl_request_create(struct media_request *req,
>> +			     struct v4l2_ctrl_handler *parent);
>> +
>>  /**
>>   * v4l2_ctrl_request_hdl_find - Find the control handler in the request
>>   *
>>
>