* [RFCv1 PATCH 2/8] v4l2-ctrls/event: remove struct v4l2_ctrl_fh, instead use v4l2_subscribed_event
2011-06-14 15:22 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Hans Verkuil
@ 2011-06-14 15:22 ` Hans Verkuil
2011-06-20 13:50 ` Laurent Pinchart
2011-06-14 15:22 ` [RFCv1 PATCH 3/8] v4l2-event/ctrls/fh: allocate events per fh and per type instead of just per-fh Hans Verkuil
` (6 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2011-06-14 15:22 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, sakari.ailus, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The v4l2_ctrl_fh struct connected v4l2_ctrl with v4l2_fh so the control
would know which filehandles subscribed to it. However, it is much easier
to use struct v4l2_subscribed_event directly for that and get rid of that
intermediate struct.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 50 ++++++++++++++-----------------------
drivers/media/video/v4l2-event.c | 34 +++++++++----------------
include/media/v4l2-ctrls.h | 17 +++++-------
include/media/v4l2-event.h | 9 +++++++
4 files changed, 47 insertions(+), 63 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index f581910..079f952 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -581,15 +581,15 @@ static void fill_event(struct v4l2_event *ev, struct v4l2_ctrl *ctrl, u32 change
static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
{
struct v4l2_event ev;
- struct v4l2_ctrl_fh *pos;
+ struct v4l2_subscribed_event *sev;
- if (list_empty(&ctrl->fhs))
+ if (list_empty(&ctrl->ev_subs))
return;
fill_event(&ev, ctrl, changes);
- list_for_each_entry(pos, &ctrl->fhs, node)
- if (pos->fh != fh)
- v4l2_event_queue_fh(pos->fh, &ev);
+ list_for_each_entry(sev, &ctrl->ev_subs, node)
+ if (sev->fh && sev->fh != fh)
+ v4l2_event_queue_fh(sev->fh, &ev);
}
/* Helper function: copy the current control value back to the caller */
@@ -867,7 +867,7 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
{
struct v4l2_ctrl_ref *ref, *next_ref;
struct v4l2_ctrl *ctrl, *next_ctrl;
- struct v4l2_ctrl_fh *ctrl_fh, *next_ctrl_fh;
+ struct v4l2_subscribed_event *sev, *next_sev;
if (hdl == NULL || hdl->buckets == NULL)
return;
@@ -881,10 +881,8 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
/* Free all controls owned by the handler */
list_for_each_entry_safe(ctrl, next_ctrl, &hdl->ctrls, node) {
list_del(&ctrl->node);
- list_for_each_entry_safe(ctrl_fh, next_ctrl_fh, &ctrl->fhs, node) {
- list_del(&ctrl_fh->node);
- kfree(ctrl_fh);
- }
+ list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
+ list_del(&sev->node);
kfree(ctrl);
}
kfree(hdl->buckets);
@@ -1084,7 +1082,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
}
INIT_LIST_HEAD(&ctrl->node);
- INIT_LIST_HEAD(&ctrl->fhs);
+ INIT_LIST_HEAD(&ctrl->ev_subs);
ctrl->handler = hdl;
ctrl->ops = ops;
ctrl->id = id;
@@ -2027,41 +2025,31 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
}
EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
-void v4l2_ctrl_add_fh(struct v4l2_ctrl_handler *hdl,
- struct v4l2_ctrl_fh *ctrl_fh,
- struct v4l2_event_subscription *sub)
+void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
+ struct v4l2_subscribed_event *sev)
{
- struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, sub->id);
-
v4l2_ctrl_lock(ctrl);
- list_add_tail(&ctrl_fh->node, &ctrl->fhs);
+ list_add_tail(&sev->node, &ctrl->ev_subs);
if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
- (sub->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
+ (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
struct v4l2_event ev;
fill_event(&ev, ctrl, V4L2_EVENT_CTRL_CH_VALUE |
V4L2_EVENT_CTRL_CH_FLAGS);
- v4l2_event_queue_fh(ctrl_fh->fh, &ev);
+ v4l2_event_queue_fh(sev->fh, &ev);
}
v4l2_ctrl_unlock(ctrl);
}
-EXPORT_SYMBOL(v4l2_ctrl_add_fh);
+EXPORT_SYMBOL(v4l2_ctrl_add_event);
-void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh)
+void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
+ struct v4l2_subscribed_event *sev)
{
- struct v4l2_ctrl_fh *pos;
-
v4l2_ctrl_lock(ctrl);
- list_for_each_entry(pos, &ctrl->fhs, node) {
- if (pos->fh == fh) {
- list_del(&pos->node);
- kfree(pos);
- break;
- }
- }
+ list_del(&sev->node);
v4l2_ctrl_unlock(ctrl);
}
-EXPORT_SYMBOL(v4l2_ctrl_del_fh);
+EXPORT_SYMBOL(v4l2_ctrl_del_event);
int v4l2_ctrl_subscribe_fh(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub, unsigned n)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 70fa82d..dc68f60 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -213,7 +213,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev, *found_ev;
struct v4l2_ctrl *ctrl = NULL;
- struct v4l2_ctrl_fh *ctrl_fh = NULL;
unsigned long flags;
if (sub->type == V4L2_EVENT_CTRL) {
@@ -222,17 +221,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
return -EINVAL;
}
- sev = kmalloc(sizeof(*sev), GFP_KERNEL);
+ sev = kzalloc(sizeof(*sev), GFP_KERNEL);
if (!sev)
return -ENOMEM;
- if (ctrl) {
- ctrl_fh = kzalloc(sizeof(*ctrl_fh), GFP_KERNEL);
- if (!ctrl_fh) {
- kfree(sev);
- return -ENOMEM;
- }
- ctrl_fh->fh = fh;
- }
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
@@ -241,22 +232,19 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
INIT_LIST_HEAD(&sev->list);
sev->type = sub->type;
sev->id = sub->id;
+ sev->fh = fh;
+ sev->flags = sub->flags;
list_add(&sev->list, &fh->subscribed);
- sev = NULL;
}
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
/* v4l2_ctrl_add_fh uses a mutex, so do this outside the spin lock */
- if (ctrl) {
- if (found_ev)
- kfree(ctrl_fh);
- else
- v4l2_ctrl_add_fh(fh->ctrl_handler, ctrl_fh, sub);
- }
-
- kfree(sev);
+ if (found_ev)
+ kfree(sev);
+ else if (ctrl)
+ v4l2_ctrl_add_event(ctrl, sev);
return 0;
}
@@ -298,15 +286,17 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL)
+ if (sev != NULL) {
list_del(&sev->list);
+ sev->fh = NULL;
+ }
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
- if (sev->type == V4L2_EVENT_CTRL) {
+ if (sev && sev->type == V4L2_EVENT_CTRL) {
struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
if (ctrl)
- v4l2_ctrl_del_fh(ctrl, fh);
+ v4l2_ctrl_del_event(ctrl, sev);
}
kfree(sev);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index de68a59..635adc2 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -31,6 +31,7 @@ struct v4l2_ctrl;
struct video_device;
struct v4l2_subdev;
struct v4l2_event_subscription;
+struct v4l2_subscribed_event;
struct v4l2_fh;
/** struct v4l2_ctrl_ops - The control operations that the driver has to provide.
@@ -53,6 +54,7 @@ struct v4l2_ctrl_ops {
/** struct v4l2_ctrl - The control structure.
* @node: The list node.
+ * @ev_subs: The list of control event subscriptions.
* @handler: The handler that owns the control.
* @cluster: Point to start of cluster array.
* @ncontrols: Number of controls in cluster array.
@@ -108,7 +110,7 @@ struct v4l2_ctrl_ops {
struct v4l2_ctrl {
/* Administrative fields */
struct list_head node;
- struct list_head fhs;
+ struct list_head ev_subs;
struct v4l2_ctrl_handler *handler;
struct v4l2_ctrl **cluster;
unsigned ncontrols;
@@ -184,11 +186,6 @@ struct v4l2_ctrl_handler {
int error;
};
-struct v4l2_ctrl_fh {
- struct list_head node;
- struct v4l2_fh *fh;
-};
-
/** struct v4l2_ctrl_config - Control configuration structure.
* @ops: The control ops.
* @id: The control ID.
@@ -497,10 +494,10 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
/* Internal helper functions that deal with control events. */
-void v4l2_ctrl_add_fh(struct v4l2_ctrl_handler *hdl,
- struct v4l2_ctrl_fh *ctrl_fh,
- struct v4l2_event_subscription *sub);
-void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh);
+void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
+ struct v4l2_subscribed_event *sev);
+void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
+ struct v4l2_subscribed_event *sev);
/** v4l2_ctrl_subscribe_fh() - Helper function that subscribes a control event.
* @fh: The file handler that subscribed the control event.
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 042b893..eda17f8 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -38,9 +38,18 @@ struct v4l2_kevent {
};
struct v4l2_subscribed_event {
+ /* list node for the v4l2_fh->subscribed list */
struct list_head list;
+ /* event type */
u32 type;
+ /* associated object ID (e.g. control ID) */
u32 id;
+ /* copy of v4l2_event_subscription->flags */
+ u32 flags;
+ /* filehandle that subscribed to this event */
+ struct v4l2_fh *fh;
+ /* list node that hooks into the object's event list (if there is one) */
+ struct list_head node;
};
int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [RFCv1 PATCH 2/8] v4l2-ctrls/event: remove struct v4l2_ctrl_fh, instead use v4l2_subscribed_event
2011-06-14 15:22 ` [RFCv1 PATCH 2/8] v4l2-ctrls/event: remove struct v4l2_ctrl_fh, instead use v4l2_subscribed_event Hans Verkuil
@ 2011-06-20 13:50 ` Laurent Pinchart
0 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2011-06-20 13:50 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, sakari.ailus, Hans Verkuil
Hi Hans,
Thanks for the patch.
On Tuesday 14 June 2011 17:22:27 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> The v4l2_ctrl_fh struct connected v4l2_ctrl with v4l2_fh so the control
> would know which filehandles subscribed to it. However, it is much easier
> to use struct v4l2_subscribed_event directly for that and get rid of that
> intermediate struct.
[snip]
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 042b893..eda17f8 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -38,9 +38,18 @@ struct v4l2_kevent {
> };
>
> struct v4l2_subscribed_event {
> + /* list node for the v4l2_fh->subscribed list */
> struct list_head list;
> + /* event type */
> u32 type;
> + /* associated object ID (e.g. control ID) */
> u32 id;
> + /* copy of v4l2_event_subscription->flags */
> + u32 flags;
> + /* filehandle that subscribed to this event */
> + struct v4l2_fh *fh;
> + /* list node that hooks into the object's event list (if there is one) */
> + struct list_head node;
> };
>
> int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
What about using kerneldoc style and document the structure in a single
comment block right above it ? I find it easier to read.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* [RFCv1 PATCH 3/8] v4l2-event/ctrls/fh: allocate events per fh and per type instead of just per-fh
2011-06-14 15:22 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Hans Verkuil
2011-06-14 15:22 ` [RFCv1 PATCH 2/8] v4l2-ctrls/event: remove struct v4l2_ctrl_fh, instead use v4l2_subscribed_event Hans Verkuil
@ 2011-06-14 15:22 ` Hans Verkuil
2011-06-14 15:22 ` [RFCv1 PATCH 4/8] v4l2-event: add optional 'merge' callback to merge two events Hans Verkuil
` (5 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Hans Verkuil @ 2011-06-14 15:22 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, sakari.ailus, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The driver had to decide how many events to allocate when the v4l2_fh struct
was created. It was possible to add more events afterwards, but there was no
way to ensure that you wouldn't miss important events if the event queue
would fill up for that filehandle.
In addition, once there were no more free events, any new events were simply
dropped on the floor.
For the control event in particular this made life very difficult since
control status/value changes could just be missed if the number of allocated
events and the speed at which the application read events was too low to keep
up with the number of generated events. The application would have no idea
what the latest state was for a control since it could have missed the latest
control change.
So this patch makes some major changes in how events are allocated. Instead
of allocating events per-filehandle they are now allocated when subscribing an
event. So for that particular event type N events (determined by the driver)
are allocated. Those events are reserved for that particular event type.
This ensures that you will not miss events for a particular type altogether.
In addition, if there are N events in use and a new event is raised, then
the oldest event is dropped and the new one is added. So the latest event
is always available.
This can be further improved by adding the ability to merge the state of
two events together, ensuring that no data is lost at all. This will be
added in the next patch.
This also makes it possible to allow the user to determine the number of
events that will be allocated. This is not implemented at the moment, but
would be trivial.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/ivtv/ivtv-fileops.c | 4 --
drivers/media/video/ivtv/ivtv-ioctl.c | 4 +-
drivers/media/video/omap3isp/ispccdc.c | 3 +-
drivers/media/video/omap3isp/ispstat.c | 3 +-
drivers/media/video/v4l2-ctrls.c | 18 ------
drivers/media/video/v4l2-event.c | 88 ++++++++++++-------------------
drivers/media/video/v4l2-fh.c | 4 +-
drivers/media/video/v4l2-subdev.c | 7 ---
drivers/media/video/vivi.c | 2 +-
drivers/usb/gadget/uvc_v4l2.c | 12 +----
include/media/v4l2-ctrls.h | 19 -------
include/media/v4l2-event.h | 18 +++++-
include/media/v4l2-fh.h | 2 -
include/media/v4l2-subdev.h | 2 -
14 files changed, 54 insertions(+), 132 deletions(-)
diff --git a/drivers/media/video/ivtv/ivtv-fileops.c b/drivers/media/video/ivtv/ivtv-fileops.c
index e507766..5796262 100644
--- a/drivers/media/video/ivtv/ivtv-fileops.c
+++ b/drivers/media/video/ivtv/ivtv-fileops.c
@@ -957,10 +957,6 @@ static int ivtv_serialized_open(struct ivtv_stream *s, struct file *filp)
return -ENOMEM;
}
v4l2_fh_init(&item->fh, s->vdev);
- if (s->type == IVTV_DEC_STREAM_TYPE_YUV ||
- s->type == IVTV_DEC_STREAM_TYPE_MPG) {
- res = v4l2_event_alloc(&item->fh, 60);
- }
if (res < 0) {
v4l2_fh_exit(&item->fh);
kfree(item);
diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index a81b4be..99b2bdc 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1444,13 +1444,11 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
switch (sub->type) {
case V4L2_EVENT_VSYNC:
case V4L2_EVENT_EOS:
- break;
case V4L2_EVENT_CTRL:
- return v4l2_ctrl_subscribe_fh(fh, sub, 0);
+ return v4l2_event_subscribe(fh, sub, 0);
default:
return -EINVAL;
}
- return v4l2_event_subscribe(fh, sub);
}
static int ivtv_log_status(struct file *file, void *fh)
diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
index 39d501b..6766247 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -1691,7 +1691,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
if (sub->type != V4L2_EVENT_OMAP3ISP_HS_VS)
return -EINVAL;
- return v4l2_event_subscribe(fh, sub);
+ return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
}
static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
@@ -2162,7 +2162,6 @@ static int ccdc_init_entities(struct isp_ccdc_device *ccdc)
sd->grp_id = 1 << 16; /* group ID for isp subdevs */
v4l2_set_subdevdata(sd, ccdc);
sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
- sd->nevents = OMAP3ISP_CCDC_NEVENTS;
pads[CCDC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
pads[CCDC_PAD_SOURCE_VP].flags = MEDIA_PAD_FL_SOURCE;
diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
index b44cb68..8080659 100644
--- a/drivers/media/video/omap3isp/ispstat.c
+++ b/drivers/media/video/omap3isp/ispstat.c
@@ -1032,7 +1032,6 @@ static int isp_stat_init_entities(struct ispstat *stat, const char *name,
snprintf(subdev->name, V4L2_SUBDEV_NAME_SIZE, "OMAP3 ISP %s", name);
subdev->grp_id = 1 << 16; /* group ID for isp subdevs */
subdev->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
- subdev->nevents = STAT_NEVENTS;
v4l2_set_subdevdata(subdev, stat);
stat->pad.flags = MEDIA_PAD_FL_SINK;
@@ -1050,7 +1049,7 @@ int omap3isp_stat_subscribe_event(struct v4l2_subdev *subdev,
if (sub->type != stat->event_type)
return -EINVAL;
- return v4l2_event_subscribe(fh, sub);
+ return v4l2_event_subscribe(fh, sub, STAT_NEVENTS);
}
int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 079f952..63a44fd 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1011,7 +1011,6 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
insertion is an O(1) operation. */
if (list_empty(&hdl->ctrl_refs) || id > node2id(hdl->ctrl_refs.prev)) {
list_add_tail(&new_ref->node, &hdl->ctrl_refs);
- hdl->nr_of_refs++;
goto insert_in_hash;
}
@@ -2050,20 +2049,3 @@ void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
v4l2_ctrl_unlock(ctrl);
}
EXPORT_SYMBOL(v4l2_ctrl_del_event);
-
-int v4l2_ctrl_subscribe_fh(struct v4l2_fh *fh,
- struct v4l2_event_subscription *sub, unsigned n)
-{
- struct v4l2_ctrl_handler *hdl = fh->ctrl_handler;
- int ret = 0;
-
- if (!ret) {
- if (hdl->nr_of_refs * 2 > n)
- n = hdl->nr_of_refs * 2;
- ret = v4l2_event_alloc(fh, n);
- }
- if (!ret)
- ret = v4l2_event_subscribe(fh, sub);
- return ret;
-}
-EXPORT_SYMBOL(v4l2_ctrl_subscribe_fh);
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index dc68f60..9e325dd 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -30,44 +30,11 @@
#include <linux/sched.h>
#include <linux/slab.h>
-static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
-
-int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
-{
- unsigned long flags;
-
- while (fh->nallocated < n) {
- struct v4l2_kevent *kev;
-
- kev = kzalloc(sizeof(*kev), GFP_KERNEL);
- if (kev == NULL)
- return -ENOMEM;
-
- spin_lock_irqsave(&fh->vdev->fh_lock, flags);
- list_add_tail(&kev->list, &fh->free);
- fh->nallocated++;
- spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
- }
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(v4l2_event_alloc);
-
-#define list_kfree(list, type, member) \
- while (!list_empty(list)) { \
- type *hi; \
- hi = list_first_entry(list, type, member); \
- list_del(&hi->member); \
- kfree(hi); \
- }
-
-void v4l2_event_free(struct v4l2_fh *fh)
+static unsigned sev_pos(const struct v4l2_subscribed_event *sev, unsigned idx)
{
- list_kfree(&fh->free, struct v4l2_kevent, list);
- list_kfree(&fh->available, struct v4l2_kevent, list);
- v4l2_event_unsubscribe_all(fh);
+ idx += sev->first;
+ return idx >= sev->elems ? idx - sev->elems : idx;
}
-EXPORT_SYMBOL_GPL(v4l2_event_free);
static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
{
@@ -84,11 +51,13 @@ static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
WARN_ON(fh->navailable == 0);
kev = list_first_entry(&fh->available, struct v4l2_kevent, list);
- list_move(&kev->list, &fh->free);
+ list_del(&kev->list);
fh->navailable--;
kev->event.pending = fh->navailable;
*event = kev->event;
+ kev->sev->first = sev_pos(kev->sev, 1);
+ kev->sev->in_use--;
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
@@ -154,17 +123,24 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
fh->sequence++;
/* Do we have any free events? */
- if (list_empty(&fh->free))
- return;
+ if (sev->in_use == sev->elems) {
+ /* no, remove the oldest one */
+ kev = sev->events + sev_pos(sev, 0);
+ list_del(&kev->list);
+ sev->in_use--;
+ sev->first = sev_pos(sev, 1);
+ fh->navailable--;
+ }
/* Take one and fill it. */
- kev = list_first_entry(&fh->free, struct v4l2_kevent, list);
+ kev = sev->events + sev_pos(sev, sev->in_use);
kev->event.type = ev->type;
kev->event.u = ev->u;
kev->event.id = ev->id;
kev->event.timestamp = *ts;
kev->event.sequence = fh->sequence;
- list_move_tail(&kev->list, &fh->available);
+ sev->in_use++;
+ list_add_tail(&kev->list, &fh->available);
fh->navailable++;
@@ -209,38 +185,39 @@ int v4l2_event_pending(struct v4l2_fh *fh)
EXPORT_SYMBOL_GPL(v4l2_event_pending);
int v4l2_event_subscribe(struct v4l2_fh *fh,
- struct v4l2_event_subscription *sub)
+ struct v4l2_event_subscription *sub, unsigned elems)
{
struct v4l2_subscribed_event *sev, *found_ev;
struct v4l2_ctrl *ctrl = NULL;
unsigned long flags;
+ unsigned i;
+ if (elems < 1)
+ elems = 1;
if (sub->type == V4L2_EVENT_CTRL) {
ctrl = v4l2_ctrl_find(fh->ctrl_handler, sub->id);
if (ctrl == NULL)
return -EINVAL;
}
- sev = kzalloc(sizeof(*sev), GFP_KERNEL);
+ sev = kzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems, GFP_KERNEL);
if (!sev)
return -ENOMEM;
+ for (i = 0; i < elems; i++)
+ sev->events[i].sev = sev;
+ sev->type = sub->type;
+ sev->id = sub->id;
+ sev->flags = sub->flags;
+ sev->fh = fh;
+ sev->elems = elems;
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
-
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (!found_ev) {
- INIT_LIST_HEAD(&sev->list);
- sev->type = sub->type;
- sev->id = sub->id;
- sev->fh = fh;
- sev->flags = sub->flags;
-
+ if (!found_ev)
list_add(&sev->list, &fh->subscribed);
- }
-
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
- /* v4l2_ctrl_add_fh uses a mutex, so do this outside the spin lock */
+ /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
if (found_ev)
kfree(sev);
else if (ctrl)
@@ -250,7 +227,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
}
EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
-static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
+void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
{
struct v4l2_event_subscription sub;
struct v4l2_subscribed_event *sev;
@@ -271,6 +248,7 @@ static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
v4l2_event_unsubscribe(fh, &sub);
} while (sev);
}
+EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe_all);
int v4l2_event_unsubscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub)
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 333b8c8..122822d 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -37,9 +37,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
INIT_LIST_HEAD(&fh->list);
set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
fh->prio = V4L2_PRIORITY_UNSET;
-
init_waitqueue_head(&fh->wait);
- INIT_LIST_HEAD(&fh->free);
INIT_LIST_HEAD(&fh->available);
INIT_LIST_HEAD(&fh->subscribed);
fh->sequence = -1;
@@ -88,7 +86,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
{
if (fh->vdev == NULL)
return;
- v4l2_event_free(fh);
+ v4l2_event_unsubscribe_all(fh);
fh->vdev = NULL;
}
EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index 3b67a85..b7967c9 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -76,13 +76,6 @@ static int subdev_open(struct file *file)
}
v4l2_fh_init(&subdev_fh->vfh, vdev);
-
- if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) {
- ret = v4l2_event_alloc(&subdev_fh->vfh, sd->nevents);
- if (ret)
- goto err;
- }
-
v4l2_fh_add(&subdev_fh->vfh);
file->private_data = &subdev_fh->vfh;
#if defined(CONFIG_MEDIA_CONTROLLER)
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 99dbaea..e52063f 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -998,7 +998,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
{
switch (sub->type) {
case V4L2_EVENT_CTRL:
- return v4l2_ctrl_subscribe_fh(fh, sub, 0);
+ return v4l2_event_subscribe(fh, sub, 0);
default:
return -EINVAL;
}
diff --git a/drivers/usb/gadget/uvc_v4l2.c b/drivers/usb/gadget/uvc_v4l2.c
index 5582870..52f8f9e 100644
--- a/drivers/usb/gadget/uvc_v4l2.c
+++ b/drivers/usb/gadget/uvc_v4l2.c
@@ -124,18 +124,12 @@ uvc_v4l2_open(struct file *file)
struct video_device *vdev = video_devdata(file);
struct uvc_device *uvc = video_get_drvdata(vdev);
struct uvc_file_handle *handle;
- int ret;
handle = kzalloc(sizeof(*handle), GFP_KERNEL);
if (handle == NULL)
return -ENOMEM;
v4l2_fh_init(&handle->vfh, vdev);
-
- ret = v4l2_event_alloc(&handle->vfh, 8);
- if (ret < 0)
- goto error;
-
v4l2_fh_add(&handle->vfh);
handle->device = &uvc->video;
@@ -143,10 +137,6 @@ uvc_v4l2_open(struct file *file)
uvc_function_connect(uvc);
return 0;
-
-error:
- v4l2_fh_exit(&handle->vfh);
- return ret;
}
static int
@@ -308,7 +298,7 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg)
if (sub->type < UVC_EVENT_FIRST || sub->type > UVC_EVENT_LAST)
return -EINVAL;
- return v4l2_event_subscribe(&handle->vfh, arg);
+ return v4l2_event_subscribe(&handle->vfh, arg, 2);
}
case VIDIOC_UNSUBSCRIBE_EVENT:
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 635adc2..d8123d9 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -171,7 +171,6 @@ struct v4l2_ctrl_ref {
* control is needed multiple times, so this is a simple
* optimization.
* @buckets: Buckets for the hashing. Allows for quick control lookup.
- * @nr_of_refs: Total number of control references in the list.
* @nr_of_buckets: Total number of buckets in the array.
* @error: The error code of the first failed control addition.
*/
@@ -181,7 +180,6 @@ struct v4l2_ctrl_handler {
struct list_head ctrl_refs;
struct v4l2_ctrl_ref *cached;
struct v4l2_ctrl_ref **buckets;
- u16 nr_of_refs;
u16 nr_of_buckets;
int error;
};
@@ -499,23 +497,6 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
struct v4l2_subscribed_event *sev);
-/** v4l2_ctrl_subscribe_fh() - Helper function that subscribes a control event.
- * @fh: The file handler that subscribed the control event.
- * @sub: The event to subscribe (type must be V4L2_EVENT_CTRL).
- * @n: How many events should be allocated? (Passed to v4l2_event_alloc).
- * Recommended to set to twice the number of controls plus whatever
- * is needed for other events. This function will set n to
- * max(n, 2 * fh->ctrl_handler->nr_of_refs).
- *
- * A helper function that initializes the fh for events, allocates the
- * list of events and subscribes the control event.
- *
- * Typically called in the handler of VIDIOC_SUBSCRIBE_EVENT in the
- * V4L2_EVENT_CTRL case.
- */
-int v4l2_ctrl_subscribe_fh(struct v4l2_fh *fh,
- struct v4l2_event_subscription *sub, unsigned n);
-
/* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index eda17f8..8d681e5 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -30,10 +30,15 @@
#include <linux/wait.h>
struct v4l2_fh;
+struct v4l2_subscribed_event;
struct video_device;
struct v4l2_kevent {
+ /* list node for the v4l2_fh->available list */
struct list_head list;
+ /* pointer to parent v4l2_subscribed_event */
+ struct v4l2_subscribed_event *sev;
+ /* event itself */
struct v4l2_event event;
};
@@ -50,18 +55,25 @@ struct v4l2_subscribed_event {
struct v4l2_fh *fh;
/* list node that hooks into the object's event list (if there is one) */
struct list_head node;
+ /* the number of elements in the events array */
+ unsigned elems;
+ /* the index of the events containing the oldest available event */
+ unsigned first;
+ /* the number of queued events */
+ unsigned in_use;
+ /* an array of elems events */
+ struct v4l2_kevent events[];
};
-int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
-void v4l2_event_free(struct v4l2_fh *fh);
int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
int nonblocking);
void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev);
void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev);
int v4l2_event_pending(struct v4l2_fh *fh);
int v4l2_event_subscribe(struct v4l2_fh *fh,
- struct v4l2_event_subscription *sub);
+ struct v4l2_event_subscription *sub, unsigned elems);
int v4l2_event_unsubscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub);
+void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
#endif /* V4L2_EVENT_H */
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index bfc0457..52513c2 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -40,10 +40,8 @@ struct v4l2_fh {
/* Events */
wait_queue_head_t wait;
struct list_head subscribed; /* Subscribed events */
- struct list_head free; /* Events ready for use */
struct list_head available; /* Dequeueable event */
unsigned int navailable;
- unsigned int nallocated; /* Number of allocated events */
u32 sequence;
};
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1562c4f..e249f78 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -509,8 +509,6 @@ struct v4l2_subdev {
void *host_priv;
/* subdev device node */
struct video_device devnode;
- /* number of events to be allocated on open */
- unsigned int nevents;
};
#define media_entity_to_v4l2_subdev(ent) \
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [RFCv1 PATCH 4/8] v4l2-event: add optional 'merge' callback to merge two events
2011-06-14 15:22 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Hans Verkuil
2011-06-14 15:22 ` [RFCv1 PATCH 2/8] v4l2-ctrls/event: remove struct v4l2_ctrl_fh, instead use v4l2_subscribed_event Hans Verkuil
2011-06-14 15:22 ` [RFCv1 PATCH 3/8] v4l2-event/ctrls/fh: allocate events per fh and per type instead of just per-fh Hans Verkuil
@ 2011-06-14 15:22 ` Hans Verkuil
2011-06-20 14:09 ` Laurent Pinchart
2011-06-14 15:22 ` [RFCv1 PATCH 5/8] v4l2-ctrls: don't initially set CH_VALUE for write-only controls Hans Verkuil
` (4 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2011-06-14 15:22 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, sakari.ailus, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
When the event queue for a subscribed event is full, then the oldest
event is dropped. It would be nice if the contents of that oldest
event could be merged with the next-oldest. That way no information is
lost, only intermediate steps are lost.
This patch adds an optional merge function that will be called to do
this job and implements it for the control event.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-event.c | 27 ++++++++++++++++++++++++++-
include/media/v4l2-event.h | 5 +++++
2 files changed, 31 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9e325dd..aeec2d5 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -113,6 +113,7 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
{
struct v4l2_subscribed_event *sev;
struct v4l2_kevent *kev;
+ bool copy_payload = true;
/* Are we subscribed? */
sev = v4l2_event_subscribed(fh, ev->type, ev->id);
@@ -130,12 +131,23 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
sev->in_use--;
sev->first = sev_pos(sev, 1);
fh->navailable--;
+ if (sev->merge) {
+ if (sev->elems == 1) {
+ sev->merge(&kev->event, ev, &kev->event);
+ copy_payload = false;
+ } else {
+ struct v4l2_kevent *second_oldest =
+ sev->events + sev_pos(sev, 0);
+ sev->merge(&second_oldest->event, &second_oldest->event, &kev->event);
+ }
+ }
}
/* Take one and fill it. */
kev = sev->events + sev_pos(sev, sev->in_use);
kev->event.type = ev->type;
- kev->event.u = ev->u;
+ if (copy_payload)
+ kev->event.u = ev->u;
kev->event.id = ev->id;
kev->event.timestamp = *ts;
kev->event.sequence = fh->sequence;
@@ -184,6 +196,17 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void ctrls_merge(struct v4l2_event *dst,
+ const struct v4l2_event *new,
+ const struct v4l2_event *old)
+{
+ u32 changes = new->u.ctrl.changes | old->u.ctrl.changes;
+
+ if (dst == old)
+ dst->u.ctrl = new->u.ctrl;
+ dst->u.ctrl.changes = changes;
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub, unsigned elems)
{
@@ -210,6 +233,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
sev->flags = sub->flags;
sev->fh = fh;
sev->elems = elems;
+ if (ctrl)
+ sev->merge = ctrls_merge;
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 8d681e5..111b2bc 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -55,6 +55,11 @@ struct v4l2_subscribed_event {
struct v4l2_fh *fh;
/* list node that hooks into the object's event list (if there is one) */
struct list_head node;
+ /* Optional callback that can merge two events.
+ Note that 'dst' can be the same as either 'new' or 'old'. */
+ void (*merge)(struct v4l2_event *dst,
+ const struct v4l2_event *new,
+ const struct v4l2_event *old);
/* the number of elements in the events array */
unsigned elems;
/* the index of the events containing the oldest available event */
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [RFCv1 PATCH 4/8] v4l2-event: add optional 'merge' callback to merge two events
2011-06-14 15:22 ` [RFCv1 PATCH 4/8] v4l2-event: add optional 'merge' callback to merge two events Hans Verkuil
@ 2011-06-20 14:09 ` Laurent Pinchart
2011-06-20 14:11 ` Hans Verkuil
0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2011-06-20 14:09 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, sakari.ailus, Hans Verkuil
Hi Hans,
Thanks for the patch.
On Tuesday 14 June 2011 17:22:29 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> When the event queue for a subscribed event is full, then the oldest
> event is dropped. It would be nice if the contents of that oldest
> event could be merged with the next-oldest. That way no information is
> lost, only intermediate steps are lost.
>
> This patch adds an optional merge function that will be called to do
> this job and implements it for the control event.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/video/v4l2-event.c | 27 ++++++++++++++++++++++++++-
> include/media/v4l2-event.h | 5 +++++
> 2 files changed, 31 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 9e325dd..aeec2d5 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -113,6 +113,7 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e {
> struct v4l2_subscribed_event *sev;
> struct v4l2_kevent *kev;
> + bool copy_payload = true;
>
> /* Are we subscribed? */
> sev = v4l2_event_subscribed(fh, ev->type, ev->id);
> @@ -130,12 +131,23 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e sev->in_use--;
> sev->first = sev_pos(sev, 1);
> fh->navailable--;
> + if (sev->merge) {
> + if (sev->elems == 1) {
> + sev->merge(&kev->event, ev, &kev->event);
> + copy_payload = false;
> + } else {
> + struct v4l2_kevent *second_oldest =
> + sev->events + sev_pos(sev, 0);
> + sev->merge(&second_oldest->event, &second_oldest->event, &kev-
>event);
> + }
> + }
> }
>
> /* Take one and fill it. */
> kev = sev->events + sev_pos(sev, sev->in_use);
> kev->event.type = ev->type;
> - kev->event.u = ev->u;
> + if (copy_payload)
> + kev->event.u = ev->u;
> kev->event.id = ev->id;
> kev->event.timestamp = *ts;
> kev->event.sequence = fh->sequence;
> @@ -184,6 +196,17 @@ int v4l2_event_pending(struct v4l2_fh *fh)
> }
> EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> +static void ctrls_merge(struct v4l2_event *dst,
> + const struct v4l2_event *new,
> + const struct v4l2_event *old)
> +{
> + u32 changes = new->u.ctrl.changes | old->u.ctrl.changes;
> +
> + if (dst == old)
> + dst->u.ctrl = new->u.ctrl;
> + dst->u.ctrl.changes = changes;
> +}
> +
> int v4l2_event_subscribe(struct v4l2_fh *fh,
> struct v4l2_event_subscription *sub, unsigned elems)
> {
> @@ -210,6 +233,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> sev->flags = sub->flags;
> sev->fh = fh;
> sev->elems = elems;
> + if (ctrl)
> + sev->merge = ctrls_merge;
>
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 8d681e5..111b2bc 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -55,6 +55,11 @@ struct v4l2_subscribed_event {
> struct v4l2_fh *fh;
> /* list node that hooks into the object's event list (if there is one) */
> struct list_head node;
> + /* Optional callback that can merge two events.
> + Note that 'dst' can be the same as either 'new' or 'old'. */
This can lead to various problems in drivers, if the code forgets that
changing dst will change new or old. Would it be possible to make it a two
arguments (dst, src) function ?
> + void (*merge)(struct v4l2_event *dst,
> + const struct v4l2_event *new,
> + const struct v4l2_event *old);
> /* the number of elements in the events array */
> unsigned elems;
> /* the index of the events containing the oldest available event */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFCv1 PATCH 4/8] v4l2-event: add optional 'merge' callback to merge two events
2011-06-20 14:09 ` Laurent Pinchart
@ 2011-06-20 14:11 ` Hans Verkuil
0 siblings, 0 replies; 17+ messages in thread
From: Hans Verkuil @ 2011-06-20 14:11 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, sakari.ailus, Hans Verkuil
On Monday, June 20, 2011 16:09:32 Laurent Pinchart wrote:
> Hi Hans,
>
> Thanks for the patch.
>
> On Tuesday 14 June 2011 17:22:29 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > When the event queue for a subscribed event is full, then the oldest
> > event is dropped. It would be nice if the contents of that oldest
> > event could be merged with the next-oldest. That way no information is
> > lost, only intermediate steps are lost.
> >
> > This patch adds an optional merge function that will be called to do
> > this job and implements it for the control event.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> > drivers/media/video/v4l2-event.c | 27 ++++++++++++++++++++++++++-
> > include/media/v4l2-event.h | 5 +++++
> > 2 files changed, 31 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/media/video/v4l2-event.c
> > b/drivers/media/video/v4l2-event.c index 9e325dd..aeec2d5 100644
> > --- a/drivers/media/video/v4l2-event.c
> > +++ b/drivers/media/video/v4l2-event.c
> > @@ -113,6 +113,7 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> > const struct v4l2_event *e {
> > struct v4l2_subscribed_event *sev;
> > struct v4l2_kevent *kev;
> > + bool copy_payload = true;
> >
> > /* Are we subscribed? */
> > sev = v4l2_event_subscribed(fh, ev->type, ev->id);
> > @@ -130,12 +131,23 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> > const struct v4l2_event *e sev->in_use--;
> > sev->first = sev_pos(sev, 1);
> > fh->navailable--;
> > + if (sev->merge) {
> > + if (sev->elems == 1) {
> > + sev->merge(&kev->event, ev, &kev->event);
> > + copy_payload = false;
> > + } else {
> > + struct v4l2_kevent *second_oldest =
> > + sev->events + sev_pos(sev, 0);
> > + sev->merge(&second_oldest->event, &second_oldest->event, &kev-
> >event);
> > + }
> > + }
> > }
> >
> > /* Take one and fill it. */
> > kev = sev->events + sev_pos(sev, sev->in_use);
> > kev->event.type = ev->type;
> > - kev->event.u = ev->u;
> > + if (copy_payload)
> > + kev->event.u = ev->u;
> > kev->event.id = ev->id;
> > kev->event.timestamp = *ts;
> > kev->event.sequence = fh->sequence;
> > @@ -184,6 +196,17 @@ int v4l2_event_pending(struct v4l2_fh *fh)
> > }
> > EXPORT_SYMBOL_GPL(v4l2_event_pending);
> >
> > +static void ctrls_merge(struct v4l2_event *dst,
> > + const struct v4l2_event *new,
> > + const struct v4l2_event *old)
> > +{
> > + u32 changes = new->u.ctrl.changes | old->u.ctrl.changes;
> > +
> > + if (dst == old)
> > + dst->u.ctrl = new->u.ctrl;
> > + dst->u.ctrl.changes = changes;
> > +}
> > +
> > int v4l2_event_subscribe(struct v4l2_fh *fh,
> > struct v4l2_event_subscription *sub, unsigned elems)
> > {
> > @@ -210,6 +233,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> > sev->flags = sub->flags;
> > sev->fh = fh;
> > sev->elems = elems;
> > + if (ctrl)
> > + sev->merge = ctrls_merge;
> >
> > spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> > found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> > diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> > index 8d681e5..111b2bc 100644
> > --- a/include/media/v4l2-event.h
> > +++ b/include/media/v4l2-event.h
> > @@ -55,6 +55,11 @@ struct v4l2_subscribed_event {
> > struct v4l2_fh *fh;
> > /* list node that hooks into the object's event list (if there is one) */
> > struct list_head node;
> > + /* Optional callback that can merge two events.
> > + Note that 'dst' can be the same as either 'new' or 'old'. */
>
> This can lead to various problems in drivers, if the code forgets that
> changing dst will change new or old. Would it be possible to make it a two
> arguments (dst, src) function ?
This has been split into a replace and a merge function in the final patch in
my pull request. It's the only change I made (besides documentation) compared
to the RFCv1. This merge function was a bit too obscure and splitting it up
into two callbacks worked much better.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* [RFCv1 PATCH 5/8] v4l2-ctrls: don't initially set CH_VALUE for write-only controls
2011-06-14 15:22 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Hans Verkuil
` (2 preceding siblings ...)
2011-06-14 15:22 ` [RFCv1 PATCH 4/8] v4l2-event: add optional 'merge' callback to merge two events Hans Verkuil
@ 2011-06-14 15:22 ` Hans Verkuil
2011-06-14 15:22 ` [RFCv1 PATCH 6/8] v4l2-ctrls: improve discovery of controls of the same cluster Hans Verkuil
` (3 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Hans Verkuil @ 2011-06-14 15:22 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, sakari.ailus, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
When sending the SEND_INITIAL event for write-only controls the
V4L2_EVENT_CTRL_CH_VALUE flag should not be set. It's meaningless.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 63a44fd..1b0422e 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -2032,9 +2032,11 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
(sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
struct v4l2_event ev;
+ u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
- fill_event(&ev, ctrl, V4L2_EVENT_CTRL_CH_VALUE |
- V4L2_EVENT_CTRL_CH_FLAGS);
+ if (!(ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY))
+ changes |= V4L2_EVENT_CTRL_CH_VALUE;
+ fill_event(&ev, ctrl, changes);
v4l2_event_queue_fh(sev->fh, &ev);
}
v4l2_ctrl_unlock(ctrl);
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [RFCv1 PATCH 6/8] v4l2-ctrls: improve discovery of controls of the same cluster
2011-06-14 15:22 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Hans Verkuil
` (3 preceding siblings ...)
2011-06-14 15:22 ` [RFCv1 PATCH 5/8] v4l2-ctrls: don't initially set CH_VALUE for write-only controls Hans Verkuil
@ 2011-06-14 15:22 ` Hans Verkuil
2011-06-14 15:22 ` [RFCv1 PATCH 7/8] v4l2-ctrls: split try_or_set_ext_ctrls() Hans Verkuil
` (2 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Hans Verkuil @ 2011-06-14 15:22 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, sakari.ailus, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The implementation of VIDIOC_G/S/TRY_EXT_CTRLS in the control framework has
to figure out which controls in the control list belong to the same cluster.
Since controls belonging to the same cluster need to be handled as a unit,
this is important information.
It did that by going over the controls in the list and for each control that
belonged to a multi-control cluster it would walk the remainder of the list
to try and find controls that belong to that same cluster.
This approach has two disadvantages:
1) it was a potentially quadratic algorithm (although highly unlikely that
it would ever be that bad in practice).
2) it took place with the control handler's lock held.
Since we want to make it possible in the future to change control values
from interrupt context, doing a lot of work while holding a lock is not a
good idea.
In the new code the algorithm is no longer quadratic but linear in the
number of controls in the list. Also, it now can be done beforehand.
Another change that was made was to so the try and set at the same time.
Before when S_TRY_EXT_CTRLS was called it would 'try' the controls first,
and then it would 'set' them. The idea was that any 'try' errors would
prevent the 'set' from happening, thus avoiding having partially set
control lists.
However, this caused more problems than it solved because between the 'try'
and the 'set' changes might have happened, so it had to try a second time,
and since actual controls with a try_ctrl op are very rare (and those that
we have just adjust values and do not return an error), I've decided to
drop that two-stage approach and just combine try and set.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 279 +++++++++++++++++++------------------
include/media/v4l2-ctrls.h | 3 +
2 files changed, 146 insertions(+), 136 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 1b0422e..8fe4e47 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -32,12 +32,14 @@
(has_op(master, op) ? master->ops->op(master) : 0)
/* Internal temporary helper struct, one for each v4l2_ext_control */
-struct ctrl_helper {
+struct v4l2_ctrl_helper {
+ /* Pointer to the control reference of the master control */
+ struct v4l2_ctrl_ref *mref;
/* The control corresponding to the v4l2_ext_control ID field. */
struct v4l2_ctrl *ctrl;
- /* Used internally to mark whether this control was already
- processed. */
- bool handled;
+ /* v4l2_ext_control index of the next control belonging to the
+ same cluster, or 0 if there isn't any. */
+ u32 next;
};
/* Small helper function to determine if the autocluster is set to manual
@@ -678,20 +680,6 @@ static int new_to_user(struct v4l2_ext_control *c,
return 0;
}
-static int ctrl_to_user(struct v4l2_ext_control *c,
- struct v4l2_ctrl *ctrl)
-{
- if (ctrl->is_volatile)
- return new_to_user(c, ctrl);
- return cur_to_user(c, ctrl);
-}
-
-static int ctrl_is_volatile(struct v4l2_ext_control *c,
- struct v4l2_ctrl *ctrl)
-{
- return ctrl->is_volatile;
-}
-
/* Copy the new value to the current value. */
static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
bool update_inactive)
@@ -779,13 +767,11 @@ static int cluster_changed(struct v4l2_ctrl *master)
return diff;
}
-/* Validate a new control */
-static int validate_new(struct v4l2_ctrl *ctrl)
+/* Validate integer-type control */
+static int validate_new_int(const struct v4l2_ctrl *ctrl, s32 *pval)
{
- s32 val = ctrl->val;
- char *s = ctrl->string;
+ s32 val = *pval;
u32 offset;
- size_t len;
switch (ctrl->type) {
case V4L2_CTRL_TYPE_INTEGER:
@@ -798,11 +784,11 @@ static int validate_new(struct v4l2_ctrl *ctrl)
offset = val - ctrl->minimum;
offset = ctrl->step * (offset / ctrl->step);
val = ctrl->minimum + offset;
- ctrl->val = val;
+ *pval = val;
return 0;
case V4L2_CTRL_TYPE_BOOLEAN:
- ctrl->val = !!ctrl->val;
+ *pval = !!val;
return 0;
case V4L2_CTRL_TYPE_MENU:
@@ -815,9 +801,28 @@ static int validate_new(struct v4l2_ctrl *ctrl)
case V4L2_CTRL_TYPE_BUTTON:
case V4L2_CTRL_TYPE_CTRL_CLASS:
- ctrl->val64 = 0;
+ *pval = 0;
return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+/* Validate a new control */
+static int validate_new(const struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
+{
+ char *s = c->string;
+ size_t len;
+
+ switch (ctrl->type) {
+ case V4L2_CTRL_TYPE_INTEGER:
+ case V4L2_CTRL_TYPE_BOOLEAN:
+ case V4L2_CTRL_TYPE_MENU:
+ case V4L2_CTRL_TYPE_BUTTON:
+ case V4L2_CTRL_TYPE_CTRL_CLASS:
+ return validate_new_int(ctrl, &c->value);
+
case V4L2_CTRL_TYPE_INTEGER64:
return 0;
@@ -1575,12 +1580,15 @@ EXPORT_SYMBOL(v4l2_subdev_querymenu);
Find the controls in the control array and do some basic checks. */
static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs,
- struct ctrl_helper *helpers)
+ struct v4l2_ctrl_helper *helpers)
{
+ struct v4l2_ctrl_helper *h;
+ bool have_clusters = false;
u32 i;
- for (i = 0; i < cs->count; i++) {
+ for (i = 0, h = helpers; i < cs->count; i++, h++) {
struct v4l2_ext_control *c = &cs->controls[i];
+ struct v4l2_ctrl_ref *ref;
struct v4l2_ctrl *ctrl;
u32 id = c->id & V4L2_CTRL_ID_MASK;
@@ -1593,53 +1601,59 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
extended controls */
if (id >= V4L2_CID_PRIVATE_BASE)
return -EINVAL;
- ctrl = v4l2_ctrl_find(hdl, id);
- if (ctrl == NULL)
+ ref = find_ref_lock(hdl, id);
+ if (ref == NULL)
return -EINVAL;
+ ctrl = ref->ctrl;
if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
return -EINVAL;
- helpers[i].ctrl = ctrl;
- helpers[i].handled = false;
+ if (ctrl->cluster[0]->ncontrols > 1)
+ have_clusters = true;
+ if (ctrl->cluster[0] != ctrl)
+ ref = find_ref_lock(hdl, ctrl->cluster[0]->id);
+ /* Store the ref to the master control of the cluster */
+ h->mref = ref;
+ h->ctrl = ctrl;
+ /* Initially set next to 0, meaning that there is no other
+ control in this helper array belonging to the same
+ cluster */
+ h->next = 0;
}
- return 0;
-}
-typedef int (*cluster_func)(struct v4l2_ext_control *c,
- struct v4l2_ctrl *ctrl);
+ /* We are done if there were no controls that belong to a multi-
+ control cluster. */
+ if (!have_clusters)
+ return 0;
-/* Walk over all controls in v4l2_ext_controls belonging to the same cluster
- and call the provided function. */
-static int cluster_walk(unsigned from,
- struct v4l2_ext_controls *cs,
- struct ctrl_helper *helpers,
- cluster_func f)
-{
- struct v4l2_ctrl **cluster = helpers[from].ctrl->cluster;
- int ret = 0;
- int i;
+ /* The code below figures out in O(n) time which controls in the list
+ belong to the same cluster. */
- /* Find any controls from the same cluster and call the function */
- for (i = from; !ret && i < cs->count; i++) {
- struct v4l2_ctrl *ctrl = helpers[i].ctrl;
+ /* This has to be done with the handler lock taken. */
+ mutex_lock(&hdl->lock);
- if (!helpers[i].handled && ctrl->cluster == cluster)
- ret = f(&cs->controls[i], ctrl);
+ /* First zero the helper field in the master control references */
+ for (i = 0; i < cs->count; i++)
+ helpers[i].mref->helper = 0;
+ for (i = 0, h = helpers; i < cs->count; i++, h++) {
+ struct v4l2_ctrl_ref *mref = h->mref;
+
+ /* If the mref->helper is set, then it points to an earlier
+ helper that belongs to the same cluster. */
+ if (mref->helper) {
+ /* Set the next field of mref->helper to the current
+ index: this means that that earlier helper now
+ points to the next helper in the same cluster. */
+ mref->helper->next = i;
+ /* mref should be set only for the first helper in the
+ cluster, clear the others. */
+ h->mref = NULL;
+ }
+ /* Point the mref helper to the current helper struct. */
+ mref->helper = h;
}
- return ret;
-}
-
-static void cluster_done(unsigned from,
- struct v4l2_ext_controls *cs,
- struct ctrl_helper *helpers)
-{
- struct v4l2_ctrl **cluster = helpers[from].ctrl->cluster;
- int i;
-
- /* Find any controls from the same cluster and mark them as handled */
- for (i = from; i < cs->count; i++)
- if (helpers[i].ctrl->cluster == cluster)
- helpers[i].handled = true;
+ mutex_unlock(&hdl->lock);
+ return 0;
}
/* Handles the corner case where cs->count == 0. It checks whether the
@@ -1657,8 +1671,8 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
/* Get extended controls. Allocates the helpers array if needed. */
int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
{
- struct ctrl_helper helper[4];
- struct ctrl_helper *helpers = helper;
+ struct v4l2_ctrl_helper helper[4];
+ struct v4l2_ctrl_helper *helpers = helper;
int ret;
int i, j;
@@ -1685,37 +1699,38 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
ret = -EACCES;
for (i = 0; !ret && i < cs->count; i++) {
- struct v4l2_ctrl *ctrl = helpers[i].ctrl;
- struct v4l2_ctrl *master = ctrl->cluster[0];
- bool has_volatiles;
+ int (*ctrl_to_user)(struct v4l2_ext_control *c,
+ struct v4l2_ctrl *ctrl) = cur_to_user;
+ struct v4l2_ctrl *master;
- if (helpers[i].handled)
+ if (helpers[i].mref == NULL)
continue;
+ master = helpers[i].mref->ctrl;
cs->error_idx = i;
- /* Any volatile controls requested from this cluster? */
- has_volatiles = ctrl->is_volatile;
- if (!has_volatiles && has_op(master, g_volatile_ctrl) &&
- master->ncontrols > 1)
- has_volatiles = cluster_walk(i, cs, helpers,
- ctrl_is_volatile);
-
v4l2_ctrl_lock(master);
/* g_volatile_ctrl will update the new control values */
- if (has_volatiles && !is_cur_manual(master)) {
+ if (has_op(master, g_volatile_ctrl) && !is_cur_manual(master)) {
for (j = 0; j < master->ncontrols; j++)
cur_to_new(master->cluster[j]);
ret = call_op(master, g_volatile_ctrl);
+ ctrl_to_user = new_to_user;
}
/* If OK, then copy the current (for non-volatile controls)
or the new (for volatile controls) control values to the
caller */
- if (!ret)
- ret = cluster_walk(i, cs, helpers, ctrl_to_user);
+ if (!ret) {
+ u32 idx = i;
+
+ do {
+ ret = ctrl_to_user(cs->controls + idx,
+ helpers[idx].ctrl);
+ idx = helpers[idx].next;
+ } while (!ret && idx);
+ }
v4l2_ctrl_unlock(master);
- cluster_done(i, cs, helpers);
}
if (cs->count > ARRAY_SIZE(helper))
@@ -1789,52 +1804,39 @@ static int try_or_set_control_cluster(struct v4l2_fh *fh,
struct v4l2_ctrl *master, bool set)
{
bool update_flag;
- bool try = !set;
- int ret = 0;
+ int ret;
int i;
/* Go through the cluster and either validate the new value or
(if no new value was set), copy the current value to the new
value, ensuring a consistent view for the control ops when
called. */
- for (i = 0; !ret && i < master->ncontrols; i++) {
+ for (i = 0; i < master->ncontrols; i++) {
struct v4l2_ctrl *ctrl = master->cluster[i];
if (ctrl == NULL)
continue;
- if (ctrl->is_new) {
- /* Double check this: it may have changed since the
- last check in try_or_set_ext_ctrls(). */
- if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
- return -EBUSY;
-
- /* Validate if required */
- if (!set)
- ret = validate_new(ctrl);
+ if (!ctrl->is_new) {
+ cur_to_new(ctrl);
continue;
}
- /* No new value was set, so copy the current and force
- a call to try_ctrl later, since the values for the cluster
- may now have changed and the end result might be invalid. */
- try = true;
- cur_to_new(ctrl);
+ /* Check again: it may have changed since the
+ previous check in try_or_set_ext_ctrls(). */
+ if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
+ return -EBUSY;
}
- /* For larger clusters you have to call try_ctrl again to
- verify that the controls are still valid after the
- 'cur_to_new' above. */
- if (!ret && try)
- ret = call_op(master, try_ctrl);
+ ret = call_op(master, try_ctrl);
/* Don't set if there is no change */
if (ret || !set || !cluster_changed(master))
return ret;
ret = call_op(master, s_ctrl);
- /* If OK, then make the new values permanent. */
if (ret)
return ret;
+ /* If OK, then make the new values permanent. */
update_flag = is_cur_manual(master) != is_new_manual(master);
for (i = 0; i < master->ncontrols; i++)
new_to_cur(fh, master->cluster[i], update_flag && i > 0);
@@ -1845,16 +1847,19 @@ static int try_or_set_control_cluster(struct v4l2_fh *fh,
static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs,
- struct ctrl_helper *helpers,
+ struct v4l2_ctrl_helper *helpers,
bool set)
{
unsigned i, j;
int ret = 0;
+ /* Phase 1: validation */
+ cs->error_idx = cs->count;
for (i = 0; i < cs->count; i++) {
struct v4l2_ctrl *ctrl = helpers[i].ctrl;
- cs->error_idx = i;
+ if (!set)
+ cs->error_idx = i;
if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
return -EACCES;
@@ -1866,17 +1871,22 @@ static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
best-effort to avoid that. */
if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
return -EBUSY;
+ ret = validate_new(ctrl, &cs->controls[i]);
+ if (ret)
+ return ret;
}
+ /* Phase 2: set/try controls */
for (i = 0; !ret && i < cs->count; i++) {
- struct v4l2_ctrl *ctrl = helpers[i].ctrl;
- struct v4l2_ctrl *master = ctrl->cluster[0];
+ struct v4l2_ctrl *master;
+ u32 idx = i;
- if (helpers[i].handled)
+ if (helpers[i].mref == NULL)
continue;
cs->error_idx = i;
- v4l2_ctrl_lock(ctrl);
+ master = helpers[i].mref->ctrl;
+ v4l2_ctrl_lock(master);
/* Reset the 'is_new' flags of the cluster */
for (j = 0; j < master->ncontrols; j++)
@@ -1885,17 +1895,24 @@ static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
/* Copy the new caller-supplied control values.
user_to_new() sets 'is_new' to 1. */
- ret = cluster_walk(i, cs, helpers, user_to_new);
+ do {
+ ret = user_to_new(cs->controls + idx, helpers[idx].ctrl);
+ idx = helpers[idx].next;
+ } while (!ret && idx);
if (!ret)
ret = try_or_set_control_cluster(fh, master, set);
/* Copy the new values back to userspace. */
- if (!ret)
- ret = cluster_walk(i, cs, helpers, new_to_user);
-
- v4l2_ctrl_unlock(ctrl);
- cluster_done(i, cs, helpers);
+ if (!ret) {
+ idx = i;
+ do {
+ ret = user_to_new(cs->controls + idx,
+ helpers[idx].ctrl);
+ idx = helpers[idx].next;
+ } while (!ret && idx);
+ }
+ v4l2_ctrl_unlock(master);
}
return ret;
}
@@ -1905,10 +1922,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs,
bool set)
{
- struct ctrl_helper helper[4];
- struct ctrl_helper *helpers = helper;
+ struct v4l2_ctrl_helper helper[4];
+ struct v4l2_ctrl_helper *helpers = helper;
int ret;
- int i;
cs->error_idx = cs->count;
cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
@@ -1925,21 +1941,10 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
return -ENOMEM;
}
ret = prepare_ext_ctrls(hdl, cs, helpers);
-
- /* First 'try' all controls and abort on error */
if (!ret)
- ret = try_or_set_ext_ctrls(NULL, hdl, cs, helpers, false);
- /* If this is a 'set' operation and the initial 'try' failed,
- then set error_idx to count to tell the application that no
- controls changed value yet. */
- if (set)
+ ret = try_or_set_ext_ctrls(fh, hdl, cs, helpers, set);
+ else if (set)
cs->error_idx = cs->count;
- if (!ret && set) {
- /* Reset 'handled' state */
- for (i = 0; i < cs->count; i++)
- helpers[i].handled = false;
- ret = try_or_set_ext_ctrls(fh, hdl, cs, helpers, true);
- }
if (cs->count > ARRAY_SIZE(helper))
kfree(helpers);
@@ -1978,6 +1983,10 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
int ret;
int i;
+ ret = validate_new_int(ctrl, val);
+ if (ret)
+ return ret;
+
v4l2_ctrl_lock(ctrl);
/* Reset the 'is_new' flags of the cluster */
@@ -1987,9 +1996,7 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
ctrl->val = *val;
ctrl->is_new = 1;
- ret = try_or_set_control_cluster(NULL, master, false);
- if (!ret)
- ret = try_or_set_control_cluster(fh, master, true);
+ ret = try_or_set_control_cluster(fh, master, true);
*val = ctrl->cur.val;
v4l2_ctrl_unlock(ctrl);
return ret;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index d8123d9..5fc3a2d 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -27,6 +27,7 @@
/* forward references */
struct v4l2_ctrl_handler;
+struct v4l2_ctrl_helper;
struct v4l2_ctrl;
struct video_device;
struct v4l2_subdev;
@@ -150,6 +151,7 @@ struct v4l2_ctrl {
* @node: List node for the sorted list.
* @next: Single-link list node for the hash.
* @ctrl: The actual control information.
+ * @helper: Pointer to helper struct. Used internally in prepare_ext_ctrls().
*
* Each control handler has a list of these refs. The list_head is used to
* keep a sorted-by-control-ID list of all controls, while the next pointer
@@ -159,6 +161,7 @@ struct v4l2_ctrl_ref {
struct list_head node;
struct v4l2_ctrl_ref *next;
struct v4l2_ctrl *ctrl;
+ struct v4l2_ctrl_helper *helper;
};
/** struct v4l2_ctrl_handler - The control handler keeps track of all the
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [RFCv1 PATCH 7/8] v4l2-ctrls: split try_or_set_ext_ctrls()
2011-06-14 15:22 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Hans Verkuil
` (4 preceding siblings ...)
2011-06-14 15:22 ` [RFCv1 PATCH 6/8] v4l2-ctrls: improve discovery of controls of the same cluster Hans Verkuil
@ 2011-06-14 15:22 ` Hans Verkuil
2011-06-14 15:22 ` [RFCv1 PATCH 8/8] v4l2-ctrls: v4l2_ctrl_handler_setup code simplification Hans Verkuil
2011-06-15 9:30 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Sakari Ailus
7 siblings, 0 replies; 17+ messages in thread
From: Hans Verkuil @ 2011-06-14 15:22 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, sakari.ailus, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Split try_or_set_ext_ctrls() into a validate_ctrls() part ('Phase 1')
and merge the second part ('Phase 2') into try_set_ext_ctrls().
This makes a lot more sense and it also does the validation before
trying to try/set the controls.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 88 ++++++++++++++++++--------------------
1 files changed, 41 insertions(+), 47 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 8fe4e47..040d5c9 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1800,8 +1800,8 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
/* Core function that calls try/s_ctrl and ensures that the new value is
copied to the current value on a set.
Must be called with ctrl->handler->lock held. */
-static int try_or_set_control_cluster(struct v4l2_fh *fh,
- struct v4l2_ctrl *master, bool set)
+static int try_or_set_cluster(struct v4l2_fh *fh,
+ struct v4l2_ctrl *master, bool set)
{
bool update_flag;
int ret;
@@ -1843,23 +1843,18 @@ static int try_or_set_control_cluster(struct v4l2_fh *fh,
return 0;
}
-/* Try or set controls. */
-static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
- struct v4l2_ctrl_handler *hdl,
- struct v4l2_ext_controls *cs,
- struct v4l2_ctrl_helper *helpers,
- bool set)
+/* Validate controls. */
+static int validate_ctrls(struct v4l2_ext_controls *cs,
+ struct v4l2_ctrl_helper *helpers, bool set)
{
- unsigned i, j;
+ unsigned i;
int ret = 0;
- /* Phase 1: validation */
cs->error_idx = cs->count;
for (i = 0; i < cs->count; i++) {
struct v4l2_ctrl *ctrl = helpers[i].ctrl;
- if (!set)
- cs->error_idx = i;
+ cs->error_idx = i;
if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
return -EACCES;
@@ -1875,8 +1870,38 @@ static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
if (ret)
return ret;
}
+ return 0;
+}
+
+/* Try or try-and-set controls */
+static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+ struct v4l2_ext_controls *cs,
+ bool set)
+{
+ struct v4l2_ctrl_helper helper[4];
+ struct v4l2_ctrl_helper *helpers = helper;
+ unsigned i, j;
+ int ret;
+
+ cs->error_idx = cs->count;
+ cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
+
+ if (hdl == NULL)
+ return -EINVAL;
+
+ if (cs->count == 0)
+ return class_check(hdl, cs->ctrl_class);
- /* Phase 2: set/try controls */
+ if (cs->count > ARRAY_SIZE(helper)) {
+ helpers = kmalloc(sizeof(helper[0]) * cs->count, GFP_KERNEL);
+ if (!helpers)
+ return -ENOMEM;
+ }
+ ret = prepare_ext_ctrls(hdl, cs, helpers);
+ if (!ret)
+ ret = validate_ctrls(cs, helpers, set);
+ if (ret && set)
+ cs->error_idx = cs->count;
for (i = 0; !ret && i < cs->count; i++) {
struct v4l2_ctrl *master;
u32 idx = i;
@@ -1901,50 +1926,19 @@ static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
} while (!ret && idx);
if (!ret)
- ret = try_or_set_control_cluster(fh, master, set);
+ ret = try_or_set_cluster(fh, master, set);
/* Copy the new values back to userspace. */
if (!ret) {
idx = i;
do {
ret = user_to_new(cs->controls + idx,
- helpers[idx].ctrl);
+ helpers[idx].ctrl);
idx = helpers[idx].next;
} while (!ret && idx);
}
v4l2_ctrl_unlock(master);
}
- return ret;
-}
-
-/* Try or try-and-set controls */
-static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
- struct v4l2_ext_controls *cs,
- bool set)
-{
- struct v4l2_ctrl_helper helper[4];
- struct v4l2_ctrl_helper *helpers = helper;
- int ret;
-
- cs->error_idx = cs->count;
- cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
-
- if (hdl == NULL)
- return -EINVAL;
-
- if (cs->count == 0)
- return class_check(hdl, cs->ctrl_class);
-
- if (cs->count > ARRAY_SIZE(helper)) {
- helpers = kmalloc(sizeof(helper[0]) * cs->count, GFP_KERNEL);
- if (!helpers)
- return -ENOMEM;
- }
- ret = prepare_ext_ctrls(hdl, cs, helpers);
- if (!ret)
- ret = try_or_set_ext_ctrls(fh, hdl, cs, helpers, set);
- else if (set)
- cs->error_idx = cs->count;
if (cs->count > ARRAY_SIZE(helper))
kfree(helpers);
@@ -1996,7 +1990,7 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
ctrl->val = *val;
ctrl->is_new = 1;
- ret = try_or_set_control_cluster(fh, master, true);
+ ret = try_or_set_cluster(fh, master, true);
*val = ctrl->cur.val;
v4l2_ctrl_unlock(ctrl);
return ret;
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [RFCv1 PATCH 8/8] v4l2-ctrls: v4l2_ctrl_handler_setup code simplification
2011-06-14 15:22 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Hans Verkuil
` (5 preceding siblings ...)
2011-06-14 15:22 ` [RFCv1 PATCH 7/8] v4l2-ctrls: split try_or_set_ext_ctrls() Hans Verkuil
@ 2011-06-14 15:22 ` Hans Verkuil
2011-06-15 9:30 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Sakari Ailus
7 siblings, 0 replies; 17+ messages in thread
From: Hans Verkuil @ 2011-06-14 15:22 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, sakari.ailus, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 13 ++++---------
1 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 040d5c9..627a1e4 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1410,26 +1410,21 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl)
int i;
/* Skip if this control was already handled by a cluster. */
- if (ctrl->done)
+ /* Skip button controls and read-only controls. */
+ if (ctrl->done || ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
+ (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY))
continue;
for (i = 0; i < master->ncontrols; i++) {
if (master->cluster[i]) {
cur_to_new(master->cluster[i]);
master->cluster[i]->is_new = 1;
+ master->cluster[i]->done = true;
}
}
-
- /* Skip button controls and read-only controls. */
- if (ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
- (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY))
- continue;
ret = call_op(master, s_ctrl);
if (ret)
break;
- for (i = 0; i < master->ncontrols; i++)
- if (master->cluster[i])
- master->cluster[i]->done = true;
}
mutex_unlock(&hdl->lock);
return ret;
--
1.7.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh
2011-06-14 15:22 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Hans Verkuil
` (6 preceding siblings ...)
2011-06-14 15:22 ` [RFCv1 PATCH 8/8] v4l2-ctrls: v4l2_ctrl_handler_setup code simplification Hans Verkuil
@ 2011-06-15 9:30 ` Sakari Ailus
2011-06-15 16:39 ` Hans Verkuil
7 siblings, 1 reply; 17+ messages in thread
From: Sakari Ailus @ 2011-06-15 9:30 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, laurent.pinchart, Hans Verkuil
Hi Hans,
Many thanks for the patch. I'm very happy to see this!
I have just one comment below.
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 45e9c1e..042b893 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -43,17 +43,6 @@ struct v4l2_subscribed_event {
> u32 id;
> };
>
> -struct v4l2_events {
> - wait_queue_head_t wait;
> - struct list_head subscribed; /* Subscribed events */
> - struct list_head free; /* Events ready for use */
> - struct list_head available; /* Dequeueable event */
> - unsigned int navailable;
> - unsigned int nallocated; /* Number of allocated events */
> - u32 sequence;
> -};
> -
> -int v4l2_event_init(struct v4l2_fh *fh);
> int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
> void v4l2_event_free(struct v4l2_fh *fh);
> int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index d247111..bfc0457 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -29,15 +29,22 @@
> #include <linux/list.h>
>
> struct video_device;
> -struct v4l2_events;
> struct v4l2_ctrl_handler;
>
> struct v4l2_fh {
> struct list_head list;
> struct video_device *vdev;
> - struct v4l2_events *events; /* events, pending and subscribed */
> struct v4l2_ctrl_handler *ctrl_handler;
> enum v4l2_priority prio;
> +
> + /* Events */
> + wait_queue_head_t wait;
> + struct list_head subscribed; /* Subscribed events */
> + struct list_head free; /* Events ready for use */
> + struct list_head available; /* Dequeueable event */
> + unsigned int navailable;
> + unsigned int nallocated; /* Number of allocated events */
> + u32 sequence;
A question: why to move the fields from v4l2_events to v4l2_fh? Events may
be more important part of V4L2 than before but they're still not file
handles. :-) The event related field names have no hing they'd be related to
events --- "free", for example.
Regards,
--
Sakari Ailus
sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh
2011-06-15 9:30 ` [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh Sakari Ailus
@ 2011-06-15 16:39 ` Hans Verkuil
2011-06-15 16:59 ` Sakari Ailus
0 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2011-06-15 16:39 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, laurent.pinchart, Hans Verkuil
On Wednesday, June 15, 2011 11:30:07 Sakari Ailus wrote:
> Hi Hans,
>
> Many thanks for the patch. I'm very happy to see this!
>
> I have just one comment below.
>
> > diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> > index 45e9c1e..042b893 100644
> > --- a/include/media/v4l2-event.h
> > +++ b/include/media/v4l2-event.h
> > @@ -43,17 +43,6 @@ struct v4l2_subscribed_event {
> > u32 id;
> > };
> >
> > -struct v4l2_events {
> > - wait_queue_head_t wait;
> > - struct list_head subscribed; /* Subscribed events */
> > - struct list_head free; /* Events ready for use */
> > - struct list_head available; /* Dequeueable event */
> > - unsigned int navailable;
> > - unsigned int nallocated; /* Number of allocated events */
> > - u32 sequence;
> > -};
> > -
> > -int v4l2_event_init(struct v4l2_fh *fh);
> > int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
> > void v4l2_event_free(struct v4l2_fh *fh);
> > int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
> > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > index d247111..bfc0457 100644
> > --- a/include/media/v4l2-fh.h
> > +++ b/include/media/v4l2-fh.h
> > @@ -29,15 +29,22 @@
> > #include <linux/list.h>
> >
> > struct video_device;
> > -struct v4l2_events;
> > struct v4l2_ctrl_handler;
> >
> > struct v4l2_fh {
> > struct list_head list;
> > struct video_device *vdev;
> > - struct v4l2_events *events; /* events, pending and subscribed */
> > struct v4l2_ctrl_handler *ctrl_handler;
> > enum v4l2_priority prio;
> > +
> > + /* Events */
> > + wait_queue_head_t wait;
> > + struct list_head subscribed; /* Subscribed events */
> > + struct list_head free; /* Events ready for use */
> > + struct list_head available; /* Dequeueable event */
> > + unsigned int navailable;
> > + unsigned int nallocated; /* Number of allocated events */
> > + u32 sequence;
>
> A question: why to move the fields from v4l2_events to v4l2_fh? Events may
> be more important part of V4L2 than before but they're still not file
> handles. :-) The event related field names have no hing they'd be related to
> events --- "free", for example.
The only reason that the v4l2_events struct existed was that there were so few
drivers that needed events. So why allocate memory that you don't need? That
all changes with the control event: almost all drivers will need that since
almost all drivers have events.
Merging it makes the code easier and v4l2_fh_init can become a void function
(so no more error checking needed). And since these fields are always there, I
no longer need to check whether fh->events is NULL or not.
I can add a patch renaming some of the event fields if you prefer, but I don't
think they are that bad. Note that 'free' and 'nallocated' are removed
completely in a later patch.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh
2011-06-15 16:39 ` Hans Verkuil
@ 2011-06-15 16:59 ` Sakari Ailus
2011-06-15 17:10 ` Hans Verkuil
0 siblings, 1 reply; 17+ messages in thread
From: Sakari Ailus @ 2011-06-15 16:59 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, laurent.pinchart, Hans Verkuil
Hans Verkuil wrote:
> On Wednesday, June 15, 2011 11:30:07 Sakari Ailus wrote:
>> Hi Hans,
>>
>> Many thanks for the patch. I'm very happy to see this!
>>
>> I have just one comment below.
>>
>>> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
>>> index 45e9c1e..042b893 100644
>>> --- a/include/media/v4l2-event.h
>>> +++ b/include/media/v4l2-event.h
>>> @@ -43,17 +43,6 @@ struct v4l2_subscribed_event {
>>> u32 id;
>>> };
>>>
>>> -struct v4l2_events {
>>> - wait_queue_head_t wait;
>>> - struct list_head subscribed; /* Subscribed events */
>>> - struct list_head free; /* Events ready for use */
>>> - struct list_head available; /* Dequeueable event */
>>> - unsigned int navailable;
>>> - unsigned int nallocated; /* Number of allocated events */
>>> - u32 sequence;
>>> -};
>>> -
>>> -int v4l2_event_init(struct v4l2_fh *fh);
>>> int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
>>> void v4l2_event_free(struct v4l2_fh *fh);
>>> int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
>>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
>>> index d247111..bfc0457 100644
>>> --- a/include/media/v4l2-fh.h
>>> +++ b/include/media/v4l2-fh.h
>>> @@ -29,15 +29,22 @@
>>> #include<linux/list.h>
>>>
>>> struct video_device;
>>> -struct v4l2_events;
>>> struct v4l2_ctrl_handler;
>>>
>>> struct v4l2_fh {
>>> struct list_head list;
>>> struct video_device *vdev;
>>> - struct v4l2_events *events; /* events, pending and subscribed */
>>> struct v4l2_ctrl_handler *ctrl_handler;
>>> enum v4l2_priority prio;
>>> +
>>> + /* Events */
>>> + wait_queue_head_t wait;
>>> + struct list_head subscribed; /* Subscribed events */
>>> + struct list_head free; /* Events ready for use */
>>> + struct list_head available; /* Dequeueable event */
>>> + unsigned int navailable;
>>> + unsigned int nallocated; /* Number of allocated events */
>>> + u32 sequence;
>>
>> A question: why to move the fields from v4l2_events to v4l2_fh? Events may
>> be more important part of V4L2 than before but they're still not file
>> handles. :-) The event related field names have no hing they'd be related to
>> events --- "free", for example.
>
> The only reason that the v4l2_events struct existed was that there were so few
> drivers that needed events. So why allocate memory that you don't need? That
> all changes with the control event: almost all drivers will need that since
> almost all drivers have events.
>
> Merging it makes the code easier and v4l2_fh_init can become a void function
> (so no more error checking needed). And since these fields are always there, I
> no longer need to check whether fh->events is NULL or not.
>
> I can add a patch renaming some of the event fields if you prefer, but I don't
> think they are that bad. Note that 'free' and 'nallocated' are removed
> completely in a later patch.
Thanks for the explanation. What I had in mind that what other fields
possibly would be added to v4l2_fh in the future? If there will be many,
in that case keeping event related fields in a separate structure might
make sense. I have none in mind right now, though, so perhaps this could
be given a second thought if we're adding more things to the v4l2_fh
structure?
I think this patchset is a significant improvement to the old behaviour.
Regards,
--
Sakari Ailus
sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFCv1 PATCH 1/8] v4l2-events/fh: merge v4l2_events into v4l2_fh
2011-06-15 16:59 ` Sakari Ailus
@ 2011-06-15 17:10 ` Hans Verkuil
0 siblings, 0 replies; 17+ messages in thread
From: Hans Verkuil @ 2011-06-15 17:10 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, laurent.pinchart, Hans Verkuil
On Wednesday, June 15, 2011 18:59:05 Sakari Ailus wrote:
> Hans Verkuil wrote:
> > On Wednesday, June 15, 2011 11:30:07 Sakari Ailus wrote:
> >> Hi Hans,
> >>
> >> Many thanks for the patch. I'm very happy to see this!
> >>
> >> I have just one comment below.
> >>
> >>> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> >>> index 45e9c1e..042b893 100644
> >>> --- a/include/media/v4l2-event.h
> >>> +++ b/include/media/v4l2-event.h
> >>> @@ -43,17 +43,6 @@ struct v4l2_subscribed_event {
> >>> u32 id;
> >>> };
> >>>
> >>> -struct v4l2_events {
> >>> - wait_queue_head_t wait;
> >>> - struct list_head subscribed; /* Subscribed events */
> >>> - struct list_head free; /* Events ready for use */
> >>> - struct list_head available; /* Dequeueable event */
> >>> - unsigned int navailable;
> >>> - unsigned int nallocated; /* Number of allocated events */
> >>> - u32 sequence;
> >>> -};
> >>> -
> >>> -int v4l2_event_init(struct v4l2_fh *fh);
> >>> int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
> >>> void v4l2_event_free(struct v4l2_fh *fh);
> >>> int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
> >>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> >>> index d247111..bfc0457 100644
> >>> --- a/include/media/v4l2-fh.h
> >>> +++ b/include/media/v4l2-fh.h
> >>> @@ -29,15 +29,22 @@
> >>> #include<linux/list.h>
> >>>
> >>> struct video_device;
> >>> -struct v4l2_events;
> >>> struct v4l2_ctrl_handler;
> >>>
> >>> struct v4l2_fh {
> >>> struct list_head list;
> >>> struct video_device *vdev;
> >>> - struct v4l2_events *events; /* events, pending and subscribed */
> >>> struct v4l2_ctrl_handler *ctrl_handler;
> >>> enum v4l2_priority prio;
> >>> +
> >>> + /* Events */
> >>> + wait_queue_head_t wait;
> >>> + struct list_head subscribed; /* Subscribed events */
> >>> + struct list_head free; /* Events ready for use */
> >>> + struct list_head available; /* Dequeueable event */
> >>> + unsigned int navailable;
> >>> + unsigned int nallocated; /* Number of allocated events */
> >>> + u32 sequence;
> >>
> >> A question: why to move the fields from v4l2_events to v4l2_fh? Events may
> >> be more important part of V4L2 than before but they're still not file
> >> handles. :-) The event related field names have no hing they'd be related to
> >> events --- "free", for example.
> >
> > The only reason that the v4l2_events struct existed was that there were so few
> > drivers that needed events. So why allocate memory that you don't need? That
> > all changes with the control event: almost all drivers will need that since
> > almost all drivers have events.
> >
> > Merging it makes the code easier and v4l2_fh_init can become a void function
> > (so no more error checking needed). And since these fields are always there, I
> > no longer need to check whether fh->events is NULL or not.
> >
> > I can add a patch renaming some of the event fields if you prefer, but I don't
> > think they are that bad. Note that 'free' and 'nallocated' are removed
> > completely in a later patch.
>
> Thanks for the explanation. What I had in mind that what other fields
> possibly would be added to v4l2_fh in the future? If there will be many,
> in that case keeping event related fields in a separate structure might
> make sense. I have none in mind right now, though, so perhaps this could
> be given a second thought if we're adding more things to the v4l2_fh
> structure?
I guess any future extensions will need to be considered on their own merits.
If it is a rarely used extension, then it can be allocated on demand, if it
is a commonly used extension, then it's easier to add it to this struct.
> I think this patchset is a significant improvement to the old behaviour.
Thank you, I have to say I'm very pleased with it. It gives the user certain
guarantees with respect to arrival of events that are hard to realize otherwise.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread