> Hi Hans,
>
> I have some more comments below. :-)
>
> 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);
>> + }
>
> Shouldn't we do v4l2_ctrl_lock(ctrl) here? Or does something prevent
> changes to the file handle list while we loop over it?
This function is always called with the lock taken.
> v4l2_ctrl_lock() locks a mutex. Events often arrive from interrupt
> context, which would mean the drivers would need to create a work queue
> to tell about changes to control values.
I will have to check whether it is possible to make a function that can be
called from interrupt context. I have my doubts though whether it is 1)
possible and 2) desirable. At least in the area of HDMI
receivers/transmitters you will want to have a workqueue anyway.
>
>> +}
>> +
>> /* 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);
>> + }
>
> Wouldn't this also require holding v4l2_ctrl_lock(ctrl), since otherwise
> the file handle list may change in the middle of doing this?
>
> Then we'd hold two mutexes at once. I'd guess it's fine as long as we're
> certain never do the locking of the two in a different order.
The lock is already taken. Note that the locking is done at the control
handler level. So there is no per-control lock. v4l2_ctrl_lock will
actually lock ctrl->handler->lock.
>
>> 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);
>
> Doesn't this allow adding two events for the same control id, if the
> v4l2_event_subscribed() call is performed when another is checking for
> the same control id before it has been added to the list in ctrl by
> v4l2_ctrl_add_fh()?
No, because only one of the two will get a found_ev == NULL result. The
check whether the event is already subscribed and adding the new event is
done with the spinlock held, so that is atomic.
Regards,
Hans
>
> ...
>
> Regards,
>
> --
> Sakari Ailus
> sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx
> --
> 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
>
--
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
