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

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

Hi Laurent,

One more comment-to-comment:

On Mon, 7 May 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday, 10 April 2018 14:31:35 EEST Guennadi Liakhovetski wrote:
> > On Fri, 23 Mar 2018, Laurent Pinchart wrote:
> > > On Friday, 23 March 2018 11:24:00 EET Laurent Pinchart wrote:
> > >> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > >> 
> > >> UVC defines a method of handling asynchronous controls, which sends a
> > >> USB packet over the interrupt pipe. This patch implements support for
> > >> such packets by sending a control event to the user. Since this can
> > >> involve USB traffic and, therefore, scheduling, this has to be done
> > >> in a work queue.
> > >> 
> > >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > >> ---
> > >> 
> > >>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 +++++++++++++++++++++++++++---
> > >>  drivers/media/usb/uvc/uvc_status.c | 111 ++++++++++++++++++++++---
> > >>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> > >>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> > >>  include/uapi/linux/uvcvideo.h      |   2 +
> > >>  5 files changed, 269 insertions(+), 29 deletions(-)

[snip]

> > >> diff --git a/drivers/media/usb/uvc/uvc_status.c
> > >> b/drivers/media/usb/uvc/uvc_status.c index 7b710410584a..d1d83aed6a1d
> > >> 100644
> > >> --- a/drivers/media/usb/uvc/uvc_status.c
> > >> +++ b/drivers/media/usb/uvc/uvc_status.c

[snip]

> > >> +				ctrl = uvc_event_entity_ctrl(entity,
> > >> +							     status->bSelector);
> > >> +				/*
> > >> +				 * Some buggy cameras send asynchronous Control
> > >> +				 * Change events for control, other than the
> > >> +				 * ones, that had been changed, even though the
> > >> +				 * AutoUpdate flag isn't set for the control.
> > >> +				 */
> > > 
> > > That's lots of commas, I'm not sure what you mean here. Are there cameras
> > > that send event for controls that haven't changed ? Or cameras that send
> > > events for controls that don't have the auto-update flag set ? Do you
> > > know what cameras are affected ?
> > 
> > I meant a case like
> > 
> > set_control(x=X)
> > interrupt(x=X)
> > interrupt(y=Y)
> > 
> > where y is a different control and it doesn't have an auto-update flag
> > set. I think those were some early versions of our cameras, but as far as
> > I can see, we need the check anyway for autoupdate controls, so, I can
> > just remove the comment.
> 
> OK. But now that I read the comment again, how is it related to the check ? 
> Why do you need ctrl->handle->chain == *chain ? Isn't that only a partial 
> guard for the case above, as both x and y could be part of the same chain ?

The uvc_event_entity_ctrl() function checks the selector, so, a selector + 
chain should be unique?

Thanks
Guennadi