Re: [RFC/RFT PATCH 1/6] uvcvideo: Refactor URB descriptors

2018-01-06 Thread Kieran Bingham 
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

2018-01-04 Thread Guennadi Liakhovetski 
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

2018-01-03 Thread Kieran Bingham 
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 = >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 = >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, >urb_dma[i]);
+   gfp_flags | __GFP_NOWARN, _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 = >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 = >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) {
+