[5/5] usb: gadget/uvc: Add support for 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command

This patch adds the support in UVC webcam gadget design for providing
USB_GADGET_DELAYED_STATUS in response to a set_interface(alt setting 1) command
issue by the Host.

The current UVC webcam gadget design generates a STREAMON event corresponding
to a set_interface(alt setting 1) command from the Host. This STREAMON event
will eventually be routed to a real V4L2 device.

To start video streaming, it may be required to perform some register writes to
a camera sensor device over slow external busses like I2C or SPI. So, it makes
sense to ensure that we delay the STATUS stage of the
set_interface(alt setting 1) command.

Otherwise, a lot of ISOC IN tokens sent by the Host will be replied to by
zero-length packets by the webcam device. On certain Hosts this may even lead
to ISOC URBs been cancelled from the Host side.

So, as soon as we finish doing all the "streaming" related stuff on the real
V4L2 device, we call a STREAMON ioctl on the UVC side and from here we call the
'usb_composite_setup_continue' function to complete the status stage of the
set_interface(alt setting 1) command.

Further, we need to ensure that we queue no video buffers on the UVC webcam
gadget, until we de-queue a video buffer from the V4L2 device. Also, we need to
enable UVC video related stuff at the first QBUF ioctl call itself,
as the application will call the STREAMON on UVC side only when it has
dequeued sufficient buffers from the V4L2 side and queued them to the UVC
gadget. So, the UVC video enable stuff cannot be done in STREAMON ioctl call.

For the same we add two more UVC states:
	- PRE_STREAMING : not even a single buffer has been queued to UVC
	- BUF_QUEUED_STREAMING_OFF : one video buffer has been queued to UVC
			but we have not yet enabled STREAMING on UVC side.

Signed-off-by: Bhupesh Sharma <bhupesh.sharma@st.com>
---
 drivers/usb/gadget/f_uvc.c    |   17 ++++++++++++-----
 drivers/usb/gadget/uvc.h      |    3 +++
 drivers/usb/gadget/uvc_v4l2.c |   38 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 51 insertions(+), 7 deletions(-)
  

Hi Laurent,

Can you please review this patch and let me know if any modifications are required..

