Em Sat, 4 Aug 2018 14:45:06 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> 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 <hans.verkuil@xxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 326 ++++++++++++++++++++++++++-
> include/media/v4l2-ctrls.h | 51 +++++
> 2 files changed, 371 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 570b6f8ae46a..b8ff6d6b14cd 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1668,6 +1668,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)
> {
> @@ -1787,6 +1794,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)
> @@ -1896,6 +1923,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]),
> @@ -1916,6 +1946,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);
Hmm... while this would work for the trivial case where object_put()
would just drop the object from the list if nobody else is using it,
nothing prevents that, if v4l2_ctrl_handler_free() is called twice,
it would do the wrong thing... as the only test here is if req_obj.reg
is not NULL, and not if the control framework is already done with the
object.
> + }
> + }
> mutex_lock(hdl->lock);
> /* Free all nodes */
> list_for_each_entry_safe(ref, next_ref, &hdl->ctrl_refs, node) {
> @@ -2837,6 +2875,123 @@ 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)
> + 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_hdl = NULL;
> + struct v4l2_ctrl_ref *ref_ctrl, *ref_ctrl_prev = NULL;
> +
> + if (list_empty(&main_hdl->requests_queued))
> + goto queue;
> +
> + prev_hdl = list_last_entry(&main_hdl->requests_queued,
> + struct v4l2_ctrl_handler, requests_queued);
> + /*
> + * Note: prev_hdl and hdl must contain the same list of control
> + * references, so if any differences are detected then that is a
> + * driver bug and the WARN_ON is triggered.
> + */
> + mutex_lock(prev_hdl->lock);
> + ref_ctrl_prev = list_first_entry(&prev_hdl->ctrl_refs,
> + struct v4l2_ctrl_ref, node);
> + list_for_each_entry(ref_ctrl, &hdl->ctrl_refs, node) {
> + if (ref_ctrl->req)
> + continue;
> + while (ref_ctrl_prev->ctrl->id < ref_ctrl->ctrl->id) {
> + /* Should never happen, but just in case... */
> + if (list_is_last(&ref_ctrl_prev->node,
> + &prev_hdl->ctrl_refs))
If should never happen, please use unlikely(). That makes clearer
while doing some speed optimization).
> + break;
> + ref_ctrl_prev = list_next_entry(ref_ctrl_prev, node);
> + }
> + if (WARN_ON(ref_ctrl_prev->ctrl->id != ref_ctrl->ctrl->id))
> + break;
> + ref_ctrl->req = ref_ctrl_prev->req;
> + }
> + mutex_unlock(prev_hdl->lock);
> +queue:
> + list_add_tail(&hdl->requests_queued, &main_hdl->requests_queued);
Shouldn't list changes be serialized? Ok, the ioctls are serialized, but
device removal/unbind can happen any time for hot-plugged devices[1].
[1] yeah, I know that, right now, the framework is meant to be used only
by codecs that are on SoC, but I'm pretty sure we'll end by using it on
other use cases in the future. As this is core code, we should be sure
that it will not cause troubles due to the lack of a proper locking, as
I doubt we'll review the locks when adding other use cases.
> + hdl->request_is_queued = true;
> +}
> +
> +static void v4l2_ctrl_request_unbind(struct media_request_object *obj)
> +{
> + struct v4l2_ctrl_handler *hdl =
> + container_of(obj, struct v4l2_ctrl_handler, req_obj);
> +
> + list_del_init(&hdl->requests);
> + if (hdl->request_is_queued) {
> + list_del_init(&hdl->requests_queued);
> + hdl->request_is_queued = false;
> + }
> +}
> +
> +static void v4l2_ctrl_request_release(struct media_request_object *obj)
> +{
> + struct v4l2_ctrl_handler *hdl =
> + container_of(obj, struct v4l2_ctrl_handler, req_obj);
> +
> + v4l2_ctrl_handler_free(hdl);
> + kfree(hdl);
> +}
> +
> +static const struct media_request_object_ops req_ops = {
> + .queue = v4l2_ctrl_request_queue,
> + .unbind = v4l2_ctrl_request_unbind,
> + .release = v4l2_ctrl_request_release,
> +};
> +
> +static int v4l2_ctrl_request_bind(struct media_request *req,
> + struct v4l2_ctrl_handler *hdl,
> + struct v4l2_ctrl_handler *from)
> +{
> + int ret;
> +
> + ret = v4l2_ctrl_request_clone(hdl, from);
> +
> + if (!ret) {
> + ret = media_request_object_bind(req, &req_ops,
> + from, &hdl->req_obj);
> + if (!ret)
> + list_add_tail(&hdl->requests, &from->requests);
> + }
> + return ret;
> +}
>
> /* Some general notes on the atomic requirements of VIDIOC_G/TRY/S_EXT_CTRLS:
>
> @@ -2898,6 +3053,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>
> if (cs->which &&
> cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
> + cs->which != V4L2_CTRL_WHICH_REQUEST_VAL &&
> V4L2_CTRL_ID2WHICH(id) != cs->which)
> return -EINVAL;
>
> @@ -2977,13 +3133,12 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> whether there are any controls at all. */
> static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
> {
> - if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
> + if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL ||
> + which == V4L2_CTRL_WHICH_REQUEST_VAL)
> return 0;
> return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
> }
>
> -
> -
> /* Get extended controls. Allocates the helpers array if needed. */
> int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
> {
> @@ -3049,8 +3204,12 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
> u32 idx = i;
>
> do {
> - ret = ctrl_to_user(cs->controls + idx,
> - helpers[idx].ref->ctrl);
> + if (helpers[idx].ref->req)
> + ret = req_to_user(cs->controls + idx,
> + helpers[idx].ref->req);
> + else
> + ret = ctrl_to_user(cs->controls + idx,
> + helpers[idx].ref->ctrl);
> idx = helpers[idx].next;
> } while (!ret && idx);
> }
> @@ -3336,7 +3495,16 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
> } while (!ret && idx);
>
> if (!ret)
> - ret = try_or_set_cluster(fh, master, set, 0);
> + ret = try_or_set_cluster(fh, master,
> + !hdl->req_obj.req && set, 0);
> + if (!ret && hdl->req_obj.req && set) {
> + for (j = 0; j < master->ncontrols; j++) {
> + struct v4l2_ctrl_ref *ref =
> + find_ref(hdl, master->cluster[j]->id);
> +
> + new_to_req(ref);
> + }
> + }
>
> /* Copy the new values back to userspace. */
> if (!ret) {
> @@ -3463,6 +3631,152 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
> }
> EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
>
> +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;
> +
> + obj = media_request_object_find(req, &req_ops, main_hdl);
> + if (!obj)
> + return;
> + hdl = container_of(obj, struct v4l2_ctrl_handler, req_obj);
> +
> + list_for_each_entry(ref, &hdl->ctrl_refs, node) {
> + struct v4l2_ctrl *ctrl = ref->ctrl;
> + struct v4l2_ctrl *master = ctrl->cluster[0];
> + unsigned int i;
> +
> + if (ctrl->flags & V4L2_CTRL_FLAG_VOLATILE) {
> + ref->req = ref;
> +
> + v4l2_ctrl_lock(master);
> + /* g_volatile_ctrl will update the current control values */
> + for (i = 0; i < master->ncontrols; i++)
> + cur_to_new(master->cluster[i]);
> + call_op(master, g_volatile_ctrl);
> + new_to_req(ref);
> + v4l2_ctrl_unlock(master);
> + continue;
> + }
> + if (ref->req == ref)
> + continue;
> +
> + v4l2_ctrl_lock(ctrl);
> + if (ref->req)
> + ptr_to_ptr(ctrl, ref->req->p_req, ref->p_req);
> + else
> + ptr_to_ptr(ctrl, ctrl->p_cur, ref->p_req);
> + v4l2_ctrl_unlock(ctrl);
> + }
> +
> + WARN_ON(!hdl->request_is_queued);
> + list_del_init(&hdl->requests_queued);
> + hdl->request_is_queued = false;
> + media_request_object_complete(obj);
> + media_request_object_put(obj);
Hmm... nothing prevents that this would race with v4l2_ctrl_handler_free()
and cause double-free (actually double object_put).
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_request_complete);
> +
> +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)
> + return;
> +
> + if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> + return;
> +
> + obj = media_request_object_find(req, &req_ops, main_hdl);
> + if (!obj)
> + return;
Shouldn't the above checks produce an error or print something at
the logs?
> + if (obj->completed) {
> + media_request_object_put(obj);
> + return;
> + }
> + hdl = container_of(obj, struct v4l2_ctrl_handler, req_obj);
> +
> + list_for_each_entry(ref, &hdl->ctrl_refs, node)
> + ref->req_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.
> + */
> + if (ref->req_done || ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
> + (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY))
> + continue;
> +
> + v4l2_ctrl_lock(master);
> + for (i = 0; i < master->ncontrols; i++) {
> + if (master->cluster[i]) {
> + struct v4l2_ctrl_ref *r =
> + find_ref(hdl, master->cluster[i]->id);
> +
> + if (r->req && r == r->req) {
> + have_new_data = true;
> + break;
> + }
> + }
> + }
> + if (!have_new_data) {
> + v4l2_ctrl_unlock(master);
> + continue;
> + }
> +
> + for (i = 0; i < master->ncontrols; i++) {
> + if (master->cluster[i]) {
> + struct v4l2_ctrl_ref *r =
> + find_ref(hdl, master->cluster[i]->id);
> +
> + req_to_new(r);
> + master->cluster[i]->is_new = 1;
> + r->req_done = true;
> + }
> + }
> + /*
> + * For volatile autoclusters that are currently in auto mode
> + * we need to discover if it will be set to manual mode.
> + * If so, then we have to copy the current volatile values
> + * first since those will become the new manual values (which
> + * may be overwritten by explicit new values from this set
> + * of controls).
> + */
> + if (master->is_auto && master->has_volatiles &&
> + !is_cur_manual(master)) {
> + s32 new_auto_val = *master->p_new.p_s32;
> +
> + /*
> + * If the new value == the manual value, then copy
> + * the current volatile values.
> + */
> + if (new_auto_val == master->manual_mode_value)
> + update_from_auto_cluster(master);
> + }
> +
> + try_or_set_cluster(NULL, master, true, 0);
> +
> + v4l2_ctrl_unlock(master);
> + }
> +
> + media_request_object_put(obj);
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_request_setup);
> +
> void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void *priv)
> {
> if (ctrl == NULL)
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 3f4e062d4e3d..3617d5077b19 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -250,6 +250,12 @@ struct v4l2_ctrl {
> * ``prepare_ext_ctrls`` function at ``v4l2-ctrl.c``.
> * @from_other_dev: If true, then @ctrl was defined in another
> * device than the &struct v4l2_ctrl_handler.
> + * @req_done: Internal flag: if the control handler containing this control
> + * reference is bound to a media request, then this is set when
> + * the control has been applied. This prevents applying controls
> + * from a cluster with multiple controls twice (when the first
> + * control of a cluster is applied, they all are).
> + * @req: If set, this refers to another request that sets this control.
> * @p_req: If the control handler containing this control reference
> * is bound to a media request, then this points to the
> * value of the control that should be applied when the request
> @@ -266,6 +272,8 @@ struct v4l2_ctrl_ref {
> struct v4l2_ctrl *ctrl;
> struct v4l2_ctrl_helper *helper;
> bool from_other_dev;
> + bool req_done;
> + struct v4l2_ctrl_ref *req;
> union v4l2_ctrl_ptr p_req;
> };
>
> @@ -290,6 +298,15 @@ struct v4l2_ctrl_ref {
> * @notify_priv: Passed as argument to the v4l2_ctrl notify callback.
> * @nr_of_buckets: Total number of buckets in the array.
> * @error: The error code of the first failed control addition.
> + * @request_is_queued: True if the request was queued.
> + * @requests: List to keep track of open control handler request objects.
> + * For the parent control handler (@req_obj.req == NULL) this
> + * is the list header. When the parent control handler is
> + * removed, it has to unbind and put all these requests since
> + * they refer to the parent.
> + * @requests_queued: List of the queued requests. This determines the order
> + * in which these controls are applied. Once the request is
> + * completed it is removed from this list.
> * @req_obj: The &struct media_request_object, used to link into a
> * &struct media_request. This request object has a refcount.
> */
> @@ -304,6 +321,9 @@ struct v4l2_ctrl_handler {
> void *notify_priv;
> u16 nr_of_buckets;
> int error;
> + bool request_is_queued;
> + struct list_head requests;
> + struct list_head requests_queued;
> struct media_request_object req_obj;
> };
>
> @@ -1062,6 +1082,37 @@ int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh,
> */
> __poll_t v4l2_ctrl_poll(struct file *file, struct poll_table_struct *wait);
>
> +/**
> + * v4l2_ctrl_request_setup - helper function to apply control values in a request
> + *
> + * @req: The request
> + * @hdl: The control handler
> + *
> + * This is a helper function to call the control handler's s_ctrl callback with
> + * the control values contained in the request. Do note that this approach of
> + * applying control values in a request is only applicable to memory-to-memory
> + * devices.
> + */
> +void v4l2_ctrl_request_setup(struct media_request *req,
> + struct v4l2_ctrl_handler *hdl);
> +
> +/**
> + * v4l2_ctrl_request_complete - Complete a control handler request object
> + *
> + * @req: The request
> + * @hdl: The control handler
> + *
> + * This function is to be called on each control handler that may have had a
> + * request object associated with it, i.e. control handlers of a driver that
> + * supports requests.
> + *
> + * The function first obtains the values of any volatile controls in the control
> + * handler and attach them to the request. Then, the function completes the
> + * request object.
> + */
> +void v4l2_ctrl_request_complete(struct media_request *req,
> + struct v4l2_ctrl_handler *hdl);
> +
> /* Helpers for ioctl_ops */
>
> /**
Thanks,
Mauro
