On 29/11/2024 12:54, Ricardo Ribalda wrote:
On Fri, 29 Nov 2024 at 12:06, Laurent Pinchart laurent.pinchart@ideasonboard.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@vger.kernel.org >> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") >> Signed-off-by: Ricardo Ribalda ribalda@chromium.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