diff mbox series

[v2,3/4] media: stk1160: move transfer_buffer and urb to same struct 'stk1160_urb'

Message ID 20220125080213.30090-4-dafna.hirschfeld@collabora.com (mailing list archive)
State New, archived
Headers show
Series stk1160: allocate urb buffs with the DMA noncontiguous API | expand

Commit Message

Dafna Hirschfeld Jan. 25, 2022, 8:02 a.m. UTC 
Instead of having two separated arrays, one for the urbs and
one for their buffers, have one array of a struct containing both.
In addition, the array is just 16 pointers, no need to dynamically
allocate it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/usb/stk1160/stk1160-v4l.c   |  2 +-
 drivers/media/usb/stk1160/stk1160-video.c | 52 ++++++++---------------
 drivers/media/usb/stk1160/stk1160.h       | 11 ++---
 3 files changed, 25 insertions(+), 40 deletions(-)

Comments

Ezequiel Garcia Feb. 7, 2022, 3:08 p.m. UTC | #1 
Hi Dafna,

On Tue, Jan 25, 2022 at 10:02:12AM +0200, Dafna Hirschfeld wrote:
> Instead of having two separated arrays, one for the urbs and
> one for their buffers, have one array of a struct containing both.
> In addition, the array is just 16 pointers, no need to dynamically
> allocate it.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks,
Ezequiel