> -----Original Message-----
> From: Bhupesh SHARMA
> Sent: Friday, June 01, 2012 3:09 PM
> To: laurent.pinchart@ideasonboard.com; linux-usb@vger.kernel.org
> Cc: balbi@ti.com; linux-media@vger.kernel.org;
> gregkh@linuxfoundation.org; Bhupesh SHARMA
> Subject: [PATCH 5/5] usb: gadget/uvc: Add support for
> 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command
> 
> This patch adds the support in UVC webcam gadget design for providing
> USB_GADGET_DELAYED_STATUS in response to a set_interface(alt setting 1)
> command
> issue by the Host.
> 
> The current UVC webcam gadget design generates a STREAMON event
> corresponding
> to a set_interface(alt setting 1) command from the Host. This STREAMON
> event
> will eventually be routed to a real V4L2 device.
> 
> To start video streaming, it may be required to perform some register
> writes to
> a camera sensor device over slow external busses like I2C or SPI. So,
> it makes
> sense to ensure that we delay the STATUS stage of the
> set_interface(alt setting 1) command.
> 
> Otherwise, a lot of ISOC IN tokens sent by the Host will be replied to
> by
> zero-length packets by the webcam device. On certain Hosts this may
> even lead
> to ISOC URBs been cancelled from the Host side.
> 
> So, as soon as we finish doing all the "streaming" related stuff on the
> real
> V4L2 device, we call a STREAMON ioctl on the UVC side and from here we
> call the
> 'usb_composite_setup_continue' function to complete the status stage of
> the
> set_interface(alt setting 1) command.
> 
> Further, we need to ensure that we queue no video buffers on the UVC
> webcam
> gadget, until we de-queue a video buffer from the V4L2 device. Also, we
> need to
> enable UVC video related stuff at the first QBUF ioctl call itself,
> as the application will call the STREAMON on UVC side only when it has
> dequeued sufficient buffers from the V4L2 side and queued them to the
> UVC
> gadget. So, the UVC video enable stuff cannot be done in STREAMON ioctl
> call.
> 
> For the same we add two more UVC states:
> 	- PRE_STREAMING : not even a single buffer has been queued to UVC
> 	- BUF_QUEUED_STREAMING_OFF : one video buffer has been queued to
> UVC
> 			but we have not yet enabled STREAMING on UVC side.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@st.com>
> ---
>  drivers/usb/gadget/f_uvc.c    |   17 ++++++++++++-----
>  drivers/usb/gadget/uvc.h      |    3 +++
>  drivers/usb/gadget/uvc_v4l2.c |   38
> ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> index 2a8bf06..3589ed0 100644
> --- a/drivers/usb/gadget/f_uvc.c
> +++ b/drivers/usb/gadget/f_uvc.c
> @@ -272,6 +272,13 @@ uvc_function_setup(struct usb_function *f, const
> struct usb_ctrlrequest *ctrl)
>  	return 0;
>  }
> 
> +void uvc_function_setup_continue(struct uvc_device *uvc)
> +{
> +	struct usb_composite_dev *cdev = uvc->func.config->cdev;
> +
> +	usb_composite_setup_continue(cdev);
> +}
> +
>  static int
>  uvc_function_get_alt(struct usb_function *f, unsigned interface)
>  {
> @@ -334,7 +341,8 @@ uvc_function_set_alt(struct usb_function *f,
> unsigned interface, unsigned alt)
>  		v4l2_event_queue(uvc->vdev, &v4l2_event);
> 
>  		uvc->state = UVC_STATE_CONNECTED;
> -		break;
> +
> +		return 0;
> 
>  	case 1:
>  		if (uvc->state != UVC_STATE_CONNECTED)
> @@ -352,14 +360,13 @@ uvc_function_set_alt(struct usb_function *f,
> unsigned interface, unsigned alt)
>  		v4l2_event.type = UVC_EVENT_STREAMON;
>  		v4l2_event_queue(uvc->vdev, &v4l2_event);
> 
> -		uvc->state = UVC_STATE_STREAMING;
> -		break;
> +		uvc->state = UVC_STATE_PRE_STREAMING;
> +
> +		return USB_GADGET_DELAYED_STATUS;
> 
>  	default:
>  		return -EINVAL;
>  	}
> -
> -	return 0;
>  }
> 
>  static void
> diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h
> index d78ea25..6cd1435 100644
> --- a/drivers/usb/gadget/uvc.h
> +++ b/drivers/usb/gadget/uvc.h
> @@ -141,6 +141,8 @@ enum uvc_state
>  {
>  	UVC_STATE_DISCONNECTED,
>  	UVC_STATE_CONNECTED,
> +	UVC_STATE_PRE_STREAMING,
> +	UVC_STATE_BUF_QUEUED_STREAMING_OFF,
>  	UVC_STATE_STREAMING,
>  };
> 
> @@ -190,6 +192,7 @@ struct uvc_file_handle
>   * Functions
>   */
> 
> +extern void uvc_function_setup_continue(struct uvc_device *uvc);
>  extern void uvc_endpoint_stream(struct uvc_device *dev);
> 
>  extern void uvc_function_connect(struct uvc_device *uvc);
> diff --git a/drivers/usb/gadget/uvc_v4l2.c
> b/drivers/usb/gadget/uvc_v4l2.c
> index 9c2b45b..5f23571 100644
> --- a/drivers/usb/gadget/uvc_v4l2.c
> +++ b/drivers/usb/gadget/uvc_v4l2.c
> @@ -235,10 +235,36 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int
> cmd, void *arg)
>  	}
> 
>  	case VIDIOC_QBUF:
> +		/*
> +		 * Theory of operation:
> +		 * - for the very first QBUF call the uvc state will be
> +		 *   UVC_STATE_PRE_STREAMING, so we need to initialize
> +		 *   the UVC video pipe (allocate requests, init queue,
> +		 *   ..) and change the uvc state to
> +		 *   UVC_STATE_BUF_QUEUED_STREAMING_OFF.
> +		 *
> +		 * - For the QBUF calls thereafter, (until STREAMON is
> +		 *   called) we just need to queue the buffers.
> +		 *
> +		 * - Once STREAMON has been called (which is handled by the
> +		 *   STREAMON case below), we need to start pumping the
> data
> +		 *   to USB side here itself.
> +		 */
>  		if ((ret = uvc_queue_buffer(&video->queue, arg)) < 0)
>  			return ret;
> 
> -		return uvc_video_pump(video);
> +		if (uvc->state == UVC_STATE_PRE_STREAMING) {
> +			ret = uvc_video_enable(video, 1);
> +			if (ret < 0)
> +				return ret;
> +
> +			uvc->state = UVC_STATE_BUF_QUEUED_STREAMING_OFF;
> +			return 0;
> +		} else if (uvc->state == UVC_STATE_STREAMING) {
> +			return uvc_video_pump(video);
> +		} else {
> +			return 0;
> +		}
> 
>  	case VIDIOC_DQBUF:
>  		return uvc_dequeue_buffer(&video->queue, arg,
> @@ -251,7 +277,15 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int
> cmd, void *arg)
>  		if (*type != video->queue.queue.type)
>  			return -EINVAL;
> 
> -		return uvc_video_enable(video, 1);
> +		/*
> +		 * since the real video device has now started streaming
> +		 * its safe now to complete the status phase of the
> +		 * set_interface (alt setting 1)
> +		 */
> +		uvc_function_setup_continue(uvc);
> +		uvc->state = UVC_STATE_STREAMING;
> +
> +		return 0;
>  	}
> 
>  	case VIDIOC_STREAMOFF:
> --
> 1.7.2.2

Regards,
Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Bhupesh,

Thanks for the patch, and sorry for the late reply.

On Friday 01 June 2012 15:08:58 Bhupesh Sharma wrote:
> This patch adds the support in UVC webcam gadget design for providing
> USB_GADGET_DELAYED_STATUS in response to a set_interface(alt setting 1)
> command issue by the Host.
> 
> The current UVC webcam gadget design generates a STREAMON event
> corresponding to a set_interface(alt setting 1) command from the Host. This
> STREAMON event will eventually be routed to a real V4L2 device.
> 
> To start video streaming, it may be required to perform some register writes
> to a camera sensor device over slow external busses like I2C or SPI. So, it
> makes sense to ensure that we delay the STATUS stage of the
> set_interface(alt setting 1) command.
> 
> Otherwise, a lot of ISOC IN tokens sent by the Host will be replied to by
> zero-length packets by the webcam device. On certain Hosts this may even
> lead to ISOC URBs been cancelled from the Host side.
> 
> So, as soon as we finish doing all the "streaming" related stuff on the real
> V4L2 device, we call a STREAMON ioctl on the UVC side and from here we call
> the 'usb_composite_setup_continue' function to complete the status stage of
> the set_interface(alt setting 1) command.

That sounds good, thank you for coming up with a solution to this issue.

> Further, we need to ensure that we queue no video buffers on the UVC webcam
> gadget, until we de-queue a video buffer from the V4L2 device. Also, we need
> to enable UVC video related stuff at the first QBUF ioctl call itself, as
> the application will call the STREAMON on UVC side only when it has
> dequeued sufficient buffers from the V4L2 side and queued them to the UVC
> gadget. So, the UVC video enable stuff cannot be done in STREAMON ioctl
> call.

Is that really required ? First of all, the userspace application can queue 
buffers before it calls VIDIOC_STREAMON. Assuming it doesn't, the gadget 
driver calls uvc_video_enable() at streamon time, which then calls 
uvc_video_pump(). As no buffer is queued, the function will return without 
queuing any USB request, so we shouldn't have any problem.

