> Hi Hans, > > On Saturday 13 February 2010 14:19:55 Hans Verkuil wrote: >> On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote: >> > This patch adds a set of new ioctls to the V4L2 API. The ioctls >> conform >> > to V4L2 Events RFC version 2.3: >> I've experimented with the events API to try and support it with ivtv >> and >> I realized that it had some problems. >> >> See comments below. >> >> > <URL:http://www.spinics.net/lists/linux-media/msg12033.html> >> > >> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx> >> > --- >> > >> > drivers/media/video/v4l2-compat-ioctl32.c | 3 +++ >> > drivers/media/video/v4l2-ioctl.c | 3 +++ >> > include/linux/videodev2.h | 23 >> +++++++++++++++++++++++ >> > 3 files changed, 29 insertions(+), 0 deletions(-) >> > >> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c >> > b/drivers/media/video/v4l2-compat-ioctl32.c index 997975d..cba704c >> > 100644 >> > --- a/drivers/media/video/v4l2-compat-ioctl32.c >> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c >> > @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file, >> > unsigned int cmd, unsigned long arg) >> > >> > case VIDIOC_DBG_G_REGISTER: >> > case VIDIOC_DBG_G_CHIP_IDENT: >> > >> > case VIDIOC_S_HW_FREQ_SEEK: >> > + case VIDIOC_DQEVENT: >> > + case VIDIOC_SUBSCRIBE_EVENT: >> > >> > + case VIDIOC_UNSUBSCRIBE_EVENT: >> > ret = do_video_ioctl(file, cmd, arg); >> > break; >> > >> > diff --git a/drivers/media/video/v4l2-ioctl.c >> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..bfc4696 100644 >> > --- a/drivers/media/video/v4l2-ioctl.c >> > +++ b/drivers/media/video/v4l2-ioctl.c >> > @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = { >> > >> > [_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT", >> > [_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)] = "VIDIOC_S_HW_FREQ_SEEK", >> > >> > + [_IOC_NR(VIDIOC_DQEVENT)] = "VIDIOC_DQEVENT", >> > + [_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = "VIDIOC_SUBSCRIBE_EVENT", >> > + [_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT", >> > >> > #endif >> > }; >> > #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls) >> > >> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h >> > index 54af357..a19ae89 100644 >> > --- a/include/linux/videodev2.h >> > +++ b/include/linux/videodev2.h >> > @@ -1536,6 +1536,26 @@ struct v4l2_streamparm { >> > >> > }; >> > >> > /* >> > >> > + * E V E N T S >> > + */ >> > + >> > +struct v4l2_event { >> > + __u32 count; >> >> The name 'count' is confusing. Count of what? I think the name 'pending' >> might be more understandable. A comment after the definition would also >> help. >> >> > + __u32 type; >> > + __u32 sequence; >> > + struct timespec timestamp; >> > + __u32 reserved[9]; >> > + __u8 data[64]; >> > +}; >> >> I also think we should reorder the fields and add a union. For ivtv I >> would >> need this: >> >> #define V4L2_EVENT_ALL 0 >> #define V4L2_EVENT_VSYNC 1 >> #define V4L2_EVENT_EOS 2 >> #define V4L2_EVENT_PRIVATE_START 0x08000000 >> >> /* Payload for V4L2_EVENT_VSYNC */ >> struct v4l2_event_vsync { >> /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */ >> u8 field; >> } __attribute__ ((packed)); >> >> struct v4l2_event { >> __u32 type; >> union { >> struct v4l2_event_vsync vsync; >> __u8 data[64]; >> } u; >> __u32 sequence; >> struct timespec timestamp; >> __u32 pending; >> __u32 reserved[9]; >> }; >> >> The reason for rearranging the fields has to do with the fact that the >> first two fields (type and the union) form the actual event data. The >> others are more for administrative purposes. Separating those two makes >> sense to me. >> >> So when I define an event for queuing it is nice if I can do just this: >> >> static const struct v4l2_event ev_top = { >> .type = V4L2_EVENT_VSYNC, >> .u.vsync.field = V4L2_FIELD_TOP, >> }; >> >> I would have preferred to have an anonymous union. Unfortunately gcc has >> problems with initializers for fields inside an anonymous union. Hence >> the >> need for a named union. > > Will all drivers add private events to the union ? This would then soon > become > a mess. Wouldn't it be better for drivers to define their own event > structures > (standard ones could be shared between drivers in videodev2.h) and cast > the > pointer to data to a pointer to the appropriate event structure ? I would prefer to have the actual event type defines in videodev2.h (just as we do for private control IDs). The actual payload structure can be defined elsewhere as far as I am concerned. An alternative in the long run might be to split off the event structs into a separate public header. I have been thinking along those lines as well for the controls. videodev2.h is getting pretty huge and it might be more managable if it is split up in multiple parts with videodev2.h including those parts. Regards, Hans > > -- > Regards, > > Laurent Pinchart > -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
