Re: [PATCH v9] uvcvideo: send a control event when a Control Change interrupt arrives
Hi Laurent, Thanks. Now we can get to the next one: https://patchwork.linuxtv.org/patch/46184/ - without that one nobody can get complete D4XX metadata. After I get your comments to it I'll address them together with Sakari's ones. Thanks Guennadi On Thu, 26 Jul 2018, Laurent Pinchart wrote: > Hi Guennadi, > > Thank you for the patch. > > On Thursday, 26 July 2018 11:17:53 EEST Guennadi Liakhovetski wrote: > > From: Guennadi Liakhovetski > > > > UVC defines a method of handling asynchronous controls, which sends a > > USB packet over the interrupt pipe. This patch implements support for > > such packets by sending a control event to the user. Since this can > > involve USB traffic and, therefore, scheduling, this has to be done > > in a work queue. > > > > Signed-off-by: Guennadi Liakhovetski > > Reviewed-by: Laurent Pinchart > > applied to my tree and pushed to the uvc/next branch. > > > --- > > > > v9: > > - multiple optimisations and style improvements from Laurent - thanks > > - avoid the handle rewriting race at least in cases, when the user only > > sends a new control after receiving an event for the previous one > > > > Laurent, you added a couple of comments, using DocBook markup like > > "@param" but you didn't mark those comments for DocBook processing with > > "/**" - I don't care that much, just wanted to check, that that was > > intentional. > > Yes, it's not worth compiling that as kerneldoc, I just wanted to mark > references to variable names for clarity. > > > drivers/media/usb/uvc/uvc_ctrl.c | 211 > > - drivers/media/usb/uvc/uvc_status.c | > > 121 ++--- > > drivers/media/usb/uvc/uvc_v4l2.c | 4 +- > > drivers/media/usb/uvc/uvcvideo.h | 15 ++- > > include/uapi/linux/uvcvideo.h | 2 + > > 5 files changed, 286 insertions(+), 67 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > > b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..ddd069b 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -20,6 +20,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > > > @@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct > > uvc_video_chain *chain, return 0; > > } > > > > +static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > + const u8 *data) > > +{ > > + s32 value = mapping->get(mapping, UVC_GET_CUR, data); > > + > > + if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) { > > + struct uvc_menu_info *menu = mapping->menu_info; > > + unsigned int i; > > + > > + for (i = 0; i < mapping->menu_count; ++i, ++menu) { > > + if (menu->value == value) { > > + value = i; > > + break; > > + } > > + } > > + } > > + > > + return value; > > +} > > + > > static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > s32 *value) > > { > > - struct uvc_menu_info *menu; > > - unsigned int i; > > int ret; > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > @@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain > > *chain, ctrl->loaded = 1; > > } > > > > - *value = mapping->get(mapping, UVC_GET_CUR, > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > - > > - if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) { > > - menu = mapping->menu_info; > > - for (i = 0; i < mapping->menu_count; ++i, ++menu) { > > - if (menu->value == *value) { > > - *value = i; > > - break; > > - } > > - } > > - } > > + *value = __uvc_ctrl_get_value(mapping, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > return 0; > > } > > @@ -1216,53 +1225,135 @@ static void uvc_ctrl_fill_event(struct > > uvc_video_chain *chain, ev->u.ctrl.default_value = v4l2_ctrl.default_value; > > } > > > > -static void uvc_ctrl_send_event(struct uvc_fh *handle, > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > - s32 value, u32 changes) > > +/* > > + * Send control change events to all subscribers for the @ctrl control. By > > + * default the subscriber that generated the event, as identified by > > @handle, + * is not notified unless it has set the > > V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag. + * @handle can be NULL for > > asynchronous events related to auto-update controls, + * in which case all > > subscribers are notified. > > + */ > > +static void uvc_ctrl_send_event(struct uvc_video_chain *chain, > > + struct uvc_fh *handle, struct uvc_control *ctrl, > > + struct uvc_control_mapping *mapping, s32 value, u32 changes) > > { > > + struct v4l2_fh *originator =
Re: [PATCH v9] uvcvideo: send a control event when a Control Change interrupt arrives
Hi Guennadi, Thank you for the patch. On Thursday, 26 July 2018 11:17:53 EEST Guennadi Liakhovetski wrote: > From: Guennadi Liakhovetski > > UVC defines a method of handling asynchronous controls, which sends a > USB packet over the interrupt pipe. This patch implements support for > such packets by sending a control event to the user. Since this can > involve USB traffic and, therefore, scheduling, this has to be done > in a work queue. > > Signed-off-by: Guennadi Liakhovetski Reviewed-by: Laurent Pinchart applied to my tree and pushed to the uvc/next branch. > --- > > v9: > - multiple optimisations and style improvements from Laurent - thanks > - avoid the handle rewriting race at least in cases, when the user only > sends a new control after receiving an event for the previous one > > Laurent, you added a couple of comments, using DocBook markup like > "@param" but you didn't mark those comments for DocBook processing with > "/**" - I don't care that much, just wanted to check, that that was > intentional. Yes, it's not worth compiling that as kerneldoc, I just wanted to mark references to variable names for clarity. > drivers/media/usb/uvc/uvc_ctrl.c | 211 > - drivers/media/usb/uvc/uvc_status.c | > 121 ++--- > drivers/media/usb/uvc/uvc_v4l2.c | 4 +- > drivers/media/usb/uvc/uvcvideo.h | 15 ++- > include/uapi/linux/uvcvideo.h | 2 + > 5 files changed, 286 insertions(+), 67 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..ddd069b 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct > uvc_video_chain *chain, return 0; > } > > +static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > + const u8 *data) > +{ > + s32 value = mapping->get(mapping, UVC_GET_CUR, data); > + > + if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) { > + struct uvc_menu_info *menu = mapping->menu_info; > + unsigned int i; > + > + for (i = 0; i < mapping->menu_count; ++i, ++menu) { > + if (menu->value == value) { > + value = i; > + break; > + } > + } > + } > + > + return value; > +} > + > static int __uvc_ctrl_get(struct uvc_video_chain *chain, > struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > s32 *value) > { > - struct uvc_menu_info *menu; > - unsigned int i; > int ret; > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > @@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain > *chain, ctrl->loaded = 1; > } > > - *value = mapping->get(mapping, UVC_GET_CUR, > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > - > - if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) { > - menu = mapping->menu_info; > - for (i = 0; i < mapping->menu_count; ++i, ++menu) { > - if (menu->value == *value) { > - *value = i; > - break; > - } > - } > - } > + *value = __uvc_ctrl_get_value(mapping, > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > return 0; > } > @@ -1216,53 +1225,135 @@ static void uvc_ctrl_fill_event(struct > uvc_video_chain *chain, ev->u.ctrl.default_value = v4l2_ctrl.default_value; > } > > -static void uvc_ctrl_send_event(struct uvc_fh *handle, > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > - s32 value, u32 changes) > +/* > + * Send control change events to all subscribers for the @ctrl control. By > + * default the subscriber that generated the event, as identified by > @handle, + * is not notified unless it has set the > V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag. + * @handle can be NULL for > asynchronous events related to auto-update controls, + * in which case all > subscribers are notified. > + */ > +static void uvc_ctrl_send_event(struct uvc_video_chain *chain, > + struct uvc_fh *handle, struct uvc_control *ctrl, > + struct uvc_control_mapping *mapping, s32 value, u32 changes) > { > + struct v4l2_fh *originator = handle ? &handle->vfh : NULL; > struct v4l2_subscribed_event *sev; > struct v4l2_event ev; > > if (list_empty(&mapping->ev_subs)) > return; > > - uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes); > + uvc_ctrl_fill_event(chain, &ev, ctrl, mapping, value, changes); > > list_for_each_entry(sev, &mapping->ev_subs, node) { > - if (sev->fh != &handle->vfh || > + if (sev->fh != originat
[PATCH v9] uvcvideo: send a control event when a Control Change interrupt arrives
From: Guennadi Liakhovetski UVC defines a method of handling asynchronous controls, which sends a USB packet over the interrupt pipe. This patch implements support for such packets by sending a control event to the user. Since this can involve USB traffic and, therefore, scheduling, this has to be done in a work queue. Signed-off-by: Guennadi Liakhovetski --- v9: - multiple optimisations and style improvements from Laurent - thanks - avoid the handle rewriting race at least in cases, when the user only sends a new control after receiving an event for the previous one Laurent, you added a couple of comments, using DocBook markup like "@param" but you didn't mark those comments for DocBook processing with "/**" - I don't care that much, just wanted to check, that that was intentional. drivers/media/usb/uvc/uvc_ctrl.c | 211 - drivers/media/usb/uvc/uvc_status.c | 121 ++--- drivers/media/usb/uvc/uvc_v4l2.c | 4 +- drivers/media/usb/uvc/uvcvideo.h | 15 ++- include/uapi/linux/uvcvideo.h | 2 + 5 files changed, 286 insertions(+), 67 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..ddd069b 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain, return 0; } +static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, + const u8 *data) +{ + s32 value = mapping->get(mapping, UVC_GET_CUR, data); + + if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) { + struct uvc_menu_info *menu = mapping->menu_info; + unsigned int i; + + for (i = 0; i < mapping->menu_count; ++i, ++menu) { + if (menu->value == value) { + value = i; + break; + } + } + } + + return value; +} + static int __uvc_ctrl_get(struct uvc_video_chain *chain, struct uvc_control *ctrl, struct uvc_control_mapping *mapping, s32 *value) { - struct uvc_menu_info *menu; - unsigned int i; int ret; if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) @@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, ctrl->loaded = 1; } - *value = mapping->get(mapping, UVC_GET_CUR, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); - - if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) { - menu = mapping->menu_info; - for (i = 0; i < mapping->menu_count; ++i, ++menu) { - if (menu->value == *value) { - *value = i; - break; - } - } - } + *value = __uvc_ctrl_get_value(mapping, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); return 0; } @@ -1216,53 +1225,135 @@ static void uvc_ctrl_fill_event(struct uvc_video_chain *chain, ev->u.ctrl.default_value = v4l2_ctrl.default_value; } -static void uvc_ctrl_send_event(struct uvc_fh *handle, - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, - s32 value, u32 changes) +/* + * Send control change events to all subscribers for the @ctrl control. By + * default the subscriber that generated the event, as identified by @handle, + * is not notified unless it has set the V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag. + * @handle can be NULL for asynchronous events related to auto-update controls, + * in which case all subscribers are notified. + */ +static void uvc_ctrl_send_event(struct uvc_video_chain *chain, + struct uvc_fh *handle, struct uvc_control *ctrl, + struct uvc_control_mapping *mapping, s32 value, u32 changes) { + struct v4l2_fh *originator = handle ? &handle->vfh : NULL; struct v4l2_subscribed_event *sev; struct v4l2_event ev; if (list_empty(&mapping->ev_subs)) return; - uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes); + uvc_ctrl_fill_event(chain, &ev, ctrl, mapping, value, changes); list_for_each_entry(sev, &mapping->ev_subs, node) { - if (sev->fh != &handle->vfh || + if (sev->fh != originator || (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) || (changes & V4L2_EVENT_CTRL_CH_FLAGS)) v4l2_event_queue_fh(sev->fh, &ev); } } -static void uvc_ctrl_send_slave_event(struct uvc_fh *handle, - struct uvc_control *master, u32 slave_id, - const struct v4l2_ext_control *xctrls, unsigned int xctrls_count) +/* + * Send control chan