> ---
>  drivers/media/usb/stk1160/stk1160-v4l.c   |  2 +-
>  drivers/media/usb/stk1160/stk1160-video.c | 52 ++++++++---------------
>  drivers/media/usb/stk1160/stk1160.h       | 11 ++---
>  3 files changed, 25 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
> index 1aa953469402..ebf245d44005 100644
> --- a/drivers/media/usb/stk1160/stk1160-v4l.c
> +++ b/drivers/media/usb/stk1160/stk1160-v4l.c
> @@ -232,7 +232,7 @@ static int stk1160_start_streaming(struct stk1160 *dev)
>  
>  	/* submit urbs and enables IRQ */
>  	for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
> -		rc = usb_submit_urb(dev->isoc_ctl.urb[i], GFP_KERNEL);
> +		rc = usb_submit_urb(dev->isoc_ctl.urb_ctl[i].urb, GFP_KERNEL);
>  		if (rc) {
>  			stk1160_err("cannot submit urb[%d] (%d)\n", i, rc);
>  			goto out_uninit;
> diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
> index 92c8b1fba2b0..f3c0497a8539 100644
> --- a/drivers/media/usb/stk1160/stk1160-video.c
> +++ b/drivers/media/usb/stk1160/stk1160-video.c
> @@ -347,7 +347,7 @@ void stk1160_cancel_isoc(struct stk1160 *dev)
>  		 * We don't care for NULL pointer since
>  		 * usb_kill_urb allows it.
>  		 */
> -		usb_kill_urb(dev->isoc_ctl.urb[i]);
> +		usb_kill_urb(dev->isoc_ctl.urb_ctl[i].urb);
>  	}
>  
>  	stk1160_dbg("all urbs killed\n");
> @@ -366,30 +366,25 @@ void stk1160_free_isoc(struct stk1160 *dev)
>  
>  	for (i = 0; i < num_bufs; i++) {
>  
> -		urb = dev->isoc_ctl.urb[i];
> +		urb = dev->isoc_ctl.urb_ctl[i].urb;
>  		if (urb) {
>  
> -			if (dev->isoc_ctl.transfer_buffer[i]) {
> +			if (dev->isoc_ctl.urb_ctl[i].transfer_buffer) {
>  #ifndef CONFIG_DMA_NONCOHERENT
>  				usb_free_coherent(dev->udev,
>  					urb->transfer_buffer_length,
> -					dev->isoc_ctl.transfer_buffer[i],
> +					dev->isoc_ctl.urb_ctl[i].transfer_buffer,
>  					urb->transfer_dma);
>  #else
> -				kfree(dev->isoc_ctl.transfer_buffer[i]);
> +				kfree(dev->isoc_ctl.urb_ctl[i].transfer_buffer);
>  #endif
>  			}
>  			usb_free_urb(urb);
> -			dev->isoc_ctl.urb[i] = NULL;
> +			dev->isoc_ctl.urb_ctl[i].urb = NULL;
>  		}
> -		dev->isoc_ctl.transfer_buffer[i] = NULL;
> +		dev->isoc_ctl.urb_ctl[i].transfer_buffer = NULL;
>  	}
>  
> -	kfree(dev->isoc_ctl.urb);
> -	kfree(dev->isoc_ctl.transfer_buffer);
> -
> -	dev->isoc_ctl.urb = NULL;
> -	dev->isoc_ctl.transfer_buffer = NULL;
>  	dev->isoc_ctl.num_bufs = 0;
>  
>  	stk1160_dbg("all urb buffers freed\n");
> @@ -429,19 +424,6 @@ int stk1160_alloc_isoc(struct stk1160 *dev)
>  
>  	dev->isoc_ctl.buf = NULL;
>  	dev->isoc_ctl.max_pkt_size = dev->max_pkt_size;
> -	dev->isoc_ctl.urb = kcalloc(num_bufs, sizeof(void *), GFP_KERNEL);
> -	if (!dev->isoc_ctl.urb) {
> -		stk1160_err("out of memory for urb array\n");
> -		return -ENOMEM;
> -	}
> -
> -	dev->isoc_ctl.transfer_buffer = kcalloc(num_bufs, sizeof(void *),
> -						GFP_KERNEL);
> -	if (!dev->isoc_ctl.transfer_buffer) {
> -		stk1160_err("out of memory for usb transfers\n");
> -		kfree(dev->isoc_ctl.urb);
> -		return -ENOMEM;
> -	}
>  
>  	/* allocate urbs and transfer buffers */
>  	for (i = 0; i < num_bufs; i++) {
> @@ -449,15 +431,17 @@ int stk1160_alloc_isoc(struct stk1160 *dev)
>  		urb = usb_alloc_urb(max_packets, GFP_KERNEL);
>  		if (!urb)
>  			goto free_i_bufs;
> -		dev->isoc_ctl.urb[i] = urb;
> +		dev->isoc_ctl.urb_ctl[i].urb = urb;
>  
>  #ifndef CONFIG_DMA_NONCOHERENT
> -		dev->isoc_ctl.transfer_buffer[i] = usb_alloc_coherent(dev->udev,
> -			sb_size, GFP_KERNEL, &urb->transfer_dma);
> +		dev->isoc_ctl.urb_ctl[i].transfer_buffer =
> +			usb_alloc_coherent(dev->udev, sb_size, GFP_KERNEL,
> +					   &urb->transfer_dma);
>  #else
> -		dev->isoc_ctl.transfer_buffer[i] = kmalloc(sb_size, GFP_KERNEL);
> +		dev->isoc_ctl.urb_ctl[i].transfer_buffer =
> +			kmalloc(sb_size, GFP_KERNEL);
>  #endif
> -		if (!dev->isoc_ctl.transfer_buffer[i]) {
> +		if (!dev->isoc_ctl.urb_ctl[i].transfer_buffer) {
>  			stk1160_err("cannot alloc %d bytes for tx[%d] buffer\n",
>  				sb_size, i);
>  
> @@ -466,14 +450,14 @@ int stk1160_alloc_isoc(struct stk1160 *dev)
>  				goto free_i_bufs;
>  			goto nomore_tx_bufs;
>  		}
> -		memset(dev->isoc_ctl.transfer_buffer[i], 0, sb_size);
> +		memset(dev->isoc_ctl.urb_ctl[i].transfer_buffer, 0, sb_size);
>  
>  		/*
>  		 * FIXME: Where can I get the endpoint?
>  		 */
>  		urb->dev = dev->udev;
>  		urb->pipe = usb_rcvisocpipe(dev->udev, STK1160_EP_VIDEO);
> -		urb->transfer_buffer = dev->isoc_ctl.transfer_buffer[i];
> +		urb->transfer_buffer = dev->isoc_ctl.urb_ctl[i].transfer_buffer;
>  		urb->transfer_buffer_length = sb_size;
>  		urb->complete = stk1160_isoc_irq;
>  		urb->context = dev;
> @@ -508,8 +492,8 @@ int stk1160_alloc_isoc(struct stk1160 *dev)
>  	 * enough to work fine, so we just free the extra urb,
>  	 * store the allocated count and keep going, fingers crossed!
>  	 */
> -	usb_free_urb(dev->isoc_ctl.urb[i]);
> -	dev->isoc_ctl.urb[i] = NULL;
> +	usb_free_urb(dev->isoc_ctl.urb_ctl[i].urb);
> +	dev->isoc_ctl.urb_ctl[i].urb = NULL;
>  
>  	stk1160_warn("%d urbs allocated. Trying to continue...\n", i);
>  
> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
> index a70963ce8753..0c355bb078c1 100644
> --- a/drivers/media/usb/stk1160/stk1160.h
> +++ b/drivers/media/usb/stk1160/stk1160.h
> @@ -84,6 +84,11 @@ struct stk1160_buffer {
>  	unsigned int pos;		/* current pos inside buffer */
>  };
>  
> +struct stk1160_urb {
> +	struct urb *urb;
> +	char *transfer_buffer;
> +};
> +
>  struct stk1160_isoc_ctl {
>  	/* max packet size of isoc transaction */
>  	int max_pkt_size;
> @@ -91,11 +96,7 @@ struct stk1160_isoc_ctl {
>  	/* number of allocated urbs */
>  	int num_bufs;
>  
> -	/* urb for isoc transfers */
> -	struct urb **urb;
> -
> -	/* transfer buffers for isoc transfer */
> -	char **transfer_buffer;
> +	struct stk1160_urb urb_ctl[STK1160_NUM_BUFS];
>  
>  	/* current buffer */
>  	struct stk1160_buffer *buf;
> -- 
> 2.17.1
>
diff mbox series

Patch

diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
index 1aa953469402..ebf245d44005 100644
--- a/drivers/media/usb/stk1160/stk1160-v4l.c
+++ b/drivers/media/usb/stk1160/stk1160-v4l.c
@@ -232,7 +232,7 @@  static int stk1160_start_streaming(struct stk1160 *dev)
 
 	/* submit urbs and enables IRQ */
 	for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {
-		rc = usb_submit_urb(dev->isoc_ctl.urb[i], GFP_KERNEL);
+		rc = usb_submit_urb(dev->isoc_ctl.urb_ctl[i].urb, GFP_KERNEL);
 		if (rc) {
 			stk1160_err("cannot submit urb[%d] (%d)\n", i, rc);
 			goto out_uninit;
diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
index 92c8b1fba2b0..f3c0497a8539 100644
--- a/drivers/media/usb/stk1160/stk1160-video.c
+++ b/drivers/media/usb/stk1160/stk1160-video.c
@@ -347,7 +347,7 @@  void stk1160_cancel_isoc(struct stk1160 *dev)
 		 * We don't care for NULL pointer since
 		 * usb_kill_urb allows it.
 		 */
-		usb_kill_urb(dev->isoc_ctl.urb[i]);
+		usb_kill_urb(dev->isoc_ctl.urb_ctl[i].urb);
 	}
 
 	stk1160_dbg("all urbs killed\n");
@@ -366,30 +366,25 @@  void stk1160_free_isoc(struct stk1160 *dev)
 
 	for (i = 0; i < num_bufs; i++) {
 
-		urb = dev->isoc_ctl.urb[i];
+		urb = dev->isoc_ctl.urb_ctl[i].urb;
 		if (urb) {
 
-			if (dev->isoc_ctl.transfer_buffer[i]) {
+			if (dev->isoc_ctl.urb_ctl[i].transfer_buffer) {
 #ifndef CONFIG_DMA_NONCOHERENT
 				usb_free_coherent(dev->udev,
 					urb->transfer_buffer_length,
-					dev->isoc_ctl.transfer_buffer[i],
+					dev->isoc_ctl.urb_ctl[i].transfer_buffer,
 					urb->transfer_dma);
 #else
-				kfree(dev->isoc_ctl.transfer_buffer[i]);
+				kfree(dev->isoc_ctl.urb_ctl[i].transfer_buffer);
 #endif
 			}
 			usb_free_urb(urb);
-			dev->isoc_ctl.urb[i] = NULL;
+			dev->isoc_ctl.urb_ctl[i].urb = NULL;
 		}
-		dev->isoc_ctl.transfer_buffer[i] = NULL;
+		dev->isoc_ctl.urb_ctl[i].transfer_buffer = NULL;
 	}
 
-	kfree(dev->isoc_ctl.urb);
-	kfree(dev->isoc_ctl.transfer_buffer);
-
-	dev->isoc_ctl.urb = NULL;
-	dev->isoc_ctl.transfer_buffer = NULL;
 	dev->isoc_ctl.num_bufs = 0;
 
 	stk1160_dbg("all urb buffers freed\n");
@@ -429,19 +424,6 @@  int stk1160_alloc_isoc(struct stk1160 *dev)
 
 	dev->isoc_ctl.buf = NULL;
 	dev->isoc_ctl.max_pkt_size = dev->max_pkt_size;
-	dev->isoc_ctl.urb = kcalloc(num_bufs, sizeof(void *), GFP_KERNEL);
-	if (!dev->isoc_ctl.urb) {
-		stk1160_err("out of memory for urb array\n");
-		return -ENOMEM;
-	}
-
-	dev->isoc_ctl.transfer_buffer = kcalloc(num_bufs, sizeof(void *),
-						GFP_KERNEL);
-	if (!dev->isoc_ctl.transfer_buffer) {
-		stk1160_err("out of memory for usb transfers\n");
-		kfree(dev->isoc_ctl.urb);
-		return -ENOMEM;
-	}
 
 	/* allocate urbs and transfer buffers */
 	for (i = 0; i < num_bufs; i++) {
@@ -449,15 +431,17 @@  int stk1160_alloc_isoc(struct stk1160 *dev)
 		urb = usb_alloc_urb(max_packets, GFP_KERNEL);
 		if (!urb)
 			goto free_i_bufs;
-		dev->isoc_ctl.urb[i] = urb;
+		dev->isoc_ctl.urb_ctl[i].urb = urb;
 
 #ifndef CONFIG_DMA_NONCOHERENT
-		dev->isoc_ctl.transfer_buffer[i] = usb_alloc_coherent(dev->udev,
-			sb_size, GFP_KERNEL, &urb->transfer_dma);
+		dev->isoc_ctl.urb_ctl[i].transfer_buffer =
+			usb_alloc_coherent(dev->udev, sb_size, GFP_KERNEL,
+					   &urb->transfer_dma);
 #else
-		dev->isoc_ctl.transfer_buffer[i] = kmalloc(sb_size, GFP_KERNEL);
+		dev->isoc_ctl.urb_ctl[i].transfer_buffer =
+			kmalloc(sb_size, GFP_KERNEL);
 #endif
-		if (!dev->isoc_ctl.transfer_buffer[i]) {
+		if (!dev->isoc_ctl.urb_ctl[i].transfer_buffer) {
 			stk1160_err("cannot alloc %d bytes for tx[%d] buffer\n",
 				sb_size, i);
 
@@ -466,14 +450,14 @@  int stk1160_alloc_isoc(struct stk1160 *dev)
 				goto free_i_bufs;
 			goto nomore_tx_bufs;
 		}
-		memset(dev->isoc_ctl.transfer_buffer[i], 0, sb_size);
+		memset(dev->isoc_ctl.urb_ctl[i].transfer_buffer, 0, sb_size);
 
 		/*
 		 * FIXME: Where can I get the endpoint?
 		 */
 		urb->dev = dev->udev;
 		urb->pipe = usb_rcvisocpipe(dev->udev, STK1160_EP_VIDEO);
-		urb->transfer_buffer = dev->isoc_ctl.transfer_buffer[i];
+		urb->transfer_buffer = dev->isoc_ctl.urb_ctl[i].transfer_buffer;
 		urb->transfer_buffer_length = sb_size;
 		urb->complete = stk1160_isoc_irq;
 		urb->context = dev;
@@ -508,8 +492,8 @@  int stk1160_alloc_isoc(struct stk1160 *dev)
 	 * enough to work fine, so we just free the extra urb,
 	 * store the allocated count and keep going, fingers crossed!
 	 */
-	usb_free_urb(dev->isoc_ctl.urb[i]);
-	dev->isoc_ctl.urb[i] = NULL;
+	usb_free_urb(dev->isoc_ctl.urb_ctl[i].urb);
+	dev->isoc_ctl.urb_ctl[i].urb = NULL;
 
 	stk1160_warn("%d urbs allocated. Trying to continue...\n", i);
 
diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
index a70963ce8753..0c355bb078c1 100644
--- a/drivers/media/usb/stk1160/stk1160.h
+++ b/drivers/media/usb/stk1160/stk1160.h
@@ -84,6 +84,11 @@  struct stk1160_buffer {
 	unsigned int pos;		/* current pos inside buffer */
 };
 
+struct stk1160_urb {
+	struct urb *urb;
+	char *transfer_buffer;
+};
+
 struct stk1160_isoc_ctl {
 	/* max packet size of isoc transaction */
 	int max_pkt_size;
@@ -91,11 +96,7 @@  struct stk1160_isoc_ctl {
 	/* number of allocated urbs */
 	int num_bufs;
 
-	/* urb for isoc transfers */
-	struct urb **urb;
-
-	/* transfer buffers for isoc transfer */
-	char **transfer_buffer;
+	struct stk1160_urb urb_ctl[STK1160_NUM_BUFS];
 
 	/* current buffer */
 	struct stk1160_buffer *buf;