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 ? > /* ------------------------------------------------------------------------ > * 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. > 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. > + > +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. > + > 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, Laurent Pinchart -- 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
