[v4,7/7] V4L: Events: Support all events

  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(-)
  

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,