Message ID | 1265813889-17847-7-git-send-email-sakari.ailus@maxwell.research.nokia.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Wed, 10 Feb 2010 14:58:50 +0000 Received: from bombadil.infradead.org [18.85.46.34] by pedra with IMAP (fetchmail-6.3.6) for <mchehab@localhost> (single-drop); Wed, 10 Feb 2010 13:00:05 -0200 (BRST) Received: from vger.kernel.org ([209.132.180.67]) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1NfE1h-0000AP-QQ; Wed, 10 Feb 2010 14:58:50 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754515Ab0BJO6t (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Wed, 10 Feb 2010 09:58:49 -0500 Received: from smtp.nokia.com ([192.100.122.230]:43130 "EHLO mgw-mx03.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754424Ab0BJO6s (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 10 Feb 2010 09:58:48 -0500 Received: from esebh105.NOE.Nokia.com (esebh105.ntc.nokia.com [172.21.138.211]) by mgw-mx03.nokia.com (Switch-3.3.3/Switch-3.3.3) with ESMTP id o1AEwIuD030315; Wed, 10 Feb 2010 16:58:45 +0200 Received: from vaebh104.NOE.Nokia.com ([10.160.244.30]) by esebh105.NOE.Nokia.com with Microsoft SMTPSVC(6.0.3790.3959); Wed, 10 Feb 2010 16:58:16 +0200 Received: from mgw-sa01.ext.nokia.com ([147.243.1.47]) by vaebh104.NOE.Nokia.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.3959); Wed, 10 Feb 2010 16:58:16 +0200 Received: from maxwell.research.nokia.com (maxwell.research.nokia.com [172.21.50.162]) by mgw-sa01.ext.nokia.com (Switch-3.3.3/Switch-3.3.3) with ESMTP id o1AEwE4P014969 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 10 Feb 2010 16:58:15 +0200 Received: from lanttu (unknown [192.168.239.74]) by maxwell.research.nokia.com (Postfix) with ESMTPS id 9E0B2700C3; Wed, 10 Feb 2010 16:58:14 +0200 (EET) Received: from sakke by lanttu with local (Exim 4.69) (envelope-from <sakari.ailus@maxwell.research.nokia.com>) id 1NfE16-0004hD-Nn; Wed, 10 Feb 2010 16:58:12 +0200 From: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> To: linux-media@vger.kernel.org Cc: hverkuil@xs4all.nl, laurent.pinchart@ideasonboard.com, iivanov@mm-sol.com, gururaj.nagendra@intel.com, david.cohen@nokia.com Subject: [PATCH v4 7/7] V4L: Events: Support all events Date: Wed, 10 Feb 2010 16:58:09 +0200 Message-Id: <1265813889-17847-7-git-send-email-sakari.ailus@maxwell.research.nokia.com> X-Mailer: git-send-email 1.5.6.5 In-Reply-To: <1265813889-17847-6-git-send-email-sakari.ailus@maxwell.research.nokia.com> References: <4B72C965.7040204@maxwell.research.nokia.com> <1265813889-17847-1-git-send-email-sakari.ailus@maxwell.research.nokia.com> <1265813889-17847-2-git-send-email-sakari.ailus@maxwell.research.nokia.com> <1265813889-17847-3-git-send-email-sakari.ailus@maxwell.research.nokia.com> <1265813889-17847-4-git-send-email-sakari.ailus@maxwell.research.nokia.com> <1265813889-17847-5-git-send-email-sakari.ailus@maxwell.research.nokia.com> <1265813889-17847-6-git-send-email-sakari.ailus@maxwell.research.nokia.com> X-OriginalArrivalTime: 10 Feb 2010 14:58:16.0351 (UTC) FILETIME=[79AE82F0:01CAAA61] X-Nokia-AV: Clean Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Sakari Ailus
Feb. 10, 2010, 2:58 p.m. UTC
Add support for subscribing all events with a special id V4L2_EVENT_ALL. If
V4L2_EVENT_ALL is subscribed, no other events may be subscribed. Otherwise
V4L2_EVENT_ALL is considered just as any other event.
Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
drivers/media/video/v4l2-event.c | 13 ++++++++++++-
include/linux/videodev2.h | 1 +
2 files changed, 13 insertions(+), 1 deletions(-)
Comments
On Wednesday 10 February 2010 15:58:09 Sakari Ailus wrote: > Add support for subscribing all events with a special id V4L2_EVENT_ALL. If > V4L2_EVENT_ALL is subscribed, no other events may be subscribed. Otherwise > V4L2_EVENT_ALL is considered just as any other event. We should do this differently. I think that EVENT_ALL should not be used internally (i.e. in the actual list of subscribed events), but just as a special value for the subscribe and unsubscribe ioctls. So when used with unsubscribe you can just unsubscribe all subscribed events and when used with subscribe, then you just subscribe all valid events (valid for that device node). So in v4l2-event.c you will have a v4l2_event_unsubscribe_all() to quickly unsubscribe all events. In order to easily add all events from the driver it would help if the v4l2_event_subscribe and v4l2_event_unsubscribe just take the event type as argument rather than the whole v4l2_event_subscription struct. You will then get something like this in the driver: if (sub->type == V4L2_EVENT_ALL) { int ret = v4l2_event_alloc(fh, 60); ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_EOS); ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_VSYNC); return ret; } An alternative might be to add a v4l2_event_subscribe_all(fh, const u32 *events) where 'events' is a 0 terminated list of events that need to be subscribed. For each event this function would then call: fh->vdev->ioctl_ops->vidioc_subscribe_event(fh, sub); The nice thing about that is that in the driver you have a minimum of fuss. I'm leaning towards this second solution due to the simple driver implementation. Handling EVENT_ALL will simplify things substantially IMHO. Regards, Hans > > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> > --- > drivers/media/video/v4l2-event.c | 13 ++++++++++++- > include/linux/videodev2.h | 1 + > 2 files changed, 13 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c > index 0af0de5..68b3cf4 100644 > --- a/drivers/media/video/v4l2-event.c > +++ b/drivers/media/video/v4l2-event.c > @@ -139,6 +139,14 @@ static struct v4l2_subscribed_event *__v4l2_event_subscribed( > struct v4l2_events *events = fh->events; > struct v4l2_subscribed_event *sev; > > + if (list_empty(&events->subscribed)) > + return NULL; > + > + sev = list_entry(events->subscribed.next, > + struct v4l2_subscribed_event, list); > + if (sev->type == V4L2_EVENT_ALL) > + return sev; > + > list_for_each_entry(sev, &events->subscribed, list) { > if (sev->type == type) > return sev; > @@ -222,6 +230,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh, > /* Allow subscribing to valid events only. */ > if (sub->type < V4L2_EVENT_PRIVATE_START) > switch (sub->type) { > + case V4L2_EVENT_ALL: > + break; > default: > return -EINVAL; > } > @@ -262,7 +272,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, > > sev = __v4l2_event_subscribed(fh, sub->type); > > - if (sev == NULL) { > + if (sev == NULL || > + (sub->type != V4L2_EVENT_ALL && sev->type == V4L2_EVENT_ALL)) { > spin_unlock_irqrestore(&fh->vdev->fh_lock, flags); > return -EINVAL; > } > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h > index a19ae89..9ae9a1c 100644 > --- a/include/linux/videodev2.h > +++ b/include/linux/videodev2.h > @@ -1553,6 +1553,7 @@ struct v4l2_event_subscription { > __u32 reserved[7]; > }; > > +#define V4L2_EVENT_ALL 0 > #define V4L2_EVENT_PRIVATE_START 0x08000000 > > /* >
Hi Hans, On Saturday 13 February 2010 15:42:20 Hans Verkuil wrote: > On Wednesday 10 February 2010 15:58:09 Sakari Ailus wrote: > > Add support for subscribing all events with a special id V4L2_EVENT_ALL. > > If V4L2_EVENT_ALL is subscribed, no other events may be subscribed. > > Otherwise V4L2_EVENT_ALL is considered just as any other event. > > We should do this differently. I think that EVENT_ALL should not be used > internally (i.e. in the actual list of subscribed events), but just as a > special value for the subscribe and unsubscribe ioctls. So when used with > unsubscribe you can just unsubscribe all subscribed events and when used > with subscribe, then you just subscribe all valid events (valid for that > device node). > > So in v4l2-event.c you will have a v4l2_event_unsubscribe_all() to quickly > unsubscribe all events. > > In order to easily add all events from the driver it would help if the > v4l2_event_subscribe and v4l2_event_unsubscribe just take the event type > as argument rather than the whole v4l2_event_subscription struct. > > You will then get something like this in the driver: > > if (sub->type == V4L2_EVENT_ALL) { > int ret = v4l2_event_alloc(fh, 60); > > ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_EOS); > ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_VSYNC); > return ret; > } > > An alternative might be to add a v4l2_event_subscribe_all(fh, const u32 > *events) where 'events' is a 0 terminated list of events that need to be > subscribed. Then don't call it v4l2_event_subscribe_all if it only subscribes to a set of event :-) > For each event this function would then call: > > fh->vdev->ioctl_ops->vidioc_subscribe_event(fh, sub); > > The nice thing about that is that in the driver you have a minimum of fuss. > > I'm leaning towards this second solution due to the simple driver > implementation. > > Handling EVENT_ALL will simplify things substantially IMHO. I'm wondering if subscribing to all events should be allowed. Do we have use cases for that ? I'm always a bit cautious when adding APIs with no users, as that means the API has often not been properly tested against possible use cases and mistakes will need to be supported forever (or at least for a long time).
> Hi Hans, > > On Saturday 13 February 2010 15:42:20 Hans Verkuil wrote: >> On Wednesday 10 February 2010 15:58:09 Sakari Ailus wrote: >> > Add support for subscribing all events with a special id >> V4L2_EVENT_ALL. >> > If V4L2_EVENT_ALL is subscribed, no other events may be subscribed. >> > Otherwise V4L2_EVENT_ALL is considered just as any other event. >> >> We should do this differently. I think that EVENT_ALL should not be used >> internally (i.e. in the actual list of subscribed events), but just as a >> special value for the subscribe and unsubscribe ioctls. So when used >> with >> unsubscribe you can just unsubscribe all subscribed events and when used >> with subscribe, then you just subscribe all valid events (valid for that >> device node). >> >> So in v4l2-event.c you will have a v4l2_event_unsubscribe_all() to >> quickly >> unsubscribe all events. >> >> In order to easily add all events from the driver it would help if the >> v4l2_event_subscribe and v4l2_event_unsubscribe just take the event type >> as argument rather than the whole v4l2_event_subscription struct. >> >> You will then get something like this in the driver: >> >> if (sub->type == V4L2_EVENT_ALL) { >> int ret = v4l2_event_alloc(fh, 60); >> >> ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_EOS); >> ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_VSYNC); >> return ret; >> } >> >> An alternative might be to add a v4l2_event_subscribe_all(fh, const u32 >> *events) where 'events' is a 0 terminated list of events that need to be >> subscribed. > > Then don't call it v4l2_event_subscribe_all if it only subscribes to a set > of > event :-) > >> For each event this function would then call: >> >> fh->vdev->ioctl_ops->vidioc_subscribe_event(fh, sub); >> >> The nice thing about that is that in the driver you have a minimum of >> fuss. >> >> I'm leaning towards this second solution due to the simple driver >> implementation. >> >> Handling EVENT_ALL will simplify things substantially IMHO. > > I'm wondering if subscribing to all events should be allowed. Do we have > use > cases for that ? I'm always a bit cautious when adding APIs with no users, > as > that means the API has often not been properly tested against possible use > cases and mistakes will need to be supported forever (or at least for a > long > time). I think that is a good point. Supporting V4L2_EVENT_ALL makes sense for unsubscribe, but does it makes sense for subscribe as well? I think it does not. It just doesn't feel right when I tried to implement it in ivtv. I also wonder whether the unsubscribe API shouldn't just receive the event type instead of the big subscription struct. Or get its own struct. I don't think it makes much sense that they both have the same struct. Regards, Hans > > -- > Regards, > > Laurent Pinchart > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hans Verkuil wrote: >> Then don't call it v4l2_event_subscribe_all if it only subscribes to a set >> of >> event :-) >> >>> For each event this function would then call: >>> >>> fh->vdev->ioctl_ops->vidioc_subscribe_event(fh, sub); >>> >>> The nice thing about that is that in the driver you have a minimum of >>> fuss. >>> >>> I'm leaning towards this second solution due to the simple driver >>> implementation. >>> >>> Handling EVENT_ALL will simplify things substantially IMHO. >> >> I'm wondering if subscribing to all events should be allowed. Do we have >> use >> cases for that ? I'm always a bit cautious when adding APIs with no users, >> as >> that means the API has often not been properly tested against possible use >> cases and mistakes will need to be supported forever (or at least for a >> long >> time). > > I think that is a good point. Supporting V4L2_EVENT_ALL makes sense for > unsubscribe, but does it makes sense for subscribe as well? I think it > does not. It just doesn't feel right when I tried to implement it in ivtv. I don't see any harm in supporting it there. We could also specify that drivers may support that. At least for testing purposes that could be quite useful. :-) Perhaps not for regular use, though. > I also wonder whether the unsubscribe API shouldn't just receive the event > type instead of the big subscription struct. Or get its own struct. I > don't think it makes much sense that they both have the same struct. So for unsubscribing the argument would be just event type as __u32? I don't see harm in having the struct there. There might be flags in future, perhaps telling that events of that type should be cleaned up from the event queue, for example. (I can't think of any other purposes now. :)) Cheers,
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c index 0af0de5..68b3cf4 100644 --- a/drivers/media/video/v4l2-event.c +++ b/drivers/media/video/v4l2-event.c @@ -139,6 +139,14 @@ static struct v4l2_subscribed_event *__v4l2_event_subscribed( struct v4l2_events *events = fh->events; struct v4l2_subscribed_event *sev; + if (list_empty(&events->subscribed)) + return NULL; + + sev = list_entry(events->subscribed.next, + struct v4l2_subscribed_event, list); + if (sev->type == V4L2_EVENT_ALL) + return sev; + list_for_each_entry(sev, &events->subscribed, list) { if (sev->type == type) return sev; @@ -222,6 +230,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh, /* Allow subscribing to valid events only. */ if (sub->type < V4L2_EVENT_PRIVATE_START) switch (sub->type) { + case V4L2_EVENT_ALL: + break; default: return -EINVAL; } @@ -262,7 +272,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, sev = __v4l2_event_subscribed(fh, sub->type); - if (sev == NULL) { + if (sev == NULL || + (sub->type != V4L2_EVENT_ALL && sev->type == V4L2_EVENT_ALL)) { spin_unlock_irqrestore(&fh->vdev->fh_lock, flags); return -EINVAL; } diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index a19ae89..9ae9a1c 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -1553,6 +1553,7 @@ struct v4l2_event_subscription { __u32 reserved[7]; }; +#define V4L2_EVENT_ALL 0 #define V4L2_EVENT_PRIVATE_START 0x08000000 /*