> For the same we add two more UVC states:
> 	- PRE_STREAMING : not even a single buffer has been queued to UVC
> 	- BUF_QUEUED_STREAMING_OFF : one video buffer has been queued to UVC
> 			but we have not yet enabled STREAMING on UVC side.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@st.com>
> ---
>  drivers/usb/gadget/f_uvc.c    |   17 ++++++++++++-----
>  drivers/usb/gadget/uvc.h      |    3 +++
>  drivers/usb/gadget/uvc_v4l2.c |   38 ++++++++++++++++++++++++++++++++++++--
> 3 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> index 2a8bf06..3589ed0 100644
> --- a/drivers/usb/gadget/f_uvc.c
> +++ b/drivers/usb/gadget/f_uvc.c
> @@ -272,6 +272,13 @@ uvc_function_setup(struct usb_function *f, const struct
> usb_ctrlrequest *ctrl) return 0;
>  }
> 
> +void uvc_function_setup_continue(struct uvc_device *uvc)
> +{
> +	struct usb_composite_dev *cdev = uvc->func.config->cdev;
> +
> +	usb_composite_setup_continue(cdev);
> +}
> +
>  static int
>  uvc_function_get_alt(struct usb_function *f, unsigned interface)
>  {
> @@ -334,7 +341,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned
> interface, unsigned alt) v4l2_event_queue(uvc->vdev, &v4l2_event);
> 
>  		uvc->state = UVC_STATE_CONNECTED;
> -		break;
> +
> +		return 0;
> 
>  	case 1:
>  		if (uvc->state != UVC_STATE_CONNECTED)
> @@ -352,14 +360,13 @@ uvc_function_set_alt(struct usb_function *f, unsigned
> interface, unsigned alt) v4l2_event.type = UVC_EVENT_STREAMON;
>  		v4l2_event_queue(uvc->vdev, &v4l2_event);
> 
> -		uvc->state = UVC_STATE_STREAMING;
> -		break;
> +		uvc->state = UVC_STATE_PRE_STREAMING;
> +
> +		return USB_GADGET_DELAYED_STATUS;
> 
>  	default:
>  		return -EINVAL;
>  	}
> -
> -	return 0;
>  }
> 
>  static void
> diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h
> index d78ea25..6cd1435 100644
> --- a/drivers/usb/gadget/uvc.h
> +++ b/drivers/usb/gadget/uvc.h
> @@ -141,6 +141,8 @@ enum uvc_state
>  {
>  	UVC_STATE_DISCONNECTED,
>  	UVC_STATE_CONNECTED,
> +	UVC_STATE_PRE_STREAMING,
> +	UVC_STATE_BUF_QUEUED_STREAMING_OFF,
>  	UVC_STATE_STREAMING,
>  };
> 
> @@ -190,6 +192,7 @@ struct uvc_file_handle
>   * Functions
>   */
> 
> +extern void uvc_function_setup_continue(struct uvc_device *uvc);
>  extern void uvc_endpoint_stream(struct uvc_device *dev);
> 
>  extern void uvc_function_connect(struct uvc_device *uvc);
> diff --git a/drivers/usb/gadget/uvc_v4l2.c b/drivers/usb/gadget/uvc_v4l2.c
> index 9c2b45b..5f23571 100644
> --- a/drivers/usb/gadget/uvc_v4l2.c
> +++ b/drivers/usb/gadget/uvc_v4l2.c
> @@ -235,10 +235,36 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd,
> void *arg) }
> 
>  	case VIDIOC_QBUF:
> +		/*
> +		 * Theory of operation:
> +		 * - for the very first QBUF call the uvc state will be
> +		 *   UVC_STATE_PRE_STREAMING, so we need to initialize
> +		 *   the UVC video pipe (allocate requests, init queue,
> +		 *   ..) and change the uvc state to
> +		 *   UVC_STATE_BUF_QUEUED_STREAMING_OFF.
> +		 *
> +		 * - For the QBUF calls thereafter, (until STREAMON is
> +		 *   called) we just need to queue the buffers.
> +		 *
> +		 * - Once STREAMON has been called (which is handled by the
> +		 *   STREAMON case below), we need to start pumping the data
> +		 *   to USB side here itself.
> +		 */
>  		if ((ret = uvc_queue_buffer(&video->queue, arg)) < 0)
>  			return ret;
> 
> -		return uvc_video_pump(video);
> +		if (uvc->state == UVC_STATE_PRE_STREAMING) {
> +			ret = uvc_video_enable(video, 1);
> +			if (ret < 0)
> +				return ret;
> +
> +			uvc->state = UVC_STATE_BUF_QUEUED_STREAMING_OFF;
> +			return 0;
> +		} else if (uvc->state == UVC_STATE_STREAMING) {
> +			return uvc_video_pump(video);
> +		} else {
> +			return 0;
> +		}
> 
>  	case VIDIOC_DQBUF:
>  		return uvc_dequeue_buffer(&video->queue, arg,
> @@ -251,7 +277,15 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd,
> void *arg) if (*type != video->queue.queue.type)
>  			return -EINVAL;
> 
> -		return uvc_video_enable(video, 1);
> +		/*
> +		 * since the real video device has now started streaming
> +		 * its safe now to complete the status phase of the
> +		 * set_interface (alt setting 1)
> +		 */
> +		uvc_function_setup_continue(uvc);
> +		uvc->state = UVC_STATE_STREAMING;
> +
> +		return 0;
>  	}
> 
>  	case VIDIOC_STREAMOFF:
  

