> Hi Hans,
>
> Thanks for the patchset! This looks really nice!
>
> Hans Verkuil wrote:
>> Whenever a control changes value an event is sent to anyone that
>> subscribed
>> to it.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>> drivers/media/video/v4l2-ctrls.c | 59 ++++++++++++++++++
>> drivers/media/video/v4l2-event.c | 126
>> +++++++++++++++++++++++++++-----------
>> drivers/media/video/v4l2-fh.c | 4 +-
>> include/linux/videodev2.h | 17 +++++-
>> include/media/v4l2-ctrls.h | 9 +++
>> include/media/v4l2-event.h | 2 +
>> 6 files changed, 177 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/media/video/v4l2-ctrls.c
>> b/drivers/media/video/v4l2-ctrls.c
>> index f75a1d4..163f412 100644
>> --- a/drivers/media/video/v4l2-ctrls.c
>> +++ b/drivers/media/video/v4l2-ctrls.c
>> @@ -23,6 +23,7 @@
>> #include <media/v4l2-ioctl.h>
>> #include <media/v4l2-device.h>
>> #include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-event.h>
>> #include <media/v4l2-dev.h>
>>
>> /* Internal temporary helper struct, one for each v4l2_ext_control */
>> @@ -537,6 +538,16 @@ static bool type_is_int(const struct v4l2_ctrl
>> *ctrl)
>> }
>> }
>>
>> +static void send_event(struct v4l2_ctrl *ctrl, struct v4l2_event *ev)
>> +{
>> + struct v4l2_ctrl_fh *pos;
>> +
>> + ev->id = ctrl->id;
>> + list_for_each_entry(pos, &ctrl->fhs, node) {
>> + v4l2_event_queue_fh(pos->fh, ev);
>> + }
>
> No need for braces here.
>
>> +}
>> +
>> /* Helper function: copy the current control value back to the caller
>> */
>> static int cur_to_user(struct v4l2_ext_control *c,
>> struct v4l2_ctrl *ctrl)
>> @@ -626,20 +637,38 @@ static int new_to_user(struct v4l2_ext_control *c,
>> /* Copy the new value to the current value. */
>> static void new_to_cur(struct v4l2_ctrl *ctrl)
>> {
>> + struct v4l2_event ev;
>> + bool changed = false;
>> +
>> if (ctrl == NULL)
>> return;
>> switch (ctrl->type) {
>> + case V4L2_CTRL_TYPE_BUTTON:
>> + changed = true;
>> + ev.u.ctrl_ch_value.value = 0;
>> + break;
>> case V4L2_CTRL_TYPE_STRING:
>> /* strings are always 0-terminated */
>> + changed = strcmp(ctrl->string, ctrl->cur.string);
>> strcpy(ctrl->cur.string, ctrl->string);
>> + ev.u.ctrl_ch_value.value64 = 0;
>> break;
>> case V4L2_CTRL_TYPE_INTEGER64:
>> + changed = ctrl->val64 != ctrl->cur.val64;
>> ctrl->cur.val64 = ctrl->val64;
>> + ev.u.ctrl_ch_value.value64 = ctrl->val64;
>> break;
>> default:
>> + changed = ctrl->val != ctrl->cur.val;
>> ctrl->cur.val = ctrl->val;
>> + ev.u.ctrl_ch_value.value = ctrl->val;
>> break;
>> }
>> + if (changed) {
>> + ev.type = V4L2_EVENT_CTRL_CH_VALUE;
>> + ev.u.ctrl_ch_value.type = ctrl->type;
>> + send_event(ctrl, &ev);
>> + }
>> }
>>
>> /* Copy the current value to the new value */
>> @@ -784,6 +813,7 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler
>> *hdl)
>> {
>> struct v4l2_ctrl_ref *ref, *next_ref;
>> struct v4l2_ctrl *ctrl, *next_ctrl;
>> + struct v4l2_ctrl_fh *ctrl_fh, *next_ctrl_fh;
>>
>> if (hdl == NULL || hdl->buckets == NULL)
>> return;
>> @@ -797,6 +827,10 @@ void v4l2_ctrl_handler_free(struct
>> v4l2_ctrl_handler *hdl)
>> /* Free all controls owned by the handler */
>> list_for_each_entry_safe(ctrl, next_ctrl, &hdl->ctrls, node) {
>> list_del(&ctrl->node);
>> + list_for_each_entry_safe(ctrl_fh, next_ctrl_fh, &ctrl->fhs, node) {
>> + list_del(&ctrl_fh->node);
>> + kfree(ctrl_fh);
>> + }
>> kfree(ctrl);
>> }
>> kfree(hdl->buckets);
>> @@ -1003,6 +1037,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
>> v4l2_ctrl_handler *hdl,
>> }
>>
>> INIT_LIST_HEAD(&ctrl->node);
>> + INIT_LIST_HEAD(&ctrl->fhs);
>> ctrl->handler = hdl;
>> ctrl->ops = ops;
>> ctrl->id = id;
>> @@ -1888,3 +1923,27 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32
>> val)
>> return set_ctrl(ctrl, &val);
>> }
>> EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
>> +
>> +void v4l2_ctrl_add_fh(struct v4l2_ctrl *ctrl, struct v4l2_ctrl_fh
>> *ctrl_fh)
>> +{
>> + v4l2_ctrl_lock(ctrl);
>> + list_add_tail(&ctrl_fh->node, &ctrl->fhs);
>> + v4l2_ctrl_unlock(ctrl);
>> +}
>> +EXPORT_SYMBOL(v4l2_ctrl_add_fh);
>> +
>> +void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh)
>> +{
>> + struct v4l2_ctrl_fh *pos;
>> +
>> + v4l2_ctrl_lock(ctrl);
>> + list_for_each_entry(pos, &ctrl->fhs, node) {
>> + if (pos->fh == fh) {
>> + list_del(&pos->node);
>> + kfree(pos);
>> + break;
>> + }
>> + }
>> + v4l2_ctrl_unlock(ctrl);
>> +}
>> +EXPORT_SYMBOL(v4l2_ctrl_del_fh);
>> diff --git a/drivers/media/video/v4l2-event.c
>> b/drivers/media/video/v4l2-event.c
>> index 69fd343..c9251a5 100644
>> --- a/drivers/media/video/v4l2-event.c
>> +++ b/drivers/media/video/v4l2-event.c
>> @@ -25,10 +25,13 @@
>> #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>
>>
>> +static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
>> +
>> int v4l2_event_init(struct v4l2_fh *fh)
>> {
>> fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
>> @@ -91,7 +94,7 @@ void v4l2_event_free(struct v4l2_fh *fh)
>>
>> list_kfree(&events->free, struct v4l2_kevent, list);
>> list_kfree(&events->available, struct v4l2_kevent, list);
>> - list_kfree(&events->subscribed, struct v4l2_subscribed_event, list);
>> + v4l2_event_unsubscribe_all(fh);
>>
>> kfree(events);
>> fh->events = NULL;
>> @@ -154,9 +157,9 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct
>> v4l2_event *event,
>> }
>> EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
>>
>> -/* Caller must hold fh->event->lock! */
>> +/* Caller must hold fh->vdev->fh_lock! */
>> static struct v4l2_subscribed_event *v4l2_event_subscribed(
>> - struct v4l2_fh *fh, u32 type)
>> + struct v4l2_fh *fh, u32 type, u32 id)
>> {
>> struct v4l2_events *events = fh->events;
>> struct v4l2_subscribed_event *sev;
>> @@ -164,13 +167,46 @@ static struct v4l2_subscribed_event
>> *v4l2_event_subscribed(
>> assert_spin_locked(&fh->vdev->fh_lock);
>>
>> list_for_each_entry(sev, &events->subscribed, list) {
>> - if (sev->type == type)
>> + if (sev->type == type && sev->id == id)
>> return sev;
>> }
>>
>> return NULL;
>> }
>>
>> +static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct
>> v4l2_event *ev,
>> + const struct timespec *ts)
>> +{
>> + struct v4l2_events *events = fh->events;
>> + struct v4l2_subscribed_event *sev;
>> + struct v4l2_kevent *kev;
>> +
>> + /* Are we subscribed? */
>> + sev = v4l2_event_subscribed(fh, ev->type, ev->id);
>> + if (sev == NULL)
>> + return;
>> +
>> + /* Increase event sequence number on fh. */
>> + events->sequence++;
>> +
>> + /* Do we have any free events? */
>> + if (list_empty(&events->free))
>> + return;
>> +
>> + /* Take one and fill it. */
>> + kev = list_first_entry(&events->free, struct v4l2_kevent, list);
>> + kev->event.type = ev->type;
>> + kev->event.u = ev->u;
>> + kev->event.id = ev->id;
>> + kev->event.timestamp = *ts;
>> + kev->event.sequence = events->sequence;
>> + list_move_tail(&kev->list, &events->available);
>> +
>> + events->navailable++;
>> +
>> + wake_up_all(&events->wait);
>> +}
>> +
>> void v4l2_event_queue(struct video_device *vdev, const struct
>> v4l2_event *ev)
>> {
>> struct v4l2_fh *fh;
>> @@ -182,37 +218,26 @@ void v4l2_event_queue(struct video_device *vdev,
>> const struct v4l2_event *ev)
>> spin_lock_irqsave(&vdev->fh_lock, flags);
>>
>> list_for_each_entry(fh, &vdev->fh_list, list) {
>> - struct v4l2_events *events = fh->events;
>> - struct v4l2_kevent *kev;
>> -
>> - /* Are we subscribed? */
>> - if (!v4l2_event_subscribed(fh, ev->type))
>> - continue;
>> -
>> - /* Increase event sequence number on fh. */
>> - events->sequence++;
>> -
>> - /* Do we have any free events? */
>> - if (list_empty(&events->free))
>> - continue;
>> -
>> - /* Take one and fill it. */
>> - kev = list_first_entry(&events->free, struct v4l2_kevent, list);
>> - kev->event.type = ev->type;
>> - kev->event.u = ev->u;
>> - kev->event.timestamp = timestamp;
>> - kev->event.sequence = events->sequence;
>> - list_move_tail(&kev->list, &events->available);
>> -
>> - events->navailable++;
>> -
>> - wake_up_all(&events->wait);
>> + __v4l2_event_queue_fh(fh, ev, ×tamp);
>> }
>>
>> spin_unlock_irqrestore(&vdev->fh_lock, flags);
>> }
>> EXPORT_SYMBOL_GPL(v4l2_event_queue);
>>
>> +void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event
>> *ev)
>> +{
>> + unsigned long flags;
>> + struct timespec timestamp;
>> +
>> + ktime_get_ts(×tamp);
>> +
>> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> + __v4l2_event_queue_fh(fh, ev, ×tamp);
>> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_queue_fh);
>> +
>> int v4l2_event_pending(struct v4l2_fh *fh)
>> {
>> return fh->events->navailable;
>> @@ -223,7 +248,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>> struct v4l2_event_subscription *sub)
>> {
>> struct v4l2_events *events = fh->events;
>> - struct v4l2_subscribed_event *sev;
>> + struct v4l2_subscribed_event *sev, *found_ev;
>> + struct v4l2_ctrl *ctrl = NULL;
>> + struct v4l2_ctrl_fh *ctrl_fh = NULL;
>> unsigned long flags;
>>
>> if (fh->events == NULL) {
>> @@ -231,15 +258,31 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>> return -ENOMEM;
>> }
>>
>> + if (sub->type == V4L2_EVENT_CTRL_CH_VALUE) {
>> + ctrl = v4l2_ctrl_find(fh->ctrl_handler, sub->id);
>> + if (ctrl == NULL)
>> + return -EINVAL;
>> + }
>> +
>> sev = kmalloc(sizeof(*sev), GFP_KERNEL);
>> if (!sev)
>> return -ENOMEM;
>> + if (ctrl) {
>> + ctrl_fh = kzalloc(sizeof(*ctrl_fh), GFP_KERNEL);
>> + if (!ctrl_fh) {
>> + kfree(sev);
>> + return -ENOMEM;
>> + }
>> + ctrl_fh->fh = fh;
>> + }
>>
>> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>>
>> - if (v4l2_event_subscribed(fh, sub->type) == NULL) {
>> + found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
>> + if (!found_ev) {
>> INIT_LIST_HEAD(&sev->list);
>> sev->type = sub->type;
>> + sev->id = sub->id;
>>
>> list_add(&sev->list, &events->subscribed);
>> sev = NULL;
>> @@ -247,6 +290,10 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>
>> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>>
>> + /* v4l2_ctrl_add_fh uses a mutex, so do this outside the spin lock */
>> + if (!found_ev && ctrl)
>> + v4l2_ctrl_add_fh(ctrl, ctrl_fh);
>> +
>> kfree(sev);
>>
>> return 0;
>> @@ -256,6 +303,7 @@ EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
>> static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
>> {
>> struct v4l2_events *events = fh->events;
>> + struct v4l2_event_subscription sub;
>> struct v4l2_subscribed_event *sev;
>> unsigned long flags;
>>
>> @@ -265,11 +313,13 @@ static void v4l2_event_unsubscribe_all(struct
>> v4l2_fh *fh)
>> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> if (!list_empty(&events->subscribed)) {
>> sev = list_first_entry(&events->subscribed,
>> - struct v4l2_subscribed_event, list);
>> - list_del(&sev->list);
>> + struct v4l2_subscribed_event, list);
>> + sub.type = sev->type;
>> + sub.id = sev->id;
>> }
>> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> - kfree(sev);
>> + if (sev)
>> + v4l2_event_unsubscribe(fh, &sub);
>> } while (sev);
>> }
>>
>> @@ -286,11 +336,17 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>>
>> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>>
>> - sev = v4l2_event_subscribed(fh, sub->type);
>> + sev = v4l2_event_subscribed(fh, sub->type, sub->id);
>> if (sev != NULL)
>> list_del(&sev->list);
>>
>> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> + if (sev->type == V4L2_EVENT_CTRL_CH_VALUE) {
>> + struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
>> +
>> + if (ctrl)
>> + v4l2_ctrl_del_fh(ctrl, fh);
>> + }
>>
>> kfree(sev);
>>
>> diff --git a/drivers/media/video/v4l2-fh.c
>> b/drivers/media/video/v4l2-fh.c
>> index 8635011..c6aef84 100644
>> --- a/drivers/media/video/v4l2-fh.c
>> +++ b/drivers/media/video/v4l2-fh.c
>> @@ -93,10 +93,8 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>> {
>> if (fh->vdev == NULL)
>> return;
>> -
>> - fh->vdev = NULL;
>> -
>> v4l2_event_free(fh);
>> + fh->vdev = NULL;
>
> This looks like a bugfix.
But it isn't :-)
v4l2_event_free didn't use fh->vdev in the past, but now it does so the
order had to be swapped.
>
>> }
>> EXPORT_SYMBOL_GPL(v4l2_fh_exit);
>>
>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>> index 92d2fdd..f7238c1 100644
>> --- a/include/linux/videodev2.h
>> +++ b/include/linux/videodev2.h
>> @@ -1787,6 +1787,7 @@ struct v4l2_streamparm {
>> #define V4L2_EVENT_ALL 0
>> #define V4L2_EVENT_VSYNC 1
>> #define V4L2_EVENT_EOS 2
>> +#define V4L2_EVENT_CTRL_CH_VALUE 3
>> #define V4L2_EVENT_PRIVATE_START 0x08000000
>>
>> /* Payload for V4L2_EVENT_VSYNC */
>> @@ -1795,21 +1796,33 @@ struct v4l2_event_vsync {
>> __u8 field;
>> } __attribute__ ((packed));
>>
>> +/* Payload for V4L2_EVENT_CTRL_CH_VALUE */
>> +struct v4l2_event_ctrl_ch_value {
>> + __u32 type;
>
> type is enum v4l2_ctrl_type in struct v4l2_ctrl and struct v4l2_queryctrl.
Yes, but enum's are frowned upon these days in the public API.
>
>> + union {
>> + __s32 value;
>> + __s64 value64;
>> + };
>> +} __attribute__ ((packed));
>> +
>> struct v4l2_event {
>> __u32 type;
>> union {
>> struct v4l2_event_vsync vsync;
>> + struct v4l2_event_ctrl_ch_value ctrl_ch_value;
>> __u8 data[64];
>> } u;
>> __u32 pending;
>> __u32 sequence;
>> struct timespec timestamp;
>> - __u32 reserved[9];
>> + __u32 id;
>
> id is valid only for control related events. Shouldn't it be part of the
> control related structures instead, or another union for control related
> event types? E.g.
>
> struct {
> enum v4l2_ctrl_type id;
> union {
> struct v4l2_event_ctrl_ch_value ch_value;
> };
> } ctrl;
The problem with making this dependent of the type is that the core event
code would have to have a switch on the event type when it comes to
matching identifiers. By making it a core field it doesn't have to do
this. Also, this makes it available for use by private events as well.
It is important to keep the send-event core code as fast as possible since
it can be called from interrupt context.
So this is by design.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
