* Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops
@ 2011-11-02 10:04 Hans de Goede
0 siblings, 0 replies; 7+ 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] 7+ messages in thread* Various ctrl and event frame work patches (version 2) @ 2011-10-31 15:16 Hans de Goede 2011-10-31 15:16 ` [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede 0 siblings, 1 reply; 7+ messages in thread From: Hans de Goede @ 2011-10-31 15:16 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. Changes since version 1: -Added a documentation update (update v4l2-framework.txt) to: "v4l2-event: Add v4l2_subscribed_event_ops" Regards, Hans ^ permalink raw reply [flat|nested] 7+ 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; 7+ 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] 7+ messages in thread
* Various ctrl and event frame work patches @ 2011-10-27 11:17 Hans de Goede 2011-10-27 11:18 ` [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede 0 siblings, 1 reply; 7+ 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] 7+ 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 @ 2011-10-27 11:18 ` Hans de Goede 2011-10-27 12:30 ` Laurent Pinchart 2011-10-30 11:17 ` Hans Verkuil 0 siblings, 2 replies; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2011-11-02 10:03 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-02 10:04 [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops Hans de Goede -- 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-10-27 11:17 Various ctrl and event frame work patches Hans de Goede 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
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.