Hi Laurent,

Thanks for your reply and sorry again for the delay in reply.

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Wednesday, June 20, 2012 3:19 AM
> To: Bhupesh SHARMA
> Cc: linux-usb@vger.kernel.org; balbi@ti.com; linux-
> media@vger.kernel.org; gregkh@linuxfoundation.org
> Subject: Re: [PATCH 5/5] usb: gadget/uvc: Add support for
> 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command
> 
> Hi Bhupesh,
> 
> Thanks for the patch, and sorry for the late reply.
> 
> On Friday 01 June 2012 15:08:58 Bhupesh Sharma wrote:
> > This patch adds the support in UVC webcam gadget design for providing
> > USB_GADGET_DELAYED_STATUS in response to a set_interface(alt setting
> 1)
> > command issue by the Host.
> >
> > The current UVC webcam gadget design generates a STREAMON event
> > corresponding to a set_interface(alt setting 1) command from the
> Host. This
> > STREAMON event will eventually be routed to a real V4L2 device.
> >
> > To start video streaming, it may be required to perform some register
> writes
> > to a camera sensor device over slow external busses like I2C or SPI.
> So, it
> > makes sense to ensure that we delay the STATUS stage of the
> > set_interface(alt setting 1) command.
> >
> > Otherwise, a lot of ISOC IN tokens sent by the Host will be replied
> to by
> > zero-length packets by the webcam device. On certain Hosts this may
> even
> > lead to ISOC URBs been cancelled from the Host side.
> >
> > So, as soon as we finish doing all the "streaming" related stuff on
> the real
> > V4L2 device, we call a STREAMON ioctl on the UVC side and from here
> we call
> > the 'usb_composite_setup_continue' function to complete the status
> stage of
> > the set_interface(alt setting 1) command.
> 
> That sounds good, thank you for coming up with a solution to this
> issue.
> 
> > Further, we need to ensure that we queue no video buffers on the UVC
> webcam
> > gadget, until we de-queue a video buffer from the V4L2 device. Also,
> we need
> > to enable UVC video related stuff at the first QBUF ioctl call
> itself, as
> > the application will call the STREAMON on UVC side only when it has
> > dequeued sufficient buffers from the V4L2 side and queued them to the
> UVC
> > gadget. So, the UVC video enable stuff cannot be done in STREAMON
> ioctl
> > call.
> 
> Is that really required ? First of all, the userspace application can
> queue
> buffers before it calls VIDIOC_STREAMON. Assuming it doesn't, the
> gadget
> driver calls uvc_video_enable() at streamon time, which then calls
> uvc_video_pump(). As no buffer is queued, the function will return
> without
> queuing any USB request, so we shouldn't have any problem.

I think that while working with a real video device, it will
be possible to queue a buffer at UVC end only when atleast
one buffer has been dequeued from the V4L2 device side (and has some real data).

This is because for a uvc buffer being queued we need to pass the v4l2 buffer's
buffer.start and buffer.length in the qbuf call at UVC side.

