Various ctrl and event frame work patches

Various ctrl and event frame work patches

[Hans de Goede]

Hi All,

This patch set obsoletes my previous "add v4l2_subscribed_event_ops" set,
while working on adding support for ctrl-events to the uvc driver I found
a few bugs in the event code, which this patchset fixes.

Regards,

Hans

[PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls

[Hans de Goede]

Otherwise the fh changing the master control won't get the inactive state
change event for the slave controls.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-ctrls.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index fc8666a..69e24f4 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -945,6 +945,7 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 			if (ctrl->cluster[0]->has_volatiles)
 				ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
 		}
+		fh = NULL;
 	}
 	if (changed || update_inactive) {
 		/* If a control was changed that was not one of the controls
-- 
1.7.7

Re: [PATCH 1/6] v4l2-ctrl: Send change events to all fh for auto cluster slave controls

[Hans Verkuil]

This is also part of a pull request from me (and so has my Signed-off-by already):

http://www.mail-archive.com/linux-media@vger.kernel.org/msg38018.html

Regards,

	Hans

On Thursday, October 27, 2011 13:17:58 Hans de Goede wrote:
> Otherwise the fh changing the master control won't get the inactive state
> change event for the slave controls.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/video/v4l2-ctrls.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index fc8666a..69e24f4 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -945,6 +945,7 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
>  			if (ctrl->cluster[0]->has_volatiles)
>  				ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
>  		}
> +		fh = NULL;
>  	}
>  	if (changed || update_inactive) {
>  		/* If a control was changed that was not one of the controls
> 

[PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-event.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 53b190c..9f56f18 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -215,6 +215,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	unsigned long flags;
 	unsigned i;
 
+	if (sub->type == V4L2_EVENT_ALL)
+		return -EINVAL;
+
 	if (elems < 1)
 		elems = 1;
 	if (sub->type == V4L2_EVENT_CTRL) {
-- 
1.7.7

Re: [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL

[Laurent Pinchart]

Hi Hans,

On Thursday 27 October 2011 13:17:59 Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

This brings the code in sync with the documentation, thanks.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/video/v4l2-event.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 53b190c..9f56f18 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -215,6 +215,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	unsigned long flags;
>  	unsigned i;
> 
> +	if (sub->type == V4L2_EVENT_ALL)
> +		return -EINVAL;
> +
>  	if (elems < 1)
>  		elems = 1;
>  	if (sub->type == V4L2_EVENT_CTRL) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL

[Hans Verkuil]

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

On Thursday, October 27, 2011 13:17:59 Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/video/v4l2-event.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 53b190c..9f56f18 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -215,6 +215,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	unsigned long flags;
>  	unsigned i;
>  
> +	if (sub->type == V4L2_EVENT_ALL)
> +		return -EINVAL;
> +
>  	if (elems < 1)
>  		elems = 1;
>  	if (sub->type == V4L2_EVENT_CTRL) {
> 

[PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans de Goede]

The kev pointers inside the pending events queue (the available queue) of the
fh point to data inside the sev, unsubscribing frees the sev, thus making these
pointers point to freed memory!

This patch fixes these dangling pointers in the available queue by removing
all matching pending events on unsubscription.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-event.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9f56f18..01cbb7f 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 			   struct v4l2_event_subscription *sub)
 {
 	struct v4l2_subscribed_event *sev;
+	struct v4l2_kevent *kev, *next;
 	unsigned long flags;
 
 	if (sub->type == V4L2_EVENT_ALL) {
@@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 
 	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
 	if (sev != NULL) {
+		/* Remove any pending events for this subscription */
+		list_for_each_entry_safe(kev, next, &fh->available, list) {
+			if (kev->sev == sev) {
+				list_del(&kev->list);
+				fh->navailable--;
+			}
+		}
 		list_del(&sev->list);
 		sev->fh = NULL;
 	}
-- 
1.7.7

Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Laurent Pinchart]

Hi Hans,

On Thursday 27 October 2011 13:18:00 Hans de Goede wrote:
> The kev pointers inside the pending events queue (the available queue) of
> the fh point to data inside the sev, unsubscribing frees the sev, thus
> making these pointers point to freed memory!
> 
> This patch fixes these dangling pointers in the available queue by removing
> all matching pending events on unsubscription.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/video/v4l2-event.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 9f56f18..01cbb7f 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			   struct v4l2_event_subscription *sub)
>  {
>  	struct v4l2_subscribed_event *sev;
> +	struct v4l2_kevent *kev, *next;
>  	unsigned long flags;
> 
>  	if (sub->type == V4L2_EVENT_ALL) {
> @@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> 
>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
>  	if (sev != NULL) {
> +		/* Remove any pending events for this subscription */
> +		list_for_each_entry_safe(kev, next, &fh->available, list) {
> +			if (kev->sev == sev) {
> +				list_del(&kev->list);
> +				fh->navailable--;
> +			}
> +		}
>  		list_del(&sev->list);
>  		sev->fh = NULL;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans Verkuil]

On Thursday, October 27, 2011 13:18:00 Hans de Goede wrote:
> The kev pointers inside the pending events queue (the available queue) of the
> fh point to data inside the sev, unsubscribing frees the sev, thus making these
> pointers point to freed memory!
> 
> This patch fixes these dangling pointers in the available queue by removing
> all matching pending events on unsubscription.

The idea is fine, but the implementation is inefficient.

Instead of the list_for_each_entry_safe you can just do:

	for (i = 0; i < sev->in_use; i++) {
		list_del(&sev->events[sev_pos(sev, i)].list);
		fh->navailable--;
	}

It's untested, but this should do the trick.

Regards,

	Hans

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/video/v4l2-event.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 9f56f18..01cbb7f 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			   struct v4l2_event_subscription *sub)
>  {
>  	struct v4l2_subscribed_event *sev;
> +	struct v4l2_kevent *kev, *next;
>  	unsigned long flags;
>  
>  	if (sub->type == V4L2_EVENT_ALL) {
> @@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  
>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
>  	if (sev != NULL) {
> +		/* Remove any pending events for this subscription */
> +		list_for_each_entry_safe(kev, next, &fh->available, list) {
> +			if (kev->sev == sev) {
> +				list_del(&kev->list);
> +				fh->navailable--;
> +			}
> +		}
>  		list_del(&sev->list);
>  		sev->fh = NULL;
>  	}
> 

[PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe

[Hans de Goede]

1: There is no reason for this after v4l2_event_unsubscribe releases the
spinlock nothing is holding a reference to the sev anymore except for the
local reference in the v4l2_event_unsubscribe function.

2: Setting sev->fh to NULL causes problems for the del op added in the next
patch of this series, since this op needs a way to get to its own data
structures, and typically this will be done by using container_of on an
embedded v4l2_fh struct.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-event.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 01cbb7f..3d27300 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -304,7 +304,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 			}
 		}
 		list_del(&sev->list);
-		sev->fh = NULL;
 	}
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-- 
1.7.7

Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe

[Laurent Pinchart]

Hi Hans,

On Thursday 27 October 2011 13:18:01 Hans de Goede wrote:
> 1: There is no reason for this after v4l2_event_unsubscribe releases the
> spinlock nothing is holding a reference to the sev anymore except for the
> local reference in the v4l2_event_unsubscribe function.
> 
> 2: Setting sev->fh to NULL causes problems for the del op added in the next
> patch of this series, since this op needs a way to get to its own data
> structures, and typically this will be done by using container_of on an
> embedded v4l2_fh struct.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

While reviewing the patch I noticed that v4l2_event_unsubscribe_all() calls 
v4l2_event_unsubscribe(), which performs control lookup again. Is there a 
reason for that, instead of handling event unsubscription directly in 
v4l2_event_unsubscribe_all() ?

> ---
>  drivers/media/video/v4l2-event.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 01cbb7f..3d27300 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -304,7 +304,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			}
>  		}
>  		list_del(&sev->list);
> -		sev->fh = NULL;
>  	}
> 
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe

