Hi Laurent, Thanks for your review comments. On 4/29/2013 8:08 PM, Laurent Pinchart wrote:
Hi Bhupesh, Thank you for the patch. On Wednesday 17 April 2013 09:44:25 Bhupesh Sharma wrote:This patch adds the support in UVC webcam gadget to allocate UVC header and payload as Scatter-Gather elements which can be used on a SG-capable UDC controller. A module parameter has been introduced for the same. One can set the module parameter 'sg_mode' to 1 to turn on this feature (by default this feature is turned-off). This ensures that we don't require a memcpy from CPU side to append UVC header in front of the UVC payload atleast for SG capable UDC contollers. Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx> --- Note that to ease review and integration of this patch, I have rebased it on the following patch already circulated for review last week: [PATCH 1/1] usb: gadget/uvc: Add support for Bulk endpoint to be used as Video Streaming ep http://www.mail-archive.com/linux-usb@xxxxxxxxxxxxxxx/msg19546.html The above patch was rebased on Laurent's UVC gadget git tree available here (head uvc-gadget): git://linuxtv.org/pinchartl/uvcvideo.git This will allow the patches to be pulled into Felipe's repo in one go after review and any subsequent rework (if required). drivers/usb/gadget/f_uvc.c | 8 +++ drivers/usb/gadget/uvc.h | 2 + drivers/usb/gadget/uvc_video.c | 113 ++++++++++++++++++++++++++++++++----- 3 files changed, 109 insertions(+), 14 deletions(-) diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c index e5953eb..ccf0253 100644 --- a/drivers/usb/gadget/f_uvc.c +++ b/drivers/usb/gadget/f_uvc.c @@ -50,6 +50,11 @@ module_param(bulk_streaming_ep, bool, S_IRUGO|S_IWUSR); MODULE_PARM_DESC(bulk_streaming_ep, "0 (Use ISOC video streaming ep) / " "1 (Use BULK video streaming ep)"); +static bool sg_mode; +module_param(sg_mode, bool, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(sg_mode, "0 (Don't use SG feature) / " + "1 (Use scatterlist for SG-capable controllers)"); +Can't this be queried automatically from the UDC at runtime ?
Yes, it can be. But I believe its better to still provide a moduleparameter to the UVC webcam module user as some UDCs may have flaky SG implementations in H/W and then the user will not be able to use the
UVC gadget without using the SG feature of SG-capable UDCs.I believe in worst-case its better to fall-back on memcpy from CPU if the SG features in the UDC are not yet properly supported.
/* ------------------------------------------------------------------------ * Function descriptors */ @@ -888,6 +893,9 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) uvc->control_req->complete = uvc_function_ep0_complete; uvc->control_req->context = uvc; + /* Use SG mode ? */ + uvc->video.sg_mode = sg_mode; + /* Avoid letting this gadget enumerate until the userspace server is * active. */ diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h index 8756853..3a54510 100644 --- a/drivers/usb/gadget/uvc.h +++ b/drivers/usb/gadget/uvc.h @@ -122,6 +122,7 @@ struct uvc_video struct usb_request *req[UVC_NUM_REQUESTS]; __u8 *req_buffer[UVC_NUM_REQUESTS]; struct list_head req_free; + unsigned char *header_buf;Don't you need one header per request ? It would probably be easier to create a uvc_request structure to group the request, the request header and the request buffer: struct uvc_request{ struct usb_request *req; __u8 *header; __u8 *buffer; }; and have a table of those in the uvc_video structure.
Ok. Will do this in V2.
spinlock_t req_lock; void (*encode) (struct usb_request *req, struct uvc_video *video, @@ -135,6 +136,7 @@ struct uvc_video unsigned int fid; bool bulk_streaming_ep; + bool sg_mode; }; enum uvc_state diff --git a/drivers/usb/gadget/uvc_video.c b/drivers/usb/gadget/uvc_video.c index 87ac526..5f92f93 100644 --- a/drivers/usb/gadget/uvc_video.c +++ b/drivers/usb/gadget/uvc_video.c @@ -39,6 +39,25 @@ uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf, } static int +uvc_video_encode_header_sg(struct uvc_video *video, struct uvc_buffer *buf, + int len) +{ + unsigned char *data = video->header_buf; + + *data++ = 2; + *data = UVC_STREAM_EOH | video->fid; + + if (!video->bulk_streaming_ep) { + if (buf->bytesused - video->queue.buf_used <= len - 2) + *data |= UVC_STREAM_EOF; + } else { + *data |= UVC_STREAM_EOF; + } + + return 2; +}Instead of duplicating the code could you just pass the header pointer to uvc_video_encode_header() ? It might be even better to pass a uvc_request structure pointer instead of a data pointer and move the sg_mode check to uvc_video_encode_header() instead of spreading it around the code.
Ok.
+ +static int uvc_video_encode_data(struct uvc_video *video, struct uvc_buffer *buf, u8 *data, int len) { @@ -56,25 +75,57 @@ uvc_video_encode_data(struct uvc_video *video, struct uvc_buffer *buf, return nbytes; } +static int +uvc_video_encode_data_sg(struct uvc_video *video, struct uvc_buffer *buf, + struct scatterlist *sg, int len) +{ + struct uvc_video_queue *queue = &video->queue; + unsigned int nbytes; + + /* Pass pointer of video frame data to the USB req->sg. */ + nbytes = min((unsigned int)len, buf->bytesused - queue->buf_used); + + sg_set_buf(sg, (void *)(buf->mem + queue->buf_used), len); + queue->buf_used += nbytes; + + return nbytes; +}Same comments here as for uvc_video_encode_header(). I think the code will be cleaner if you move the sg_mode checks to the encode* functions.
Ok.
+ static void uvc_video_encode_bulk(struct usb_request *req, struct uvc_video *video, struct uvc_buffer *buf) { - void *mem = req->buf; + struct scatterlist *sg = NULL; + void *mem = NULL; int len = video->req_size; int ret; /* Add a header at the beginning of the payload. */ + if (!video->sg_mode) + mem = req->buf; + else + sg = req->sg; + if (video->payload_size == 0) { - ret = uvc_video_encode_header(video, buf, mem, len); + if (!video->sg_mode) { + ret = uvc_video_encode_header(video, buf, mem, len); + mem += ret; + } else { + ret = uvc_video_encode_header_sg(video, buf, len); + } + video->payload_size += ret; - mem += ret; len -= ret; } /* Process video data. */ len = min((int)(video->max_payload_size - video->payload_size), len); - ret = uvc_video_encode_data(video, buf, mem, len); + if (!video->sg_mode) { + ret = uvc_video_encode_data(video, buf, mem, len); + } else { + sg++; + ret = uvc_video_encode_data_sg(video, buf, sg, len); + } video->payload_size += ret; len -= ret; @@ -100,17 +151,29 @@ static void uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, struct uvc_buffer *buf) { + struct scatterlist *sg = NULL; void *mem = req->buf; int len = video->req_size; int ret; /* Add the header. */ - ret = uvc_video_encode_header(video, buf, mem, len); + if (!video->sg_mode) + ret = uvc_video_encode_header(video, buf, mem, len); + else + ret = uvc_video_encode_header_sg(video, buf, len); + mem += ret; len -= ret; /* Process video data. */ - ret = uvc_video_encode_data(video, buf, mem, len); + if (!video->sg_mode) { + ret = uvc_video_encode_data(video, buf, mem, len); + } else { + sg = req->sg; + sg++; + ret = uvc_video_encode_data_sg(video, buf, sg, len); + } + len -= ret; req->length = video->req_size - len; @@ -212,13 +275,19 @@ uvc_video_free_requests(struct uvc_video *video) { unsigned int i; + if (video->sg_mode && video->header_buf) + kfree(video->header_buf); + for (i = 0; i < UVC_NUM_REQUESTS; ++i) { + if (video->sg_mode && video->req[i]->sg) + kfree(video->req[i]->sg); + if (video->req[i]) { usb_ep_free_request(video->ep, video->req[i]); video->req[i] = NULL; } - if (video->req_buffer[i]) { + if (!video->sg_mode && video->req_buffer[i]) { kfree(video->req_buffer[i]); video->req_buffer[i] = NULL; } @@ -243,19 +312,35 @@ uvc_video_alloc_requests(struct uvc_video *video) * max_t(unsigned int, video->ep->maxburst, 1) * (video->ep->mult + 1); else - req_size = video->ep->maxpacket - * max_t(unsigned int, video->ep->maxburst, 1); + req_size = max_t(unsigned int, video->imagesize, + video->ep->maxpacket + * max_t(unsigned int, video->ep->maxburst, 1)); - for (i = 0; i < UVC_NUM_REQUESTS; ++i) { - video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL); - if (video->req_buffer[i] == NULL) - goto error; + /* Allocate a UVC header here itself for SG mode. */ + if (video->sg_mode) + video->header_buf = kmalloc(2 * sizeof(video->header_buf), + GFP_DMA); + for (i = 0; i < UVC_NUM_REQUESTS; ++i) { video->req[i] = usb_ep_alloc_request(video->ep, GFP_KERNEL); if (video->req[i] == NULL) goto error; - video->req[i]->buf = video->req_buffer[i]; + if (!video->sg_mode) { + video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL); + if (video->req_buffer[i] == NULL) + goto error; + + video->req[i]->buf = video->req_buffer[i]; + } else { + video->req[i]->sg = + kmalloc(2 * sizeof(video->req[i]->sg), GFP_DMA); + video->req[i]->num_sgs = 2; + sg_init_table(video->req[i]->sg, 2); + sg_set_buf(video->req[i]->sg, + (void *)video->header_buf, 2); + } + video->req[i]->length = 0; video->req[i]->complete = uvc_video_complete; video->req[i]->context = video;
Regards, Bhupesh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
