Only send events for controls that have actually changed.
E.g.: We are changing entities A, B and C. If we get an error while
we change B we do not continue setting C. But the current code sends an
event also for C.
Due to the fact that the controls are grouped by entities, and the user
might group them in different orders, we cannot send the events at the
end, but when we change an entity.
Cc: stable@kernel.org
Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_ctrl.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 7e2fc97c9f43..9496ac970267 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1669,7 +1669,9 @@ static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control *xctrls,
}
static void uvc_ctrl_send_events(struct uvc_fh *handle,
- const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+ struct uvc_entity *entity,
+ const struct v4l2_ext_control *xctrls,
+ unsigned int xctrls_count)
{
struct uvc_control_mapping *mapping;
struct uvc_control *ctrl;
@@ -1680,6 +1682,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
for (i = 0; i < xctrls_count; ++i) {
ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
+ if (ctrl->entity != entity)
+ continue;
+
if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
/* Notification will be sent from an Interrupt event. */
continue;
@@ -1911,11 +1916,12 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
uvc_ctrl_find_ctrl_idx(entity, ctrls,
err_ctrl);
goto done;
+ } else if (ret > 0 && !rollback) {
+ uvc_ctrl_send_events(handle, entity,
+ ctrls->controls, ctrls->count);
}
}
- if (!rollback)
- uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
done:
mutex_unlock(&chain->ctrl_mutex);
return ret < 0 ? ret : 0;
--
2.47.0.338.g60cca15819-googHi Ricardo,
Thank you for the patch.
On Tue, Dec 10, 2024 at 10:22:23PM +0000, Ricardo Ribalda wrote:
> Only send events for controls that have actually changed.
> E.g.: We are changing entities A, B and C. If we get an error while
> we change B we do not continue setting C. But the current code sends an
> event also for C.
Does it ? If uvc_ctrl_commit_entity() fails the 'goto done' statement
skips over uvc_ctrl_send_events().
> Due to the fact that the controls are grouped by entities, and the user
> might group them in different orders, we cannot send the events at the
> end, but when we change an entity.
>
> Cc: stable@kernel.org
> Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 7e2fc97c9f43..9496ac970267 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1669,7 +1669,9 @@ static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control *xctrls,
> }
>
> static void uvc_ctrl_send_events(struct uvc_fh *handle,
> - const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> + struct uvc_entity *entity,
> + const struct v4l2_ext_control *xctrls,
> + unsigned int xctrls_count)
> {
> struct uvc_control_mapping *mapping;
> struct uvc_control *ctrl;
> @@ -1680,6 +1682,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
> for (i = 0; i < xctrls_count; ++i) {
> ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
>
> + if (ctrl->entity != entity)
> + continue;
> +
> if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> /* Notification will be sent from an Interrupt event. */
> continue;
> @@ -1911,11 +1916,12 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> uvc_ctrl_find_ctrl_idx(entity, ctrls,
> err_ctrl);
> goto done;
> + } else if (ret > 0 && !rollback) {
> + uvc_ctrl_send_events(handle, entity,
> + ctrls->controls, ctrls->count);
> }
> }
>
> - if (!rollback)
> - uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
> done:
> mutex_unlock(&chain->ctrl_mutex);
> return ret < 0 ? ret : 0;
--
Regards,
Laurent Pinchart
On Sun, 23 Feb 2025 at 18:10, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Tue, Dec 10, 2024 at 10:22:23PM +0000, Ricardo Ribalda wrote:
> > Only send events for controls that have actually changed.
> > E.g.: We are changing entities A, B and C. If we get an error while
> > we change B we do not continue setting C. But the current code sends an
> > event also for C.
>
> Does it ? If uvc_ctrl_commit_entity() fails the 'goto done' statement
> skips over uvc_ctrl_send_events().
I have no idea how that commit message ended up there :).
I mean, the code is correct (I believe), but it does not do what the
commit message says. I guess I was trying something different and
forgot to modify the commit message.
Please check v2. Sorry about that
>
> > Due to the fact that the controls are grouped by entities, and the user
> > might group them in different orders, we cannot send the events at the
> > end, but when we change an entity.
> >
> > Cc: stable@kernel.org
> > Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 7e2fc97c9f43..9496ac970267 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1669,7 +1669,9 @@ static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control *xctrls,
> > }
> >
> > static void uvc_ctrl_send_events(struct uvc_fh *handle,
> > - const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> > + struct uvc_entity *entity,
> > + const struct v4l2_ext_control *xctrls,
> > + unsigned int xctrls_count)
> > {
> > struct uvc_control_mapping *mapping;
> > struct uvc_control *ctrl;
> > @@ -1680,6 +1682,9 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
> > for (i = 0; i < xctrls_count; ++i) {
> > ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
> >
> > + if (ctrl->entity != entity)
> > + continue;
> > +
> > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > /* Notification will be sent from an Interrupt event. */
> > continue;
> > @@ -1911,11 +1916,12 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > uvc_ctrl_find_ctrl_idx(entity, ctrls,
> > err_ctrl);
> > goto done;
> > + } else if (ret > 0 && !rollback) {
> > + uvc_ctrl_send_events(handle, entity,
> > + ctrls->controls, ctrls->count);
> > }
> > }
> >
> > - if (!rollback)
> > - uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
> > done:
> > mutex_unlock(&chain->ctrl_mutex);
> > return ret < 0 ? ret : 0;
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
© 2016 - 2025 Red Hat, Inc.