[Hans de Goede]

Hi,

Thanks for the reviews!

On 10/27/2011 02:20 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Thursday 27 October 2011 13:18:01 Hans de Goede wrote:
>> 1: There is no reason for this after v4l2_event_unsubscribe releases the
>> spinlock nothing is holding a reference to the sev anymore except for the
>> local reference in the v4l2_event_unsubscribe function.
>>
>> 2: Setting sev->fh to NULL causes problems for the del op added in the next
>> patch of this series, since this op needs a way to get to its own data
>> structures, and typically this will be done by using container_of on an
>> embedded v4l2_fh struct.
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>
> Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
>
> While reviewing the patch I noticed that v4l2_event_unsubscribe_all() calls
> v4l2_event_unsubscribe(), which performs control lookup again. Is there a
> reason for that, instead of handling event unsubscription directly in
> v4l2_event_unsubscribe_all() ?

I didn't write that part, so I'll let Hans V. answer this question.

Regards,

Hans

Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe

[Laurent Pinchart]

Hi Hans,

On Friday 28 October 2011 10:35:54 Hans de Goede wrote:
> On 10/27/2011 02:20 PM, Laurent Pinchart wrote:
> > On Thursday 27 October 2011 13:18:01 Hans de Goede wrote:
> >> 1: There is no reason for this after v4l2_event_unsubscribe releases the
> >> spinlock nothing is holding a reference to the sev anymore except for
> >> the local reference in the v4l2_event_unsubscribe function.
> >> 
> >> 2: Setting sev->fh to NULL causes problems for the del op added in the
> >> next patch of this series, since this op needs a way to get to its own
> >> data structures, and typically this will be done by using container_of
> >> on an embedded v4l2_fh struct.
> >> 
> >> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
> > 
> > Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> > 
> > While reviewing the patch I noticed that v4l2_event_unsubscribe_all()
> > calls v4l2_event_unsubscribe(), which performs control lookup again. Is
> > there a reason for that, instead of handling event unsubscription
> > directly in v4l2_event_unsubscribe_all() ?
> 
> I didn't write that part, so I'll let Hans V. answer this question.

I know, that's why I still acked your patch :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe

[Hans Verkuil]

On Thursday, October 27, 2011 13:18:01 Hans de Goede wrote:
> 1: There is no reason for this after v4l2_event_unsubscribe releases the
> spinlock nothing is holding a reference to the sev anymore except for the
> local reference in the v4l2_event_unsubscribe function.

Not true. v4l2-ctrls.c may still have a reference to the sev through the
ev_subs list in struct v4l2_ctrl. The send_event() function checks for a
non-zero fh.

All that is needed is to find some different way of letting send_event()
know that this sev is no longer used. Perhaps by making sev->list empty?

Regards,

	Hans

> 2: Setting sev->fh to NULL causes problems for the del op added in the next
> patch of this series, since this op needs a way to get to its own data
> structures, and typically this will be done by using container_of on an
> embedded v4l2_fh struct.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/video/v4l2-event.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 01cbb7f..3d27300 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -304,7 +304,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			}
>  		}
>  		list_del(&sev->list);
> -		sev->fh = NULL;
>  	}
>  
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> 

[PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops

[Hans de Goede]

Just like with ctrl events, drivers may want to get called back on
listener add / remove for other event types too. Rather then special
casing all of this in subscribe / unsubscribe event it is better to
use ops for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/ivtv/ivtv-ioctl.c  |    2 +-
 drivers/media/video/omap3isp/ispccdc.c |    2 +-
 drivers/media/video/omap3isp/ispstat.c |    2 +-
 drivers/media/video/pwc/pwc-v4l.c      |    2 +-
 drivers/media/video/v4l2-event.c       |   42 ++++++++++++++++++++++++-------
 drivers/media/video/vivi.c             |    2 +-
 include/media/v4l2-event.h             |   24 +++++++++++++-----
 7 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index ecafa69..9aec8a0 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
 	case V4L2_EVENT_VSYNC:
 	case V4L2_EVENT_EOS:
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0);
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
index 40b141c..b6da736 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
 	if (sub->id != 0)
 		return -EINVAL;
 
-	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
+	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
 }
 
 static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
index 8080659..4f337a2 100644
--- a/drivers/media/video/omap3isp/ispstat.c
+++ b/drivers/media/video/omap3isp/ispstat.c
@@ -1049,7 +1049,7 @@ int omap3isp_stat_subscribe_event(struct v4l2_subdev *subdev,
 	if (sub->type != stat->event_type)
 		return -EINVAL;
 
-	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS);
+	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
 }
 
 int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 68e1323..7f159bf 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0);
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 3d27300..2dd9252 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
 		sev->first = sev_pos(sev, 1);
 		fh->navailable--;
 		if (sev->elems == 1) {
-			if (sev->replace) {
-				sev->replace(&kev->event, ev);
+			if (sev->ops && sev->ops->replace) {
+				sev->ops->replace(&kev->event, ev);
 				copy_payload = false;
 			}
-		} else if (sev->merge) {
+		} else if (sev->ops && sev->ops->merge) {
 			struct v4l2_kevent *second_oldest =
 				sev->events + sev_pos(sev, 0);
-			sev->merge(&kev->event, &second_oldest->event);
+			sev->ops->merge(&kev->event, &second_oldest->event);
 		}
 	}
 
@@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
 	new->u.ctrl.changes |= old->u.ctrl.changes;
 }
 
+const struct v4l2_subscribed_event_ops ctrl_ops = {
+	.replace = ctrls_replace,
+	.merge = ctrls_merge,
+};
+
 int v4l2_event_subscribe(struct v4l2_fh *fh,
-			 struct v4l2_event_subscription *sub, unsigned elems)
+			 struct v4l2_event_subscription *sub, unsigned elems,
+			 const struct v4l2_subscribed_event_ops *ops)
 {
 	struct v4l2_subscribed_event *sev, *found_ev;
 	struct v4l2_ctrl *ctrl = NULL;
@@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	sev->flags = sub->flags;
 	sev->fh = fh;
 	sev->elems = elems;
+	sev->ops = ops;
 	if (ctrl) {
-		sev->replace = ctrls_replace;
-		sev->merge = ctrls_merge;
+		sev->ops = &ctrl_ops;
 	}
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
@@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 		list_add(&sev->list, &fh->subscribed);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
-	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
-	if (found_ev)
+	if (found_ev) {
 		kfree(sev);
-	else if (ctrl)
+		return 0; /* Already listening */
+	}
+
+	if (sev->ops && sev->ops->add) {
+		int ret = sev->ops->add(sev);
+		if (ret) {
+			sev->ops = NULL;
+			v4l2_event_unsubscribe(fh, sub);
+			return ret;
+		}
+	}
+
+	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
+	if (ctrl)
 		v4l2_ctrl_add_event(ctrl, sev);
 
 	return 0;
@@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 	}
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	if (sev && sev->ops && sev->ops->del)
+		sev->ops->del(sev);
+
 	if (sev && sev->type == V4L2_EVENT_CTRL) {
 		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
 
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index c25787d..74ebbad 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0);
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
 	default:
 		return -EINVAL;
 	}
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 5f14e88..88fa9a1 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -78,6 +78,19 @@ struct v4l2_kevent {
 	struct v4l2_event	event;
 };
 
