Hi Hans,
On Monday 22 February 2010 08:53:53 Hans Verkuil wrote:
> On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote:
> > Hans Verkuil wrote:
> > > More comments...
> > >
> > > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote:
> > >> Add support for event handling to do_ioctl.
> > >>
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> > >> ---
> > >>
> > >> drivers/media/video/v4l2-ioctl.c | 58
> > >> ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h
> > >> | 7 ++++
> > >> 2 files changed, 65 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/drivers/media/video/v4l2-ioctl.c
> > >> b/drivers/media/video/v4l2-ioctl.c index 34c7d6e..f7d6177 100644
> > >> --- a/drivers/media/video/v4l2-ioctl.c
> > >> +++ b/drivers/media/video/v4l2-ioctl.c
> > >> @@ -25,6 +25,8 @@
> > >>
> > >> #endif
> > >> #include <media/v4l2-common.h>
> > >> #include <media/v4l2-ioctl.h>
> > >>
> > >> +#include <media/v4l2-fh.h>
> > >> +#include <media/v4l2-event.h>
> > >>
> > >> #include <media/v4l2-chip-ident.h>
> > >>
> > >> #define dbgarg(cmd, fmt, arg...) \
> > >>
> > >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file,
> > >>
> > >> }
> > >> break;
> > >>
> > >> }
> > >>
> > >> + case VIDIOC_DQEVENT:
> > >> + {
> > >> + struct v4l2_event *ev = arg;
> > >> + struct v4l2_fh *vfh = fh;
> > >> +
> > >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> > >> + || vfh->events == NULL)
> > >> + break;
> > >
> > > Change this to:
> > > if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> > >
> > > break;
> > >
> > > if (vfh->events == NULL)
> > >
> > > return -ENOENT;
> > >
> > > But see also the next comment.
> > >
> > >> +
> > >> + ret = v4l2_event_dequeue(fh, ev);
> > >
> > > There is a crucial piece of functionality missing here: if the
> > > filehandle is in blocking mode, then it should wait until an event
> > > arrives. That also means that if vfh->events == NULL, you should still
> > > call v4l2_event_dequeue, and that function should initialize
> > > vfh->events and wait for an event if the fh is in blocking mode.
> >
> > I originally left this out intentionally. Most applications using events
> > would use select / poll as well by default. For completeness it should
> > be there, I agree.
>
> It has to be there. This is important functionality. For e.g. ivtv I would
> use this to wait until the MPEG decoder flushed all buffers and displayed
> the last frame of the stream. That's something you would often do in
> blocking mode.
Blocking mode can easily be emulated using select().
> > This btw. suggests that we perhaps should put back the struct file
> > argument for the event functions in video_ioctl_ops. The blocking flag
> > is indeed part of the file structure. I'm open to better suggestions,
> > too.
>
> My long term goal is that the file struct is only used inside v4l2-ioctl.c
> and not in drivers. Drivers should not need this struct at all. The easiest
> way to ensure this is by not passing it to the drivers at all :-)
Drivers still need a way to access the blocking flag. The interim solution of
adding a file * member to v4l2_fh would allow that, while still removing most
usage of file * from drivers.
> > >> + if (ret < 0) {
> > >> + dbgarg(cmd, "no pending events?");
> > >> + break;
> > >> + }
> > >> + dbgarg(cmd,
> > >> + "pending=%d, type=0x%8.8x, sequence=%d, "
> > >> + "timestamp=%lu.%9.9lu ",
> > >> + ev->pending, ev->type, ev->sequence,
> > >> + ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
> > >> + break;
> > >> + }
> > >> + case VIDIOC_SUBSCRIBE_EVENT:
> > >> + {
> > >> + struct v4l2_event_subscription *sub = arg;
> > >> + struct v4l2_fh *vfh = fh;
> > >>
> > >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> > >
> > > Testing for this bit is unnecessarily. Just test for
> > > ops->vidioc_subscribe_event.
> > >
> > >> + || vfh->events == NULL
> > >
> > > Remove this test. If you allocate the event queue only when you first
> > > subscribe to an event (as ivtv will do), then you have to be able to
> > > call vidioc_subscribe_event even if vfh->events == NULL.
> >
> > How about calling v4l2_event_alloc() with zero events? That allocates
> > and initialises the v4l2_events structure. That's easier to handle in
> > drivers as well since they don't need to consider special cases like
> > fh->events happens to be NULL even if events are supported by the
> > driver. This is how I first thought it'd work.
>
> Proposal: export a v4l2_event_init() call that sets up fh->events. Calling
> v4l2_event_alloc(0) feels like a hack. So drivers that want to be able to
> handle events should call v4l2_event_init after initializing the file
> handle.
>
> Or (and that might even be nicer) test in v4l2_fh_init whether there is a
> subscribe op in the ioctl_ops struct and let v4l2_fh_init set up fh->events
> automatically.
--
Regards,
Laurent Pinchart
--
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
