* Various ctrl and event frame work patches
@ 2011-10-27 11:17 Hans de Goede
2011-10-27 11:17 ` [PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls Hans de Goede
` (5 more replies)
0 siblings, 6 replies; 23+ messages in thread
From: Hans de Goede @ 2011-10-27 11:17 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: hverkuil, Laurent Pinchart
Hi All,
This patch set obsoletes my previous "add v4l2_subscribed_event_ops" set,
while working on adding support for ctrl-events to the uvc driver I found
a few bugs in the event code, which this patchset fixes.
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls
2011-10-27 11:17 Various ctrl and event frame work patches Hans de Goede
@ 2011-10-27 11:17 ` Hans de Goede
2011-10-30 10:16 ` Hans Verkuil
2011-10-27 11:17 ` [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL Hans de Goede
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-10-27 11:17 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: hverkuil, Laurent Pinchart, Hans de Goede
Otherwise the fh changing the master control won't get the inactive state
change event for the slave controls.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/media/video/v4l2-ctrls.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index fc8666a..69e24f4 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -945,6 +945,7 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
if (ctrl->cluster[0]->has_volatiles)
ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
}
+ fh = NULL;
}
if (changed || update_inactive) {
/* If a control was changed that was not one of the controls
--
1.7.7
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL
2011-10-27 11:17 Various ctrl and event frame work patches Hans de Goede
2011-10-27 11:17 ` [PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls Hans de Goede
@ 2011-10-27 11:17 ` Hans de Goede
2011-10-27 12:04 ` Laurent Pinchart
2011-10-30 10:17 ` Hans Verkuil
2011-10-27 11:18 ` [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing Hans de Goede
` (3 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Hans de Goede @ 2011-10-27 11:17 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: hverkuil, Laurent Pinchart, Hans de Goede
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/media/video/v4l2-event.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 53b190c..9f56f18 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -215,6 +215,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
unsigned long flags;
unsigned i;
+ if (sub->type == V4L2_EVENT_ALL)
+ return -EINVAL;
+
if (elems < 1)
elems = 1;
if (sub->type == V4L2_EVENT_CTRL) {
--
1.7.7
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing
2011-10-27 11:17 Various ctrl and event frame work patches Hans de Goede
2011-10-27 11:17 ` [PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls Hans de Goede
2011-10-27 11:17 ` [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL Hans de Goede
@ 2011-10-27 11:18 ` Hans de Goede
2011-10-27 12:07 ` Laurent Pinchart
2011-10-30 10:24 ` Hans Verkuil
2011-10-27 11:18 ` [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe Hans de Goede
` (2 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Hans de Goede @ 2011-10-27 11:18 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: hverkuil, Laurent Pinchart, Hans de Goede
The kev pointers inside the pending events queue (the available queue) of the
fh point to data inside the sev, unsubscribing frees the sev, thus making these
pointers point to freed memory!
This patch fixes these dangling pointers in the available queue by removing
all matching pending events on unsubscription.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/media/video/v4l2-event.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9f56f18..01cbb7f 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub)
{
struct v4l2_subscribed_event *sev;
+ struct v4l2_kevent *kev, *next;
unsigned long flags;
if (sub->type == V4L2_EVENT_ALL) {
@@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
if (sev != NULL) {
+ /* Remove any pending events for this subscription */
+ list_for_each_entry_safe(kev, next, &fh->available, list) {
+ if (kev->sev == sev) {
+ list_del(&kev->list);
+ fh->navailable--;
+ }
+ }
list_del(&sev->list);
sev->fh = NULL;
}
--
1.7.7
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe
2011-10-27 11:17 Various ctrl and event frame work patches Hans de Goede
` (2 preceding siblings ...)
2011-10-27 11:18 ` [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing Hans de Goede
@ 2011-10-27 11:18 ` Hans de Goede
2011-10-27 12:20 ` Laurent Pinchart
2011-10-30 10:30 ` Hans Verkuil
2011-10-27 11:18 ` [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede
2011-10-27 11:18 ` [PATCH 6/6] v4l2-ctrls: Use v4l2_subscribed_event_ops Hans de Goede
5 siblings, 2 replies; 23+ messages in thread
From: Hans de Goede @ 2011-10-27 11:18 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: hverkuil, Laurent Pinchart, Hans de Goede
1: There is no reason for this after v4l2_event_unsubscribe releases the
spinlock nothing is holding a reference to the sev anymore except for the
local reference in the v4l2_event_unsubscribe function.
2: Setting sev->fh to NULL causes problems for the del op added in the next
patch of this series, since this op needs a way to get to its own data
structures, and typically this will be done by using container_of on an
embedded v4l2_fh struct.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/media/video/v4l2-event.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 01cbb7f..3d27300 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -304,7 +304,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
}
}
list_del(&sev->list);
- sev->fh = NULL;
}
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
--
1.7.7
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops
2011-10-27 11:17 Various ctrl and event frame work patches Hans de Goede
` (3 preceding siblings ...)
2011-10-27 11:18 ` [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe Hans de Goede
@ 2011-10-27 11:18 ` Hans de Goede
2011-10-27 12:30 ` Laurent Pinchart
2011-10-30 11:17 ` Hans Verkuil
2011-10-27 11:18 ` [PATCH 6/6] v4l2-ctrls: Use v4l2_subscribed_event_ops Hans de Goede
5 siblings, 2 replies; 23+ messages in thread
From: Hans de Goede @ 2011-10-27 11:18 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: hverkuil, Laurent Pinchart, Hans de Goede
Just like with ctrl events, drivers may want to get called back on
listener add / remove for other event types too. Rather then special
casing all of this in subscribe / unsubscribe event it is better to
use ops for this.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/media/video/ivtv/ivtv-ioctl.c | 2 +-
drivers/media/video/omap3isp/ispccdc.c | 2 +-
drivers/media/video/omap3isp/ispstat.c | 2 +-
drivers/media/video/pwc/pwc-v4l.c | 2 +-
drivers/media/video/v4l2-event.c | 42 ++++++++++++++++++++++++-------
drivers/media/video/vivi.c | 2 +-
include/media/v4l2-event.h | 24 +++++++++++++-----
7 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index ecafa69..9aec8a0 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
case V4L2_EVENT_VSYNC:
case V4L2_EVENT_EOS:
case V4L2_EVENT_CTRL:
- return v4l2_event_subscribe(fh, sub, 0);
+ return v4l2_event_subscribe(fh, sub, 0, NULL);
default:
return -EINVAL;
}
diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
index 40b141c..b6da736 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
if (sub->id != 0)
return -EINVAL;
- return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
+ return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
}
static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
index 8080659..4f337a2 100644
--- a/drivers/media/video/omap3isp/ispstat.c
+++ b/drivers/media/video/omap3isp/ispstat.c
@@ -1049,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, STAT_NEVENTS);
+ return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
}
int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 68e1323..7f159bf 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
{
switch (sub->type) {
case V4L2_EVENT_CTRL:
- return v4l2_event_subscribe(fh, sub, 0);
+ return v4l2_event_subscribe(fh, sub, 0, NULL);
default:
return -EINVAL;
}
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 3d27300..2dd9252 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
sev->first = sev_pos(sev, 1);
fh->navailable--;
if (sev->elems == 1) {
- if (sev->replace) {
- sev->replace(&kev->event, ev);
+ if (sev->ops && sev->ops->replace) {
+ sev->ops->replace(&kev->event, ev);
copy_payload = false;
}
- } else if (sev->merge) {
+ } else if (sev->ops && sev->ops->merge) {
struct v4l2_kevent *second_oldest =
sev->events + sev_pos(sev, 0);
- sev->merge(&kev->event, &second_oldest->event);
+ sev->ops->merge(&kev->event, &second_oldest->event);
}
}
@@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
new->u.ctrl.changes |= old->u.ctrl.changes;
}
+const struct v4l2_subscribed_event_ops ctrl_ops = {
+ .replace = ctrls_replace,
+ .merge = ctrls_merge,
+};
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
- struct v4l2_event_subscription *sub, unsigned elems)
+ struct v4l2_event_subscription *sub, unsigned elems,
+ const struct v4l2_subscribed_event_ops *ops)
{
struct v4l2_subscribed_event *sev, *found_ev;
struct v4l2_ctrl *ctrl = NULL;
@@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
sev->flags = sub->flags;
sev->fh = fh;
sev->elems = elems;
+ sev->ops = ops;
if (ctrl) {
- sev->replace = ctrls_replace;
- sev->merge = ctrls_merge;
+ sev->ops = &ctrl_ops;
}
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
@@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
list_add(&sev->list, &fh->subscribed);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
- /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
- if (found_ev)
+ if (found_ev) {
kfree(sev);
- else if (ctrl)
+ return 0; /* Already listening */
+ }
+
+ if (sev->ops && sev->ops->add) {
+ int ret = sev->ops->add(sev);
+ if (ret) {
+ sev->ops = NULL;
+ v4l2_event_unsubscribe(fh, sub);
+ return ret;
+ }
+ }
+
+ /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
+ if (ctrl)
v4l2_ctrl_add_event(ctrl, sev);
return 0;
@@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
}
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+ if (sev && sev->ops && sev->ops->del)
+ sev->ops->del(sev);
+
if (sev && sev->type == V4L2_EVENT_CTRL) {
struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index c25787d..74ebbad 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
{
switch (sub->type) {
case V4L2_EVENT_CTRL:
- return v4l2_event_subscribe(fh, sub, 0);
+ return v4l2_event_subscribe(fh, sub, 0, NULL);
default:
return -EINVAL;
}
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 5f14e88..88fa9a1 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -78,6 +78,19 @@ struct v4l2_kevent {
struct v4l2_event event;
};
+/** struct v4l2_subscribed_event_ops - Subscribed event operations.
+ * @add: Optional callback, called when a new listener is added
+ * @del: Optional callback, called when a listener stops listening
+ * @replace: Optional callback that can replace event 'old' with event 'new'.
+ * @merge: Optional callback that can merge event 'old' into event 'new'.
+ */
+struct v4l2_subscribed_event_ops {
+ int (*add)(struct v4l2_subscribed_event *sev);
+ void (*del)(struct v4l2_subscribed_event *sev);
+ void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
+ void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
+};
+
/** struct v4l2_subscribed_event - Internal struct representing a subscribed event.
* @list: List node for the v4l2_fh->subscribed list.
* @type: Event type.
@@ -85,8 +98,7 @@ struct v4l2_kevent {
* @flags: Copy of v4l2_event_subscription->flags.
* @fh: Filehandle that subscribed to this event.
* @node: List node that hooks into the object's event list (if there is one).
- * @replace: Optional callback that can replace event 'old' with event 'new'.
- * @merge: Optional callback that can merge event 'old' into event 'new'.
+ * @ops: v4l2_subscribed_event_ops
* @elems: The number of elements in the events array.
* @first: The index of the events containing the oldest available event.
* @in_use: The number of queued events.
@@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
u32 flags;
struct v4l2_fh *fh;
struct list_head node;
- void (*replace)(struct v4l2_event *old,
- const struct v4l2_event *new);
- void (*merge)(const struct v4l2_event *old,
- struct v4l2_event *new);
+ const struct v4l2_subscribed_event_ops *ops;
unsigned elems;
unsigned first;
unsigned in_use;
@@ -115,7 +124,8 @@ 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, unsigned elems);
+ struct v4l2_event_subscription *sub, unsigned elems,
+ const struct v4l2_subscribed_event_ops *ops);
int v4l2_event_unsubscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub);
void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
--
1.7.7
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/6] v4l2-ctrls: Use v4l2_subscribed_event_ops
2011-10-27 11:17 Various ctrl and event frame work patches Hans de Goede
` (4 preceding siblings ...)
2011-10-27 11:18 ` [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede
@ 2011-10-27 11:18 ` Hans de Goede
2011-10-30 11:24 ` Hans Verkuil
5 siblings, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-10-27 11:18 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: hverkuil, Laurent Pinchart, Hans de Goede
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/media/video/ivtv/ivtv-ioctl.c | 3 +-
drivers/media/video/pwc/pwc-v4l.c | 2 +-
drivers/media/video/v4l2-ctrls.c | 56 +++++++++++++++++++++++++++------
drivers/media/video/v4l2-event.c | 39 -----------------------
drivers/media/video/vivi.c | 2 +-
include/media/v4l2-ctrls.h | 7 ++--
6 files changed, 53 insertions(+), 56 deletions(-)
diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index 9aec8a0..72fd74f 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1455,8 +1455,9 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
switch (sub->type) {
case V4L2_EVENT_VSYNC:
case V4L2_EVENT_EOS:
- case V4L2_EVENT_CTRL:
return v4l2_event_subscribe(fh, sub, 0, NULL);
+ case V4L2_EVENT_CTRL:
+ return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
default:
return -EINVAL;
}
diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 7f159bf..afc5b15 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
{
switch (sub->type) {
case V4L2_EVENT_CTRL:
- return v4l2_event_subscribe(fh, sub, 0, NULL);
+ return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
default:
return -EINVAL;
}
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 69e24f4..c4dec20 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -2329,10 +2329,22 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
}
EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
-void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
- struct v4l2_subscribed_event *sev)
+static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev)
{
- v4l2_ctrl_lock(ctrl);
+ struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
+ struct v4l2_ctrl_ref *ref;
+ struct v4l2_ctrl *ctrl;
+ int ret = 0;
+
+ mutex_lock(&hdl->lock);
+
+ ref = find_ref(hdl, sev->id);
+ if (!ref) {
+ ret = -EINVAL;
+ goto leave;
+ }
+ ctrl = ref->ctrl;
+
list_add_tail(&sev->node, &ctrl->ev_subs);
if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
(sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
@@ -2344,15 +2356,39 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
fill_event(&ev, ctrl, changes);
v4l2_event_queue_fh(sev->fh, &ev);
}
- v4l2_ctrl_unlock(ctrl);
+leave:
+ mutex_unlock(&hdl->lock);
+ return ret;
}
-EXPORT_SYMBOL(v4l2_ctrl_add_event);
-void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
- struct v4l2_subscribed_event *sev)
+static void v4l2_ctrl_del_event(struct v4l2_subscribed_event *sev)
{
- v4l2_ctrl_lock(ctrl);
+ struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
+
+ mutex_lock(&hdl->lock);
list_del(&sev->node);
- v4l2_ctrl_unlock(ctrl);
+ mutex_unlock(&hdl->lock);
}
-EXPORT_SYMBOL(v4l2_ctrl_del_event);
+
+void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new)
+{
+ u32 old_changes = old->u.ctrl.changes;
+
+ old->u.ctrl = new->u.ctrl;
+ old->u.ctrl.changes |= old_changes;
+}
+EXPORT_SYMBOL(v4l2_ctrl_replace);
+
+void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new)
+{
+ new->u.ctrl.changes |= old->u.ctrl.changes;
+}
+EXPORT_SYMBOL(v4l2_ctrl_merge);
+
+const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops = {
+ .add = v4l2_ctrl_add_event,
+ .del = v4l2_ctrl_del_event,
+ .replace = v4l2_ctrl_replace,
+ .merge = v4l2_ctrl_merge,
+};
+EXPORT_SYMBOL(v4l2_ctrl_sub_ev_ops);
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 2dd9252..2f5ee7b 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -25,7 +25,6 @@
#include <media/v4l2-dev.h>
#include <media/v4l2-fh.h>
#include <media/v4l2-event.h>
-#include <media/v4l2-ctrls.h>
#include <linux/sched.h>
#include <linux/slab.h>
@@ -194,30 +193,11 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
-static void ctrls_replace(struct v4l2_event *old, const struct v4l2_event *new)
-{
- u32 old_changes = old->u.ctrl.changes;
-
- old->u.ctrl = new->u.ctrl;
- old->u.ctrl.changes |= old_changes;
-}
-
-static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
-{
- new->u.ctrl.changes |= old->u.ctrl.changes;
-}
-
-const struct v4l2_subscribed_event_ops ctrl_ops = {
- .replace = ctrls_replace,
- .merge = ctrls_merge,
-};
-
int v4l2_event_subscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub, unsigned elems,
const struct v4l2_subscribed_event_ops *ops)
{
struct v4l2_subscribed_event *sev, *found_ev;
- struct v4l2_ctrl *ctrl = NULL;
unsigned long flags;
unsigned i;
@@ -226,11 +206,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
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) + sizeof(struct v4l2_kevent) * elems, GFP_KERNEL);
if (!sev)
@@ -243,9 +218,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
sev->fh = fh;
sev->elems = elems;
sev->ops = ops;
- if (ctrl) {
- sev->ops = &ctrl_ops;
- }
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -267,10 +239,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
}
}
- /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
- if (ctrl)
- v4l2_ctrl_add_event(ctrl, sev);
-
return 0;
}
EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
@@ -329,13 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
if (sev && sev->ops && sev->ops->del)
sev->ops->del(sev);
- if (sev && sev->type == V4L2_EVENT_CTRL) {
- struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
-
- if (ctrl)
- v4l2_ctrl_del_event(ctrl, sev);
- }
-
kfree(sev);
return 0;
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 74ebbad..ce1783d 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
{
switch (sub->type) {
case V4L2_EVENT_CTRL:
- return v4l2_event_subscribe(fh, sub, 0, NULL);
+ return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
default:
return -EINVAL;
}
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index eeb3df6..fde17ff 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -487,10 +487,9 @@ 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_event(struct v4l2_ctrl *ctrl,
- struct v4l2_subscribed_event *sev);
-void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
- struct v4l2_subscribed_event *sev);
+extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
+void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
+void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new);
/* 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);
--
1.7.7
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL
2011-10-27 11:17 ` [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL Hans de Goede
@ 2011-10-27 12:04 ` Laurent Pinchart
2011-10-30 10:17 ` Hans Verkuil
1 sibling, 0 replies; 23+ messages in thread
From: Laurent Pinchart @ 2011-10-27 12:04 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, hverkuil
Hi Hans,
On Thursday 27 October 2011 13:17:59 Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
This brings the code in sync with the documentation, thanks.
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/video/v4l2-event.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 53b190c..9f56f18 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -215,6 +215,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> unsigned long flags;
> unsigned i;
>
> + if (sub->type == V4L2_EVENT_ALL)
> + return -EINVAL;
> +
> if (elems < 1)
> elems = 1;
> if (sub->type == V4L2_EVENT_CTRL) {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing
2011-10-27 11:18 ` [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing Hans de Goede
@ 2011-10-27 12:07 ` Laurent Pinchart
2011-10-30 10:24 ` Hans Verkuil
1 sibling, 0 replies; 23+ messages in thread
From: Laurent Pinchart @ 2011-10-27 12:07 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, hverkuil
Hi Hans,
On Thursday 27 October 2011 13:18:00 Hans de Goede wrote:
> The kev pointers inside the pending events queue (the available queue) of
> the fh point to data inside the sev, unsubscribing frees the sev, thus
> making these pointers point to freed memory!
>
> This patch fixes these dangling pointers in the available queue by removing
> all matching pending events on unsubscription.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/video/v4l2-event.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 9f56f18..01cbb7f 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> struct v4l2_event_subscription *sub)
> {
> struct v4l2_subscribed_event *sev;
> + struct v4l2_kevent *kev, *next;
> unsigned long flags;
>
> if (sub->type == V4L2_EVENT_ALL) {
> @@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> if (sev != NULL) {
> + /* Remove any pending events for this subscription */
> + list_for_each_entry_safe(kev, next, &fh->available, list) {
> + if (kev->sev == sev) {
> + list_del(&kev->list);
> + fh->navailable--;
> + }
> + }
> list_del(&sev->list);
> sev->fh = NULL;
> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe
2011-10-27 11:18 ` [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe Hans de Goede
@ 2011-10-27 12:20 ` Laurent Pinchart
2011-10-28 8:35 ` Hans de Goede
2011-10-30 10:30 ` Hans Verkuil
1 sibling, 1 reply; 23+ messages in thread
From: Laurent Pinchart @ 2011-10-27 12:20 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, hverkuil
Hi Hans,
On Thursday 27 October 2011 13:18:01 Hans de Goede wrote:
> 1: There is no reason for this after v4l2_event_unsubscribe releases the
> spinlock nothing is holding a reference to the sev anymore except for the
> local reference in the v4l2_event_unsubscribe function.
>
> 2: Setting sev->fh to NULL causes problems for the del op added in the next
> patch of this series, since this op needs a way to get to its own data
> structures, and typically this will be done by using container_of on an
> embedded v4l2_fh struct.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
While reviewing the patch I noticed that v4l2_event_unsubscribe_all() calls
v4l2_event_unsubscribe(), which performs control lookup again. Is there a
reason for that, instead of handling event unsubscription directly in
v4l2_event_unsubscribe_all() ?
> ---
> drivers/media/video/v4l2-event.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 01cbb7f..3d27300 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -304,7 +304,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> }
> }
> list_del(&sev->list);
> - sev->fh = NULL;
> }
>
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops
2011-10-27 11:18 ` [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede
@ 2011-10-27 12:30 ` Laurent Pinchart
2011-10-28 8:37 ` Hans de Goede
2011-10-30 11:17 ` Hans Verkuil
1 sibling, 1 reply; 23+ messages in thread
From: Laurent Pinchart @ 2011-10-27 12:30 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, hverkuil
Hi Hans,
On Thursday 27 October 2011 13:18:02 Hans de Goede wrote:
> Just like with ctrl events, drivers may want to get called back on
> listener add / remove for other event types too. Rather then special
> casing all of this in subscribe / unsubscribe event it is better to
> use ops for this.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/media/video/ivtv/ivtv-ioctl.c | 2 +-
> drivers/media/video/omap3isp/ispccdc.c | 2 +-
> drivers/media/video/omap3isp/ispstat.c | 2 +-
> drivers/media/video/pwc/pwc-v4l.c | 2 +-
> drivers/media/video/v4l2-event.c | 42 ++++++++++++++++++++++-------
> drivers/media/video/vivi.c | 2 +-
> include/media/v4l2-event.h | 24 +++++++++++++-----
Haven't you forgotten to update Documentation/video4linux/v4l2-framework.txt ?
> 7 files changed, 54 insertions(+), 22 deletions(-)
[snip]
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 3d27300..2dd9252 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e sev->first = sev_pos(sev, 1);
> fh->navailable--;
> if (sev->elems == 1) {
> - if (sev->replace) {
> - sev->replace(&kev->event, ev);
> + if (sev->ops && sev->ops->replace) {
> + sev->ops->replace(&kev->event, ev);
> copy_payload = false;
> }
> - } else if (sev->merge) {
> + } else if (sev->ops && sev->ops->merge) {
> struct v4l2_kevent *second_oldest =
> sev->events + sev_pos(sev, 0);
> - sev->merge(&kev->event, &second_oldest->event);
> + sev->ops->merge(&kev->event, &second_oldest->event);
> }
> }
>
> @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old,
> struct v4l2_event *new) new->u.ctrl.changes |= old->u.ctrl.changes;
> }
>
> +const struct v4l2_subscribed_event_ops ctrl_ops = {
Shouldn't this be static const ?
> + .replace = ctrls_replace,
> + .merge = ctrls_merge,
> +};
> +
> int v4l2_event_subscribe(struct v4l2_fh *fh,
> - struct v4l2_event_subscription *sub, unsigned elems)
> + struct v4l2_event_subscription *sub, unsigned elems,
> + const struct v4l2_subscribed_event_ops *ops)
> {
> struct v4l2_subscribed_event *sev, *found_ev;
> struct v4l2_ctrl *ctrl = NULL;
> @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> sev->flags = sub->flags;
> sev->fh = fh;
> sev->elems = elems;
> + sev->ops = ops;
> if (ctrl) {
> - sev->replace = ctrls_replace;
> - sev->merge = ctrls_merge;
> + sev->ops = &ctrl_ops;
> }
You can remove the brackets here.
>
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> list_add(&sev->list, &fh->subscribed);
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>
> - /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> - if (found_ev)
> + if (found_ev) {
> kfree(sev);
> - else if (ctrl)
> + return 0; /* Already listening */
> + }
> +
> + if (sev->ops && sev->ops->add) {
> + int ret = sev->ops->add(sev);
> + if (ret) {
> + sev->ops = NULL;
> + v4l2_event_unsubscribe(fh, sub);
> + return ret;
> + }
> + }
> +
> + /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> + if (ctrl)
> v4l2_ctrl_add_event(ctrl, sev);
>
> return 0;
> @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> }
>
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> + if (sev && sev->ops && sev->ops->del)
> + sev->ops->del(sev);
> +
> if (sev && sev->type == V4L2_EVENT_CTRL) {
> struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe
2011-10-27 12:20 ` Laurent Pinchart
@ 2011-10-28 8:35 ` Hans de Goede
2011-10-28 11:48 ` Laurent Pinchart
0 siblings, 1 reply; 23+ messages in thread
From: Hans de Goede @ 2011-10-28 8:35 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Linux Media Mailing List, hverkuil
Hi,
Thanks for the reviews!
On 10/27/2011 02:20 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Thursday 27 October 2011 13:18:01 Hans de Goede wrote:
>> 1: There is no reason for this after v4l2_event_unsubscribe releases the
>> spinlock nothing is holding a reference to the sev anymore except for the
>> local reference in the v4l2_event_unsubscribe function.
>>
>> 2: Setting sev->fh to NULL causes problems for the del op added in the next
>> patch of this series, since this op needs a way to get to its own data
>> structures, and typically this will be done by using container_of on an
>> embedded v4l2_fh struct.
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>
> Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>
> While reviewing the patch I noticed that v4l2_event_unsubscribe_all() calls
> v4l2_event_unsubscribe(), which performs control lookup again. Is there a
> reason for that, instead of handling event unsubscription directly in
> v4l2_event_unsubscribe_all() ?
I didn't write that part, so I'll let Hans V. answer this question.
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops
2011-10-27 12:30 ` Laurent Pinchart
@ 2011-10-28 8:37 ` Hans de Goede
0 siblings, 0 replies; 23+ messages in thread
From: Hans de Goede @ 2011-10-28 8:37 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Linux Media Mailing List, hverkuil
Hi,
On 10/27/2011 02:30 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Thursday 27 October 2011 13:18:02 Hans de Goede wrote:
>> Just like with ctrl events, drivers may want to get called back on
>> listener add / remove for other event types too. Rather then special
>> casing all of this in subscribe / unsubscribe event it is better to
>> use ops for this.
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>> drivers/media/video/ivtv/ivtv-ioctl.c | 2 +-
>> drivers/media/video/omap3isp/ispccdc.c | 2 +-
>> drivers/media/video/omap3isp/ispstat.c | 2 +-
>> drivers/media/video/pwc/pwc-v4l.c | 2 +-
>> drivers/media/video/v4l2-event.c | 42 ++++++++++++++++++++++-------
>> drivers/media/video/vivi.c | 2 +-
>> include/media/v4l2-event.h | 24 +++++++++++++-----
>
> Haven't you forgotten to update Documentation/video4linux/v4l2-framework.txt ?
>
I think I have, I'll go and do another revision fixing this.
>> 7 files changed, 54 insertions(+), 22 deletions(-)
>
> [snip]
>
>> diff --git a/drivers/media/video/v4l2-event.c
>> b/drivers/media/video/v4l2-event.c index 3d27300..2dd9252 100644
>> --- a/drivers/media/video/v4l2-event.c
>> +++ b/drivers/media/video/v4l2-event.c
>> @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
>> const struct v4l2_event *e sev->first = sev_pos(sev, 1);
>> fh->navailable--;
>> if (sev->elems == 1) {
>> - if (sev->replace) {
>> - sev->replace(&kev->event, ev);
>> + if (sev->ops&& sev->ops->replace) {
>> + sev->ops->replace(&kev->event, ev);
>> copy_payload = false;
>> }
>> - } else if (sev->merge) {
>> + } else if (sev->ops&& sev->ops->merge) {
>> struct v4l2_kevent *second_oldest =
>> sev->events + sev_pos(sev, 0);
>> - sev->merge(&kev->event,&second_oldest->event);
>> + sev->ops->merge(&kev->event,&second_oldest->event);
>> }
>> }
>>
>> @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old,
>> struct v4l2_event *new) new->u.ctrl.changes |= old->u.ctrl.changes;
>> }
>>
>> +const struct v4l2_subscribed_event_ops ctrl_ops = {
>
> Shouldn't this be static const ?
>
You're right I will fix this in the next iteration.
>> + .replace = ctrls_replace,
>> + .merge = ctrls_merge,
>> +};
>> +
>> int v4l2_event_subscribe(struct v4l2_fh *fh,
>> - struct v4l2_event_subscription *sub, unsigned elems)
>> + struct v4l2_event_subscription *sub, unsigned elems,
>> + const struct v4l2_subscribed_event_ops *ops)
>> {
>> struct v4l2_subscribed_event *sev, *found_ev;
>> struct v4l2_ctrl *ctrl = NULL;
>> @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>> sev->flags = sub->flags;
>> sev->fh = fh;
>> sev->elems = elems;
>> + sev->ops = ops;
>> if (ctrl) {
>> - sev->replace = ctrls_replace;
>> - sev->merge = ctrls_merge;
>> + sev->ops =&ctrl_ops;
>> }
>
> You can remove the brackets here.
>
I was aiming for minimal diff size here, since this bit will go away in the
next patch in the series anyways.
>>
>> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>> list_add(&sev->list,&fh->subscribed);
>> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>>
>> - /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
>> - if (found_ev)
>> + if (found_ev) {
>> kfree(sev);
>> - else if (ctrl)
>> + return 0; /* Already listening */
>> + }
>> +
>> + if (sev->ops&& sev->ops->add) {
>> + int ret = sev->ops->add(sev);
>> + if (ret) {
>> + sev->ops = NULL;
>> + v4l2_event_unsubscribe(fh, sub);
>> + return ret;
>> + }
>> + }
>> +
>> + /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
>> + if (ctrl)
>> v4l2_ctrl_add_event(ctrl, sev);
>>
>> return 0;
>> @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>> }
>>
>> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +
>> + if (sev&& sev->ops&& sev->ops->del)
>> + sev->ops->del(sev);
>> +
>> if (sev&& sev->type == V4L2_EVENT_CTRL) {
>> struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
>>
>
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe
2011-10-28 8:35 ` Hans de Goede
@ 2011-10-28 11:48 ` Laurent Pinchart
0 siblings, 0 replies; 23+ messages in thread
From: Laurent Pinchart @ 2011-10-28 11:48 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, hverkuil
Hi Hans,
On Friday 28 October 2011 10:35:54 Hans de Goede wrote:
> On 10/27/2011 02:20 PM, Laurent Pinchart wrote:
> > On Thursday 27 October 2011 13:18:01 Hans de Goede wrote:
> >> 1: There is no reason for this after v4l2_event_unsubscribe releases the
> >> spinlock nothing is holding a reference to the sev anymore except for
> >> the local reference in the v4l2_event_unsubscribe function.
> >>
> >> 2: Setting sev->fh to NULL causes problems for the del op added in the
> >> next patch of this series, since this op needs a way to get to its own
> >> data structures, and typically this will be done by using container_of
> >> on an embedded v4l2_fh struct.
> >>
> >> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
> >
> > Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> >
> > While reviewing the patch I noticed that v4l2_event_unsubscribe_all()
> > calls v4l2_event_unsubscribe(), which performs control lookup again. Is
> > there a reason for that, instead of handling event unsubscription
> > directly in v4l2_event_unsubscribe_all() ?
>
> I didn't write that part, so I'll let Hans V. answer this question.
I know, that's why I still acked your patch :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls
2011-10-27 11:17 ` [PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls Hans de Goede
@ 2011-10-30 10:16 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2011-10-30 10:16 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, Laurent Pinchart
This is also part of a pull request from me (and so has my Signed-off-by already):
http://www.mail-archive.com/linux-media@vger.kernel.org/msg38018.html
Regards,
Hans
On Thursday, October 27, 2011 13:17:58 Hans de Goede wrote:
> Otherwise the fh changing the master control won't get the inactive state
> change event for the slave controls.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/media/video/v4l2-ctrls.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index fc8666a..69e24f4 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -945,6 +945,7 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
> if (ctrl->cluster[0]->has_volatiles)
> ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> }
> + fh = NULL;
> }
> if (changed || update_inactive) {
> /* If a control was changed that was not one of the controls
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL
2011-10-27 11:17 ` [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL Hans de Goede
2011-10-27 12:04 ` Laurent Pinchart
@ 2011-10-30 10:17 ` Hans Verkuil
1 sibling, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2011-10-30 10:17 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, Laurent Pinchart
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Regards,
Hans
On Thursday, October 27, 2011 13:17:59 Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/media/video/v4l2-event.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 53b190c..9f56f18 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -215,6 +215,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> unsigned long flags;
> unsigned i;
>
> + if (sub->type == V4L2_EVENT_ALL)
> + return -EINVAL;
> +
> if (elems < 1)
> elems = 1;
> if (sub->type == V4L2_EVENT_CTRL) {
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing
2011-10-27 11:18 ` [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing Hans de Goede
2011-10-27 12:07 ` Laurent Pinchart
@ 2011-10-30 10:24 ` Hans Verkuil
1 sibling, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2011-10-30 10:24 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, Laurent Pinchart
On Thursday, October 27, 2011 13:18:00 Hans de Goede wrote:
> The kev pointers inside the pending events queue (the available queue) of the
> fh point to data inside the sev, unsubscribing frees the sev, thus making these
> pointers point to freed memory!
>
> This patch fixes these dangling pointers in the available queue by removing
> all matching pending events on unsubscription.
The idea is fine, but the implementation is inefficient.
Instead of the list_for_each_entry_safe you can just do:
for (i = 0; i < sev->in_use; i++) {
list_del(&sev->events[sev_pos(sev, i)].list);
fh->navailable--;
}
It's untested, but this should do the trick.
Regards,
Hans
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/media/video/v4l2-event.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 9f56f18..01cbb7f 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> struct v4l2_event_subscription *sub)
> {
> struct v4l2_subscribed_event *sev;
> + struct v4l2_kevent *kev, *next;
> unsigned long flags;
>
> if (sub->type == V4L2_EVENT_ALL) {
> @@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> if (sev != NULL) {
> + /* Remove any pending events for this subscription */
> + list_for_each_entry_safe(kev, next, &fh->available, list) {
> + if (kev->sev == sev) {
> + list_del(&kev->list);
> + fh->navailable--;
> + }
> + }
> list_del(&sev->list);
> sev->fh = NULL;
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe
2011-10-27 11:18 ` [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe Hans de Goede
2011-10-27 12:20 ` Laurent Pinchart
@ 2011-10-30 10:30 ` Hans Verkuil
1 sibling, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2011-10-30 10:30 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, Laurent Pinchart
On Thursday, October 27, 2011 13:18:01 Hans de Goede wrote:
> 1: There is no reason for this after v4l2_event_unsubscribe releases the
> spinlock nothing is holding a reference to the sev anymore except for the
> local reference in the v4l2_event_unsubscribe function.
Not true. v4l2-ctrls.c may still have a reference to the sev through the
ev_subs list in struct v4l2_ctrl. The send_event() function checks for a
non-zero fh.
All that is needed is to find some different way of letting send_event()
know that this sev is no longer used. Perhaps by making sev->list empty?
Regards,
Hans
> 2: Setting sev->fh to NULL causes problems for the del op added in the next
> patch of this series, since this op needs a way to get to its own data
> structures, and typically this will be done by using container_of on an
> embedded v4l2_fh struct.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/media/video/v4l2-event.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 01cbb7f..3d27300 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -304,7 +304,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> }
> }
> list_del(&sev->list);
> - sev->fh = NULL;
> }
>
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops
2011-10-27 11:18 ` [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede
2011-10-27 12:30 ` Laurent Pinchart
@ 2011-10-30 11:17 ` Hans Verkuil
2011-10-30 11:37 ` Hans Verkuil
1 sibling, 1 reply; 23+ messages in thread
From: Hans Verkuil @ 2011-10-30 11:17 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, Laurent Pinchart
On Thursday, October 27, 2011 13:18:02 Hans de Goede wrote:
> Just like with ctrl events, drivers may want to get called back on
> listener add / remove for other event types too. Rather then special
> casing all of this in subscribe / unsubscribe event it is better to
> use ops for this.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Regards,
Hans
> ---
> drivers/media/video/ivtv/ivtv-ioctl.c | 2 +-
> drivers/media/video/omap3isp/ispccdc.c | 2 +-
> drivers/media/video/omap3isp/ispstat.c | 2 +-
> drivers/media/video/pwc/pwc-v4l.c | 2 +-
> drivers/media/video/v4l2-event.c | 42 ++++++++++++++++++++++++-------
> drivers/media/video/vivi.c | 2 +-
> include/media/v4l2-event.h | 24 +++++++++++++-----
> 7 files changed, 54 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
> index ecafa69..9aec8a0 100644
> --- a/drivers/media/video/ivtv/ivtv-ioctl.c
> +++ b/drivers/media/video/ivtv/ivtv-ioctl.c
> @@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
> case V4L2_EVENT_VSYNC:
> case V4L2_EVENT_EOS:
> case V4L2_EVENT_CTRL:
> - return v4l2_event_subscribe(fh, sub, 0);
> + return v4l2_event_subscribe(fh, sub, 0, NULL);
> default:
> return -EINVAL;
> }
> diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
> index 40b141c..b6da736 100644
> --- a/drivers/media/video/omap3isp/ispccdc.c
> +++ b/drivers/media/video/omap3isp/ispccdc.c
> @@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> if (sub->id != 0)
> return -EINVAL;
>
> - return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
> + return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
> }
>
> static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
> index 8080659..4f337a2 100644
> --- a/drivers/media/video/omap3isp/ispstat.c
> +++ b/drivers/media/video/omap3isp/ispstat.c
> @@ -1049,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, STAT_NEVENTS);
> + return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
> }
>
> int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
> diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
> index 68e1323..7f159bf 100644
> --- a/drivers/media/video/pwc/pwc-v4l.c
> +++ b/drivers/media/video/pwc/pwc-v4l.c
> @@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
> {
> switch (sub->type) {
> case V4L2_EVENT_CTRL:
> - return v4l2_event_subscribe(fh, sub, 0);
> + return v4l2_event_subscribe(fh, sub, 0, NULL);
> default:
> return -EINVAL;
> }
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 3d27300..2dd9252 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
> sev->first = sev_pos(sev, 1);
> fh->navailable--;
> if (sev->elems == 1) {
> - if (sev->replace) {
> - sev->replace(&kev->event, ev);
> + if (sev->ops && sev->ops->replace) {
> + sev->ops->replace(&kev->event, ev);
> copy_payload = false;
> }
> - } else if (sev->merge) {
> + } else if (sev->ops && sev->ops->merge) {
> struct v4l2_kevent *second_oldest =
> sev->events + sev_pos(sev, 0);
> - sev->merge(&kev->event, &second_oldest->event);
> + sev->ops->merge(&kev->event, &second_oldest->event);
> }
> }
>
> @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
> new->u.ctrl.changes |= old->u.ctrl.changes;
> }
>
> +const struct v4l2_subscribed_event_ops ctrl_ops = {
> + .replace = ctrls_replace,
> + .merge = ctrls_merge,
> +};
> +
> int v4l2_event_subscribe(struct v4l2_fh *fh,
> - struct v4l2_event_subscription *sub, unsigned elems)
> + struct v4l2_event_subscription *sub, unsigned elems,
> + const struct v4l2_subscribed_event_ops *ops)
> {
> struct v4l2_subscribed_event *sev, *found_ev;
> struct v4l2_ctrl *ctrl = NULL;
> @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> sev->flags = sub->flags;
> sev->fh = fh;
> sev->elems = elems;
> + sev->ops = ops;
> if (ctrl) {
> - sev->replace = ctrls_replace;
> - sev->merge = ctrls_merge;
> + sev->ops = &ctrl_ops;
> }
>
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> list_add(&sev->list, &fh->subscribed);
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>
> - /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> - if (found_ev)
> + if (found_ev) {
> kfree(sev);
> - else if (ctrl)
> + return 0; /* Already listening */
> + }
> +
> + if (sev->ops && sev->ops->add) {
> + int ret = sev->ops->add(sev);
> + if (ret) {
> + sev->ops = NULL;
> + v4l2_event_unsubscribe(fh, sub);
> + return ret;
> + }
> + }
> +
> + /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> + if (ctrl)
> v4l2_ctrl_add_event(ctrl, sev);
>
> return 0;
> @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> }
>
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> + if (sev && sev->ops && sev->ops->del)
> + sev->ops->del(sev);
> +
> if (sev && sev->type == V4L2_EVENT_CTRL) {
> struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
>
> diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> index c25787d..74ebbad 100644
> --- a/drivers/media/video/vivi.c
> +++ b/drivers/media/video/vivi.c
> @@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
> {
> switch (sub->type) {
> case V4L2_EVENT_CTRL:
> - return v4l2_event_subscribe(fh, sub, 0);
> + return v4l2_event_subscribe(fh, sub, 0, NULL);
> default:
> return -EINVAL;
> }
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 5f14e88..88fa9a1 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -78,6 +78,19 @@ struct v4l2_kevent {
> struct v4l2_event event;
> };
>
> +/** struct v4l2_subscribed_event_ops - Subscribed event operations.
> + * @add: Optional callback, called when a new listener is added
> + * @del: Optional callback, called when a listener stops listening
> + * @replace: Optional callback that can replace event 'old' with event 'new'.
> + * @merge: Optional callback that can merge event 'old' into event 'new'.
> + */
> +struct v4l2_subscribed_event_ops {
> + int (*add)(struct v4l2_subscribed_event *sev);
> + void (*del)(struct v4l2_subscribed_event *sev);
> + void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
> + void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
> +};
> +
> /** struct v4l2_subscribed_event - Internal struct representing a subscribed event.
> * @list: List node for the v4l2_fh->subscribed list.
> * @type: Event type.
> @@ -85,8 +98,7 @@ struct v4l2_kevent {
> * @flags: Copy of v4l2_event_subscription->flags.
> * @fh: Filehandle that subscribed to this event.
> * @node: List node that hooks into the object's event list (if there is one).
> - * @replace: Optional callback that can replace event 'old' with event 'new'.
> - * @merge: Optional callback that can merge event 'old' into event 'new'.
> + * @ops: v4l2_subscribed_event_ops
> * @elems: The number of elements in the events array.
> * @first: The index of the events containing the oldest available event.
> * @in_use: The number of queued events.
> @@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
> u32 flags;
> struct v4l2_fh *fh;
> struct list_head node;
> - void (*replace)(struct v4l2_event *old,
> - const struct v4l2_event *new);
> - void (*merge)(const struct v4l2_event *old,
> - struct v4l2_event *new);
> + const struct v4l2_subscribed_event_ops *ops;
> unsigned elems;
> unsigned first;
> unsigned in_use;
> @@ -115,7 +124,8 @@ 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, unsigned elems);
> + struct v4l2_event_subscription *sub, unsigned elems,
> + const struct v4l2_subscribed_event_ops *ops);
> int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> struct v4l2_event_subscription *sub);
> void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/6] v4l2-ctrls: Use v4l2_subscribed_event_ops
2011-10-27 11:18 ` [PATCH 6/6] v4l2-ctrls: Use v4l2_subscribed_event_ops Hans de Goede
@ 2011-10-30 11:24 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2011-10-30 11:24 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, Laurent Pinchart
On Thursday, October 27, 2011 13:18:03 Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/video/ivtv/ivtv-ioctl.c | 3 +-
> drivers/media/video/pwc/pwc-v4l.c | 2 +-
> drivers/media/video/v4l2-ctrls.c | 56 +++++++++++++++++++++++++++------
> drivers/media/video/v4l2-event.c | 39 -----------------------
> drivers/media/video/vivi.c | 2 +-
> include/media/v4l2-ctrls.h | 7 ++--
> 6 files changed, 53 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
> index 9aec8a0..72fd74f 100644
> --- a/drivers/media/video/ivtv/ivtv-ioctl.c
> +++ b/drivers/media/video/ivtv/ivtv-ioctl.c
> @@ -1455,8 +1455,9 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
> switch (sub->type) {
> case V4L2_EVENT_VSYNC:
> case V4L2_EVENT_EOS:
> - case V4L2_EVENT_CTRL:
> return v4l2_event_subscribe(fh, sub, 0, NULL);
> + case V4L2_EVENT_CTRL:
> + return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
> default:
> return -EINVAL;
> }
> diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
> index 7f159bf..afc5b15 100644
> --- a/drivers/media/video/pwc/pwc-v4l.c
> +++ b/drivers/media/video/pwc/pwc-v4l.c
> @@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
> {
> switch (sub->type) {
> case V4L2_EVENT_CTRL:
> - return v4l2_event_subscribe(fh, sub, 0, NULL);
> + return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
> default:
> return -EINVAL;
> }
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index 69e24f4..c4dec20 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -2329,10 +2329,22 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
> }
> EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
>
> -void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
> - struct v4l2_subscribed_event *sev)
> +static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev)
> {
> - v4l2_ctrl_lock(ctrl);
> + struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
> + struct v4l2_ctrl_ref *ref;
> + struct v4l2_ctrl *ctrl;
> + int ret = 0;
> +
> + mutex_lock(&hdl->lock);
> +
> + ref = find_ref(hdl, sev->id);
> + if (!ref) {
> + ret = -EINVAL;
> + goto leave;
> + }
> + ctrl = ref->ctrl;
> +
> list_add_tail(&sev->node, &ctrl->ev_subs);
> if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
> (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
> @@ -2344,15 +2356,39 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
> fill_event(&ev, ctrl, changes);
> v4l2_event_queue_fh(sev->fh, &ev);
> }
> - v4l2_ctrl_unlock(ctrl);
> +leave:
> + mutex_unlock(&hdl->lock);
> + return ret;
> }
> -EXPORT_SYMBOL(v4l2_ctrl_add_event);
>
> -void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
> - struct v4l2_subscribed_event *sev)
> +static void v4l2_ctrl_del_event(struct v4l2_subscribed_event *sev)
> {
> - v4l2_ctrl_lock(ctrl);
> + struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
> +
> + mutex_lock(&hdl->lock);
> list_del(&sev->node);
> - v4l2_ctrl_unlock(ctrl);
> + mutex_unlock(&hdl->lock);
> }
> -EXPORT_SYMBOL(v4l2_ctrl_del_event);
> +
> +void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new)
> +{
> + u32 old_changes = old->u.ctrl.changes;
> +
> + old->u.ctrl = new->u.ctrl;
> + old->u.ctrl.changes |= old_changes;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_replace);
> +
> +void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new)
> +{
> + new->u.ctrl.changes |= old->u.ctrl.changes;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_merge);
> +
> +const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops = {
> + .add = v4l2_ctrl_add_event,
> + .del = v4l2_ctrl_del_event,
> + .replace = v4l2_ctrl_replace,
> + .merge = v4l2_ctrl_merge,
> +};
> +EXPORT_SYMBOL(v4l2_ctrl_sub_ev_ops);
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 2dd9252..2f5ee7b 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -25,7 +25,6 @@
> #include <media/v4l2-dev.h>
> #include <media/v4l2-fh.h>
> #include <media/v4l2-event.h>
> -#include <media/v4l2-ctrls.h>
>
> #include <linux/sched.h>
> #include <linux/slab.h>
> @@ -194,30 +193,11 @@ int v4l2_event_pending(struct v4l2_fh *fh)
> }
> EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> -static void ctrls_replace(struct v4l2_event *old, const struct v4l2_event *new)
> -{
> - u32 old_changes = old->u.ctrl.changes;
> -
> - old->u.ctrl = new->u.ctrl;
> - old->u.ctrl.changes |= old_changes;
> -}
> -
> -static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
> -{
> - new->u.ctrl.changes |= old->u.ctrl.changes;
> -}
> -
> -const struct v4l2_subscribed_event_ops ctrl_ops = {
> - .replace = ctrls_replace,
> - .merge = ctrls_merge,
> -};
> -
> int v4l2_event_subscribe(struct v4l2_fh *fh,
> struct v4l2_event_subscription *sub, unsigned elems,
> const struct v4l2_subscribed_event_ops *ops)
> {
> struct v4l2_subscribed_event *sev, *found_ev;
> - struct v4l2_ctrl *ctrl = NULL;
> unsigned long flags;
> unsigned i;
>
> @@ -226,11 +206,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>
> 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) + sizeof(struct v4l2_kevent) * elems, GFP_KERNEL);
> if (!sev)
> @@ -243,9 +218,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> sev->fh = fh;
> sev->elems = elems;
> sev->ops = ops;
> - if (ctrl) {
> - sev->ops = &ctrl_ops;
> - }
>
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> @@ -267,10 +239,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> }
> }
>
> - /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> - if (ctrl)
> - v4l2_ctrl_add_event(ctrl, sev);
> -
> return 0;
> }
> EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> @@ -329,13 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> if (sev && sev->ops && sev->ops->del)
> sev->ops->del(sev);
>
> - if (sev && sev->type == V4L2_EVENT_CTRL) {
> - struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
> -
> - if (ctrl)
> - v4l2_ctrl_del_event(ctrl, sev);
> - }
> -
> kfree(sev);
>
> return 0;
> diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> index 74ebbad..ce1783d 100644
> --- a/drivers/media/video/vivi.c
> +++ b/drivers/media/video/vivi.c
> @@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
> {
> switch (sub->type) {
> case V4L2_EVENT_CTRL:
> - return v4l2_event_subscribe(fh, sub, 0, NULL);
> + return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
> default:
> return -EINVAL;
> }
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index eeb3df6..fde17ff 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -487,10 +487,9 @@ 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_event(struct v4l2_ctrl *ctrl,
> - struct v4l2_subscribed_event *sev);
> -void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
> - struct v4l2_subscribed_event *sev);
> +extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
> +void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
> +void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new);
>
> /* 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);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops
2011-10-30 11:17 ` Hans Verkuil
@ 2011-10-30 11:37 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2011-10-30 11:37 UTC (permalink / raw)
To: Hans de Goede; +Cc: Linux Media Mailing List, Laurent Pinchart
On Sunday, October 30, 2011 12:17:28 Hans Verkuil wrote:
> On Thursday, October 27, 2011 13:18:02 Hans de Goede wrote:
> > Just like with ctrl events, drivers may want to get called back on
> > listener add / remove for other event types too. Rather then special
> > casing all of this in subscribe / unsubscribe event it is better to
> > use ops for this.
> >
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Scratch this ack. I've found a race condition, see below...
>
> Regards,
>
> Hans
>
> > ---
> > drivers/media/video/ivtv/ivtv-ioctl.c | 2 +-
> > drivers/media/video/omap3isp/ispccdc.c | 2 +-
> > drivers/media/video/omap3isp/ispstat.c | 2 +-
> > drivers/media/video/pwc/pwc-v4l.c | 2 +-
> > drivers/media/video/v4l2-event.c | 42 ++++++++++++++++++++++++-------
> > drivers/media/video/vivi.c | 2 +-
> > include/media/v4l2-event.h | 24 +++++++++++++-----
> > 7 files changed, 54 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
> > index ecafa69..9aec8a0 100644
> > --- a/drivers/media/video/ivtv/ivtv-ioctl.c
> > +++ b/drivers/media/video/ivtv/ivtv-ioctl.c
> > @@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
> > case V4L2_EVENT_VSYNC:
> > case V4L2_EVENT_EOS:
> > case V4L2_EVENT_CTRL:
> > - return v4l2_event_subscribe(fh, sub, 0);
> > + return v4l2_event_subscribe(fh, sub, 0, NULL);
> > default:
> > return -EINVAL;
> > }
> > diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
> > index 40b141c..b6da736 100644
> > --- a/drivers/media/video/omap3isp/ispccdc.c
> > +++ b/drivers/media/video/omap3isp/ispccdc.c
> > @@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> > if (sub->id != 0)
> > return -EINVAL;
> >
> > - return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
> > + return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
> > }
> >
> > static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> > diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
> > index 8080659..4f337a2 100644
> > --- a/drivers/media/video/omap3isp/ispstat.c
> > +++ b/drivers/media/video/omap3isp/ispstat.c
> > @@ -1049,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, STAT_NEVENTS);
> > + return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
> > }
> >
> > int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
> > diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
> > index 68e1323..7f159bf 100644
> > --- a/drivers/media/video/pwc/pwc-v4l.c
> > +++ b/drivers/media/video/pwc/pwc-v4l.c
> > @@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
> > {
> > switch (sub->type) {
> > case V4L2_EVENT_CTRL:
> > - return v4l2_event_subscribe(fh, sub, 0);
> > + return v4l2_event_subscribe(fh, sub, 0, NULL);
> > default:
> > return -EINVAL;
> > }
> > diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> > index 3d27300..2dd9252 100644
> > --- a/drivers/media/video/v4l2-event.c
> > +++ b/drivers/media/video/v4l2-event.c
> > @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
> > sev->first = sev_pos(sev, 1);
> > fh->navailable--;
> > if (sev->elems == 1) {
> > - if (sev->replace) {
> > - sev->replace(&kev->event, ev);
> > + if (sev->ops && sev->ops->replace) {
> > + sev->ops->replace(&kev->event, ev);
> > copy_payload = false;
> > }
> > - } else if (sev->merge) {
> > + } else if (sev->ops && sev->ops->merge) {
> > struct v4l2_kevent *second_oldest =
> > sev->events + sev_pos(sev, 0);
> > - sev->merge(&kev->event, &second_oldest->event);
> > + sev->ops->merge(&kev->event, &second_oldest->event);
> > }
> > }
> >
> > @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
> > new->u.ctrl.changes |= old->u.ctrl.changes;
> > }
> >
> > +const struct v4l2_subscribed_event_ops ctrl_ops = {
> > + .replace = ctrls_replace,
> > + .merge = ctrls_merge,
> > +};
> > +
> > int v4l2_event_subscribe(struct v4l2_fh *fh,
> > - struct v4l2_event_subscription *sub, unsigned elems)
> > + struct v4l2_event_subscription *sub, unsigned elems,
> > + const struct v4l2_subscribed_event_ops *ops)
> > {
> > struct v4l2_subscribed_event *sev, *found_ev;
> > struct v4l2_ctrl *ctrl = NULL;
> > @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> > sev->flags = sub->flags;
> > sev->fh = fh;
> > sev->elems = elems;
> > + sev->ops = ops;
> > if (ctrl) {
> > - sev->replace = ctrls_replace;
> > - sev->merge = ctrls_merge;
> > + sev->ops = &ctrl_ops;
> > }
> >
> > spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> > @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> > list_add(&sev->list, &fh->subscribed);
> > spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >
> > - /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> > - if (found_ev)
> > + if (found_ev) {
> > kfree(sev);
> > - else if (ctrl)
> > + return 0; /* Already listening */
> > + }
> > +
> > + if (sev->ops && sev->ops->add) {
> > + int ret = sev->ops->add(sev);
> > + if (ret) {
> > + sev->ops = NULL;
> > + v4l2_event_unsubscribe(fh, sub);
> > + return ret;
> > + }
The problem here is that the event is basically available for use after the
spin_unlock_irqrestore(), but before the sev->ops->add() call. In the past I
just 'knew' that the event would never be generated by the control framework
until after v4l2_ctrl_add_event was called, but this should be formalized now
that these ops are added.
I see two options:
1) Have some method to mark the sev as being 'invalid' so functions sending
events can skip it (needed as well for your patch 4/6).
2) Document that drivers should never be able to send an event until after
the add() callback has succeeded.
I am leaning towards option 1 myself.
How about leaving sev->elems at 0 until after the add() op succeeds?
That's easy to test against and easy to implement.
Regards,
Hans
> > + }
> > +
> > + /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> > + if (ctrl)
> > v4l2_ctrl_add_event(ctrl, sev);
> >
> > return 0;
> > @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> > }
> >
> > spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> > +
> > + if (sev && sev->ops && sev->ops->del)
> > + sev->ops->del(sev);
> > +
> > if (sev && sev->type == V4L2_EVENT_CTRL) {
> > struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
> >
> > diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> > index c25787d..74ebbad 100644
> > --- a/drivers/media/video/vivi.c
> > +++ b/drivers/media/video/vivi.c
> > @@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
> > {
> > switch (sub->type) {
> > case V4L2_EVENT_CTRL:
> > - return v4l2_event_subscribe(fh, sub, 0);
> > + return v4l2_event_subscribe(fh, sub, 0, NULL);
> > default:
> > return -EINVAL;
> > }
> > diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> > index 5f14e88..88fa9a1 100644
> > --- a/include/media/v4l2-event.h
> > +++ b/include/media/v4l2-event.h
> > @@ -78,6 +78,19 @@ struct v4l2_kevent {
> > struct v4l2_event event;
> > };
> >
> > +/** struct v4l2_subscribed_event_ops - Subscribed event operations.
> > + * @add: Optional callback, called when a new listener is added
> > + * @del: Optional callback, called when a listener stops listening
> > + * @replace: Optional callback that can replace event 'old' with event 'new'.
> > + * @merge: Optional callback that can merge event 'old' into event 'new'.
> > + */
> > +struct v4l2_subscribed_event_ops {
> > + int (*add)(struct v4l2_subscribed_event *sev);
> > + void (*del)(struct v4l2_subscribed_event *sev);
> > + void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
> > + void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
> > +};
> > +
> > /** struct v4l2_subscribed_event - Internal struct representing a subscribed event.
> > * @list: List node for the v4l2_fh->subscribed list.
> > * @type: Event type.
> > @@ -85,8 +98,7 @@ struct v4l2_kevent {
> > * @flags: Copy of v4l2_event_subscription->flags.
> > * @fh: Filehandle that subscribed to this event.
> > * @node: List node that hooks into the object's event list (if there is one).
> > - * @replace: Optional callback that can replace event 'old' with event 'new'.
> > - * @merge: Optional callback that can merge event 'old' into event 'new'.
> > + * @ops: v4l2_subscribed_event_ops
> > * @elems: The number of elements in the events array.
> > * @first: The index of the events containing the oldest available event.
> > * @in_use: The number of queued events.
> > @@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
> > u32 flags;
> > struct v4l2_fh *fh;
> > struct list_head node;
> > - void (*replace)(struct v4l2_event *old,
> > - const struct v4l2_event *new);
> > - void (*merge)(const struct v4l2_event *old,
> > - struct v4l2_event *new);
> > + const struct v4l2_subscribed_event_ops *ops;
> > unsigned elems;
> > unsigned first;
> > unsigned in_use;
> > @@ -115,7 +124,8 @@ 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, unsigned elems);
> > + struct v4l2_event_subscription *sub, unsigned elems,
> > + const struct v4l2_subscribed_event_ops *ops);
> > int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> > struct v4l2_event_subscription *sub);
> > void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops
2011-10-31 15:16 Various ctrl and event frame work patches (version 2) Hans de Goede
@ 2011-10-31 15:16 ` Hans de Goede
0 siblings, 0 replies; 23+ messages in thread
From: Hans de Goede @ 2011-10-31 15:16 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: hverkuil, Laurent Pinchart, Hans de Goede
Just like with ctrl events, drivers may want to get called back on
listener add / remove for other event types too. Rather then special
casing all of this in subscribe / unsubscribe event it is better to
use ops for this.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Documentation/video4linux/v4l2-framework.txt | 28 +++++++++++++----
drivers/media/video/ivtv/ivtv-ioctl.c | 2 +-
drivers/media/video/omap3isp/ispccdc.c | 2 +-
drivers/media/video/omap3isp/ispstat.c | 2 +-
drivers/media/video/pwc/pwc-v4l.c | 2 +-
drivers/media/video/v4l2-event.c | 42 +++++++++++++++++++------
drivers/media/video/vivi.c | 2 +-
include/media/v4l2-event.h | 24 ++++++++++----
8 files changed, 75 insertions(+), 29 deletions(-)
diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index f8dcabf..16eb8af 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -930,21 +930,35 @@ fast.
Useful functions:
-- v4l2_event_queue()
+void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
Queue events to video device. The driver's only responsibility is to fill
in the type and the data fields. The other fields will be filled in by
V4L2.
-- v4l2_event_subscribe()
+int v4l2_event_subscribe(struct v4l2_fh *fh,
+ struct v4l2_event_subscription *sub, unsigned elems,
+ const struct v4l2_subscribed_event_ops *ops)
The video_device->ioctl_ops->vidioc_subscribe_event must check the driver
is able to produce events with specified event id. Then it calls
- v4l2_event_subscribe() to subscribe the event. The last argument is the
- size of the event queue for this event. If it is 0, then the framework
- will fill in a default value (this depends on the event type).
+ v4l2_event_subscribe() to subscribe the event.
-- v4l2_event_unsubscribe()
+ The elems argument is the size of the event queue for this event. If it is 0,
+ then the framework will fill in a default value (this depends on the event
+ type).
+
+ The ops argument allows the driver to specify a number of callbacks:
+ * add: called when a new listener gets added (subscribing to the same
+ event twice will only cause this callback to get called once)
+ * del: called when a listener stops listening
+ * replace: replace event 'old' with event 'new'.
+ * merge: merge event 'old' into event 'new'.
+ All 4 callbacks are optional, if you don't want to specify any callbacks
+ the ops argument itself maybe NULL.
+
+int v4l2_event_unsubscribe(struct v4l2_fh *fh,
+ struct v4l2_event_subscription *sub)
vidioc_unsubscribe_event in struct v4l2_ioctl_ops. A driver may use
v4l2_event_unsubscribe() directly unless it wants to be involved in
@@ -953,7 +967,7 @@ Useful functions:
The special type V4L2_EVENT_ALL may be used to unsubscribe all events. The
drivers may want to handle this in a special way.
-- v4l2_event_pending()
+int v4l2_event_pending(struct v4l2_fh *fh)
Returns the number of pending events. Useful when implementing poll.
diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index ecafa69..9aec8a0 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
case V4L2_EVENT_VSYNC:
case V4L2_EVENT_EOS:
case V4L2_EVENT_CTRL:
- return v4l2_event_subscribe(fh, sub, 0);
+ return v4l2_event_subscribe(fh, sub, 0, NULL);
default:
return -EINVAL;
}
diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
index 40b141c..b6da736 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
if (sub->id != 0)
return -EINVAL;
- return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
+ return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
}
static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
index 8080659..4f337a2 100644
--- a/drivers/media/video/omap3isp/ispstat.c
+++ b/drivers/media/video/omap3isp/ispstat.c
@@ -1049,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, STAT_NEVENTS);
+ return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
}
int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 68e1323..7f159bf 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
{
switch (sub->type) {
case V4L2_EVENT_CTRL:
- return v4l2_event_subscribe(fh, sub, 0);
+ return v4l2_event_subscribe(fh, sub, 0, NULL);
default:
return -EINVAL;
}
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 3d27300..c56dc35 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
sev->first = sev_pos(sev, 1);
fh->navailable--;
if (sev->elems == 1) {
- if (sev->replace) {
- sev->replace(&kev->event, ev);
+ if (sev->ops && sev->ops->replace) {
+ sev->ops->replace(&kev->event, ev);
copy_payload = false;
}
- } else if (sev->merge) {
+ } else if (sev->ops && sev->ops->merge) {
struct v4l2_kevent *second_oldest =
sev->events + sev_pos(sev, 0);
- sev->merge(&kev->event, &second_oldest->event);
+ sev->ops->merge(&kev->event, &second_oldest->event);
}
}
@@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
new->u.ctrl.changes |= old->u.ctrl.changes;
}
+static const struct v4l2_subscribed_event_ops ctrl_ops = {
+ .replace = ctrls_replace,
+ .merge = ctrls_merge,
+};
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
- struct v4l2_event_subscription *sub, unsigned elems)
+ struct v4l2_event_subscription *sub, unsigned elems,
+ const struct v4l2_subscribed_event_ops *ops)
{
struct v4l2_subscribed_event *sev, *found_ev;
struct v4l2_ctrl *ctrl = NULL;
@@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
sev->flags = sub->flags;
sev->fh = fh;
sev->elems = elems;
+ sev->ops = ops;
if (ctrl) {
- sev->replace = ctrls_replace;
- sev->merge = ctrls_merge;
+ sev->ops = &ctrl_ops;
}
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
@@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
list_add(&sev->list, &fh->subscribed);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
- /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
- if (found_ev)
+ if (found_ev) {
kfree(sev);
- else if (ctrl)
+ return 0; /* Already listening */
+ }
+
+ if (sev->ops && sev->ops->add) {
+ int ret = sev->ops->add(sev);
+ if (ret) {
+ sev->ops = NULL;
+ v4l2_event_unsubscribe(fh, sub);
+ return ret;
+ }
+ }
+
+ /* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
+ if (ctrl)
v4l2_ctrl_add_event(ctrl, sev);
return 0;
@@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
}
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+ if (sev && sev->ops && sev->ops->del)
+ sev->ops->del(sev);
+
if (sev && sev->type == V4L2_EVENT_CTRL) {
struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index c25787d..74ebbad 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
{
switch (sub->type) {
case V4L2_EVENT_CTRL:
- return v4l2_event_subscribe(fh, sub, 0);
+ return v4l2_event_subscribe(fh, sub, 0, NULL);
default:
return -EINVAL;
}
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 5f14e88..88fa9a1 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -78,6 +78,19 @@ struct v4l2_kevent {
struct v4l2_event event;
};
+/** struct v4l2_subscribed_event_ops - Subscribed event operations.
+ * @add: Optional callback, called when a new listener is added
+ * @del: Optional callback, called when a listener stops listening
+ * @replace: Optional callback that can replace event 'old' with event 'new'.
+ * @merge: Optional callback that can merge event 'old' into event 'new'.
+ */
+struct v4l2_subscribed_event_ops {
+ int (*add)(struct v4l2_subscribed_event *sev);
+ void (*del)(struct v4l2_subscribed_event *sev);
+ void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
+ void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
+};
+
/** struct v4l2_subscribed_event - Internal struct representing a subscribed event.
* @list: List node for the v4l2_fh->subscribed list.
* @type: Event type.
@@ -85,8 +98,7 @@ struct v4l2_kevent {
* @flags: Copy of v4l2_event_subscription->flags.
* @fh: Filehandle that subscribed to this event.
* @node: List node that hooks into the object's event list (if there is one).
- * @replace: Optional callback that can replace event 'old' with event 'new'.
- * @merge: Optional callback that can merge event 'old' into event 'new'.
+ * @ops: v4l2_subscribed_event_ops
* @elems: The number of elements in the events array.
* @first: The index of the events containing the oldest available event.
* @in_use: The number of queued events.
@@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
u32 flags;
struct v4l2_fh *fh;
struct list_head node;
- void (*replace)(struct v4l2_event *old,
- const struct v4l2_event *new);
- void (*merge)(const struct v4l2_event *old,
- struct v4l2_event *new);
+ const struct v4l2_subscribed_event_ops *ops;
unsigned elems;
unsigned first;
unsigned in_use;
@@ -115,7 +124,8 @@ 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, unsigned elems);
+ struct v4l2_event_subscription *sub, unsigned elems,
+ const struct v4l2_subscribed_event_ops *ops);
int v4l2_event_unsubscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub);
void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
--
1.7.7
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops
@ 2011-11-02 10:04 Hans de Goede
0 siblings, 0 replies; 23+ messages in thread
From: Hans de Goede @ 2011-11-02 10:04 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List
Hi,
hverkuil wrote:
> > > + if (sev->ops && sev->ops->add) {
> > > + int ret = sev->ops->add(sev);
> > > + if (ret) {
> > > + sev->ops = NULL;
> > > + v4l2_event_unsubscribe(fh, sub);
> > > + return ret;
> > > + }
>
> The problem here is that the event is basically available for use after the
> spin_unlock_irqrestore(), but before the sev->ops->add() call. In the past I
> just 'knew' that the event would never be generated by the control framework
> until after v4l2_ctrl_add_event was called, but this should be formalized now
> that these ops are added.
>
> I see two options:
>
> 1) Have some method to mark the sev as being 'invalid' so functions sending
> events can skip it (needed as well for your patch 4/6).
>
> 2) Document that drivers should never be able to send an event until after
> the add() callback has succeeded.
>
> I am leaning towards option 1 myself.
>
> How about leaving sev->elems at 0 until after the add() op succeeds?
>
> That's easy to test against and easy to implement.
I agree that option 1 is the best and that leaving sev->elems 0 is a good
way to do this. This will be in my next revision of this patch set.
Thanks & Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2011-11-02 10:03 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-27 11:17 Various ctrl and event frame work patches Hans de Goede
2011-10-27 11:17 ` [PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls Hans de Goede
2011-10-30 10:16 ` Hans Verkuil
2011-10-27 11:17 ` [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL Hans de Goede
2011-10-27 12:04 ` Laurent Pinchart
2011-10-30 10:17 ` Hans Verkuil
2011-10-27 11:18 ` [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing Hans de Goede
2011-10-27 12:07 ` Laurent Pinchart
2011-10-30 10:24 ` Hans Verkuil
2011-10-27 11:18 ` [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe Hans de Goede
2011-10-27 12:20 ` Laurent Pinchart
2011-10-28 8:35 ` Hans de Goede
2011-10-28 11:48 ` Laurent Pinchart
2011-10-30 10:30 ` Hans Verkuil
2011-10-27 11:18 ` [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede
2011-10-27 12:30 ` Laurent Pinchart
2011-10-28 8:37 ` Hans de Goede
2011-10-30 11:17 ` Hans Verkuil
2011-10-30 11:37 ` Hans Verkuil
2011-10-27 11:18 ` [PATCH 6/6] v4l2-ctrls: Use v4l2_subscribed_event_ops Hans de Goede
2011-10-30 11:24 ` Hans Verkuil
-- strict thread matches above, loose matches on Subject: below --
2011-10-31 15:16 Various ctrl and event frame work patches (version 2) Hans de Goede
2011-10-31 15:16 ` [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede
2011-11-02 10:04 Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).