+/** struct v4l2_subscribed_event_ops - Subscribed event operations.
+  * @add:	Optional callback, called when a new listener is added
+  * @del:	Optional callback, called when a listener stops listening
+  * @replace:	Optional callback that can replace event 'old' with event 'new'.
+  * @merge:	Optional callback that can merge event 'old' into event 'new'.
+  */
+struct v4l2_subscribed_event_ops {
+	int  (*add)(struct v4l2_subscribed_event *sev);
+	void (*del)(struct v4l2_subscribed_event *sev);
+	void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
+	void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
+};
+
 /** struct v4l2_subscribed_event - Internal struct representing a subscribed event.
   * @list:	List node for the v4l2_fh->subscribed list.
   * @type:	Event type.
@@ -85,8 +98,7 @@ struct v4l2_kevent {
   * @flags:	Copy of v4l2_event_subscription->flags.
   * @fh:	Filehandle that subscribed to this event.
   * @node:	List node that hooks into the object's event list (if there is one).
-  * @replace:	Optional callback that can replace event 'old' with event 'new'.
-  * @merge:	Optional callback that can merge event 'old' into event 'new'.
+  * @ops:	v4l2_subscribed_event_ops
   * @elems:	The number of elements in the events array.
   * @first:	The index of the events containing the oldest available event.
   * @in_use:	The number of queued events.
@@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
 	u32			flags;
 	struct v4l2_fh		*fh;
 	struct list_head	node;
-	void			(*replace)(struct v4l2_event *old,
-					   const struct v4l2_event *new);
-	void			(*merge)(const struct v4l2_event *old,
-					 struct v4l2_event *new);
+	const struct v4l2_subscribed_event_ops *ops;
 	unsigned		elems;
 	unsigned		first;
 	unsigned		in_use;
@@ -115,7 +124,8 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev);
 void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev);
 int v4l2_event_pending(struct v4l2_fh *fh);
 int v4l2_event_subscribe(struct v4l2_fh *fh,
-			 struct v4l2_event_subscription *sub, unsigned elems);
+			 struct v4l2_event_subscription *sub, unsigned elems,
+			 const struct v4l2_subscribed_event_ops *ops);
 int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 			   struct v4l2_event_subscription *sub);
 void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
-- 
1.7.7

Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops

[Laurent Pinchart]

Hi Hans,

On Thursday 27 October 2011 13:18:02 Hans de Goede wrote:
> Just like with ctrl events, drivers may want to get called back on
> listener add / remove for other event types too. Rather then special
> casing all of this in subscribe / unsubscribe event it is better to
> use ops for this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/video/ivtv/ivtv-ioctl.c  |    2 +-
>  drivers/media/video/omap3isp/ispccdc.c |    2 +-
>  drivers/media/video/omap3isp/ispstat.c |    2 +-
>  drivers/media/video/pwc/pwc-v4l.c      |    2 +-
>  drivers/media/video/v4l2-event.c       |   42 ++++++++++++++++++++++-------
>  drivers/media/video/vivi.c             |    2 +-
>  include/media/v4l2-event.h             |   24 +++++++++++++-----

Haven't you forgotten to update Documentation/video4linux/v4l2-framework.txt ?

>  7 files changed, 54 insertions(+), 22 deletions(-)

[snip]

> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 3d27300..2dd9252 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e sev->first = sev_pos(sev, 1);
>  		fh->navailable--;
>  		if (sev->elems == 1) {
> -			if (sev->replace) {
> -				sev->replace(&kev->event, ev);
> +			if (sev->ops && sev->ops->replace) {
> +				sev->ops->replace(&kev->event, ev);
>  				copy_payload = false;
>  			}
> -		} else if (sev->merge) {
> +		} else if (sev->ops && sev->ops->merge) {
>  			struct v4l2_kevent *second_oldest =
>  				sev->events + sev_pos(sev, 0);
> -			sev->merge(&kev->event, &second_oldest->event);
> +			sev->ops->merge(&kev->event, &second_oldest->event);
>  		}
>  	}
> 
> @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old,
> struct v4l2_event *new) new->u.ctrl.changes |= old->u.ctrl.changes;
>  }
> 
> +const struct v4l2_subscribed_event_ops ctrl_ops = {

Shouldn't this be static const ?

> +	.replace = ctrls_replace,
> +	.merge = ctrls_merge,
> +};
> +
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
> -			 struct v4l2_event_subscription *sub, unsigned elems)
> +			 struct v4l2_event_subscription *sub, unsigned elems,
> +			 const struct v4l2_subscribed_event_ops *ops)
>  {
>  	struct v4l2_subscribed_event *sev, *found_ev;
>  	struct v4l2_ctrl *ctrl = NULL;
> @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	sev->flags = sub->flags;
>  	sev->fh = fh;
>  	sev->elems = elems;
> +	sev->ops = ops;
>  	if (ctrl) {
> -		sev->replace = ctrls_replace;
> -		sev->merge = ctrls_merge;
> +		sev->ops = &ctrl_ops;
>  	}

You can remove the brackets here.

> 
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  		list_add(&sev->list, &fh->subscribed);
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> 
> -	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> -	if (found_ev)
> +	if (found_ev) {
>  		kfree(sev);
> -	else if (ctrl)
> +		return 0; /* Already listening */
> +	}
> +
> +	if (sev->ops && sev->ops->add) {
> +		int ret = sev->ops->add(sev);
> +		if (ret) {
> +			sev->ops = NULL;
> +			v4l2_event_unsubscribe(fh, sub);
> +			return ret;
> +		}
> +	}
> +
> +	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> +	if (ctrl)
>  		v4l2_ctrl_add_event(ctrl, sev);
> 
>  	return 0;
> @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  	}
> 
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	if (sev && sev->ops && sev->ops->del)
> +		sev->ops->del(sev);
> +
>  	if (sev && sev->type == V4L2_EVENT_CTRL) {
>  		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops

[Hans de Goede]

Hi,

On 10/27/2011 02:30 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Thursday 27 October 2011 13:18:02 Hans de Goede wrote:
>> Just like with ctrl events, drivers may want to get called back on
>> listener add / remove for other event types too. Rather then special
>> casing all of this in subscribe / unsubscribe event it is better to
>> use ops for this.
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>>   drivers/media/video/ivtv/ivtv-ioctl.c  |    2 +-
>>   drivers/media/video/omap3isp/ispccdc.c |    2 +-
>>   drivers/media/video/omap3isp/ispstat.c |    2 +-
>>   drivers/media/video/pwc/pwc-v4l.c      |    2 +-
>>   drivers/media/video/v4l2-event.c       |   42 ++++++++++++++++++++++-------
>>   drivers/media/video/vivi.c             |    2 +-
>>   include/media/v4l2-event.h             |   24 +++++++++++++-----
>
> Haven't you forgotten to update Documentation/video4linux/v4l2-framework.txt ?
>

I think I have, I'll go and do another revision fixing this.

>>   7 files changed, 54 insertions(+), 22 deletions(-)
>
> [snip]
>
>> diff --git a/drivers/media/video/v4l2-event.c
>> b/drivers/media/video/v4l2-event.c index 3d27300..2dd9252 100644
>> --- a/drivers/media/video/v4l2-event.c
>> +++ b/drivers/media/video/v4l2-event.c
>> @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
>> const struct v4l2_event *e sev->first = sev_pos(sev, 1);
>>   		fh->navailable--;
>>   		if (sev->elems == 1) {
>> -			if (sev->replace) {
>> -				sev->replace(&kev->event, ev);
>> +			if (sev->ops&&  sev->ops->replace) {
>> +				sev->ops->replace(&kev->event, ev);
>>   				copy_payload = false;
>>   			}
>> -		} else if (sev->merge) {
>> +		} else if (sev->ops&&  sev->ops->merge) {
>>   			struct v4l2_kevent *second_oldest =
>>   				sev->events + sev_pos(sev, 0);
>> -			sev->merge(&kev->event,&second_oldest->event);
>> +			sev->ops->merge(&kev->event,&second_oldest->event);
>>   		}
>>   	}
>>
>> @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old,
>> struct v4l2_event *new) new->u.ctrl.changes |= old->u.ctrl.changes;
>>   }
>>
>> +const struct v4l2_subscribed_event_ops ctrl_ops = {
>
> Shouldn't this be static const ?
>

You're right I will fix this in the next iteration.

>> +	.replace = ctrls_replace,
>> +	.merge = ctrls_merge,
>> +};
>> +
>>   int v4l2_event_subscribe(struct v4l2_fh *fh,
>> -			 struct v4l2_event_subscription *sub, unsigned elems)
>> +			 struct v4l2_event_subscription *sub, unsigned elems,
>> +			 const struct v4l2_subscribed_event_ops *ops)
>>   {
>>   	struct v4l2_subscribed_event *sev, *found_ev;
>>   	struct v4l2_ctrl *ctrl = NULL;
>> @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>   	sev->flags = sub->flags;
>>   	sev->fh = fh;
>>   	sev->elems = elems;
>> +	sev->ops = ops;
>>   	if (ctrl) {
>> -		sev->replace = ctrls_replace;
>> -		sev->merge = ctrls_merge;
>> +		sev->ops =&ctrl_ops;
>>   	}
>
> You can remove the brackets here.
>

I was aiming for minimal diff size here, since this bit will go away in the
next patch in the series anyways.

>>
>>   	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>   		list_add(&sev->list,&fh->subscribed);
>>   	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>>
>> -	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
>> -	if (found_ev)
>> +	if (found_ev) {
>>   		kfree(sev);
>> -	else if (ctrl)
>> +		return 0; /* Already listening */
>> +	}
>> +
>> +	if (sev->ops&&  sev->ops->add) {
>> +		int ret = sev->ops->add(sev);
>> +		if (ret) {
>> +			sev->ops = NULL;
>> +			v4l2_event_unsubscribe(fh, sub);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
>> +	if (ctrl)
>>   		v4l2_ctrl_add_event(ctrl, sev);
>>
>>   	return 0;
>> @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>>   	}
>>
>>   	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +
>> +	if (sev&&  sev->ops&&  sev->ops->del)
>> +		sev->ops->del(sev);
>> +
>>   	if (sev&&  sev->type == V4L2_EVENT_CTRL) {
>>   		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
>>
>

Regards,

Hans

Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops

[Hans Verkuil]

On Thursday, October 27, 2011 13:18:02 Hans de Goede wrote:
> Just like with ctrl events, drivers may want to get called back on
> listener add / remove for other event types too. Rather then special
> casing all of this in subscribe / unsubscribe event it is better to
> use ops for this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  drivers/media/video/ivtv/ivtv-ioctl.c  |    2 +-
>  drivers/media/video/omap3isp/ispccdc.c |    2 +-
>  drivers/media/video/omap3isp/ispstat.c |    2 +-
>  drivers/media/video/pwc/pwc-v4l.c      |    2 +-
>  drivers/media/video/v4l2-event.c       |   42 ++++++++++++++++++++++++-------
>  drivers/media/video/vivi.c             |    2 +-
>  include/media/v4l2-event.h             |   24 +++++++++++++-----
>  7 files changed, 54 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
> index ecafa69..9aec8a0 100644
> --- a/drivers/media/video/ivtv/ivtv-ioctl.c
> +++ b/drivers/media/video/ivtv/ivtv-ioctl.c
> @@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
>  	case V4L2_EVENT_VSYNC:
>  	case V4L2_EVENT_EOS:
>  	case V4L2_EVENT_CTRL:
> -		return v4l2_event_subscribe(fh, sub, 0);
> +		return v4l2_event_subscribe(fh, sub, 0, NULL);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
> index 40b141c..b6da736 100644
> --- a/drivers/media/video/omap3isp/ispccdc.c
> +++ b/drivers/media/video/omap3isp/ispccdc.c
> @@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
>  	if (sub->id != 0)
>  		return -EINVAL;
>  
> -	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
> +	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
>  }
>  
>  static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
> index 8080659..4f337a2 100644
> --- a/drivers/media/video/omap3isp/ispstat.c
> +++ b/drivers/media/video/omap3isp/ispstat.c
> @@ -1049,7 +1049,7 @@ int omap3isp_stat_subscribe_event(struct v4l2_subdev *subdev,
>  	if (sub->type != stat->event_type)
>  		return -EINVAL;
>  
> -	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS);
> +	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
>  }
>  
>  int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
> diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
> index 68e1323..7f159bf 100644
> --- a/drivers/media/video/pwc/pwc-v4l.c
> +++ b/drivers/media/video/pwc/pwc-v4l.c
> @@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
>  {
>  	switch (sub->type) {
>  	case V4L2_EVENT_CTRL:
> -		return v4l2_event_subscribe(fh, sub, 0);
> +		return v4l2_event_subscribe(fh, sub, 0, NULL);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 3d27300..2dd9252 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
>  		sev->first = sev_pos(sev, 1);
>  		fh->navailable--;
>  		if (sev->elems == 1) {
> -			if (sev->replace) {
> -				sev->replace(&kev->event, ev);
> +			if (sev->ops && sev->ops->replace) {
> +				sev->ops->replace(&kev->event, ev);
>  				copy_payload = false;
>  			}
> -		} else if (sev->merge) {
> +		} else if (sev->ops && sev->ops->merge) {
>  			struct v4l2_kevent *second_oldest =
>  				sev->events + sev_pos(sev, 0);
> -			sev->merge(&kev->event, &second_oldest->event);
> +			sev->ops->merge(&kev->event, &second_oldest->event);
>  		}
>  	}
>  
> @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
>  	new->u.ctrl.changes |= old->u.ctrl.changes;
>  }
>  
> +const struct v4l2_subscribed_event_ops ctrl_ops = {
> +	.replace = ctrls_replace,
> +	.merge = ctrls_merge,
> +};
> +
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
> -			 struct v4l2_event_subscription *sub, unsigned elems)
> +			 struct v4l2_event_subscription *sub, unsigned elems,
> +			 const struct v4l2_subscribed_event_ops *ops)
>  {
>  	struct v4l2_subscribed_event *sev, *found_ev;
>  	struct v4l2_ctrl *ctrl = NULL;
> @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	sev->flags = sub->flags;
>  	sev->fh = fh;
>  	sev->elems = elems;
> +	sev->ops = ops;
>  	if (ctrl) {
> -		sev->replace = ctrls_replace;
> -		sev->merge = ctrls_merge;
> +		sev->ops = &ctrl_ops;
>  	}
>  
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  		list_add(&sev->list, &fh->subscribed);
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  
> -	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> -	if (found_ev)
> +	if (found_ev) {
>  		kfree(sev);
> -	else if (ctrl)
> +		return 0; /* Already listening */
> +	}
> +
> +	if (sev->ops && sev->ops->add) {
> +		int ret = sev->ops->add(sev);
> +		if (ret) {
> +			sev->ops = NULL;
> +			v4l2_event_unsubscribe(fh, sub);
> +			return ret;
> +		}
> +	}
> +
> +	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> +	if (ctrl)
>  		v4l2_ctrl_add_event(ctrl, sev);
>  
>  	return 0;
> @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  	}
>  
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	if (sev && sev->ops && sev->ops->del)
> +		sev->ops->del(sev);
> +
>  	if (sev && sev->type == V4L2_EVENT_CTRL) {
>  		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
>  
> diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> index c25787d..74ebbad 100644
> --- a/drivers/media/video/vivi.c
> +++ b/drivers/media/video/vivi.c
> @@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
>  {
>  	switch (sub->type) {
>  	case V4L2_EVENT_CTRL:
> -		return v4l2_event_subscribe(fh, sub, 0);
> +		return v4l2_event_subscribe(fh, sub, 0, NULL);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 5f14e88..88fa9a1 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -78,6 +78,19 @@ struct v4l2_kevent {
>  	struct v4l2_event	event;
>  };
>  
> +/** struct v4l2_subscribed_event_ops - Subscribed event operations.
> +  * @add:	Optional callback, called when a new listener is added
> +  * @del:	Optional callback, called when a listener stops listening
> +  * @replace:	Optional callback that can replace event 'old' with event 'new'.
> +  * @merge:	Optional callback that can merge event 'old' into event 'new'.
> +  */
> +struct v4l2_subscribed_event_ops {
> +	int  (*add)(struct v4l2_subscribed_event *sev);
> +	void (*del)(struct v4l2_subscribed_event *sev);
> +	void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
> +	void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
> +};
> +
>  /** struct v4l2_subscribed_event - Internal struct representing a subscribed event.
>    * @list:	List node for the v4l2_fh->subscribed list.
>    * @type:	Event type.
> @@ -85,8 +98,7 @@ struct v4l2_kevent {
>    * @flags:	Copy of v4l2_event_subscription->flags.
>    * @fh:	Filehandle that subscribed to this event.
>    * @node:	List node that hooks into the object's event list (if there is one).
> -  * @replace:	Optional callback that can replace event 'old' with event 'new'.
> -  * @merge:	Optional callback that can merge event 'old' into event 'new'.
> +  * @ops:	v4l2_subscribed_event_ops
>    * @elems:	The number of elements in the events array.
>    * @first:	The index of the events containing the oldest available event.
>    * @in_use:	The number of queued events.
> @@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
>  	u32			flags;
>  	struct v4l2_fh		*fh;
>  	struct list_head	node;
> -	void			(*replace)(struct v4l2_event *old,
> -					   const struct v4l2_event *new);
> -	void			(*merge)(const struct v4l2_event *old,
> -					 struct v4l2_event *new);
> +	const struct v4l2_subscribed_event_ops *ops;
>  	unsigned		elems;
>  	unsigned		first;
>  	unsigned		in_use;
> @@ -115,7 +124,8 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev);
>  void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev);
>  int v4l2_event_pending(struct v4l2_fh *fh);
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
> -			 struct v4l2_event_subscription *sub, unsigned elems);
> +			 struct v4l2_event_subscription *sub, unsigned elems,
> +			 const struct v4l2_subscribed_event_ops *ops);
>  int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			   struct v4l2_event_subscription *sub);
>  void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
> 

Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops

[Hans Verkuil]

On Sunday, October 30, 2011 12:17:28 Hans Verkuil wrote:
> On Thursday, October 27, 2011 13:18:02 Hans de Goede wrote:
> > Just like with ctrl events, drivers may want to get called back on
> > listener add / remove for other event types too. Rather then special
> > casing all of this in subscribe / unsubscribe event it is better to
> > use ops for this.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Scratch this ack. I've found a race condition, see below...

> 
> Regards,
> 
> 	Hans
> 
> > ---
> >  drivers/media/video/ivtv/ivtv-ioctl.c  |    2 +-
> >  drivers/media/video/omap3isp/ispccdc.c |    2 +-
> >  drivers/media/video/omap3isp/ispstat.c |    2 +-
> >  drivers/media/video/pwc/pwc-v4l.c      |    2 +-
> >  drivers/media/video/v4l2-event.c       |   42 ++++++++++++++++++++++++-------
> >  drivers/media/video/vivi.c             |    2 +-
> >  include/media/v4l2-event.h             |   24 +++++++++++++-----
> >  7 files changed, 54 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
> > index ecafa69..9aec8a0 100644
> > --- a/drivers/media/video/ivtv/ivtv-ioctl.c
> > +++ b/drivers/media/video/ivtv/ivtv-ioctl.c
> > @@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
> >  	case V4L2_EVENT_VSYNC:
> >  	case V4L2_EVENT_EOS:
> >  	case V4L2_EVENT_CTRL:
> > -		return v4l2_event_subscribe(fh, sub, 0);
> > +		return v4l2_event_subscribe(fh, sub, 0, NULL);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
> > index 40b141c..b6da736 100644
> > --- a/drivers/media/video/omap3isp/ispccdc.c
> > +++ b/drivers/media/video/omap3isp/ispccdc.c
> > @@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> >  	if (sub->id != 0)
> >  		return -EINVAL;
> >  
> > -	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
> > +	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
> >  }
> >  
> >  static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> > diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
> > index 8080659..4f337a2 100644
> > --- a/drivers/media/video/omap3isp/ispstat.c
> > +++ b/drivers/media/video/omap3isp/ispstat.c
> > @@ -1049,7 +1049,7 @@ int omap3isp_stat_subscribe_event(struct v4l2_subdev *subdev,
> >  	if (sub->type != stat->event_type)
> >  		return -EINVAL;
> >  
> > -	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS);
> > +	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
> >  }
> >  
> >  int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
> > diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
> > index 68e1323..7f159bf 100644
> > --- a/drivers/media/video/pwc/pwc-v4l.c
> > +++ b/drivers/media/video/pwc/pwc-v4l.c
> > @@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
> >  {
> >  	switch (sub->type) {
> >  	case V4L2_EVENT_CTRL:
> > -		return v4l2_event_subscribe(fh, sub, 0);
> > +		return v4l2_event_subscribe(fh, sub, 0, NULL);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> > index 3d27300..2dd9252 100644
> > --- a/drivers/media/video/v4l2-event.c
> > +++ b/drivers/media/video/v4l2-event.c
> > @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
> >  		sev->first = sev_pos(sev, 1);
> >  		fh->navailable--;
> >  		if (sev->elems == 1) {
> > -			if (sev->replace) {
> > -				sev->replace(&kev->event, ev);
> > +			if (sev->ops && sev->ops->replace) {
> > +				sev->ops->replace(&kev->event, ev);
> >  				copy_payload = false;
> >  			}
> > -		} else if (sev->merge) {
> > +		} else if (sev->ops && sev->ops->merge) {
> >  			struct v4l2_kevent *second_oldest =
> >  				sev->events + sev_pos(sev, 0);
> > -			sev->merge(&kev->event, &second_oldest->event);
> > +			sev->ops->merge(&kev->event, &second_oldest->event);
> >  		}
> >  	}
> >  
> > @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
> >  	new->u.ctrl.changes |= old->u.ctrl.changes;
> >  }
> >  
> > +const struct v4l2_subscribed_event_ops ctrl_ops = {
> > +	.replace = ctrls_replace,
> > +	.merge = ctrls_merge,
> > +};
> > +
> >  int v4l2_event_subscribe(struct v4l2_fh *fh,
> > -			 struct v4l2_event_subscription *sub, unsigned elems)
> > +			 struct v4l2_event_subscription *sub, unsigned elems,
> > +			 const struct v4l2_subscribed_event_ops *ops)
> >  {
> >  	struct v4l2_subscribed_event *sev, *found_ev;
> >  	struct v4l2_ctrl *ctrl = NULL;
> > @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> >  	sev->flags = sub->flags;
> >  	sev->fh = fh;
> >  	sev->elems = elems;
> > +	sev->ops = ops;
> >  	if (ctrl) {
> > -		sev->replace = ctrls_replace;
> > -		sev->merge = ctrls_merge;
> > +		sev->ops = &ctrl_ops;
> >  	}
> >  
> >  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> > @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> >  		list_add(&sev->list, &fh->subscribed);
> >  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >  
> > -	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> > -	if (found_ev)
> > +	if (found_ev) {
> >  		kfree(sev);
> > -	else if (ctrl)
> > +		return 0; /* Already listening */
> > +	}
> > +
> > +	if (sev->ops && sev->ops->add) {
> > +		int ret = sev->ops->add(sev);
> > +		if (ret) {
> > +			sev->ops = NULL;
> > +			v4l2_event_unsubscribe(fh, sub);
> > +			return ret;
> > +		}

The problem here is that the event is basically available for use after the
spin_unlock_irqrestore(), but before the sev->ops->add() call. In the past I
just 'knew' that the event would never be generated by the control framework
until after v4l2_ctrl_add_event was called, but this should be formalized now
that these ops are added.

I see two options:

1) Have some method to mark the sev as being 'invalid' so functions sending
events can skip it (needed as well for your patch 4/6).

2) Document that drivers should never be able to send an event until after
the add() callback has succeeded.

I am leaning towards option 1 myself.

How about leaving sev->elems at 0 until after the add() op succeeds?

That's easy to test against and easy to implement.

Regards,

	Hans

> > +	}
> > +
> > +	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> > +	if (ctrl)
> >  		v4l2_ctrl_add_event(ctrl, sev);
> >  
> >  	return 0;
> > @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> >  	}
> >  
> >  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> > +
> > +	if (sev && sev->ops && sev->ops->del)
> > +		sev->ops->del(sev);
> > +
> >  	if (sev && sev->type == V4L2_EVENT_CTRL) {
> >  		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
> >  
> > diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> > index c25787d..74ebbad 100644
> > --- a/drivers/media/video/vivi.c
> > +++ b/drivers/media/video/vivi.c
> > @@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
> >  {
> >  	switch (sub->type) {
> >  	case V4L2_EVENT_CTRL:
> > -		return v4l2_event_subscribe(fh, sub, 0);
> > +		return v4l2_event_subscribe(fh, sub, 0, NULL);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> > index 5f14e88..88fa9a1 100644
> > --- a/include/media/v4l2-event.h
> > +++ b/include/media/v4l2-event.h
> > @@ -78,6 +78,19 @@ struct v4l2_kevent {
> >  	struct v4l2_event	event;
> >  };
> >  
> > +/** struct v4l2_subscribed_event_ops - Subscribed event operations.
> > +  * @add:	Optional callback, called when a new listener is added
> > +  * @del:	Optional callback, called when a listener stops listening
> > +  * @replace:	Optional callback that can replace event 'old' with event 'new'.
> > +  * @merge:	Optional callback that can merge event 'old' into event 'new'.
> > +  */
> > +struct v4l2_subscribed_event_ops {
> > +	int  (*add)(struct v4l2_subscribed_event *sev);
> > +	void (*del)(struct v4l2_subscribed_event *sev);
> > +	void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
> > +	void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
> > +};
> > +
> >  /** struct v4l2_subscribed_event - Internal struct representing a subscribed event.
> >    * @list:	List node for the v4l2_fh->subscribed list.
> >    * @type:	Event type.
> > @@ -85,8 +98,7 @@ struct v4l2_kevent {
> >    * @flags:	Copy of v4l2_event_subscription->flags.
> >    * @fh:	Filehandle that subscribed to this event.
> >    * @node:	List node that hooks into the object's event list (if there is one).
> > -  * @replace:	Optional callback that can replace event 'old' with event 'new'.
> > -  * @merge:	Optional callback that can merge event 'old' into event 'new'.
> > +  * @ops:	v4l2_subscribed_event_ops
> >    * @elems:	The number of elements in the events array.
> >    * @first:	The index of the events containing the oldest available event.
> >    * @in_use:	The number of queued events.
> > @@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
> >  	u32			flags;
> >  	struct v4l2_fh		*fh;
> >  	struct list_head	node;
> > -	void			(*replace)(struct v4l2_event *old,
> > -					   const struct v4l2_event *new);
> > -	void			(*merge)(const struct v4l2_event *old,
> > -					 struct v4l2_event *new);
> > +	const struct v4l2_subscribed_event_ops *ops;
> >  	unsigned		elems;
> >  	unsigned		first;
> >  	unsigned		in_use;
> > @@ -115,7 +124,8 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev);
> >  void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev);
> >  int v4l2_event_pending(struct v4l2_fh *fh);
> >  int v4l2_event_subscribe(struct v4l2_fh *fh,
> > -			 struct v4l2_event_subscription *sub, unsigned elems);
> > +			 struct v4l2_event_subscription *sub, unsigned elems,
> > +			 const struct v4l2_subscribed_event_ops *ops);
> >  int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> >  			   struct v4l2_event_subscription *sub);
> >  void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
> > 
> 

[PATCH 6/6] v4l2-ctrls: Use v4l2_subscribed_event_ops

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/ivtv/ivtv-ioctl.c |    3 +-
 drivers/media/video/pwc/pwc-v4l.c     |    2 +-
 drivers/media/video/v4l2-ctrls.c      |   56 +++++++++++++++++++++++++++------
 drivers/media/video/v4l2-event.c      |   39 -----------------------
 drivers/media/video/vivi.c            |    2 +-
 include/media/v4l2-ctrls.h            |    7 ++--
 6 files changed, 53 insertions(+), 56 deletions(-)

diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index 9aec8a0..72fd74f 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1455,8 +1455,9 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
 	switch (sub->type) {
 	case V4L2_EVENT_VSYNC:
 	case V4L2_EVENT_EOS:
-	case V4L2_EVENT_CTRL:
 		return v4l2_event_subscribe(fh, sub, 0, NULL);
+	case V4L2_EVENT_CTRL:
+		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 7f159bf..afc5b15 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0, NULL);
+		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 69e24f4..c4dec20 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -2329,10 +2329,22 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
 
-void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
-				struct v4l2_subscribed_event *sev)
+static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev)
 {
-	v4l2_ctrl_lock(ctrl);
+	struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
+	struct v4l2_ctrl_ref *ref;
+	struct v4l2_ctrl *ctrl;
+	int ret = 0;
+
+	mutex_lock(&hdl->lock);
+
+	ref = find_ref(hdl, sev->id);
+	if (!ref) {
+		ret = -EINVAL;
+		goto leave;
+	}
+	ctrl = ref->ctrl;
+
 	list_add_tail(&sev->node, &ctrl->ev_subs);
 	if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
 	    (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
@@ -2344,15 +2356,39 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
 		fill_event(&ev, ctrl, changes);
 		v4l2_event_queue_fh(sev->fh, &ev);
 	}
-	v4l2_ctrl_unlock(ctrl);
+leave:
+	mutex_unlock(&hdl->lock);
+	return ret;
 }
-EXPORT_SYMBOL(v4l2_ctrl_add_event);
 
-void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
-				struct v4l2_subscribed_event *sev)
+static void v4l2_ctrl_del_event(struct v4l2_subscribed_event *sev)
 {
-	v4l2_ctrl_lock(ctrl);
+	struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
+
+	mutex_lock(&hdl->lock);
 	list_del(&sev->node);
-	v4l2_ctrl_unlock(ctrl);
+	mutex_unlock(&hdl->lock);
 }
-EXPORT_SYMBOL(v4l2_ctrl_del_event);
+
+void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new)
+{
+	u32 old_changes = old->u.ctrl.changes;
+
+	old->u.ctrl = new->u.ctrl;
+	old->u.ctrl.changes |= old_changes;
+}
+EXPORT_SYMBOL(v4l2_ctrl_replace);
+
+void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new)
+{
+	new->u.ctrl.changes |= old->u.ctrl.changes;
+}
+EXPORT_SYMBOL(v4l2_ctrl_merge);
+
+const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops = {
+	.add = v4l2_ctrl_add_event,
+	.del = v4l2_ctrl_del_event,
+	.replace = v4l2_ctrl_replace,
+	.merge = v4l2_ctrl_merge,
+};
+EXPORT_SYMBOL(v4l2_ctrl_sub_ev_ops);
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 2dd9252..2f5ee7b 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -25,7 +25,6 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
-#include <media/v4l2-ctrls.h>
 
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -194,30 +193,11 @@ int v4l2_event_pending(struct v4l2_fh *fh)
 }
 EXPORT_SYMBOL_GPL(v4l2_event_pending);
 
-static void ctrls_replace(struct v4l2_event *old, const struct v4l2_event *new)
-{
-	u32 old_changes = old->u.ctrl.changes;
-
-	old->u.ctrl = new->u.ctrl;
-	old->u.ctrl.changes |= old_changes;
-}
-
-static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
-{
-	new->u.ctrl.changes |= old->u.ctrl.changes;
-}
-
-const struct v4l2_subscribed_event_ops ctrl_ops = {
-	.replace = ctrls_replace,
-	.merge = ctrls_merge,
-};
-
 int v4l2_event_subscribe(struct v4l2_fh *fh,
 			 struct v4l2_event_subscription *sub, unsigned elems,
 			 const struct v4l2_subscribed_event_ops *ops)
 {
 	struct v4l2_subscribed_event *sev, *found_ev;
-	struct v4l2_ctrl *ctrl = NULL;
 	unsigned long flags;
 	unsigned i;
 
@@ -226,11 +206,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 
 	if (elems < 1)
 		elems = 1;
-	if (sub->type == V4L2_EVENT_CTRL) {
-		ctrl = v4l2_ctrl_find(fh->ctrl_handler, sub->id);
-		if (ctrl == NULL)
-			return -EINVAL;
-	}
 
 	sev = kzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems, GFP_KERNEL);
 	if (!sev)
@@ -243,9 +218,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	sev->fh = fh;
 	sev->elems = elems;
 	sev->ops = ops;
-	if (ctrl) {
-		sev->ops = &ctrl_ops;
-	}
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -267,10 +239,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 		}
 	}
 
-	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
-	if (ctrl)
-		v4l2_ctrl_add_event(ctrl, sev);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
@@ -329,13 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 	if (sev && sev->ops && sev->ops->del)
 		sev->ops->del(sev);
 
-	if (sev && sev->type == V4L2_EVENT_CTRL) {
-		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
-
-		if (ctrl)
-			v4l2_ctrl_del_event(ctrl, sev);
-	}
-
 	kfree(sev);
 
 	return 0;
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 74ebbad..ce1783d 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0, NULL);
+		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
 	default:
 		return -EINVAL;
 	}
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index eeb3df6..fde17ff 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -487,10 +487,9 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
 
 /* Internal helper functions that deal with control events. */
-void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
-		struct v4l2_subscribed_event *sev);
-void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
-		struct v4l2_subscribed_event *sev);
+extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
+void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
+void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new);
 
 /* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
 int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
-- 
1.7.7

Re: [PATCH 6/6] v4l2-ctrls: Use v4l2_subscribed_event_ops

[Hans Verkuil]

On Thursday, October 27, 2011 13:18:03 Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  drivers/media/video/ivtv/ivtv-ioctl.c |    3 +-
>  drivers/media/video/pwc/pwc-v4l.c     |    2 +-
>  drivers/media/video/v4l2-ctrls.c      |   56 +++++++++++++++++++++++++++------
>  drivers/media/video/v4l2-event.c      |   39 -----------------------
>  drivers/media/video/vivi.c            |    2 +-
>  include/media/v4l2-ctrls.h            |    7 ++--
>  6 files changed, 53 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
> index 9aec8a0..72fd74f 100644
> --- a/drivers/media/video/ivtv/ivtv-ioctl.c
> +++ b/drivers/media/video/ivtv/ivtv-ioctl.c
> @@ -1455,8 +1455,9 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
>  	switch (sub->type) {
>  	case V4L2_EVENT_VSYNC:
>  	case V4L2_EVENT_EOS:
> -	case V4L2_EVENT_CTRL:
>  		return v4l2_event_subscribe(fh, sub, 0, NULL);
> +	case V4L2_EVENT_CTRL:
> +		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
> index 7f159bf..afc5b15 100644
> --- a/drivers/media/video/pwc/pwc-v4l.c
> +++ b/drivers/media/video/pwc/pwc-v4l.c
> @@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
>  {
>  	switch (sub->type) {
>  	case V4L2_EVENT_CTRL:
> -		return v4l2_event_subscribe(fh, sub, 0, NULL);
> +		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index 69e24f4..c4dec20 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -2329,10 +2329,22 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
>  
> -void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
> -				struct v4l2_subscribed_event *sev)
> +static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev)
>  {
> -	v4l2_ctrl_lock(ctrl);
> +	struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
> +	struct v4l2_ctrl_ref *ref;
> +	struct v4l2_ctrl *ctrl;
> +	int ret = 0;
> +
> +	mutex_lock(&hdl->lock);
> +
> +	ref = find_ref(hdl, sev->id);
> +	if (!ref) {
> +		ret = -EINVAL;
> +		goto leave;
> +	}
> +	ctrl = ref->ctrl;
> +
>  	list_add_tail(&sev->node, &ctrl->ev_subs);
>  	if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
>  	    (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
> @@ -2344,15 +2356,39 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
>  		fill_event(&ev, ctrl, changes);
>  		v4l2_event_queue_fh(sev->fh, &ev);
>  	}
> -	v4l2_ctrl_unlock(ctrl);
> +leave:
> +	mutex_unlock(&hdl->lock);
> +	return ret;
>  }
> -EXPORT_SYMBOL(v4l2_ctrl_add_event);
>  
> -void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
> -				struct v4l2_subscribed_event *sev)
> +static void v4l2_ctrl_del_event(struct v4l2_subscribed_event *sev)
>  {
> -	v4l2_ctrl_lock(ctrl);
> +	struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
> +
> +	mutex_lock(&hdl->lock);
>  	list_del(&sev->node);
> -	v4l2_ctrl_unlock(ctrl);
> +	mutex_unlock(&hdl->lock);
>  }
> -EXPORT_SYMBOL(v4l2_ctrl_del_event);
> +
> +void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new)
> +{
> +	u32 old_changes = old->u.ctrl.changes;
> +
> +	old->u.ctrl = new->u.ctrl;
> +	old->u.ctrl.changes |= old_changes;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_replace);
> +
> +void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new)
> +{
> +	new->u.ctrl.changes |= old->u.ctrl.changes;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_merge);
> +
> +const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops = {
> +	.add = v4l2_ctrl_add_event,
> +	.del = v4l2_ctrl_del_event,
> +	.replace = v4l2_ctrl_replace,
> +	.merge = v4l2_ctrl_merge,
> +};
> +EXPORT_SYMBOL(v4l2_ctrl_sub_ev_ops);
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 2dd9252..2f5ee7b 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -25,7 +25,6 @@
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-event.h>
> -#include <media/v4l2-ctrls.h>
>  
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -194,30 +193,11 @@ int v4l2_event_pending(struct v4l2_fh *fh)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_pending);
>  
> -static void ctrls_replace(struct v4l2_event *old, const struct v4l2_event *new)
> -{
> -	u32 old_changes = old->u.ctrl.changes;
> -
> -	old->u.ctrl = new->u.ctrl;
> -	old->u.ctrl.changes |= old_changes;
> -}
> -
> -static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
> -{
> -	new->u.ctrl.changes |= old->u.ctrl.changes;
> -}
> -
> -const struct v4l2_subscribed_event_ops ctrl_ops = {
> -	.replace = ctrls_replace,
> -	.merge = ctrls_merge,
> -};
> -
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
>  			 struct v4l2_event_subscription *sub, unsigned elems,
>  			 const struct v4l2_subscribed_event_ops *ops)
>  {
>  	struct v4l2_subscribed_event *sev, *found_ev;
> -	struct v4l2_ctrl *ctrl = NULL;
>  	unsigned long flags;
>  	unsigned i;
>  
> @@ -226,11 +206,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  
>  	if (elems < 1)
>  		elems = 1;
> -	if (sub->type == V4L2_EVENT_CTRL) {
> -		ctrl = v4l2_ctrl_find(fh->ctrl_handler, sub->id);
> -		if (ctrl == NULL)
> -			return -EINVAL;
> -	}
>  
>  	sev = kzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems, GFP_KERNEL);
>  	if (!sev)
> @@ -243,9 +218,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	sev->fh = fh;
>  	sev->elems = elems;
>  	sev->ops = ops;
> -	if (ctrl) {
> -		sev->ops = &ctrl_ops;
> -	}
>  
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>  	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> @@ -267,10 +239,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  		}
>  	}
>  
> -	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
> -	if (ctrl)
> -		v4l2_ctrl_add_event(ctrl, sev);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> @@ -329,13 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  	if (sev && sev->ops && sev->ops->del)
>  		sev->ops->del(sev);
>  
> -	if (sev && sev->type == V4L2_EVENT_CTRL) {
> -		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
> -
> -		if (ctrl)
> -			v4l2_ctrl_del_event(ctrl, sev);
> -	}
> -
>  	kfree(sev);
>  
>  	return 0;
> diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> index 74ebbad..ce1783d 100644
> --- a/drivers/media/video/vivi.c
> +++ b/drivers/media/video/vivi.c
> @@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
>  {
>  	switch (sub->type) {
>  	case V4L2_EVENT_CTRL:
> -		return v4l2_event_subscribe(fh, sub, 0, NULL);
> +		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index eeb3df6..fde17ff 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -487,10 +487,9 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
>  int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
>  
>  /* Internal helper functions that deal with control events. */
> -void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
> -		struct v4l2_subscribed_event *sev);
> -void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
> -		struct v4l2_subscribed_event *sev);
> +extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
> +void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
> +void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new);
>  
>  /* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
>  int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
> 

Various ctrl and event frame work patches (version 2)

[Hans de Goede]

Hi All,

This patch set obsoletes my previous "add v4l2_subscribed_event_ops" set,
while working on adding support for ctrl-events to the uvc driver I found
a few bugs in the event code, which this patchset fixes.

Changes since version 1:
-Added a documentation update (update v4l2-framework.txt) to:
 "v4l2-event: Add v4l2_subscribed_event_ops"

Regards,

Hans

[PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubcribe

[Hans de Goede]

1: There is no reason for this after v4l2_event_unsubscribe releases the
spinlock nothing is holding a reference to the sev anymore except for the
local reference in the v4l2_event_unsubscribe function.

2: Setting sev->fh to NULL causes problems for the del op added in the next
patch of this series, since this op needs a way to get to its own data
structures, and typically this will be done by using container_of on an
embedded v4l2_fh struct.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-event.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 01cbb7f..3d27300 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -304,7 +304,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 			}
 		}
 		list_del(&sev->list);
-		sev->fh = NULL;
 	}
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-- 
1.7.7