Re: [RFC/RFT PATCH 1/6] uvcvideo: Refactor URB descriptors
Hi Guennadi, Thanks for your review, On 04/01/18 18:24, Guennadi Liakhovetski wrote: > Hi Kieran, > > Just minor suggestions below: > > On Wed, 3 Jan 2018, Kieran Bingham wrote: > >> From: Kieran Bingham >> >> We currently store three separate arrays for each URB reference we hold. >> >> Objectify the data needed to track URBs into a single uvc_urb structure, >> allowing better object management and tracking of the URB. >> >> All accesses to the data pointers through stream, are converted to use a >> uvc_urb pointer for consistency. >> >> Signed-off-by: Kieran Bingham >> Reviewed-by: Laurent Pinchart >> --- >> drivers/media/usb/uvc/uvc_video.c | 46 >> drivers/media/usb/uvc/uvcvideo.h | 18 ++--- >> 2 files changed, 44 insertions(+), 20 deletions(-) > > [snip] > >> diff --git a/drivers/media/usb/uvc/uvcvideo.h >> b/drivers/media/usb/uvc/uvcvideo.h >> index 19e725e2bda5..4afa8ce13ea7 100644 >> --- a/drivers/media/usb/uvc/uvcvideo.h >> +++ b/drivers/media/usb/uvc/uvcvideo.h >> @@ -479,6 +479,20 @@ struct uvc_stats_stream { >> unsigned int max_sof; /* Maximum STC.SOF value */ >> }; >> >> +/** >> + * struct uvc_urb - URB context management structure >> + * >> + * @urb: described URB. Must be allocated with usb_alloc_urb() > > Didn't you mean "describes?" Hrm ... I think I meant described as in "This is the URB described by this uvc_urb structure", rather than "this variable describes the URB" Perhaps I'll change this to: @urb: The URB described by this context structure. I think the 'must be allocated with usb_alloc_urb() is fairly implicit, so could be dropped in that case. > >> + * @urb_buffer: memory storage for the URB >> + * @urb_dma: DMA coherent addressing for the urb_buffer > > The whole struct describes URBs, so, I wouldn't repeat that in these two > field names, I'd just call them "buffer" and "dma." OTOH, later you add > more fields like "stream," which aren't per-URB, so, maybe you want to > keep these prefixes. These names were kept from the original fields. But actually I think you're right here - it wouldn't hurt to shorten the names, even with the other fields added. > Thanks > Guennadi > >> + */ >> +struct uvc_urb { >> +struct urb *urb; >> + >> +char *urb_buffer; >> +dma_addr_t urb_dma; >> +}; >> + >> struct uvc_streaming { >> struct list_head list; >> struct uvc_device *dev; >> @@ -521,9 +535,7 @@ struct uvc_streaming { >> __u32 max_payload_size; >> } bulk; >> >> -struct urb *urb[UVC_URBS]; >> -char *urb_buffer[UVC_URBS]; >> -dma_addr_t urb_dma[UVC_URBS]; >> +struct uvc_urb uvc_urb[UVC_URBS]; >> unsigned int urb_size; >> >> __u32 sequence; >> -- >> git-series 0.9.1 >> -- Kieran
Re: [RFC/RFT PATCH 1/6] uvcvideo: Refactor URB descriptors
Hi Kieran, Just minor suggestions below: On Wed, 3 Jan 2018, Kieran Bingham wrote: > From: Kieran Bingham > > We currently store three separate arrays for each URB reference we hold. > > Objectify the data needed to track URBs into a single uvc_urb structure, > allowing better object management and tracking of the URB. > > All accesses to the data pointers through stream, are converted to use a > uvc_urb pointer for consistency. > > Signed-off-by: Kieran Bingham > Reviewed-by: Laurent Pinchart > --- > drivers/media/usb/uvc/uvc_video.c | 46 > drivers/media/usb/uvc/uvcvideo.h | 18 ++--- > 2 files changed, 44 insertions(+), 20 deletions(-) [snip] > diff --git a/drivers/media/usb/uvc/uvcvideo.h > b/drivers/media/usb/uvc/uvcvideo.h > index 19e725e2bda5..4afa8ce13ea7 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -479,6 +479,20 @@ struct uvc_stats_stream { > unsigned int max_sof; /* Maximum STC.SOF value */ > }; > > +/** > + * struct uvc_urb - URB context management structure > + * > + * @urb: described URB. Must be allocated with usb_alloc_urb() Didn't you mean "describes?" > + * @urb_buffer: memory storage for the URB > + * @urb_dma: DMA coherent addressing for the urb_buffer The whole struct describes URBs, so, I wouldn't repeat that in these two field names, I'd just call them "buffer" and "dma." OTOH, later you add more fields like "stream," which aren't per-URB, so, maybe you want to keep these prefixes. Thanks Guennadi > + */ > +struct uvc_urb { > + struct urb *urb; > + > + char *urb_buffer; > + dma_addr_t urb_dma; > +}; > + > struct uvc_streaming { > struct list_head list; > struct uvc_device *dev; > @@ -521,9 +535,7 @@ struct uvc_streaming { > __u32 max_payload_size; > } bulk; > > - struct urb *urb[UVC_URBS]; > - char *urb_buffer[UVC_URBS]; > - dma_addr_t urb_dma[UVC_URBS]; > + struct uvc_urb uvc_urb[UVC_URBS]; > unsigned int urb_size; > > __u32 sequence; > -- > git-series 0.9.1 >
[RFC/RFT PATCH 1/6] uvcvideo: Refactor URB descriptors
From: Kieran Bingham We currently store three separate arrays for each URB reference we hold. Objectify the data needed to track URBs into a single uvc_urb structure, allowing better object management and tracking of the URB. All accesses to the data pointers through stream, are converted to use a uvc_urb pointer for consistency. Signed-off-by: Kieran Bingham Reviewed-by: Laurent Pinchart --- drivers/media/usb/uvc/uvc_video.c | 46 drivers/media/usb/uvc/uvcvideo.h | 18 ++--- 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 73cd44e8bd81..86512f6229dd 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1357,14 +1357,16 @@ static void uvc_free_urb_buffers(struct uvc_streaming *stream) unsigned int i; for (i = 0; i < UVC_URBS; ++i) { - if (stream->urb_buffer[i]) { + struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; + + if (uvc_urb->urb_buffer) { #ifndef CONFIG_DMA_NONCOHERENT usb_free_coherent(stream->dev->udev, stream->urb_size, - stream->urb_buffer[i], stream->urb_dma[i]); + uvc_urb->urb_buffer, uvc_urb->urb_dma); #else - kfree(stream->urb_buffer[i]); + kfree(uvc_urb->urb_buffer); #endif - stream->urb_buffer[i] = NULL; + uvc_urb->urb_buffer = NULL; } } @@ -1402,16 +1404,18 @@ 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]; + stream->urb_size = psize * npackets; #ifndef CONFIG_DMA_NONCOHERENT - stream->urb_buffer[i] = usb_alloc_coherent( + uvc_urb->urb_buffer = usb_alloc_coherent( stream->dev->udev, stream->urb_size, - gfp_flags | __GFP_NOWARN, &stream->urb_dma[i]); + gfp_flags | __GFP_NOWARN, &uvc_urb->urb_dma); #else - stream->urb_buffer[i] = + uvc_urb->urb_buffer = kmalloc(stream->urb_size, gfp_flags | __GFP_NOWARN); #endif - if (!stream->urb_buffer[i]) { + if (!uvc_urb->urb_buffer) { uvc_free_urb_buffers(stream); break; } @@ -1441,13 +1445,15 @@ static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers) uvc_video_stats_stop(stream); for (i = 0; i < UVC_URBS; ++i) { - urb = stream->urb[i]; + struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; + + urb = uvc_urb->urb; if (urb == NULL) continue; usb_kill_urb(urb); usb_free_urb(urb); - stream->urb[i] = NULL; + uvc_urb->urb = NULL; } if (free_buffers) @@ -1502,6 +1508,8 @@ 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]; + urb = usb_alloc_urb(npackets, gfp_flags); if (urb == NULL) { uvc_uninit_video(stream, 1); @@ -1514,12 +1522,12 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream, ep->desc.bEndpointAddress); #ifndef CONFIG_DMA_NONCOHERENT urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP; - urb->transfer_dma = stream->urb_dma[i]; + urb->transfer_dma = uvc_urb->urb_dma; #else urb->transfer_flags = URB_ISO_ASAP; #endif urb->interval = ep->desc.bInterval; - urb->transfer_buffer = stream->urb_buffer[i]; + urb->transfer_buffer = uvc_urb->urb_buffer; urb->complete = uvc_video_complete; urb->number_of_packets = npackets; urb->transfer_buffer_length = size; @@ -1529,7 +1537,7 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream, urb->iso_frame_desc[j].length = psize; } - stream->urb[i] = urb; + uvc_urb->urb = urb; } return 0; @@ -1568,6 +1576,8 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream, size = 0; for (i = 0; i < UVC_URBS; ++i) { + struct uvc_urb *uvc_urb = &stream->uvc_urb[i]; +