Re: [PATCH 2/3] media: uvcvideo: Do not send events for not changed controls

[Ricardo Ribalda]

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