Message-ID: <e95b7d74-2c56-4f5a-a2f2-9c460d52fdb4@xs4all.nl>
Date: Fri, 29 Nov 2024 14:13:16 +0100
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Ricardo Ribalda <ribalda@...omium.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: 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 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,

	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
> 
> 
> 

Powered by blists - more mailing lists