Date:	Fri, 18 May 2012 00:51:04 +0200
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Bhupesh Sharma <bhupesh.sharma@...com>
Cc:	linux-usb@...r.kernel.org, balbi@...com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] usb: gadget/uvc: Add support for 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command

Hi Bhupesh,

Thank you for the patch.

On Monday 07 May 2012 12:42:37 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.

Very good point.

> 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.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@...com>
> ---
>  drivers/usb/gadget/f_uvc.c    |   12 +++++++++++-
>  drivers/usb/gadget/uvc.h      |    4 ++++
>  drivers/usb/gadget/uvc_v4l2.c |   27 +++++++++++++++++++++++++--
>  3 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> index d2569b2..6f084e3 100644
> --- a/drivers/usb/gadget/f_uvc.c
> +++ b/drivers/usb/gadget/f_uvc.c
> @@ -209,6 +209,14 @@ uvc_function_get_alt(struct usb_function *f, unsigned
> interface) return uvc->state == UVC_STATE_STREAMING ? 1 : 0;
>  }
> 
> +void
> +uvc_process_setup_continue(struct uvc_device *uvc)

No need for a line split, this won't exceed the 80 columns limit.

Could you please rename the function to uvc_function_setup_continue to be 
coherent with the other non-static functions in this file (and, while you're 
at it, move it right above uvc_function_get_alt) ?

> +{
> +	struct usb_composite_dev *cdev = uvc->func.config->cdev;
> +
> +	usb_composite_setup_continue(cdev);
> +}
> +
>  static int
>  uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned
> alt) {
> @@ -271,7 +279,9 @@ uvc_function_set_alt(struct usb_function *f, unsigned
> interface, unsigned alt) v4l2_event_queue(uvc->vdev, &v4l2_event);
> 
>  		uvc->state = UVC_STATE_STREAMING;
> -		break;
> +		uvc->vdev_is_streaming = false;

Do you think you could combine the state and vdev_is_streaming fields ? Maybe 
adding a UVC_STATE_PRE_STREAMING state would be enough, you could set the 
state to that value here, and then set it to UVC_STATE_STREAMING in the 
STREAMON ioctl handler (I would actually prefer doing that in the 
uvc_video_enable function).

If you think it makes sense, could you please try it ?

> +
> +		return USB_GADGET_DELAYED_STATUS;

Could you please move the return 0 at the end of this function to the case 0 
above ? Each case will then have its return statement.

>  	default:
>  		return -EINVAL;
> diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h
> index bc78c60..56ac883 100644
> --- a/drivers/usb/gadget/uvc.h
> +++ b/drivers/usb/gadget/uvc.h
> @@ -168,6 +168,9 @@ struct uvc_device
>  	/* Events */
>  	unsigned int event_length;
>  	unsigned int event_setup_out : 1;
> +
> +	/* flags */
> +	bool vdev_is_streaming;
>  };
> 
>  static inline struct uvc_device *to_uvc(struct usb_function *f)
> @@ -188,6 +191,7 @@ struct uvc_file_handle
>   * Functions
>   */
> 
> +extern void uvc_process_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 f761bcb..379d8ac 100644
> --- a/drivers/usb/gadget/uvc_v4l2.c
> +++ b/drivers/usb/gadget/uvc_v4l2.c
> @@ -242,7 +242,18 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd,
> void *arg) if ((ret = uvc_queue_buffer(&video->queue, arg)) < 0)
>  			return ret;
> 
> -		return uvc_video_pump(video);
> +		/*
> +		 * for the very first QBUF calls (until STREAMON is
> +		 * called) we just need to queue the buffers and start
> +		 * pumping the data to USB side only after STREAMON has
> +		 * been called which is handled by the STREAMON case
> +		 * below. For QBUF calls after STREAMON, we need to pump
> +		 * the data to the USB side here itself.
> +		 */
> +		if (uvc->vdev_is_streaming)
> +			return uvc_video_pump(video);
> +
> +		return 0;
> 
>  	case VIDIOC_DQBUF:
>  		return uvc_dequeue_buffer(&video->queue, arg,
> @@ -255,7 +266,19 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd,
> void *arg) if (*type != video->queue.type)
>  			return -EINVAL;
> 
> -		return uvc_video_enable(video, 1);
> +		ret = uvc_video_enable(video, 1);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * 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_process_setup_continue(uvc);
> +		uvc->vdev_is_streaming = true;

Could you please move this to uvc_video_enable() ?

> +
> +		return 0;
>  	}
> 
>  	case VIDIOC_STREAMOFF:

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists