On Sun, 23 Feb 2025 at 18:10, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx
> > Fixes: b4012002f3a3 ("[media] uvcvideo: Add support for control events")
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> > 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