> > For the same we add two more UVC states:
> > 	- PRE_STREAMING : not even a single buffer has been queued to UVC
> > 	- BUF_QUEUED_STREAMING_OFF : one video buffer has been queued to
> UVC
> > 			but we have not yet enabled STREAMING on UVC side.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@st.com>
> > ---
> >  drivers/usb/gadget/f_uvc.c    |   17 ++++++++++++-----
> >  drivers/usb/gadget/uvc.h      |    3 +++
> >  drivers/usb/gadget/uvc_v4l2.c |   38
> ++++++++++++++++++++++++++++++++++++--
> > 3 files changed, 51 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> > index 2a8bf06..3589ed0 100644
> > --- a/drivers/usb/gadget/f_uvc.c
> > +++ b/drivers/usb/gadget/f_uvc.c
> > @@ -272,6 +272,13 @@ uvc_function_setup(struct usb_function *f, const
> struct
> > usb_ctrlrequest *ctrl) return 0;
> >  }
> >
> > +void uvc_function_setup_continue(struct uvc_device *uvc)
> > +{
> > +	struct usb_composite_dev *cdev = uvc->func.config->cdev;
> > +
> > +	usb_composite_setup_continue(cdev);
> > +}
> > +
> >  static int
> >  uvc_function_get_alt(struct usb_function *f, unsigned interface)
> >  {
> > @@ -334,7 +341,8 @@ uvc_function_set_alt(struct usb_function *f,
> unsigned
> > interface, unsigned alt) v4l2_event_queue(uvc->vdev, &v4l2_event);
> >
> >  		uvc->state = UVC_STATE_CONNECTED;
> > -		break;
> > +
> > +		return 0;
> >
> >  	case 1:
> >  		if (uvc->state != UVC_STATE_CONNECTED)
> > @@ -352,14 +360,13 @@ uvc_function_set_alt(struct usb_function *f,
> unsigned
> > interface, unsigned alt) v4l2_event.type = UVC_EVENT_STREAMON;
> >  		v4l2_event_queue(uvc->vdev, &v4l2_event);
> >
> > -		uvc->state = UVC_STATE_STREAMING;
> > -		break;
> > +		uvc->state = UVC_STATE_PRE_STREAMING;
> > +
> > +		return USB_GADGET_DELAYED_STATUS;
> >
> >  	default:
> >  		return -EINVAL;
> >  	}
> > -
> > -	return 0;
> >  }
> >
> >  static void
> > diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h
> > index d78ea25..6cd1435 100644
> > --- a/drivers/usb/gadget/uvc.h
> > +++ b/drivers/usb/gadget/uvc.h
> > @@ -141,6 +141,8 @@ enum uvc_state
> >  {
> >  	UVC_STATE_DISCONNECTED,
> >  	UVC_STATE_CONNECTED,
> > +	UVC_STATE_PRE_STREAMING,
> > +	UVC_STATE_BUF_QUEUED_STREAMING_OFF,
> >  	UVC_STATE_STREAMING,
> >  };
> >
> > @@ -190,6 +192,7 @@ struct uvc_file_handle
> >   * Functions
> >   */
> >
> > +extern void uvc_function_setup_continue(struct uvc_device *uvc);
> >  extern void uvc_endpoint_stream(struct uvc_device *dev);
> >
> >  extern void uvc_function_connect(struct uvc_device *uvc);
> > diff --git a/drivers/usb/gadget/uvc_v4l2.c
> b/drivers/usb/gadget/uvc_v4l2.c
> > index 9c2b45b..5f23571 100644
> > --- a/drivers/usb/gadget/uvc_v4l2.c
> > +++ b/drivers/usb/gadget/uvc_v4l2.c
> > @@ -235,10 +235,36 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned
> int cmd,
> > void *arg) }
> >
> >  	case VIDIOC_QBUF:
> > +		/*
> > +		 * Theory of operation:
> > +		 * - for the very first QBUF call the uvc state will be
> > +		 *   UVC_STATE_PRE_STREAMING, so we need to initialize
> > +		 *   the UVC video pipe (allocate requests, init queue,
> > +		 *   ..) and change the uvc state to
> > +		 *   UVC_STATE_BUF_QUEUED_STREAMING_OFF.
> > +		 *
> > +		 * - For the QBUF calls thereafter, (until STREAMON is
> > +		 *   called) we just need to queue the buffers.
> > +		 *
> > +		 * - Once STREAMON has been called (which is handled by the
> > +		 *   STREAMON case below), we need to start pumping the
> data
> > +		 *   to USB side here itself.
> > +		 */
> >  		if ((ret = uvc_queue_buffer(&video->queue, arg)) < 0)
> >  			return ret;
> >
> > -		return uvc_video_pump(video);
> > +		if (uvc->state == UVC_STATE_PRE_STREAMING) {
> > +			ret = uvc_video_enable(video, 1);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			uvc->state = UVC_STATE_BUF_QUEUED_STREAMING_OFF;
> > +			return 0;
> > +		} else if (uvc->state == UVC_STATE_STREAMING) {
> > +			return uvc_video_pump(video);
> > +		} else {
> > +			return 0;
> > +		}
> >
> >  	case VIDIOC_DQBUF:
> >  		return uvc_dequeue_buffer(&video->queue, arg,
> > @@ -251,7 +277,15 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned
> int cmd,
> > void *arg) if (*type != video->queue.queue.type)
> >  			return -EINVAL;
> >
> > -		return uvc_video_enable(video, 1);
> > +		/*
> > +		 * since the real video device has now started streaming
> > +		 * its safe now to complete the status phase of the
> > +		 * set_interface (alt setting 1)
> > +		 */
> > +		uvc_function_setup_continue(uvc);
> > +		uvc->state = UVC_STATE_STREAMING;
> > +
> > +		return 0;
> >  	}
> >
> >  	case VIDIOC_STREAMOFF:
> --
> Regards,
> 
> Laurent Pinchart

