Message-ID: <CANiDSCvj4VVAcQOpR-u-BcnKA+2ifcuq_8ZML=BNOHT_55fBog@mail.gmail.com>
Date: Fri, 29 Nov 2024 14:41:52 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans Verkuil <hverkuil-cisco@...all.nl>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Hans de Goede <hdegoede@...hat.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>, 
	Mauro Carvalho Chehab <mchehab+samsung@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2 2/4] media: uvcvideo: Do not set an async control owned
 by other fh

On Fri, 29 Nov 2024 at 14:13, Hans Verkuil <hverkuil-cisco@...all.nl> wrote:
>
> On 29/11/2024 12:54, Ricardo Ribalda wrote:
> > On Fri, 29 Nov 2024 at 12:06, Laurent Pinchart
> > <laurent.pinchart@...asonboard.com> wrote:
> >>
> >> On Fri, Nov 29, 2024 at 11:59:27AM +0100, Ricardo Ribalda wrote:
> >>> On Fri, 29 Nov 2024 at 11:36, Hans Verkuil wrote:
> >>>> On 28/11/2024 23:33, Laurent Pinchart wrote:
> >>>>> On Thu, Nov 28, 2024 at 11:28:29PM +0100, Ricardo Ribalda wrote:
> >>>>>> On Thu, 28 Nov 2024 at 23:22, Laurent Pinchart wrote:
> >>>>>>>
> >>>>>>> Hi Ricardo,
> >>>>>>>
> >>>>>>> (CC'ing Hans Verkuil)
> >>>>>>>
> >>>>>>> Thank you for the patch.
> >>>>>>>
> >>>>>>> On Wed, Nov 27, 2024 at 12:14:50PM +0000, Ricardo Ribalda wrote:
> >>>>>>>> If a file handle is waiting for a response from an async control, avoid
> >>>>>>>> that other file handle operate with it.
> >>>>>>>>
> >>>>>>>> Without this patch, the first file handle will never get the event
> >>>>>>>> associated with that operation, which can lead to endless loops in
> >>>>>>>> applications. Eg:
> >>>>>>>> If an application A wants to change the zoom and to know when the
> >>>>>>>> operation has completed:
> >>>>>>>> it will open the video node, subscribe to the zoom event, change the
> >>>>>>>> control and wait for zoom to finish.
> >>>>>>>> If before the zoom operation finishes, another application B changes
> >>>>>>>> the zoom, the first app A will loop forever.
> >>>>>>>
> >>>>>>> Hans, the V4L2 specification isn't very clear on this. I see pros and
> >>>>>>> cons for both behaviours, with a preference for the current behaviour,
> >>>>>>> as with this patch the control will remain busy until the file handle is
> >>>>>>> closed if the device doesn't send the control event for any reason. What
> >>>>>>> do you think ?
> >>>>>>
> >>>>>> Just one small clarification. The same file handler can change the
> >>>>>> value of the async control as many times as it wants, even if the
> >>>>>> operation has not finished.
> >>>>>>
> >>>>>> It will be other file handles that will get -EBUSY if they try to use
> >>>>>> an async control with an unfinished operation started by another fh.
> >>>>>
> >>>>> Yes, I should have been more precised. If the device doesn't send the
> >>>>> control event, then all other file handles will be prevented from
> >>>>> setting the control until the file handle that set it first gets closed.
> >>>>
> >>>> I think I need a bit more background here:
> >>>>
> >>>> First of all, what is an asynchronous control in UVC? I think that means
> >>>> you can set it, but it takes time for that operation to finish, so you
> >>>> get an event later when the operation is done. So zoom and similar operations
> >>>> are examples of that.
> >>>>
> >>>> And only when the operation finishes will the control event be sent, correct?
> >>>
> >>> You are correct.  This diagrams from the spec is more or less clear:
> >>> https://ibb.co/MDGn7F3
> >>>
> >>>> While the operation is ongoing, if you query the control value, is that reporting
> >>>> the current position or the final position?
> >>>
> >>> I'd expect hardware will return either the current position, the start
> >>> position or the final position. I could not find anything in the spec
> >>> that points in one direction or the others.
> >>
> >> Figure 2-21 in UVC 1.5 indicates that the device should STALL the
> >> GET_CUR and SET_CUR requests if a state change is in progress.
> >>
> >>> And in the driver I believe that we might have a bug handling this
> >>> case (will send a patch if I can confirm it)
> >>> the zoom is at 0 and you set it 10
> >>> if you read the value 2 times before the camera reaches value 10:
> >>> - First value will come from the hardware and the response will be cached
> >>
> >> Only if the control doesn't have the auto-update flag set, so it will be
> >> device-dependent. As GET_CUR should stall that's not really relevant,
> >> except for the fact that devices may not stall the request.
> >
> > I missed that the device will likely stall during async operations.
> >
> > What do you think of something like this? I believe it can work with
> > compliant and non compliant devices.
> > Note that the event will be received by the device that originated the
> > operation, not to the second one that might receive an error during
> > write/read.
> >
> >
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 4fe26e82e3d1..9a86c912e7a2 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1826,14 +1826,15 @@ static int uvc_ctrl_commit_entity(struct
> > uvc_device *dev,
> >                         continue;
> >
> >                 /*
> > -                * Reset the loaded flag for auto-update controls that were
> > +                * Reset the loaded flag for auto-update controls and for
> > +                * asynchronous controls with pending operations, that were
> >                  * marked as loaded in uvc_ctrl_get/uvc_ctrl_set to prevent
> >                  * uvc_ctrl_get from using the cached value, and for write-only
> >                  * controls to prevent uvc_ctrl_set from setting bits not
> >                  * explicitly set by the user.
> >                  */
> >                 if (ctrl->info.flags & UVC_CTRL_FLAG_AUTO_UPDATE ||
> > -                   !(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> > +                   !(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) || ctrl->handle)
> >                         ctrl->loaded = 0;
> >
> >                 if (!ctrl->dirty)
> > @@ -2046,8 +2047,18 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> >         mapping->set(mapping, value,
> >                 uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> >
> > -       if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > -               ctrl->handle = handle;
> > +       if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) {
> > +               /*
> > +                * Other file handle is waiting for an operation on
> > +                * this asynchronous control. If the device is compliant
> > +                * this operation will fail.
> > +                *
> > +                * Do not replace the handle pointer, so the original file
> > +                * descriptor will get the completion event.
> > +                */
> > +               if (!ctrl->handle)
> > +                       ctrl->handle = handle;
>
> I don't think this is right: you want the completion event for async
> controls to go to all filehandles that are subscribed to that control.
>
> Which is what happens if handle == NULL (as I understand the code).
>
> Regards,

The code is correct, but the comment is not :). It should say:
 * Do not replace the handle pointer, or the originator of
 * the operation will receive an event.

The originar should NOT receive the event.
>From uvc_ctrl_send_event():
/*
 * 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.
 */



>
>         Hans
>
> > +       }
> >
> >         ctrl->dirty = 1;
> >         ctrl->modified = 1;
> >
> >>
> >>> - Second value will be the cached one
> >>>
> >>> now the camera  is at zoom 10
> >>> If you read the value, you will read the cached value
> >>>
> >>>> E.g.: the zoom control is at value 0 and I set it to 10, then I poll the zoom control
> >>>> value: will that report the intermediate values until it reaches 10? And when it is
> >>>> at 10, the control event is sent?
> >>>>
> >>>>>>>> Cc: stable@...r.kernel.org
> >>>>>>>> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
> >>>>>>>> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> >>>>>>>> ---
> >>>>>>>>  drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++
> >>>>>>>>  1 file changed, 4 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> >>>>>>>> index b6af4ff92cbd..3f8ae35cb3bc 100644
> >>>>>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> >>>>>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> >>>>>>>> @@ -1955,6 +1955,10 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> >>>>>>>>       if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
> >>>>>>>>               return -EACCES;
> >>>>>>>>
> >>>>>>>> +     /* Other file handle is waiting a response from this async control. */
> >>>>>>>> +     if (ctrl->handle && ctrl->handle != handle)
> >>>>>>>> +             return -EBUSY;
> >>>>>>>> +
> >>>>>>>>       /* Clamp out of range values. */
> >>>>>>>>       switch (mapping->v4l2_type) {
> >>>>>>>>       case V4L2_CTRL_TYPE_INTEGER:
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
> >
> >
> >
>


-- 
Ricardo Ribalda

Powered by blists - more mailing lists