[v5,5/6] V4L: Events: Support event handling in do_ioctl

  Add support for event handling to do_ioctl.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-ioctl.c |   58 ++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-ioctl.h       |    7 ++++
 2 files changed, 65 insertions(+), 0 deletions(-)
  

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@maxwell.research.nokia.com>
> ---
>  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.

So I would remove the events == NULL test here and just call v4l2_event_dequeue
and let that function handle it.

> +		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.

> +		    || !ops->vidioc_subscribe_event)
> +			break;
> +
> +		ret = ops->vidioc_subscribe_event(fh, sub);
> +		if (ret < 0) {
> +			dbgarg(cmd, "failed, ret=%ld", ret);
> +			break;
> +		}
> +		dbgarg(cmd, "type=0x%8.8x", sub->type);
> +		break;
> +	}
> +	case VIDIOC_UNSUBSCRIBE_EVENT:
> +	{
> +		struct v4l2_event_subscription *sub = arg;
> +		struct v4l2_fh *vfh = fh;
> +
> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> +		    || vfh->events == NULL
> +		    || !ops->vidioc_unsubscribe_event)

No need for the test_bit. This is sufficient:

		if (!ops->vidioc_unsubscribe_event ||
		    vfh->events == NULL)

Note that I am not so keen on testing against vfh->events here. I consider
this a v4l2_fh-internal field that should not be used outside the v4l2_event_
and v4l2_fh_ functions.

> +			break;
> +
> +		ret = ops->vidioc_unsubscribe_event(fh, sub);
> +		if (ret < 0) {
> +			dbgarg(cmd, "failed, ret=%ld", ret);
> +			break;
> +		}
> +		dbgarg(cmd, "type=0x%8.8x", sub->type);
> +		break;
> +	}
>  	default:
>  	{
>  		if (!ops->vidioc_default)
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index e8ba0f2..06daa6e 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -21,6 +21,8 @@
>  #include <linux/videodev2.h>
>  #endif
>  
> +struct v4l2_fh;
> +
>  struct v4l2_ioctl_ops {
>  	/* ioctl callbacks */
>  
> @@ -254,6 +256,11 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_g_dv_timings) (struct file *file, void *fh,
>  				    struct v4l2_dv_timings *timings);
>  
> +	int (*vidioc_subscribe_event)  (struct v4l2_fh *fh,
> +					struct v4l2_event_subscription *sub);
> +	int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh,
> +					struct v4l2_event_subscription *sub);
> +
>  	/* For other private ioctls */
>  	long (*vidioc_default)	       (struct file *file, void *fh,
>  					int cmd, void *arg);
> 

Regards,

	Hans
  

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@maxwell.research.nokia.com>
>> ---
>>  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.

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.

>> +		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.
  

Hi Sakari,

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@maxwell.research.nokia.com>
> >> ---
> >> 
> >>  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.
> 
> 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.

If the only information we need from struct file is the flags, they could be 
copied to v4l2_fh in the open handler. We could also put a struct file * 
member in v4l2_fh.
  

Laurent Pinchart wrote:
> Hi Sakari,

Salut Laurent,

>>> 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.
>>
>> 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.
> 
> If the only information we need from struct file is the flags, they could be 
> copied to v4l2_fh in the open handler. We could also put a struct file * 
> member in v4l2_fh.

That could be one possibility. Copying the flags in open() isn't enough
as they can change as a result of fcntl call. As we're not handling that
call the flags in struct v4l2_fh would have to be updated for every
ioctl. I'm not a big fan of caching information in general either. :-) I
could accept this, though.
  

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@maxwell.research.nokia.com>
> >> ---
> >>  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.

> 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 :-)
 
> >> +		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,

	Hans
  

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@maxwell.research.nokia.com>
> > >> ---
> > >> 
> > >>  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.
  

> 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@maxwell.research.nokia.com>
>> > >> ---
>> > >>
>> > >>  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.

Why not just add a 'blocking' argument to the v4l2_event_dequeue? And let
v4l2-ioctl.c fill in that argument? That's how I would do it.

Regards,

        Hans
  

Hans Verkuil wrote:
>>>>> 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().

It's quite simple to implement still so I'll do that in the
VIDIOC_DQEVENT. Easier for applications anyway in use cases that I
haven't been thinking about, e.g. ivtv.

>>>> 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.
> 
> Why not just add a 'blocking' argument to the v4l2_event_dequeue? And let
> v4l2-ioctl.c fill in that argument? That's how I would do it.

Implemented already before reading your mail... :-) I'll try to repost
the patches today.