Regards,
Bhupesh

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Bhupesh,

On Tuesday 03 July 2012 23:47:14 Bhupesh SHARMA wrote:
> On Wednesday, June 20, 2012 3:19 AM Laurent Pinchart wrote:
> > On Friday 01 June 2012 15:08:58 Bhupesh Sharma wrote:
> > > This patch adds the support in UVC webcam gadget design for providing
> > > USB_GADGET_DELAYED_STATUS in response to a set_interface(alt setting 1)
> > > command issue by the Host.
> > > 
> > > The current UVC webcam gadget design generates a STREAMON event
> > > corresponding to a set_interface(alt setting 1) command from the Host.
> > > This STREAMON event will eventually be routed to a real V4L2 device.
> > > 
> > > To start video streaming, it may be required to perform some register
> > > writes to a camera sensor device over slow external busses like I2C or
> > > SPI. So, it makes sense to ensure that we delay the STATUS stage of the
> > > set_interface(alt setting 1) command.
> > > 
> > > Otherwise, a lot of ISOC IN tokens sent by the Host will be replied to
> > > by zero-length packets by the webcam device. On certain Hosts this may
> > > even> lead to ISOC URBs been cancelled from the Host side.
> > > 
> > > So, as soon as we finish doing all the "streaming" related stuff on the
> > > real V4L2 device, we call a STREAMON ioctl on the UVC side and from here
> > > we call the 'usb_composite_setup_continue' function to complete the
> > > status stage of the set_interface(alt setting 1) command.
> > 
> > That sounds good, thank you for coming up with a solution to this
> > issue.
> > 
> > > Further, we need to ensure that we queue no video buffers on the UVC
> > > webcam gadget, until we de-queue a video buffer from the V4L2 device.
> > > Also, we need to enable UVC video related stuff at the first QBUF ioctl
> > > call itself, as the application will call the STREAMON on UVC side only
> > > when it has dequeued sufficient buffers from the V4L2 side and queued
> > > them to the UVC gadget. So, the UVC video enable stuff cannot be done in
> > > STREAMON ioctl call.
> > 
> > Is that really required ? First of all, the userspace application can
> > queue buffers before it calls VIDIOC_STREAMON. Assuming it doesn't, the
> > gadget driver calls uvc_video_enable() at streamon time, which then calls
> > uvc_video_pump(). As no buffer is queued, the function will return without
> > queuing any USB request, so we shouldn't have any problem.
> 
> I think that while working with a real video device, it will be possible to
> queue a buffer at UVC end only when atleast one buffer has been dequeued
> from the V4L2 device side (and has some real data).
> 
> This is because for a uvc buffer being queued we need to pass the v4l2
> buffer's buffer.start and buffer.length in the qbuf call at UVC side.

I agree with you, queuing a buffer on the UVC gadget side will usually require 
starting capture and waiting for a frame on a V4L2 source device. However, 
unless I'm mistaken, the UVC gadget code already deals with that situation 
correctly. As I explained, when your application calls VIDIOC_STREAMON on the 
UVC gadget, the uvc_video_pump() will not find any V4L2 buffer to be 
transferred, and will return without queuing any URB. It will then be called 
again by VIDIOC_QBUF, and will then start the transfer.

Adding support for USB_GADGET_DELAYED_STATUS is thus a good idea, but the pre-
streaming state is in my opinion not needed. Feel free to prove me wrong 
though :-)

> > > For the same we add two more UVC states:
> > > 	- PRE_STREAMING : not even a single buffer has been queued to UVC
> > > 	- BUF_QUEUED_STREAMING_OFF : one video buffer has been queued to UVC
> > > 			but we have not yet enabled STREAMING on UVC side.
> > > 
> > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@st.com>
  

