Re: [PATCH v7 1/2] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Guennadi,

On Thursday, 26 April 2018 12:28:42 EEST Guennadi Liakhovetski wrote:
> On Tue, 10 Apr 2018, Guennadi Liakhovetski wrote:
> 
> [snip]
> 
> >>> @@ -1488,6 +1591,25 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
> >>>  		return -EINVAL;
> >>>  	if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
> >>>  		return -EACCES;
> >>> +	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) {
> >>> +		if (ctrl->handle)
> >>> +			/*
> >>> +			 * We have already sent this control to the camera
> >>> +			 * recently and are currently waiting for a completion
> >>> +			 * notification. The camera might already have completed
> >>> +			 * its processing and is ready to accept a new control
> >>> +			 * or it's still busy processing. If we send a new
> >>> +			 * instance of this control now, in the former case the
> >>> +			 * camera will process this one too and we'll get
> >>> +			 * completions for both, but we will only deliver an
> >>> +			 * event for one of them back to the user. In the latter
> >>> +			 * case the camera will reply with a STALL. It's easier
> >>> +			 * and more reliable to return an error now and let the
> >>> +			 * user retry.
> >>> +			 */
> >>> +			return -EBUSY;
> >>> +		ctrl->handle = handle;
> >> 
> >> This part worries me. If the control change event isn't received for any
> >> reason (such as a buggy device for instance, or uvc_ctrl_status_event()
> >> being called with the previous event not processed yet), the control
> >> will stay busy forever.
> >> 
> >> I see two approaches to fix this. One would be to forward all received
> >> control change events to all file handles unconditionally and remove
> >> the handle field from the uvc_control structure.
> > 
> > How is this a solution? A case of senging a repeated control to the camera
> > and causing a STALL would still be possible. If you prefer STALLs, you
> > could just remove this check here.
> > 
> >> Another one would be to add a timeout, storing
> >> the time at which the control has been set in the uvc_control structure,
> >> and checking whether the time difference exceeds a fixed timeout here.
> >> We could also combine the two, replacing the handle field with a
> >> timestamp field.
> > 
> > Don't think you can remove the handle field, there are a couple of things,
> > that need it, also you'll have to send events to all listeners, including
> > the thread, that has sent the control, which contradicts the API? Assuming
> > it hasn't set the V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag.
> > 
> > I can add a timeout, even though that doesn't seem to be very clean to me.
> > According to the UVC standard some controls can take a while to complete,
> > possibly seconds. How long would you propose that timeout to be?
> 
> How about just not checking when setting control and letting the camera
> decide? That's what the STALL mechanism is there for. The only advantage
> of using this is to provide a short-cut when we're "sure," that the
> hardware isn't ready to take a new request yet, but if implementing such a
> shortcut becomes too cumbersome, we can give control back to the hardware.

I'm fine with that, we can always implement a more complex mechanism later if 
it ends up being needed. It would be nice if the STALL could result in -EBUSY 
being returned in that case (through patch 2/2 for instance).

-- 
Regards,

Laurent Pinchart