Re: [RFCv11 PATCH 14/29] v4l2-ctrls: add core request support
On 04/11/2018 11:43 AM, Tomasz Figa wrote:
> Hi Hans,
>
> On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil wrote:
>
>> From: Hans Verkuil
>
>> Integrate the request support. This adds the v4l2_ctrl_request_complete
>> and v4l2_ctrl_request_setup functions to complete a request and (as a
>> helper function) to apply a request to the hardware.
>
> Please see my comments inline.
>
> [snip]
>> +static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
>> + const struct v4l2_ctrl_handler *from)
>> +{
>> + struct v4l2_ctrl_ref *ref;
>> + int err;
>> +
>> + if (WARN_ON(!hdl || hdl == from))
>> + return -EINVAL;
>> +
>> + if (hdl->error)
>> + return hdl->error;
>> +
>> + WARN_ON(hdl->lock != &hdl->_lock);
>> +
>> + mutex_lock(from->lock);
>> + list_for_each_entry(ref, &from->ctrl_refs, node) {
>> + struct v4l2_ctrl *ctrl = ref->ctrl;
>> + struct v4l2_ctrl_ref *new_ref;
>> +
>> + /* Skip refs inherited from other devices */
>> + if (ref->from_other_dev)
>> + continue;
>> + /* And buttons */
>> + if (ctrl->type == V4L2_CTRL_TYPE_BUTTON)
>> + continue;
>> + err = handler_new_ref(hdl, ctrl, &new_ref, false, true);
>> + if (err) {
>> + printk("%s: handler_new_ref on control %x (%s)
> returned %d\n", __func__, ctrl->id, ctrl->name, err);
>
> Perhaps pr_err()?
>
>> + err = 0;
>> + continue;
>
> Hmm, is it really fine to ignore the error and continue here?
It's a debugging left-over. It should just break and return the error.
>
>> + }
>> + if (err)
>> + break;
>> + }
>> + mutex_unlock(from->lock);
>> + return err;
>> +}
>> +
>> +static void v4l2_ctrl_request_queue(struct media_request_object *obj)
>> +{
>> + struct v4l2_ctrl_handler *hdl =
>> + container_of(obj, struct v4l2_ctrl_handler, req_obj);
>> + struct v4l2_ctrl_handler *main_hdl = obj->priv;
>> + struct v4l2_ctrl_handler *prev = NULL;
>> + struct v4l2_ctrl_ref *ref_hdl, *ref_prev = NULL;
>> +
>> + if (list_empty(&main_hdl->requests_queued))
>> + goto queue;
>> +
>> + prev = list_last_entry(&main_hdl->requests_queued,
>> + struct v4l2_ctrl_handler, requests_queued);
>> + mutex_lock(prev->lock);
>> + ref_prev = list_first_entry(&prev->ctrl_refs,
>> + struct v4l2_ctrl_ref, node);
>> + list_for_each_entry(ref_hdl, &hdl->ctrl_refs, node) {
>> + if (ref_hdl->req)
>> + continue;
>> + while (ref_prev->ctrl->id < ref_hdl->ctrl->id)
>> + ref_prev = list_next_entry(ref_prev, node);
>
> Is this really safe? The only stop condition here is the control id.
> Perhaps the code below could be better?
The two ctrl_refs lists must contain the same controls, so it is safe.
However, I modified the code to check that ref_prev isn't the last
element in the list, just in case something gets messed up.
BTW, the variable names in this function are awful :-)
I've changed them so this code is hopefully easier to understand.
>
> list_for_each_entry_from(ref_prev, &prev->ctrl_refs, node)
> if (ref_prev->ctrl->id >= ref_hdl->ctrl->id)
> break;
>
>> + if (WARN_ON(ref_prev->ctrl->id != ref_hdl->ctrl->id))
>> + break;
>> + ref_hdl->req = ref_prev->req;
>> + }
>> + mutex_unlock(prev->lock);
>> +queue:
>> + list_add_tail(&hdl->requests_queued, &main_hdl->requests_queued);
>> + hdl->request_is_queued = true;
>> +}
>> +
>
> [snip]
>> +void v4l2_ctrl_request_complete(struct media_request *req,
>> + struct v4l2_ctrl_handler *main_hdl)
>> +{
>> + struct media_request_object *obj;
>> + struct v4l2_ctrl_handler *hdl;
>> + struct v4l2_ctrl_ref *ref;
>> +
>> + if (!req || !main_hdl)
>> + return;
>
> Can this happen normally? Perhaps WARN_ON() would make sense?
Yes, this can happen. See e.g. vim2m_stop_streaming(). This function is
called for all buffers, but not all buffers may have a request associated
with them. In that case req == NULL and this function should do nothing.
There are also cases (rare, but they exist) where a handler may be NULL
because the specific hardware version does not support the controls in that
handler. In that case the handler may never be created.
Basically this function means: if there is a request, and if there is a
handler, then complete the request.
>
> [snip]
>> +void v4l2_ctrl_request_setup(struct media_request *req,
>> +struct v4l2_ctrl_handler *main_hd
Re: [RFCv11 PATCH 14/29] v4l2-ctrls: add core request support
Hi Hans,
On Mon, Apr 9, 2018 at 11:20 PM Hans Verkuil wrote:
> From: Hans Verkuil
> Integrate the request support. This adds the v4l2_ctrl_request_complete
> and v4l2_ctrl_request_setup functions to complete a request and (as a
> helper function) to apply a request to the hardware.
Please see my comments inline.
[snip]
> +static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
> + const struct v4l2_ctrl_handler *from)
> +{
> + struct v4l2_ctrl_ref *ref;
> + int err;
> +
> + if (WARN_ON(!hdl || hdl == from))
> + return -EINVAL;
> +
> + if (hdl->error)
> + return hdl->error;
> +
> + WARN_ON(hdl->lock != &hdl->_lock);
> +
> + mutex_lock(from->lock);
> + list_for_each_entry(ref, &from->ctrl_refs, node) {
> + struct v4l2_ctrl *ctrl = ref->ctrl;
> + struct v4l2_ctrl_ref *new_ref;
> +
> + /* Skip refs inherited from other devices */
> + if (ref->from_other_dev)
> + continue;
> + /* And buttons */
> + if (ctrl->type == V4L2_CTRL_TYPE_BUTTON)
> + continue;
> + err = handler_new_ref(hdl, ctrl, &new_ref, false, true);
> + if (err) {
> + printk("%s: handler_new_ref on control %x (%s)
returned %d\n", __func__, ctrl->id, ctrl->name, err);
Perhaps pr_err()?
> + err = 0;
> + continue;
Hmm, is it really fine to ignore the error and continue here?
> + }
> + if (err)
> + break;
> + }
> + mutex_unlock(from->lock);
> + return err;
> +}
> +
> +static void v4l2_ctrl_request_queue(struct media_request_object *obj)
> +{
> + struct v4l2_ctrl_handler *hdl =
> + container_of(obj, struct v4l2_ctrl_handler, req_obj);
> + struct v4l2_ctrl_handler *main_hdl = obj->priv;
> + struct v4l2_ctrl_handler *prev = NULL;
> + struct v4l2_ctrl_ref *ref_hdl, *ref_prev = NULL;
> +
> + if (list_empty(&main_hdl->requests_queued))
> + goto queue;
> +
> + prev = list_last_entry(&main_hdl->requests_queued,
> + struct v4l2_ctrl_handler, requests_queued);
> + mutex_lock(prev->lock);
> + ref_prev = list_first_entry(&prev->ctrl_refs,
> + struct v4l2_ctrl_ref, node);
> + list_for_each_entry(ref_hdl, &hdl->ctrl_refs, node) {
> + if (ref_hdl->req)
> + continue;
> + while (ref_prev->ctrl->id < ref_hdl->ctrl->id)
> + ref_prev = list_next_entry(ref_prev, node);
Is this really safe? The only stop condition here is the control id.
Perhaps the code below could be better?
list_for_each_entry_from(ref_prev, &prev->ctrl_refs, node)
if (ref_prev->ctrl->id >= ref_hdl->ctrl->id)
break;
> + if (WARN_ON(ref_prev->ctrl->id != ref_hdl->ctrl->id))
> + break;
> + ref_hdl->req = ref_prev->req;
> + }
> + mutex_unlock(prev->lock);
> +queue:
> + list_add_tail(&hdl->requests_queued, &main_hdl->requests_queued);
> + hdl->request_is_queued = true;
> +}
> +
[snip]
> +void v4l2_ctrl_request_complete(struct media_request *req,
> + struct v4l2_ctrl_handler *main_hdl)
> +{
> + struct media_request_object *obj;
> + struct v4l2_ctrl_handler *hdl;
> + struct v4l2_ctrl_ref *ref;
> +
> + if (!req || !main_hdl)
> + return;
Can this happen normally? Perhaps WARN_ON() would make sense?
[snip]
> +void v4l2_ctrl_request_setup(struct media_request *req,
> +struct v4l2_ctrl_handler *main_hdl)
> +{
> + struct media_request_object *obj;
> + struct v4l2_ctrl_handler *hdl;
> + struct v4l2_ctrl_ref *ref;
> +
> + if (!req || !main_hdl)
Can this happen normally? Perhaps WARN_ON() would make sense?
> + return;
> +
> + obj = media_request_object_find(req, &req_ops, main_hdl);
> + if (!obj)
> + return;
> + if (obj->completed) {
> + media_request_object_put(obj);
> + return;
> + }
> + hdl = container_of(obj, struct v4l2_ctrl_handler, req_obj);
> +
> + mutex_lock(hdl->lock);
> +
> + list_for_each_entry(ref, &hdl->ctrl_refs, node)
> + ref->done = false;
> +
> + list_for_each_entry(ref, &hdl->ctrl_refs, node) {
> + struct v4l2_ctrl *ctrl = ref->ctrl;
> + struct v4l2_ctrl *master = ctrl->cluster[0];
> + bool have_new_data = false;
> + int i;
> +
> + /* Skip if this control was already handled by a cluster.
*/
> + /* Skip button controls and read-only controls. */
> +
Re: [RFCv11 PATCH 14/29] v4l2-ctrls: add core request support
Hi,
On Mon, 2018-04-09 at 16:20 +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> Integrate the request support. This adds the
> v4l2_ctrl_request_complete
> and v4l2_ctrl_request_setup functions to complete a request and (as a
> helper function) to apply a request to the hardware.
>
> It takes care of queuing requests and correctly chaining control
> values
> in the request queue.
>
> Note that when a request is marked completed it will copy control
> values
> to the internal request state. This can be optimized in the future
> since
> this is sub-optimal when dealing with large compound and/or array
> controls.
>
> For the initial 'stateless codec' use-case the current implementation
> is
> sufficient.
See one comment about a missing unlock below.
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 325
> ++-
> include/media/v4l2-ctrls.h | 23 +++
> 2 files changed, 342 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index da4cc1485dc4..6e2c5e24734f 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1647,6 +1647,13 @@ static int new_to_user(struct v4l2_ext_control
> *c,
> return ptr_to_user(c, ctrl, ctrl->p_new);
> }
>
> +/* Helper function: copy the request value back to the caller */
> +static int req_to_user(struct v4l2_ext_control *c,
> +struct v4l2_ctrl_ref *ref)
> +{
> + return ptr_to_user(c, ref->ctrl, ref->p_req);
> +}
> +
> /* Helper function: copy the initial control value back to the caller
> */
> static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl
> *ctrl)
> {
> @@ -1766,6 +1773,26 @@ static void cur_to_new(struct v4l2_ctrl *ctrl)
> ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
> }
>
> +/* Copy the new value to the request value */
> +static void new_to_req(struct v4l2_ctrl_ref *ref)
> +{
> + if (!ref)
> + return;
> + ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
> + ref->req = ref;
> +}
> +
> +/* Copy the request value to the new value */
> +static void req_to_new(struct v4l2_ctrl_ref *ref)
> +{
> + if (!ref)
> + return;
> + if (ref->req)
> + ptr_to_ptr(ref->ctrl, ref->req->p_req, ref->ctrl-
> >p_new);
> + else
> + ptr_to_ptr(ref->ctrl, ref->ctrl->p_cur, ref->ctrl-
> >p_new);
> +}
> +
> /* Return non-zero if one or more of the controls in the cluster has
> a new
> value that differs from the current value. */
> static int cluster_changed(struct v4l2_ctrl *master)
> @@ -1875,6 +1902,9 @@ int v4l2_ctrl_handler_init_class(struct
> v4l2_ctrl_handler *hdl,
> lockdep_set_class_and_name(hdl->lock, key, name);
> INIT_LIST_HEAD(&hdl->ctrls);
> INIT_LIST_HEAD(&hdl->ctrl_refs);
> + INIT_LIST_HEAD(&hdl->requests);
> + INIT_LIST_HEAD(&hdl->requests_queued);
> + hdl->request_is_queued = false;
> hdl->nr_of_buckets = 1 + nr_of_controls_hint / 8;
> hdl->buckets = kvmalloc_array(hdl->nr_of_buckets,
> sizeof(hdl->buckets[0]),
> @@ -1895,6 +1925,14 @@ void v4l2_ctrl_handler_free(struct
> v4l2_ctrl_handler *hdl)
> if (hdl == NULL || hdl->buckets == NULL)
> return;
>
> + if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
> + struct v4l2_ctrl_handler *req, *next_req;
> +
> + list_for_each_entry_safe(req, next_req, &hdl-
> >requests, requests) {
> + media_request_object_unbind(&req->req_obj);
> + media_request_object_put(&req->req_obj);
> + }
> + }
> mutex_lock(hdl->lock);
> /* Free all nodes */
> list_for_each_entry_safe(ref, next_ref, &hdl->ctrl_refs,
> node) {
> @@ -2816,6 +2854,126 @@ int v4l2_querymenu(struct v4l2_ctrl_handler
> *hdl, struct v4l2_querymenu *qm)
> }
> EXPORT_SYMBOL(v4l2_querymenu);
>
> +static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
> +const struct v4l2_ctrl_handler
> *from)
> +{
> + struct v4l2_ctrl_ref *ref;
> + int err;
> +
> + if (WARN_ON(!hdl || hdl == from))
> + return -EINVAL;
> +
> + if (hdl->error)
> + return hdl->error;
> +
> + WARN_ON(hdl->lock != &hdl->_lock);
> +
> + mutex_lock(from->lock);
> + list_for_each_entry(ref, &from->ctrl_refs, node) {
> + struct v4l2_ctrl *ctrl = ref->ctrl;
> + struct v4l2_ctrl_ref *new_ref;
> +
> + /* Skip refs inherited from other devices */
> + if (ref->from_other_dev)
> + continue;
> + /* And buttons */
> + if (ctrl->type == V4L2_CTRL_TYPE_BUTTON)
> + continue;
> + err = handler_new_ref(hdl, ctrl, &new_ref, false,
> true);
> + if (err) {
> +
[RFCv11 PATCH 14/29] v4l2-ctrls: add core request support
From: Hans Verkuil
Integrate the request support. This adds the v4l2_ctrl_request_complete
and v4l2_ctrl_request_setup functions to complete a request and (as a
helper function) to apply a request to the hardware.
It takes care of queuing requests and correctly chaining control values
in the request queue.
Note that when a request is marked completed it will copy control values
to the internal request state. This can be optimized in the future since
this is sub-optimal when dealing with large compound and/or array controls.
For the initial 'stateless codec' use-case the current implementation is
sufficient.
Signed-off-by: Hans Verkuil
---
drivers/media/v4l2-core/v4l2-ctrls.c | 325 ++-
include/media/v4l2-ctrls.h | 23 +++
2 files changed, 342 insertions(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index da4cc1485dc4..6e2c5e24734f 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1647,6 +1647,13 @@ static int new_to_user(struct v4l2_ext_control *c,
return ptr_to_user(c, ctrl, ctrl->p_new);
}
+/* Helper function: copy the request value back to the caller */
+static int req_to_user(struct v4l2_ext_control *c,
+ struct v4l2_ctrl_ref *ref)
+{
+ return ptr_to_user(c, ref->ctrl, ref->p_req);
+}
+
/* Helper function: copy the initial control value back to the caller */
static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
{
@@ -1766,6 +1773,26 @@ static void cur_to_new(struct v4l2_ctrl *ctrl)
ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
}
+/* Copy the new value to the request value */
+static void new_to_req(struct v4l2_ctrl_ref *ref)
+{
+ if (!ref)
+ return;
+ ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
+ ref->req = ref;
+}
+
+/* Copy the request value to the new value */
+static void req_to_new(struct v4l2_ctrl_ref *ref)
+{
+ if (!ref)
+ return;
+ if (ref->req)
+ ptr_to_ptr(ref->ctrl, ref->req->p_req, ref->ctrl->p_new);
+ else
+ ptr_to_ptr(ref->ctrl, ref->ctrl->p_cur, ref->ctrl->p_new);
+}
+
/* Return non-zero if one or more of the controls in the cluster has a new
value that differs from the current value. */
static int cluster_changed(struct v4l2_ctrl *master)
@@ -1875,6 +1902,9 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler
*hdl,
lockdep_set_class_and_name(hdl->lock, key, name);
INIT_LIST_HEAD(&hdl->ctrls);
INIT_LIST_HEAD(&hdl->ctrl_refs);
+ INIT_LIST_HEAD(&hdl->requests);
+ INIT_LIST_HEAD(&hdl->requests_queued);
+ hdl->request_is_queued = false;
hdl->nr_of_buckets = 1 + nr_of_controls_hint / 8;
hdl->buckets = kvmalloc_array(hdl->nr_of_buckets,
sizeof(hdl->buckets[0]),
@@ -1895,6 +1925,14 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler
*hdl)
if (hdl == NULL || hdl->buckets == NULL)
return;
+ if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
+ struct v4l2_ctrl_handler *req, *next_req;
+
+ list_for_each_entry_safe(req, next_req, &hdl->requests,
requests) {
+ media_request_object_unbind(&req->req_obj);
+ media_request_object_put(&req->req_obj);
+ }
+ }
mutex_lock(hdl->lock);
/* Free all nodes */
list_for_each_entry_safe(ref, next_ref, &hdl->ctrl_refs, node) {
@@ -2816,6 +2854,126 @@ int v4l2_querymenu(struct v4l2_ctrl_handler *hdl,
struct v4l2_querymenu *qm)
}
EXPORT_SYMBOL(v4l2_querymenu);
+static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
+ const struct v4l2_ctrl_handler *from)
+{
+ struct v4l2_ctrl_ref *ref;
+ int err;
+
+ if (WARN_ON(!hdl || hdl == from))
+ return -EINVAL;
+
+ if (hdl->error)
+ return hdl->error;
+
+ WARN_ON(hdl->lock != &hdl->_lock);
+
+ mutex_lock(from->lock);
+ list_for_each_entry(ref, &from->ctrl_refs, node) {
+ struct v4l2_ctrl *ctrl = ref->ctrl;
+ struct v4l2_ctrl_ref *new_ref;
+
+ /* Skip refs inherited from other devices */
+ if (ref->from_other_dev)
+ continue;
+ /* And buttons */
+ if (ctrl->type == V4L2_CTRL_TYPE_BUTTON)
+ continue;
+ err = handler_new_ref(hdl, ctrl, &new_ref, false, true);
+ if (err) {
+ printk("%s: handler_new_ref on control %x (%s) returned
%d\n", __func__, ctrl->id, ctrl->name, err);
+ err = 0;
+ continue;
+ }
+ if (err)
+ break;
+ }
+ mutex_unlock(from-