Hi Laurent,

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Saturday, July 07, 2012 6:37 PM
> To: Bhupesh SHARMA
> Cc: linux-usb@vger.kernel.org; balbi@ti.com; linux-
> media@vger.kernel.org; gregkh@linuxfoundation.org
> Subject: Re: [PATCH 5/5] usb: gadget/uvc: Add support for
> 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command
> 
> Hi Bhupesh,
> 
> On Tuesday 03 July 2012 23:47:14 Bhupesh SHARMA wrote:
> > On Wednesday, June 20, 2012 3:19 AM Laurent Pinchart wrote:
> > > On Friday 01 June 2012 15:08:58 Bhupesh Sharma wrote:
> > > > This patch adds the support in UVC webcam gadget design for
> providing
> > > > USB_GADGET_DELAYED_STATUS in response to a set_interface(alt
> setting 1)
> > > > command issue by the Host.
> > > >
> > > > The current UVC webcam gadget design generates a STREAMON event
> > > > corresponding to a set_interface(alt setting 1) command from the
> Host.
> > > > This STREAMON event will eventually be routed to a real V4L2
> device.
> > > >
> > > > To start video streaming, it may be required to perform some
> register
> > > > writes to a camera sensor device over slow external busses like
> I2C or
> > > > SPI. So, it makes sense to ensure that we delay the STATUS stage
> of the
> > > > set_interface(alt setting 1) command.
> > > >
> > > > Otherwise, a lot of ISOC IN tokens sent by the Host will be
> replied to
> > > > by zero-length packets by the webcam device. On certain Hosts
> this may
> > > > even> lead to ISOC URBs been cancelled from the Host side.
> > > >
> > > > So, as soon as we finish doing all the "streaming" related stuff
> on the
> > > > real V4L2 device, we call a STREAMON ioctl on the UVC side and
> from here
> > > > we call the 'usb_composite_setup_continue' function to complete
> the
> > > > status stage of the set_interface(alt setting 1) command.
> > >
> > > That sounds good, thank you for coming up with a solution to this
> > > issue.
> > >
> > > > Further, we need to ensure that we queue no video buffers on the
> UVC
> > > > webcam gadget, until we de-queue a video buffer from the V4L2
> device.
> > > > Also, we need to enable UVC video related stuff at the first QBUF
> ioctl
> > > > call itself, as the application will call the STREAMON on UVC
> side only
> > > > when it has dequeued sufficient buffers from the V4L2 side and
> queued
> > > > them to the UVC gadget. So, the UVC video enable stuff cannot be
> done in
> > > > STREAMON ioctl call.
> > >
> > > Is that really required ? First of all, the userspace application
> can
> > > queue buffers before it calls VIDIOC_STREAMON. Assuming it doesn't,
> the
> > > gadget driver calls uvc_video_enable() at streamon time, which then
> calls
> > > uvc_video_pump(). As no buffer is queued, the function will return
> without
> > > queuing any USB request, so we shouldn't have any problem.
> >
> > I think that while working with a real video device, it will be
> possible to
> > queue a buffer at UVC end only when atleast one buffer has been
> dequeued
> > from the V4L2 device side (and has some real data).
> >
> > This is because for a uvc buffer being queued we need to pass the
> v4l2
> > buffer's buffer.start and buffer.length in the qbuf call at UVC side.
> 
> I agree with you, queuing a buffer on the UVC gadget side will usually
> require
> starting capture and waiting for a frame on a V4L2 source device.
> However,
> unless I'm mistaken, the UVC gadget code already deals with that
> situation
> correctly. As I explained, when your application calls VIDIOC_STREAMON
> on the
> UVC gadget, the uvc_video_pump() will not find any V4L2 buffer to be
> transferred, and will return without queuing any URB. It will then be
> called
> again by VIDIOC_QBUF, and will then start the transfer.
> 
> Adding support for USB_GADGET_DELAYED_STATUS is thus a good idea, but
> the pre-
> streaming state is in my opinion not needed. Feel free to prove me
> wrong
> though :-)

I did some extensive tests and also managed to reproduce the problem Georgios
mentioned regarding UVC not able to stream when an application using the webcam
is stopped and started again (sorry Georgios, it took me some time to finish all the checks,
and thanks for pointing out the issue)

You are right the PRE_STREAMING state is not required.

I will resend the patches 4/5 and 5/5 of this patchset with all your comments
addressed in the next couple of days.

> > > > For the same we add two more UVC states:
> > > > 	- PRE_STREAMING : not even a single buffer has been queued
> to UVC
> > > > 	- BUF_QUEUED_STREAMING_OFF : one video buffer has been
> queued to UVC
> > > > 			but we have not yet enabled STREAMING on UVC
> side.
> > > >
> > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@st.com>
> 
> --

Regards,
Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html