Re: [PATCHv17 16/34] v4l2-ctrls: add v4l2_ctrl_request_hdl_find/put/ctrl_find functions
On 14/08/18 10:55, Mauro Carvalho Chehab wrote:
> Em Tue, 14 Aug 2018 10:45:57 +0200
> Hans Verkuil escreveu:
>
>> On 13/08/18 13:07, Mauro Carvalho Chehab wrote:
>>> Em Sat, 4 Aug 2018 14:45:08 +0200
>>> Hans Verkuil escreveu:
>>>
If a driver needs to find/inspect the controls set in a request then
it can use these functions.
E.g. to check if a required control is set in a request use this in the
req_validate() implementation:
int res = -EINVAL;
hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
if (hdl) {
if (v4l2_ctrl_request_hdl_ctrl_find(hdl, ctrl_id))
res = 0;
v4l2_ctrl_request_hdl_put(hdl);
}
return res;
Signed-off-by: Hans Verkuil
---
drivers/media/v4l2-core/v4l2-ctrls.c | 25
include/media/v4l2-ctrls.h | 44 +++-
2 files changed, 68 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 86a6ae54ccaa..2a30be824491 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2976,6 +2976,31 @@ static const struct media_request_object_ops
req_ops = {
.release = v4l2_ctrl_request_release,
};
+struct v4l2_ctrl_handler *v4l2_ctrl_request_hdl_find(struct media_request
*req,
+ struct v4l2_ctrl_handler *parent)
+{
+ struct media_request_object *obj;
+
+ if (WARN_ON(req->state != MEDIA_REQUEST_STATE_VALIDATING &&
+ req->state != MEDIA_REQUEST_STATE_QUEUED))
+ return NULL;
+
+ obj = media_request_object_find(req, &req_ops, parent);
+ if (obj)
+ return container_of(obj, struct v4l2_ctrl_handler, req_obj);
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_find);
+
+struct v4l2_ctrl *
+v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id)
+{
+ struct v4l2_ctrl_ref *ref = find_ref_lock(hdl, id);
+
+ return (ref && ref->req == ref) ? ref->ctrl : NULL;
>>>
>>> Doesn't those helper functions (including this one) be serialized?
>>
>> v4l2_ctrl_request_hdl_find() checks the request state to ensure this:
>> it is either VALIDATING (then the req_queue_mutex is locked) or QUEUED
>> and then it is under control of the driver. Of course, in that case the
>> driver should make sure that it doesn't complete the request in the
>> middle of calling this function. If a driver does that, then it is a driver
>> bug.
>
> Please document it then, as I guess anyone that didn't worked at the
> request API patchset wouldn't guess when the driver needs to take
> the lock themselves.
>
> From what I'm understanding, the driver needs to take the lock only
> when it is running a code that it is not called from an ioctl.
> right?
Drivers never take req_queue_mutex themselves. If they call this function
when in state QUEUED, then they might need to take a driver-specific mutex
or spinlock that protects against the request being completed by an
interrupt handler.
I very much doubt that this function is ever called when in QUEUED state,
the typical use-case is to call this during validation or when queuing
a validated request. In both cases there is no need to do any locking
since it is guaranteed that no control objects will be added/deleted during
that phase.
I've extended the req_queue documentation in media-device.h with
two extra sentences at the end:
* @req_queue: Queue a validated request, cannot fail. If something goes
* wrong when queueing this request then it should be marked
* as such internally in the driver and any related buffers
* must eventually return to vb2 with state VB2_BUF_STATE_ERROR.
* The req_queue_mutex lock is held when this op is called.
* It is important that vb2 buffer objects are queued last after
* all other object types are queued: queueing a buffer kickstarts
* the request processing, so all other objects related to the
* request (and thus the buffer) must be available to the driver.
* And once a buffer is queued, then the driver can complete
* or delete objects from the request before req_queue exits.
This is important information that wasn't documented.
So nobody can add/remove objects from a request while in the req_validate()
callback. And nobody can add/remove objects from a request while in req_queue()
and non-buffer objects are queued.
Only once buffer objects are queued can objects be completed or removed from
the request and you have to be careful about that.
vb2_request_queue() does the right thing there: notice the
list_for_each_entry_safe()
when iterating over the
Re: [PATCHv17 16/34] v4l2-ctrls: add v4l2_ctrl_request_hdl_find/put/ctrl_find functions
Em Tue, 14 Aug 2018 10:45:57 +0200
Hans Verkuil escreveu:
> On 13/08/18 13:07, Mauro Carvalho Chehab wrote:
> > Em Sat, 4 Aug 2018 14:45:08 +0200
> > Hans Verkuil escreveu:
> >
> >> If a driver needs to find/inspect the controls set in a request then
> >> it can use these functions.
> >>
> >> E.g. to check if a required control is set in a request use this in the
> >> req_validate() implementation:
> >>
> >>int res = -EINVAL;
> >>
> >>hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
> >>if (hdl) {
> >>if (v4l2_ctrl_request_hdl_ctrl_find(hdl, ctrl_id))
> >>res = 0;
> >>v4l2_ctrl_request_hdl_put(hdl);
> >>}
> >>return res;
> >>
> >> Signed-off-by: Hans Verkuil
> >> ---
> >> drivers/media/v4l2-core/v4l2-ctrls.c | 25
> >> include/media/v4l2-ctrls.h | 44 +++-
> >> 2 files changed, 68 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> >> b/drivers/media/v4l2-core/v4l2-ctrls.c
> >> index 86a6ae54ccaa..2a30be824491 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >> @@ -2976,6 +2976,31 @@ static const struct media_request_object_ops
> >> req_ops = {
> >>.release = v4l2_ctrl_request_release,
> >> };
> >>
> >> +struct v4l2_ctrl_handler *v4l2_ctrl_request_hdl_find(struct media_request
> >> *req,
> >> + struct v4l2_ctrl_handler *parent)
> >> +{
> >> + struct media_request_object *obj;
> >> +
> >> + if (WARN_ON(req->state != MEDIA_REQUEST_STATE_VALIDATING &&
> >> + req->state != MEDIA_REQUEST_STATE_QUEUED))
> >> + return NULL;
> >> +
> >> + obj = media_request_object_find(req, &req_ops, parent);
> >> + if (obj)
> >> + return container_of(obj, struct v4l2_ctrl_handler, req_obj);
> >> + return NULL;
> >> +}
> >> +EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_find);
> >> +
> >> +struct v4l2_ctrl *
> >> +v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id)
> >> +{
> >> + struct v4l2_ctrl_ref *ref = find_ref_lock(hdl, id);
> >> +
> >> + return (ref && ref->req == ref) ? ref->ctrl : NULL;
> >
> > Doesn't those helper functions (including this one) be serialized?
>
> v4l2_ctrl_request_hdl_find() checks the request state to ensure this:
> it is either VALIDATING (then the req_queue_mutex is locked) or QUEUED
> and then it is under control of the driver. Of course, in that case the
> driver should make sure that it doesn't complete the request in the
> middle of calling this function. If a driver does that, then it is a driver
> bug.
Please document it then, as I guess anyone that didn't worked at the
request API patchset wouldn't guess when the driver needs to take
the lock themselves.
From what I'm understanding, the driver needs to take the lock only
when it is running a code that it is not called from an ioctl.
right?
Thanks,
Mauro
Re: [PATCHv17 16/34] v4l2-ctrls: add v4l2_ctrl_request_hdl_find/put/ctrl_find functions
On 13/08/18 13:07, Mauro Carvalho Chehab wrote:
> Em Sat, 4 Aug 2018 14:45:08 +0200
> Hans Verkuil escreveu:
>
>> If a driver needs to find/inspect the controls set in a request then
>> it can use these functions.
>>
>> E.g. to check if a required control is set in a request use this in the
>> req_validate() implementation:
>>
>> int res = -EINVAL;
>>
>> hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
>> if (hdl) {
>> if (v4l2_ctrl_request_hdl_ctrl_find(hdl, ctrl_id))
>> res = 0;
>> v4l2_ctrl_request_hdl_put(hdl);
>> }
>> return res;
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> drivers/media/v4l2-core/v4l2-ctrls.c | 25
>> include/media/v4l2-ctrls.h | 44 +++-
>> 2 files changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 86a6ae54ccaa..2a30be824491 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -2976,6 +2976,31 @@ static const struct media_request_object_ops req_ops
>> = {
>> .release = v4l2_ctrl_request_release,
>> };
>>
>> +struct v4l2_ctrl_handler *v4l2_ctrl_request_hdl_find(struct media_request
>> *req,
>> +struct v4l2_ctrl_handler *parent)
>> +{
>> +struct media_request_object *obj;
>> +
>> +if (WARN_ON(req->state != MEDIA_REQUEST_STATE_VALIDATING &&
>> +req->state != MEDIA_REQUEST_STATE_QUEUED))
>> +return NULL;
>> +
>> +obj = media_request_object_find(req, &req_ops, parent);
>> +if (obj)
>> +return container_of(obj, struct v4l2_ctrl_handler, req_obj);
>> +return NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_find);
>> +
>> +struct v4l2_ctrl *
>> +v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id)
>> +{
>> +struct v4l2_ctrl_ref *ref = find_ref_lock(hdl, id);
>> +
>> +return (ref && ref->req == ref) ? ref->ctrl : NULL;
>
> Doesn't those helper functions (including this one) be serialized?
v4l2_ctrl_request_hdl_find() checks the request state to ensure this:
it is either VALIDATING (then the req_queue_mutex is locked) or QUEUED
and then it is under control of the driver. Of course, in that case the
driver should make sure that it doesn't complete the request in the
middle of calling this function. If a driver does that, then it is a driver
bug.
Regards,
Hans
>
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_ctrl_find);
>> +
>> static int v4l2_ctrl_request_bind(struct media_request *req,
>> struct v4l2_ctrl_handler *hdl,
>> struct v4l2_ctrl_handler *from)
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index 98b1e70a4a46..aeb7f3c24ef7 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -,7 +,49 @@ void v4l2_ctrl_request_setup(struct media_request
>> *req,
>> * request object.
>> */
>> void v4l2_ctrl_request_complete(struct media_request *req,
>> -struct v4l2_ctrl_handler *hdl);
>> +struct v4l2_ctrl_handler *parent);
>> +
>> +/**
>> + * v4l2_ctrl_request_hdl_find - Find the control handler in the request
>> + *
>> + * @req: The request
>> + * @parent: The parent control handler ('priv' in
>> media_request_object_find())
>> + *
>> + * This function finds the control handler in the request. It may return
>> + * NULL if not found. When done, you must call v4l2_ctrl_request_put_hdl()
>> + * with the returned handler pointer.
>> + *
>> + * If the request is not in state VALIDATING or QUEUED, then this function
>> + * will always return NULL.
>> + */
>> +struct v4l2_ctrl_handler *v4l2_ctrl_request_hdl_find(struct media_request
>> *req,
>> +struct v4l2_ctrl_handler *parent);
>> +
>> +/**
>> + * v4l2_ctrl_request_hdl_put - Put the control handler
>> + *
>> + * @hdl: Put this control handler
>> + *
>> + * This function released the control handler previously obtained from'
>> + * v4l2_ctrl_request_hdl_find().
>> + */
>> +static inline void v4l2_ctrl_request_hdl_put(struct v4l2_ctrl_handler *hdl)
>> +{
>> +if (hdl)
>> +media_request_object_put(&hdl->req_obj);
>> +}
>> +
>> +/**
>> + * v4l2_ctrl_request_ctrl_find() - Find a control with the given ID.
>> + *
>> + * @hdl: The control handler from the request.
>> + * @id: The ID of the control to find.
>> + *
>> + * This function returns a pointer to the control if this control is
>> + * part of the request or NULL otherwise.
>> + */
>> +struct v4l2_ctrl *
>> +v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id);
>>
>> /* Helpers for ioctl_ops */
>>
>
>
>
> Thanks,
> Mauro
>
Re: [PATCHv17 16/34] v4l2-ctrls: add v4l2_ctrl_request_hdl_find/put/ctrl_find functions
Em Sat, 4 Aug 2018 14:45:08 +0200
Hans Verkuil escreveu:
> If a driver needs to find/inspect the controls set in a request then
> it can use these functions.
>
> E.g. to check if a required control is set in a request use this in the
> req_validate() implementation:
>
> int res = -EINVAL;
>
> hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
> if (hdl) {
> if (v4l2_ctrl_request_hdl_ctrl_find(hdl, ctrl_id))
> res = 0;
> v4l2_ctrl_request_hdl_put(hdl);
> }
> return res;
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 25
> include/media/v4l2-ctrls.h | 44 +++-
> 2 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 86a6ae54ccaa..2a30be824491 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2976,6 +2976,31 @@ static const struct media_request_object_ops req_ops =
> {
> .release = v4l2_ctrl_request_release,
> };
>
> +struct v4l2_ctrl_handler *v4l2_ctrl_request_hdl_find(struct media_request
> *req,
> + struct v4l2_ctrl_handler *parent)
> +{
> + struct media_request_object *obj;
> +
> + if (WARN_ON(req->state != MEDIA_REQUEST_STATE_VALIDATING &&
> + req->state != MEDIA_REQUEST_STATE_QUEUED))
> + return NULL;
> +
> + obj = media_request_object_find(req, &req_ops, parent);
> + if (obj)
> + return container_of(obj, struct v4l2_ctrl_handler, req_obj);
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_find);
> +
> +struct v4l2_ctrl *
> +v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id)
> +{
> + struct v4l2_ctrl_ref *ref = find_ref_lock(hdl, id);
> +
> + return (ref && ref->req == ref) ? ref->ctrl : NULL;
Doesn't those helper functions (including this one) be serialized?
> +}
> +EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_ctrl_find);
> +
> static int v4l2_ctrl_request_bind(struct media_request *req,
> struct v4l2_ctrl_handler *hdl,
> struct v4l2_ctrl_handler *from)
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 98b1e70a4a46..aeb7f3c24ef7 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -,7 +,49 @@ void v4l2_ctrl_request_setup(struct media_request *req,
> * request object.
> */
> void v4l2_ctrl_request_complete(struct media_request *req,
> - struct v4l2_ctrl_handler *hdl);
> + struct v4l2_ctrl_handler *parent);
> +
> +/**
> + * v4l2_ctrl_request_hdl_find - Find the control handler in the request
> + *
> + * @req: The request
> + * @parent: The parent control handler ('priv' in
> media_request_object_find())
> + *
> + * This function finds the control handler in the request. It may return
> + * NULL if not found. When done, you must call v4l2_ctrl_request_put_hdl()
> + * with the returned handler pointer.
> + *
> + * If the request is not in state VALIDATING or QUEUED, then this function
> + * will always return NULL.
> + */
> +struct v4l2_ctrl_handler *v4l2_ctrl_request_hdl_find(struct media_request
> *req,
> + struct v4l2_ctrl_handler *parent);
> +
> +/**
> + * v4l2_ctrl_request_hdl_put - Put the control handler
> + *
> + * @hdl: Put this control handler
> + *
> + * This function released the control handler previously obtained from'
> + * v4l2_ctrl_request_hdl_find().
> + */
> +static inline void v4l2_ctrl_request_hdl_put(struct v4l2_ctrl_handler *hdl)
> +{
> + if (hdl)
> + media_request_object_put(&hdl->req_obj);
> +}
> +
> +/**
> + * v4l2_ctrl_request_ctrl_find() - Find a control with the given ID.
> + *
> + * @hdl: The control handler from the request.
> + * @id: The ID of the control to find.
> + *
> + * This function returns a pointer to the control if this control is
> + * part of the request or NULL otherwise.
> + */
> +struct v4l2_ctrl *
> +v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id);
>
> /* Helpers for ioctl_ops */
>
Thanks,
Mauro
[PATCHv17 16/34] v4l2-ctrls: add v4l2_ctrl_request_hdl_find/put/ctrl_find functions
If a driver needs to find/inspect the controls set in a request then
it can use these functions.
E.g. to check if a required control is set in a request use this in the
req_validate() implementation:
int res = -EINVAL;
hdl = v4l2_ctrl_request_hdl_find(req, parent_hdl);
if (hdl) {
if (v4l2_ctrl_request_hdl_ctrl_find(hdl, ctrl_id))
res = 0;
v4l2_ctrl_request_hdl_put(hdl);
}
return res;
Signed-off-by: Hans Verkuil
---
drivers/media/v4l2-core/v4l2-ctrls.c | 25
include/media/v4l2-ctrls.h | 44 +++-
2 files changed, 68 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 86a6ae54ccaa..2a30be824491 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2976,6 +2976,31 @@ static const struct media_request_object_ops req_ops = {
.release = v4l2_ctrl_request_release,
};
+struct v4l2_ctrl_handler *v4l2_ctrl_request_hdl_find(struct media_request *req,
+ struct v4l2_ctrl_handler *parent)
+{
+ struct media_request_object *obj;
+
+ if (WARN_ON(req->state != MEDIA_REQUEST_STATE_VALIDATING &&
+ req->state != MEDIA_REQUEST_STATE_QUEUED))
+ return NULL;
+
+ obj = media_request_object_find(req, &req_ops, parent);
+ if (obj)
+ return container_of(obj, struct v4l2_ctrl_handler, req_obj);
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_find);
+
+struct v4l2_ctrl *
+v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id)
+{
+ struct v4l2_ctrl_ref *ref = find_ref_lock(hdl, id);
+
+ return (ref && ref->req == ref) ? ref->ctrl : NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_ctrl_find);
+
static int v4l2_ctrl_request_bind(struct media_request *req,
struct v4l2_ctrl_handler *hdl,
struct v4l2_ctrl_handler *from)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 98b1e70a4a46..aeb7f3c24ef7 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -,7 +,49 @@ void v4l2_ctrl_request_setup(struct media_request *req,
* request object.
*/
void v4l2_ctrl_request_complete(struct media_request *req,
- struct v4l2_ctrl_handler *hdl);
+ struct v4l2_ctrl_handler *parent);
+
+/**
+ * v4l2_ctrl_request_hdl_find - Find the control handler in the request
+ *
+ * @req: The request
+ * @parent: The parent control handler ('priv' in media_request_object_find())
+ *
+ * This function finds the control handler in the request. It may return
+ * NULL if not found. When done, you must call v4l2_ctrl_request_put_hdl()
+ * with the returned handler pointer.
+ *
+ * If the request is not in state VALIDATING or QUEUED, then this function
+ * will always return NULL.
+ */
+struct v4l2_ctrl_handler *v4l2_ctrl_request_hdl_find(struct media_request *req,
+ struct v4l2_ctrl_handler *parent);
+
+/**
+ * v4l2_ctrl_request_hdl_put - Put the control handler
+ *
+ * @hdl: Put this control handler
+ *
+ * This function released the control handler previously obtained from'
+ * v4l2_ctrl_request_hdl_find().
+ */
+static inline void v4l2_ctrl_request_hdl_put(struct v4l2_ctrl_handler *hdl)
+{
+ if (hdl)
+ media_request_object_put(&hdl->req_obj);
+}
+
+/**
+ * v4l2_ctrl_request_ctrl_find() - Find a control with the given ID.
+ *
+ * @hdl: The control handler from the request.
+ * @id: The ID of the control to find.
+ *
+ * This function returns a pointer to the control if this control is
+ * part of the request or NULL otherwise.
+ */
+struct v4l2_ctrl *
+v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id);
/* Helpers for ioctl_ops */
--
2.18.0
