On 29/11/2024 12:06, Laurent Pinchart 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. OK, that helps a lot. If an operation is in progress, then setting a new control value should result in -EBUSY. Based on the description above, I gather that even the same fh that made the request cannot update it while the operation is ongoing? Getting the control should just return the value that was set. I assume that is cached in uvc? Regards, Hans > >> - 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@xxxxxxxxxxxxxxx >>>>>>> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") >>>>>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> >>>>>>> --- >>>>>>> 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: >
