| Message ID | 8e669260c648ec44eefce9f554c5a871311832d9.1541534872.git-series.kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Asynchronous UVC | expand |
Hi Kieran, Thank you for the patch. On Tuesday, 6 November 2018 23:27:20 EET Kieran Bingham wrote: > From: Kieran Bingham <kieran.bingham@ideasonboard.com> > > A new iterator is available for processing UVC URB structures. This > simplifies the processing of the internal stream data. > > Convert the manual loop iterators to the new helper, adding an index > helper to keep the existing debug print. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > This patch converts to using the iterator which for most hunks makes > sense. The only one with uncertainty is in uvc_alloc_urb_buffers() where > the loop index is used to determine if all the buffers were successfully > allocated. > > As the loop index is not incremented by the loops, we can obtain the > buffer index - but then we are offset and out-by-one. > > Adjusting this in the code is fine - but at that point it feels like > it's not adding much value. I've left that hunk in for this patch - but > that part could be reverted if desired - unless anyone has a better > rework of the buffer check? > > drivers/media/usb/uvc/uvc_video.c | 51 ++++++++++++++++---------------- > drivers/media/usb/uvc/uvcvideo.h | 3 ++- > 2 files changed, 29 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c > b/drivers/media/usb/uvc/uvc_video.c index 020022e6ade4..f6e5db7ea50e 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -1556,20 +1556,19 @@ static void uvc_video_complete(struct urb *urb) > */ > static void uvc_free_urb_buffers(struct uvc_streaming *stream) > { > - unsigned int i; > + struct uvc_urb *uvc_urb; > > - for (i = 0; i < UVC_URBS; ++i) { > - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; > + for_each_uvc_urb(uvc_urb, stream) { > + if (!uvc_urb->buffer) > + continue; > > - if (uvc_urb->buffer) { > #ifndef CONFIG_DMA_NONCOHERENT > - usb_free_coherent(stream->dev->udev, stream->urb_size, > - uvc_urb->buffer, uvc_urb->dma); > + usb_free_coherent(stream->dev->udev, stream->urb_size, > + uvc_urb->buffer, uvc_urb->dma); > #else > - kfree(uvc_urb->buffer); > + kfree(uvc_urb->buffer); > #endif > - uvc_urb->buffer = NULL; > - } > + uvc_urb->buffer = NULL; > } > > stream->urb_size = 0; > @@ -1589,8 +1588,9 @@ static void uvc_free_urb_buffers(struct uvc_streaming > *stream) static int uvc_alloc_urb_buffers(struct uvc_streaming *stream, > unsigned int size, unsigned int psize, gfp_t gfp_flags) > { > + struct uvc_urb *uvc_urb; > unsigned int npackets; > - unsigned int i; > + unsigned int i = 0; > > /* Buffers are already allocated, bail out. */ > if (stream->urb_size) > @@ -1605,8 +1605,12 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming > *stream, > > /* Retry allocations until one succeed. */ > for (; npackets > 1; npackets /= 2) { > - for (i = 0; i < UVC_URBS; ++i) { > - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; > + for_each_uvc_urb(uvc_urb, stream) { > + /* > + * Track how many URBs we allocate, adding one to the > + * index to account for our zero index. > + */ > + i = uvc_urb_index(uvc_urb) + 1; That's a bit ugly indeed, I think we could keep the existing loop; > stream->urb_size = psize * npackets; > #ifndef CONFIG_DMA_NONCOHERENT > @@ -1700,7 +1704,8 @@ static int uvc_init_video_isoc(struct uvc_streaming > *stream, struct usb_host_endpoint *ep, gfp_t gfp_flags) > { > struct urb *urb; > - unsigned int npackets, i, j; > + struct uvc_urb *uvc_urb; > + unsigned int npackets, j; j without i seems weird, could you rename it ? > u16 psize; > u32 size; > > @@ -1713,9 +1718,7 @@ static int uvc_init_video_isoc(struct uvc_streaming > *stream, > > size = npackets * psize; > > - for (i = 0; i < UVC_URBS; ++i) { > - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; > - > + for_each_uvc_urb(uvc_urb, stream) { > urb = usb_alloc_urb(npackets, gfp_flags); > if (urb == NULL) { > uvc_video_stop(stream, 1); > @@ -1757,7 +1760,8 @@ static int uvc_init_video_bulk(struct uvc_streaming > *stream, struct usb_host_endpoint *ep, gfp_t gfp_flags) > { > struct urb *urb; > - unsigned int npackets, pipe, i; > + struct uvc_urb *uvc_urb; > + unsigned int npackets, pipe; > u16 psize; > u32 size; > > @@ -1781,9 +1785,7 @@ static int uvc_init_video_bulk(struct uvc_streaming > *stream, if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) > size = 0; > > - for (i = 0; i < UVC_URBS; ++i) { > - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; > - > + for_each_uvc_urb(uvc_urb, stream) { > urb = usb_alloc_urb(0, gfp_flags); > if (urb == NULL) { > uvc_video_stop(stream, 1); > @@ -1810,6 +1812,7 @@ static int uvc_video_start(struct uvc_streaming > *stream, gfp_t gfp_flags) { > struct usb_interface *intf = stream->intf; > struct usb_host_endpoint *ep; > + struct uvc_urb *uvc_urb; > unsigned int i; > int ret; > > @@ -1887,13 +1890,11 @@ static int uvc_video_start(struct uvc_streaming > *stream, gfp_t gfp_flags) return ret; > > /* Submit the URBs. */ > - for (i = 0; i < UVC_URBS; ++i) { > - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; > - > + for_each_uvc_urb(uvc_urb, stream) { > ret = usb_submit_urb(uvc_urb->urb, gfp_flags); > if (ret < 0) { > - uvc_printk(KERN_ERR, "Failed to submit URB %u " > - "(%d).\n", i, ret); > + uvc_printk(KERN_ERR, "Failed to submit URB %u (%d).\n", > + uvc_urb_index(uvc_urb), ret); > uvc_video_stop(stream, 1); > return ret; > } > diff --git a/drivers/media/usb/uvc/uvcvideo.h > b/drivers/media/usb/uvc/uvcvideo.h index c0a120496a1f..6a0f1b59356c 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -617,6 +617,9 @@ struct uvc_streaming { > (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \ > ++(uvc_urb)) > > +#define uvc_urb_index(uvc_urb) \ > + (unsigned int)((uvc_urb) - (&(uvc_urb)->stream->uvc_urb[0])) > + > struct uvc_device_info { > u32 quirks; > u32 meta_format;
Hi Laurent, On 06/11/2018 23:21, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Tuesday, 6 November 2018 23:27:20 EET Kieran Bingham wrote: >> From: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> A new iterator is available for processing UVC URB structures. This >> simplifies the processing of the internal stream data. >> >> Convert the manual loop iterators to the new helper, adding an index >> helper to keep the existing debug print. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> --- >> This patch converts to using the iterator which for most hunks makes >> sense. The only one with uncertainty is in uvc_alloc_urb_buffers() where >> the loop index is used to determine if all the buffers were successfully >> allocated. >> >> As the loop index is not incremented by the loops, we can obtain the >> buffer index - but then we are offset and out-by-one. >> >> Adjusting this in the code is fine - but at that point it feels like >> it's not adding much value. I've left that hunk in for this patch - but >> that part could be reverted if desired - unless anyone has a better >> rework of the buffer check? >> >> drivers/media/usb/uvc/uvc_video.c | 51 ++++++++++++++++---------------- >> drivers/media/usb/uvc/uvcvideo.h | 3 ++- >> 2 files changed, 29 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_video.c >> b/drivers/media/usb/uvc/uvc_video.c index 020022e6ade4..f6e5db7ea50e 100644 >> --- a/drivers/media/usb/uvc/uvc_video.c >> +++ b/drivers/media/usb/uvc/uvc_video.c >> @@ -1556,20 +1556,19 @@ static void uvc_video_complete(struct urb *urb) >> */ >> static void uvc_free_urb_buffers(struct uvc_streaming *stream) >> { >> - unsigned int i; >> + struct uvc_urb *uvc_urb; >> >> - for (i = 0; i < UVC_URBS; ++i) { >> - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; >> + for_each_uvc_urb(uvc_urb, stream) { >> + if (!uvc_urb->buffer) >> + continue; >> >> - if (uvc_urb->buffer) { >> #ifndef CONFIG_DMA_NONCOHERENT >> - usb_free_coherent(stream->dev->udev, stream->urb_size, >> - uvc_urb->buffer, uvc_urb->dma); >> + usb_free_coherent(stream->dev->udev, stream->urb_size, >> + uvc_urb->buffer, uvc_urb->dma); >> #else >> - kfree(uvc_urb->buffer); >> + kfree(uvc_urb->buffer); >> #endif >> - uvc_urb->buffer = NULL; >> - } >> + uvc_urb->buffer = NULL; >> } >> >> stream->urb_size = 0; >> @@ -1589,8 +1588,9 @@ static void uvc_free_urb_buffers(struct uvc_streaming >> *stream) static int uvc_alloc_urb_buffers(struct uvc_streaming *stream, >> unsigned int size, unsigned int psize, gfp_t gfp_flags) >> { >> + struct uvc_urb *uvc_urb; >> unsigned int npackets; >> - unsigned int i; >> + unsigned int i = 0; >> >> /* Buffers are already allocated, bail out. */ >> if (stream->urb_size) >> @@ -1605,8 +1605,12 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming >> *stream, >> >> /* Retry allocations until one succeed. */ >> for (; npackets > 1; npackets /= 2) { >> - for (i = 0; i < UVC_URBS; ++i) { >> - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; >> + for_each_uvc_urb(uvc_urb, stream) { >> + /* >> + * Track how many URBs we allocate, adding one to the >> + * index to account for our zero index. >> + */ >> + i = uvc_urb_index(uvc_urb) + 1; > > That's a bit ugly indeed, I think we could keep the existing loop; I do agree, as stated I left it in for 'completeness' of the patch. But I don't think it adds value here. Will remove. >> stream->urb_size = psize * npackets; >> #ifndef CONFIG_DMA_NONCOHERENT >> @@ -1700,7 +1704,8 @@ static int uvc_init_video_isoc(struct uvc_streaming >> *stream, struct usb_host_endpoint *ep, gfp_t gfp_flags) >> { >> struct urb *urb; >> - unsigned int npackets, i, j; >> + struct uvc_urb *uvc_urb; >> + unsigned int npackets, j; > > j without i seems weird, could you rename it ? Sure. Done > >> u16 psize; >> u32 size; >> >> @@ -1713,9 +1718,7 @@ static int uvc_init_video_isoc(struct uvc_streaming >> *stream, >> >> size = npackets * psize; >> >> - for (i = 0; i < UVC_URBS; ++i) { >> - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; >> - >> + for_each_uvc_urb(uvc_urb, stream) { >> urb = usb_alloc_urb(npackets, gfp_flags); >> if (urb == NULL) { >> uvc_video_stop(stream, 1); >> @@ -1757,7 +1760,8 @@ static int uvc_init_video_bulk(struct uvc_streaming >> *stream, struct usb_host_endpoint *ep, gfp_t gfp_flags) >> { >> struct urb *urb; >> - unsigned int npackets, pipe, i; >> + struct uvc_urb *uvc_urb; >> + unsigned int npackets, pipe; >> u16 psize; >> u32 size; >> >> @@ -1781,9 +1785,7 @@ static int uvc_init_video_bulk(struct uvc_streaming >> *stream, if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) >> size = 0; >> >> - for (i = 0; i < UVC_URBS; ++i) { >> - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; >> - >> + for_each_uvc_urb(uvc_urb, stream) { >> urb = usb_alloc_urb(0, gfp_flags); >> if (urb == NULL) { >> uvc_video_stop(stream, 1); >> @@ -1810,6 +1812,7 @@ static int uvc_video_start(struct uvc_streaming >> *stream, gfp_t gfp_flags) { >> struct usb_interface *intf = stream->intf; >> struct usb_host_endpoint *ep; >> + struct uvc_urb *uvc_urb; >> unsigned int i; >> int ret; >> >> @@ -1887,13 +1890,11 @@ static int uvc_video_start(struct uvc_streaming >> *stream, gfp_t gfp_flags) return ret; >> >> /* Submit the URBs. */ >> - for (i = 0; i < UVC_URBS; ++i) { >> - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; >> - >> + for_each_uvc_urb(uvc_urb, stream) { >> ret = usb_submit_urb(uvc_urb->urb, gfp_flags); >> if (ret < 0) { >> - uvc_printk(KERN_ERR, "Failed to submit URB %u " >> - "(%d).\n", i, ret); >> + uvc_printk(KERN_ERR, "Failed to submit URB %u (%d).\n", >> + uvc_urb_index(uvc_urb), ret); >> uvc_video_stop(stream, 1); >> return ret; >> } >> diff --git a/drivers/media/usb/uvc/uvcvideo.h >> b/drivers/media/usb/uvc/uvcvideo.h index c0a120496a1f..6a0f1b59356c 100644 >> --- a/drivers/media/usb/uvc/uvcvideo.h >> +++ b/drivers/media/usb/uvc/uvcvideo.h >> @@ -617,6 +617,9 @@ struct uvc_streaming { >> (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \ >> ++(uvc_urb)) >> >> +#define uvc_urb_index(uvc_urb) \ >> + (unsigned int)((uvc_urb) - (&(uvc_urb)->stream->uvc_urb[0])) >> + >> struct uvc_device_info { >> u32 quirks; >> u32 meta_format; >
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 020022e6ade4..f6e5db7ea50e 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1556,20 +1556,19 @@ static void uvc_video_complete(struct urb *urb) */ static void uvc_free_urb_buffers(struct uvc_streaming *stream) { - unsigned int i; + struct uvc_urb *uvc_urb; - for (i = 0; i < UVC_URBS; ++i) { - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; + for_each_uvc_urb(uvc_urb, stream) { + if (!uvc_urb->buffer) + continue; - if (uvc_urb->buffer) { #ifndef CONFIG_DMA_NONCOHERENT - usb_free_coherent(stream->dev->udev, stream->urb_size, - uvc_urb->buffer, uvc_urb->dma); + usb_free_coherent(stream->dev->udev, stream->urb_size, + uvc_urb->buffer, uvc_urb->dma); #else - kfree(uvc_urb->buffer); + kfree(uvc_urb->buffer); #endif - uvc_urb->buffer = NULL; - } + uvc_urb->buffer = NULL; } stream->urb_size = 0; @@ -1589,8 +1588,9 @@ static void uvc_free_urb_buffers(struct uvc_streaming *stream) static int uvc_alloc_urb_buffers(struct uvc_streaming *stream, unsigned int size, unsigned int psize, gfp_t gfp_flags) { + struct uvc_urb *uvc_urb; unsigned int npackets; - unsigned int i; + unsigned int i = 0; /* Buffers are already allocated, bail out. */ if (stream->urb_size) @@ -1605,8 +1605,12 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream, /* Retry allocations until one succeed. */ for (; npackets > 1; npackets /= 2) { - for (i = 0; i < UVC_URBS; ++i) { - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; + for_each_uvc_urb(uvc_urb, stream) { + /* + * Track how many URBs we allocate, adding one to the + * index to account for our zero index. + */ + i = uvc_urb_index(uvc_urb) + 1; stream->urb_size = psize * npackets; #ifndef CONFIG_DMA_NONCOHERENT @@ -1700,7 +1704,8 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream, struct usb_host_endpoint *ep, gfp_t gfp_flags) { struct urb *urb; - unsigned int npackets, i, j; + struct uvc_urb *uvc_urb; + unsigned int npackets, j; u16 psize; u32 size; @@ -1713,9 +1718,7 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream, size = npackets * psize; - for (i = 0; i < UVC_URBS; ++i) { - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; - + for_each_uvc_urb(uvc_urb, stream) { urb = usb_alloc_urb(npackets, gfp_flags); if (urb == NULL) { uvc_video_stop(stream, 1); @@ -1757,7 +1760,8 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream, struct usb_host_endpoint *ep, gfp_t gfp_flags) { struct urb *urb; - unsigned int npackets, pipe, i; + struct uvc_urb *uvc_urb; + unsigned int npackets, pipe; u16 psize; u32 size; @@ -1781,9 +1785,7 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream, if (stream->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) size = 0; - for (i = 0; i < UVC_URBS; ++i) { - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; - + for_each_uvc_urb(uvc_urb, stream) { urb = usb_alloc_urb(0, gfp_flags); if (urb == NULL) { uvc_video_stop(stream, 1); @@ -1810,6 +1812,7 @@ static int uvc_video_start(struct uvc_streaming *stream, gfp_t gfp_flags) { struct usb_interface *intf = stream->intf; struct usb_host_endpoint *ep; + struct uvc_urb *uvc_urb; unsigned int i; int ret; @@ -1887,13 +1890,11 @@ static int uvc_video_start(struct uvc_streaming *stream, gfp_t gfp_flags) return ret; /* Submit the URBs. */ - for (i = 0; i < UVC_URBS; ++i) { - struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; - + for_each_uvc_urb(uvc_urb, stream) { ret = usb_submit_urb(uvc_urb->urb, gfp_flags); if (ret < 0) { - uvc_printk(KERN_ERR, "Failed to submit URB %u " - "(%d).\n", i, ret); + uvc_printk(KERN_ERR, "Failed to submit URB %u (%d).\n", + uvc_urb_index(uvc_urb), ret); uvc_video_stop(stream, 1); return ret; } diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index c0a120496a1f..6a0f1b59356c 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -617,6 +617,9 @@ struct uvc_streaming { (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \ ++(uvc_urb)) +#define uvc_urb_index(uvc_urb) \ + (unsigned int)((uvc_urb) - (&(uvc_urb)->stream->uvc_urb[0])) + struct uvc_device_info { u32 quirks; u32 meta_format;