All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 00/10] Asynchronous UVC
@ 2018-11-09 17:05 Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 01/10] media: uvcvideo: Refactor URB descriptors Kieran Bingham
                   ` (9 more replies)
  0 siblings, 10 replies; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

The Linux UVC driver has long provided adequate performance capabilities for
web-cams and low data rate video devices in Linux while resolutions were low.

Modern USB cameras are now capable of high data rates thanks to USB3 with
1080p, and even 4k capture resolutions supported.

Cameras such as the Stereolabs ZED (bulk transfers) or the Logitech BRIO
(isochronous transfers) can generate more data than an embedded ARM core is
able to process on a single core, resulting in frame loss.

A large part of this performance impact is from the requirement to
‘memcpy’ frames out from URB packets to destination frames. This unfortunate
requirement is due to the UVC protocol allowing a variable length header, and
thus it is not possible to provide the target frame buffers directly.

Extra throughput is possible by moving the actual memcpy actions to a work
queue, and moving the memcpy out of interrupt context thus allowing work tasks
to be scheduled across multiple cores.

This series has been tested on both the ZED and BRIO cameras on arm64
platforms, and with thanks to Randy Dunlap, a Dynex 1.3MP Webcam, a Sonix USB2
Camera, and a built in Toshiba Laptop camera, and with thanks to Philipp Zabel
for testing on a Lite-On internal Laptop Webcam, Logitech C910 (USB2 isoc),
Oculus Sensor (USB3 isoc), and Microsoft HoloLens Sensors (USB3 bulk).

As far as I am aware iSight devices, and devices which use UVC to encode data
(output device) have not yet been tested - but should find no ill effect (at
least not until they are tested of course :D )

Tested-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Philipp Zabel <philipp.zabel@gmail.com>

This series is based upon Laurent Pinchart's uvc/next available at:
  git://linuxtv.org/pinchartl/media.git uvc/next

and the series itself is available at my kernel.org repo:
  git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/linux.git uvc/async

v2:
 - Fix race reported by Guennadi

v3:
 - Fix similar race reported by Laurent
 - Only queue work if required (encode/isight do not queue work)
 - Refactor/Rename variables for clarity

v4:
 - (Yet another) Rework of the uninitialise path.
   This time to hopefully clean up the shutdown races for good.
   use usb_poison_urb() to halt all URBs, then flush the work queue
   before freeing.
 - Rebase to latest linux-media/master

v5:
 - Provide lockdep validation
 - rename uvc_queue_requeue -> uvc_queue_buffer_requeue()
 - Fix comments and periods throughout
 - Rebase to media/v4.20-2
 - Use GFP_KERNEL allocation in uvc_video_copy_data_work()
 - Fix function documentation for uvc_video_copy_data_work()
 - Add periods to the end of sentences
 - Rename 'decode' variable to 'op' in uvc_video_decode_data()
 - Move uvc_urb->async_operations initialisation to before use
 - Move async workqueue to match uvc_streaming lifetime instead of
   streamon/streamoff
 - bracket the for_each_uvc_urb() macro

 - New patches added to series:
    media: uvcvideo: Split uvc_video_enable into two
    media: uvcvideo: Rename uvc_{un,}init_video()
    media: uvcvideo: Utilise for_each_uvc_urb iterator

v6:
 - New patch added to series
    media: uvcvideo: Abstract streaming object lifetime

 - Utilise the new streaming object lifetime functions to perform
   allocation and destruction of the async workqueue.
 - Append _transfer to {_stop,_start} in uvc_video_{stop,start}_transfer
 - rename lone 'j' iterator to 'i'
 - Remove conversion which doesn't make sense due to needing the
   iterator value.

Kieran Bingham (10):
  media: uvcvideo: Refactor URB descriptors
  media: uvcvideo: Convert decode functions to use new context structure
  media: uvcvideo: Protect queue internals with helper
  media: uvcvideo: queue: Simplify spin-lock usage
  media: uvcvideo: queue: Support asynchronous buffer handling
  media: uvcvideo: Abstract streaming object lifetime
  media: uvcvideo: Move decode processing to process context
  media: uvcvideo: Split uvc_video_enable into two
  media: uvcvideo: Rename uvc_{un,}init_video()
  media: uvcvideo: Utilise for_each_uvc_urb iterator

 drivers/media/usb/uvc/uvc_driver.c |  65 +++++--
 drivers/media/usb/uvc/uvc_isight.c |   6 +-
 drivers/media/usb/uvc/uvc_queue.c  | 110 +++++++++---
 drivers/media/usb/uvc/uvc_video.c  | 275 ++++++++++++++++++------------
 drivers/media/usb/uvc/uvcvideo.h   |  65 ++++++-
 5 files changed, 370 insertions(+), 151 deletions(-)

base-commit: b82b45e8c9d5d015c6887216868e3335897662d3
-- 
git-series 0.9.1

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH v6 01/10] media: uvcvideo: Refactor URB descriptors
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 02/10] media: uvcvideo: Convert decode functions to use new context structure Kieran Bingham
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

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 <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
v2:
 - Re-describe URB context structure
 - Re-name uvc_urb->{urb_buffer,urb_dma}{buffer,dma}

v3:
 - No change

v4:
 - Rebase on top of linux-media/master (v4.16-rc4, metadata additions)

 drivers/media/usb/uvc/uvc_video.c | 49 +++++++++++++++++++-------------
 drivers/media/usb/uvc/uvcvideo.h  | 18 ++++++++++--
 2 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 86a99f461fd8..113881bed2a4 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1506,14 +1506,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->buffer) {
 #ifndef CONFIG_DMA_NONCOHERENT
 			usb_free_coherent(stream->dev->udev, stream->urb_size,
-				stream->urb_buffer[i], stream->urb_dma[i]);
+					uvc_urb->buffer, uvc_urb->dma);
 #else
-			kfree(stream->urb_buffer[i]);
+			kfree(uvc_urb->buffer);
 #endif
-			stream->urb_buffer[i] = NULL;
+			uvc_urb->buffer = NULL;
 		}
 	}
 
@@ -1551,16 +1553,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->buffer = usb_alloc_coherent(
 				stream->dev->udev, stream->urb_size,
-				gfp_flags | __GFP_NOWARN, &stream->urb_dma[i]);
+				gfp_flags | __GFP_NOWARN, &uvc_urb->dma);
 #else
-			stream->urb_buffer[i] =
+			uvc_urb->buffer =
 			    kmalloc(stream->urb_size, gfp_flags | __GFP_NOWARN);
 #endif
-			if (!stream->urb_buffer[i]) {
+			if (!uvc_urb->buffer) {
 				uvc_free_urb_buffers(stream);
 				break;
 			}
@@ -1590,13 +1594,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)
@@ -1651,6 +1657,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);
@@ -1663,12 +1671,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->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->buffer;
 		urb->complete = uvc_video_complete;
 		urb->number_of_packets = npackets;
 		urb->transfer_buffer_length = size;
@@ -1678,7 +1686,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;
@@ -1717,21 +1725,22 @@ 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];
+
 		urb = usb_alloc_urb(0, gfp_flags);
 		if (urb == NULL) {
 			uvc_uninit_video(stream, 1);
 			return -ENOMEM;
 		}
 
-		usb_fill_bulk_urb(urb, stream->dev->udev, pipe,
-			stream->urb_buffer[i], size, uvc_video_complete,
-			stream);
+		usb_fill_bulk_urb(urb, stream->dev->udev, pipe, uvc_urb->buffer,
+				  size, uvc_video_complete, stream);
 #ifndef CONFIG_DMA_NONCOHERENT
 		urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
-		urb->transfer_dma = stream->urb_dma[i];
+		urb->transfer_dma = uvc_urb->dma;
 #endif
 
-		stream->urb[i] = urb;
+		uvc_urb->urb = urb;
 	}
 
 	return 0;
@@ -1822,7 +1831,9 @@ static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
 
 	/* Submit the URBs. */
 	for (i = 0; i < UVC_URBS; ++i) {
-		ret = usb_submit_urb(stream->urb[i], gfp_flags);
+		struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+
+		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);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index facf77192b19..7233249b59f5 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -490,6 +490,20 @@ struct uvc_stats_stream {
 
 #define UVC_METATADA_BUF_SIZE 1024
 
+/**
+ * struct uvc_urb - URB context management structure
+ *
+ * @urb: the URB described by this context structure
+ * @buffer: memory storage for the URB
+ * @dma: DMA coherent addressing for the urb_buffer
+ */
+struct uvc_urb {
+	struct urb *urb;
+
+	char *buffer;
+	dma_addr_t dma;
+};
+
 struct uvc_streaming {
 	struct list_head list;
 	struct uvc_device *dev;
@@ -538,9 +552,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

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 02/10] media: uvcvideo: Convert decode functions to use new context structure
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 01/10] media: uvcvideo: Refactor URB descriptors Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 03/10] media: uvcvideo: Protect queue internals with helper Kieran Bingham
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

The URB completion handlers currently reference the stream context.

Now that each URB has its own context structure, convert the decode (and
one encode) functions to utilise this context for URB management.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
v2:
 - fix checkpatch warning (pre-existing in code)

v3: (none)

v4:
 - Rebase on top of linux-media/master (v4.16-rc4, metadata additions)

 drivers/media/usb/uvc/uvc_isight.c |  6 ++++--
 drivers/media/usb/uvc/uvc_video.c  | 26 ++++++++++++++++++--------
 drivers/media/usb/uvc/uvcvideo.h   |  8 +++++---
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c
index 81e6f2187bfb..39a4e4482b23 100644
--- a/drivers/media/usb/uvc/uvc_isight.c
+++ b/drivers/media/usb/uvc/uvc_isight.c
@@ -99,9 +99,11 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf,
 	return 0;
 }
 
-void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
-			struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
+void uvc_video_decode_isight(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
+			struct uvc_buffer *meta_buf)
 {
+	struct urb *urb = uvc_urb->urb;
+	struct uvc_streaming *stream = uvc_urb->stream;
 	int ret, i;
 
 	for (i = 0; i < urb->number_of_packets; ++i) {
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 113881bed2a4..6d4384695964 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1291,9 +1291,11 @@ static void uvc_video_next_buffers(struct uvc_streaming *stream,
 	*video_buf = uvc_queue_next_buffer(&stream->queue, *video_buf);
 }
 
-static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
+static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
 			struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
 {
+	struct urb *urb = uvc_urb->urb;
+	struct uvc_streaming *stream = uvc_urb->stream;
 	u8 *mem;
 	int ret, i;
 
@@ -1334,9 +1336,11 @@ static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
 	}
 }
 
-static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
+static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
 			struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
 {
+	struct urb *urb = uvc_urb->urb;
+	struct uvc_streaming *stream = uvc_urb->stream;
 	u8 *mem;
 	int len, ret;
 
@@ -1402,9 +1406,12 @@ static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
 	}
 }
 
-static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming *stream,
+static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
 	struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
 {
+	struct urb *urb = uvc_urb->urb;
+	struct uvc_streaming *stream = uvc_urb->stream;
+
 	u8 *mem = urb->transfer_buffer;
 	int len = stream->urb_size, ret;
 
@@ -1447,7 +1454,8 @@ static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming *stream,
 
 static void uvc_video_complete(struct urb *urb)
 {
-	struct uvc_streaming *stream = urb->context;
+	struct uvc_urb *uvc_urb = urb->context;
+	struct uvc_streaming *stream = uvc_urb->stream;
 	struct uvc_video_queue *queue = &stream->queue;
 	struct uvc_video_queue *qmeta = &stream->meta.queue;
 	struct vb2_queue *vb2_qmeta = stream->meta.vdev.queue;
@@ -1490,7 +1498,7 @@ static void uvc_video_complete(struct urb *urb)
 		spin_unlock_irqrestore(&qmeta->irqlock, flags);
 	}
 
-	stream->decode(urb, stream, buf, buf_meta);
+	stream->decode(uvc_urb, buf, buf_meta);
 
 	if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
 		uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
@@ -1568,6 +1576,8 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
 				uvc_free_urb_buffers(stream);
 				break;
 			}
+
+			uvc_urb->stream = stream;
 		}
 
 		if (i == UVC_URBS) {
@@ -1666,7 +1676,7 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 		}
 
 		urb->dev = stream->dev->udev;
-		urb->context = stream;
+		urb->context = uvc_urb;
 		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
 				ep->desc.bEndpointAddress);
 #ifndef CONFIG_DMA_NONCOHERENT
@@ -1733,8 +1743,8 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
 			return -ENOMEM;
 		}
 
-		usb_fill_bulk_urb(urb, stream->dev->udev, pipe, uvc_urb->buffer,
-				  size, uvc_video_complete, stream);
+		usb_fill_bulk_urb(urb, stream->dev->udev, pipe,	uvc_urb->buffer,
+				  size, uvc_video_complete, uvc_urb);
 #ifndef CONFIG_DMA_NONCOHERENT
 		urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
 		urb->transfer_dma = uvc_urb->dma;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 7233249b59f5..7c83185c5b87 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -494,11 +494,13 @@ struct uvc_stats_stream {
  * struct uvc_urb - URB context management structure
  *
  * @urb: the URB described by this context structure
+ * @stream: UVC streaming context
  * @buffer: memory storage for the URB
  * @dma: DMA coherent addressing for the urb_buffer
  */
 struct uvc_urb {
 	struct urb *urb;
+	struct uvc_streaming *stream;
 
 	char *buffer;
 	dma_addr_t dma;
@@ -534,8 +536,8 @@ struct uvc_streaming {
 	/* Buffers queue. */
 	unsigned int frozen : 1;
 	struct uvc_video_queue queue;
-	void (*decode) (struct urb *urb, struct uvc_streaming *video,
-			struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
+	void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
+		       struct uvc_buffer *meta_buf);
 
 	struct {
 		struct video_device vdev;
@@ -821,7 +823,7 @@ struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts,
 					    u8 epaddr);
 
 /* Quirks support */
-void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
+void uvc_video_decode_isight(struct uvc_urb *uvc_urb,
 			     struct uvc_buffer *buf,
 			     struct uvc_buffer *meta_buf);
 
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 03/10] media: uvcvideo: Protect queue internals with helper
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 01/10] media: uvcvideo: Refactor URB descriptors Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 02/10] media: uvcvideo: Convert decode functions to use new context structure Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 04/10] media: uvcvideo: queue: Simplify spin-lock usage Kieran Bingham
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

The URB completion operation obtains the current buffer by reading
directly into the queue internal interface.

Protect this queue abstraction by providing a helper
uvc_queue_get_current_buffer() which can be used by both the decode
task, and the uvc_queue_next_buffer() functions.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---

v2:
 - Fix coding style of conditional statements

v3:
 - No change

v4:
 - Rebase on top of linux-media/master (v4.16-rc4, metadata additions)

 drivers/media/usb/uvc/uvc_queue.c | 33 +++++++++++++++++++++++++++-----
 drivers/media/usb/uvc/uvc_video.c |  6 +-----
 drivers/media/usb/uvc/uvcvideo.h  |  1 +-
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index fecccb5e7628..adcc4928fae4 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -429,6 +429,33 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
 	spin_unlock_irqrestore(&queue->irqlock, flags);
 }
 
+/*
+ * uvc_queue_get_current_buffer: Obtain the current working output buffer
+ *
+ * Buffers may span multiple packets, and even URBs, therefore the active buffer
+ * remains on the queue until the EOF marker.
+ */
+static struct uvc_buffer *
+__uvc_queue_get_current_buffer(struct uvc_video_queue *queue)
+{
+	if (list_empty(&queue->irqqueue))
+		return NULL;
+
+	return list_first_entry(&queue->irqqueue, struct uvc_buffer, queue);
+}
+
+struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue)
+{
+	struct uvc_buffer *nextbuf;
+	unsigned long flags;
+
+	spin_lock_irqsave(&queue->irqlock, flags);
+	nextbuf = __uvc_queue_get_current_buffer(queue);
+	spin_unlock_irqrestore(&queue->irqlock, flags);
+
+	return nextbuf;
+}
+
 struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 		struct uvc_buffer *buf)
 {
@@ -445,11 +472,7 @@ struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 
 	spin_lock_irqsave(&queue->irqlock, flags);
 	list_del(&buf->queue);
-	if (!list_empty(&queue->irqqueue))
-		nextbuf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
-					   queue);
-	else
-		nextbuf = NULL;
+	nextbuf = __uvc_queue_get_current_buffer(queue);
 	spin_unlock_irqrestore(&queue->irqlock, flags);
 
 	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 6d4384695964..7a7779e1b466 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1484,11 +1484,7 @@ static void uvc_video_complete(struct urb *urb)
 		return;
 	}
 
-	spin_lock_irqsave(&queue->irqlock, flags);
-	if (!list_empty(&queue->irqqueue))
-		buf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
-				       queue);
-	spin_unlock_irqrestore(&queue->irqlock, flags);
+	buf = uvc_queue_get_current_buffer(queue);
 
 	if (vb2_qmeta) {
 		spin_lock_irqsave(&qmeta->irqlock, flags);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 7c83185c5b87..16b8348f7ff0 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -727,6 +727,7 @@ int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type);
 void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect);
 struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 					 struct uvc_buffer *buf);
+struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
 int uvc_queue_mmap(struct uvc_video_queue *queue,
 		   struct vm_area_struct *vma);
 __poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 04/10] media: uvcvideo: queue: Simplify spin-lock usage
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
                   ` (2 preceding siblings ...)
  2018-11-09 17:05 ` [PATCH v6 03/10] media: uvcvideo: Protect queue internals with helper Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 05/10] media: uvcvideo: queue: Support asynchronous buffer handling Kieran Bingham
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

Both uvc_start_streaming(), and uvc_stop_streaming() are called from
userspace context, with interrupts enabled. As such, they do not need to
save the IRQ state, and can use spin_lock_irq() and spin_unlock_irq()
respectively.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---

v4:
 - Rebase to v4.16 (linux-media/master)

v5:
 - Provide lockdep validation

 drivers/media/usb/uvc/uvc_queue.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index adcc4928fae4..fa7059aab49a 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -169,18 +169,19 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
 	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
-	unsigned long flags;
 	int ret;
 
+	lockdep_assert_irqs_enabled();
+
 	queue->buf_used = 0;
 
 	ret = uvc_video_enable(stream, 1);
 	if (ret == 0)
 		return 0;
 
-	spin_lock_irqsave(&queue->irqlock, flags);
+	spin_lock_irq(&queue->irqlock);
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
-	spin_unlock_irqrestore(&queue->irqlock, flags);
+	spin_unlock_irq(&queue->irqlock);
 
 	return ret;
 }
@@ -188,14 +189,15 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
 static void uvc_stop_streaming(struct vb2_queue *vq)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
-	unsigned long flags;
+
+	lockdep_assert_irqs_enabled();
 
 	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
 		uvc_video_enable(uvc_queue_to_stream(queue), 0);
 
-	spin_lock_irqsave(&queue->irqlock, flags);
+	spin_lock_irq(&queue->irqlock);
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
-	spin_unlock_irqrestore(&queue->irqlock, flags);
+	spin_unlock_irq(&queue->irqlock);
 }
 
 static const struct vb2_ops uvc_queue_qops = {
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 05/10] media: uvcvideo: queue: Support asynchronous buffer handling
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
                   ` (3 preceding siblings ...)
  2018-11-09 17:05 ` [PATCH v6 04/10] media: uvcvideo: queue: Simplify spin-lock usage Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-11-10 16:30   ` Laurent Pinchart
  2018-11-09 17:05 ` [PATCH v6 06/10] media: uvcvideo: Abstract streaming object lifetime Kieran Bingham
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

The buffer queue interface currently operates sequentially, processing
buffers after they have fully completed.

In preparation for supporting parallel tasks operating on the buffers,
we will need to support buffers being processed on multiple CPUs.

Adapt the uvc_queue_next_buffer() such that a reference count tracks the
active use of the buffer, returning the buffer to the VB2 stack at
completion.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

v5:
 - uvc_queue_requeue() -> uvc_queue_buffer_requeue()
 - Fix comment
---
 drivers/media/usb/uvc/uvc_queue.c | 61 ++++++++++++++++++++++++++------
 drivers/media/usb/uvc/uvcvideo.h  |  4 ++-
 2 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index fa7059aab49a..2752e386f1e8 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -142,6 +142,7 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
 
 	spin_lock_irqsave(&queue->irqlock, flags);
 	if (likely(!(queue->flags & UVC_QUEUE_DISCONNECTED))) {
+		kref_init(&buf->ref);
 		list_add_tail(&buf->queue, &queue->irqqueue);
 	} else {
 		/* If the device is disconnected return the buffer to userspace
@@ -458,28 +459,66 @@ struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue)
 	return nextbuf;
 }
 
-struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
+/*
+ * uvc_queue_buffer_requeue: Requeue a buffer on our internal irqqueue
+ *
+ * Reuse a buffer through our internal queue without the need to 'prepare'.
+ * The buffer will be returned to userspace through the uvc_buffer_queue call if
+ * the device has been disconnected.
+ */
+static void uvc_queue_buffer_requeue(struct uvc_video_queue *queue,
 		struct uvc_buffer *buf)
 {
-	struct uvc_buffer *nextbuf;
-	unsigned long flags;
+	buf->error = 0;
+	buf->state = UVC_BUF_STATE_QUEUED;
+	buf->bytesused = 0;
+	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
+
+	uvc_buffer_queue(&buf->buf.vb2_buf);
+}
+
+static void uvc_queue_buffer_complete(struct kref *ref)
+{
+	struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref);
+	struct vb2_buffer *vb = &buf->buf.vb2_buf;
+	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
 
 	if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
-		buf->error = 0;
-		buf->state = UVC_BUF_STATE_QUEUED;
-		buf->bytesused = 0;
-		vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
-		return buf;
+		uvc_queue_buffer_requeue(queue, buf);
+		return;
 	}
 
+	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
+	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
+	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+/*
+ * Release a reference on the buffer. Complete the buffer when the last
+ * reference is released.
+ */
+void uvc_queue_buffer_release(struct uvc_buffer *buf)
+{
+	kref_put(&buf->ref, uvc_queue_buffer_complete);
+}
+
+/*
+ * Remove this buffer from the queue. Lifetime will persist while async actions
+ * are still running (if any), and uvc_queue_buffer_release will give the buffer
+ * back to VB2 when all users have completed.
+ */
+struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
+		struct uvc_buffer *buf)
+{
+	struct uvc_buffer *nextbuf;
+	unsigned long flags;
+
 	spin_lock_irqsave(&queue->irqlock, flags);
 	list_del(&buf->queue);
 	nextbuf = __uvc_queue_get_current_buffer(queue);
 	spin_unlock_irqrestore(&queue->irqlock, flags);
 
-	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
-	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
-	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
+	uvc_queue_buffer_release(buf);
 
 	return nextbuf;
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 16b8348f7ff0..7f884c60ae59 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -413,6 +413,9 @@ struct uvc_buffer {
 	unsigned int bytesused;
 
 	u32 pts;
+
+	/* Asynchronous buffer handling. */
+	struct kref ref;
 };
 
 #define UVC_QUEUE_DISCONNECTED		(1 << 0)
@@ -728,6 +731,7 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect);
 struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 					 struct uvc_buffer *buf);
 struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
+void uvc_queue_buffer_release(struct uvc_buffer *buf);
 int uvc_queue_mmap(struct uvc_video_queue *queue,
 		   struct vm_area_struct *vma);
 __poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 06/10] media: uvcvideo: Abstract streaming object lifetime
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
                   ` (4 preceding siblings ...)
  2018-11-09 17:05 ` [PATCH v6 05/10] media: uvcvideo: queue: Support asynchronous buffer handling Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-12-04 12:51   ` Laurent Pinchart
  2018-11-09 17:05 ` [PATCH v6 07/10] media: uvcvideo: Move decode processing to process context Kieran Bingham
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

The streaming object is a key part of handling the UVC device. Although
not critical, we are currently missing a call to destroy the mutex on
clean up paths, and we are due to extend the objects complexity in the
near future.

Facilitate easy management of a stream object by creating a pair of
functions to handle creating and destroying the allocation. The new
uvc_stream_delete() function also performs the missing mutex_destroy()
operation.

Previously a failed streaming object allocation would cause
uvc_parse_streaming() to return -EINVAL, which is inappropriate. If the
constructor failes, we will instead return -ENOMEM.

While we're here, fix the trivial spelling error in the function banner
of uvc_delete().

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 54 +++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 67bd58c6f397..afb44d1c9d04 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -396,6 +396,39 @@ static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
 }
 
 /* ------------------------------------------------------------------------
+ * Streaming Object Management
+ */
+
+static void uvc_stream_delete(struct uvc_streaming *stream)
+{
+	mutex_destroy(&stream->mutex);
+
+	usb_put_intf(stream->intf);
+
+	kfree(stream->format);
+	kfree(stream->header.bmaControls);
+	kfree(stream);
+}
+
+static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
+					    struct usb_interface *intf)
+{
+	struct uvc_streaming *stream;
+
+	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+	if (stream == NULL)
+		return NULL;
+
+	mutex_init(&stream->mutex);
+
+	stream->dev = dev;
+	stream->intf = usb_get_intf(intf);
+	stream->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
+
+	return stream;
+}
+
+/* ------------------------------------------------------------------------
  * Descriptors parsing
  */
 
@@ -687,17 +720,12 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 		return -EINVAL;
 	}
 
-	streaming = kzalloc(sizeof(*streaming), GFP_KERNEL);
+	streaming = uvc_stream_new(dev, intf);
 	if (streaming == NULL) {
 		usb_driver_release_interface(&uvc_driver.driver, intf);
-		return -EINVAL;
+		return -ENOMEM;
 	}
 
-	mutex_init(&streaming->mutex);
-	streaming->dev = dev;
-	streaming->intf = usb_get_intf(intf);
-	streaming->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
-
 	/* The Pico iMage webcam has its class-specific interface descriptors
 	 * after the endpoint descriptors.
 	 */
@@ -904,10 +932,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 
 error:
 	usb_driver_release_interface(&uvc_driver.driver, intf);
-	usb_put_intf(intf);
-	kfree(streaming->format);
-	kfree(streaming->header.bmaControls);
-	kfree(streaming);
+	uvc_stream_delete(streaming);
 	return ret;
 }
 
@@ -1815,7 +1840,7 @@ static int uvc_scan_device(struct uvc_device *dev)
  * is released.
  *
  * As this function is called after or during disconnect(), all URBs have
- * already been canceled by the USB core. There is no need to kill the
+ * already been cancelled by the USB core. There is no need to kill the
  * interrupt URB manually.
  */
 static void uvc_delete(struct kref *kref)
@@ -1853,10 +1878,7 @@ static void uvc_delete(struct kref *kref)
 		streaming = list_entry(p, struct uvc_streaming, list);
 		usb_driver_release_interface(&uvc_driver.driver,
 			streaming->intf);
-		usb_put_intf(streaming->intf);
-		kfree(streaming->format);
-		kfree(streaming->header.bmaControls);
-		kfree(streaming);
+		uvc_stream_delete(streaming);
 	}
 
 	kfree(dev);
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 07/10] media: uvcvideo: Move decode processing to process context
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
                   ` (5 preceding siblings ...)
  2018-11-09 17:05 ` [PATCH v6 06/10] media: uvcvideo: Abstract streaming object lifetime Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-12-04 12:55   ` Laurent Pinchart
  2018-11-09 17:05 ` [PATCH v6 08/10] media: uvcvideo: Split uvc_video_enable into two Kieran Bingham
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

Newer high definition cameras, and cameras with multiple lenses such as
the range of stereo-vision cameras now available have ever increasing
data rates.

The inclusion of a variable length packet header in URB packets mean
that we must memcpy the frame data out to our destination 'manually'.
This can result in data rates of up to 2 gigabits per second being
processed.

To improve efficiency, and maximise throughput, handle the URB decode
processing through a work queue to move it from interrupt context, and
allow multiple processors to work on URBs in parallel.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v2:
 - Lock full critical section of usb_submit_urb()

v3:
 - Fix race on submitting uvc_video_decode_data_work() to work queue.
 - Rename uvc_decode_op -> uvc_copy_op (Generic to encode/decode)
 - Rename decodes -> copy_operations
 - Don't queue work if there is no async task
 - obtain copy op structure directly in uvc_video_decode_data()
 - uvc_video_decode_data_work() -> uvc_video_copy_data_work()

v4:
 - Provide for_each_uvc_urb()
 - Simplify fix for shutdown race to flush queue before freeing URBs
 - Rebase to v4.16-rc4 (linux-media/master) adjusting for metadata
   conflicts.

v5:
 - Rebase to media/v4.20-2
 - Use GFP_KERNEL allocation in uvc_video_copy_data_work()
 - Fix function documentation for uvc_video_copy_data_work()
 - Add periods to the end of sentences
 - Rename 'decode' variable to 'op' in uvc_video_decode_data()
 - Move uvc_urb->async_operations initialisation to before use
 - Move async workqueue to match uvc_streaming lifetime instead of
   streamon/streamoff

v6:
 - Utilise the new streaming object lifetime functions to perform
   allocation and destruction of the async workqueue.

 drivers/media/usb/uvc/uvc_driver.c |  11 +++-
 drivers/media/usb/uvc/uvc_video.c  | 104 +++++++++++++++++++++++-------
 drivers/media/usb/uvc/uvcvideo.h   |  28 ++++++++-
 3 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index afb44d1c9d04..b62cbd800111 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -401,6 +401,9 @@ static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
 
 static void uvc_stream_delete(struct uvc_streaming *stream)
 {
+	if (stream->async_wq)
+		destroy_workqueue(stream->async_wq);
+
 	mutex_destroy(&stream->mutex);
 
 	usb_put_intf(stream->intf);
@@ -425,6 +428,14 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
 	stream->intf = usb_get_intf(intf);
 	stream->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
 
+	/* Allocate a stream specific work queue for asynchronous tasks. */
+	stream->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI,
+					   0);
+	if (!stream->async_wq) {
+		uvc_stream_delete(stream);
+		return NULL;
+	}
+
 	return stream;
 }
 
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 7a7779e1b466..e19bdf089cc4 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1094,21 +1094,54 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 	return data[0];
 }
 
-static void uvc_video_decode_data(struct uvc_streaming *stream,
+/*
+ * uvc_video_decode_data_work: Asynchronous memcpy processing
+ *
+ * Copy URB data to video buffers in process context, releasing buffer
+ * references and requeuing the URB when done.
+ */
+static void uvc_video_copy_data_work(struct work_struct *work)
+{
+	struct uvc_urb *uvc_urb = container_of(work, struct uvc_urb, work);
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < uvc_urb->async_operations; i++) {
+		struct uvc_copy_op *op = &uvc_urb->copy_operations[i];
+
+		memcpy(op->dst, op->src, op->len);
+
+		/* Release reference taken on this buffer. */
+		uvc_queue_buffer_release(op->buf);
+	}
+
+	ret = usb_submit_urb(uvc_urb->urb, GFP_KERNEL);
+	if (ret < 0)
+		uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
+			   ret);
+}
+
+static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
 		struct uvc_buffer *buf, const u8 *data, int len)
 {
-	unsigned int maxlen, nbytes;
-	void *mem;
+	unsigned int active_op = uvc_urb->async_operations;
+	struct uvc_copy_op *op = &uvc_urb->copy_operations[active_op];
+	unsigned int maxlen;
 
 	if (len <= 0)
 		return;
 
-	/* Copy the video data to the buffer. */
 	maxlen = buf->length - buf->bytesused;
-	mem = buf->mem + buf->bytesused;
-	nbytes = min((unsigned int)len, maxlen);
-	memcpy(mem, data, nbytes);
-	buf->bytesused += nbytes;
+
+	/* Take a buffer reference for async work. */
+	kref_get(&buf->ref);
+
+	op->buf = buf;
+	op->src = data;
+	op->dst = buf->mem + buf->bytesused;
+	op->len = min_t(unsigned int, len, maxlen);
+
+	buf->bytesused += op->len;
 
 	/* Complete the current frame if the buffer size was exceeded. */
 	if (len > maxlen) {
@@ -1116,6 +1149,8 @@ static void uvc_video_decode_data(struct uvc_streaming *stream,
 		buf->error = 1;
 		buf->state = UVC_BUF_STATE_READY;
 	}
+
+	uvc_urb->async_operations++;
 }
 
 static void uvc_video_decode_end(struct uvc_streaming *stream,
@@ -1324,7 +1359,7 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
 		uvc_video_decode_meta(stream, meta_buf, mem, ret);
 
 		/* Decode the payload data. */
-		uvc_video_decode_data(stream, buf, mem + ret,
+		uvc_video_decode_data(uvc_urb, buf, mem + ret,
 			urb->iso_frame_desc[i].actual_length - ret);
 
 		/* Process the header again. */
@@ -1384,9 +1419,9 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
 	 * sure buf is never dereferenced if NULL.
 	 */
 
-	/* Process video data. */
+	/* Prepare video data for processing. */
 	if (!stream->bulk.skip_payload && buf != NULL)
-		uvc_video_decode_data(stream, buf, mem, len);
+		uvc_video_decode_data(uvc_urb, buf, mem, len);
 
 	/* Detect the payload end by a URB smaller than the maximum size (or
 	 * a payload size equal to the maximum) and process the header again.
@@ -1472,7 +1507,7 @@ static void uvc_video_complete(struct urb *urb)
 		uvc_printk(KERN_WARNING, "Non-zero status (%d) in video "
 			"completion handler.\n", urb->status);
 		/* fall through */
-	case -ENOENT:		/* usb_kill_urb() called. */
+	case -ENOENT:		/* usb_poison_urb() called. */
 		if (stream->frozen)
 			return;
 		/* fall through */
@@ -1494,12 +1529,26 @@ static void uvc_video_complete(struct urb *urb)
 		spin_unlock_irqrestore(&qmeta->irqlock, flags);
 	}
 
+	/* Re-initialise the URB async work. */
+	uvc_urb->async_operations = 0;
+
+	/*
+	 * Process the URB headers, and optionally queue expensive memcpy tasks
+	 * to be deferred to a work queue.
+	 */
 	stream->decode(uvc_urb, buf, buf_meta);
 
-	if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
-		uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
-			ret);
+	/* If no async work is needed, resubmit the URB immediately. */
+	if (!uvc_urb->async_operations) {
+		ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
+		if (ret < 0)
+			uvc_printk(KERN_ERR,
+				   "Failed to resubmit video URB (%d).\n",
+				   ret);
+		return;
 	}
+
+	queue_work(stream->async_wq, &uvc_urb->work);
 }
 
 /*
@@ -1594,20 +1643,22 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
  */
 static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers)
 {
-	struct urb *urb;
-	unsigned int i;
+	struct uvc_urb *uvc_urb;
 
 	uvc_video_stats_stop(stream);
 
-	for (i = 0; i < UVC_URBS; ++i) {
-		struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+	/*
+	 * We must poison the URBs rather than kill them to ensure that even
+	 * after the completion handler returns, any asynchronous workqueues
+	 * will be prevented from resubmitting the URBs.
+	 */
+	for_each_uvc_urb(uvc_urb, stream)
+		usb_poison_urb(uvc_urb->urb);
 
-		urb = uvc_urb->urb;
-		if (urb == NULL)
-			continue;
+	flush_workqueue(stream->async_wq);
 
-		usb_kill_urb(urb);
-		usb_free_urb(urb);
+	for_each_uvc_urb(uvc_urb, stream) {
+		usb_free_urb(uvc_urb->urb);
 		uvc_urb->urb = NULL;
 	}
 
@@ -1932,6 +1983,7 @@ int uvc_video_init(struct uvc_streaming *stream)
 	struct uvc_streaming_control *probe = &stream->ctrl;
 	struct uvc_format *format = NULL;
 	struct uvc_frame *frame = NULL;
+	struct uvc_urb *uvc_urb;
 	unsigned int i;
 	int ret;
 
@@ -2017,6 +2069,10 @@ int uvc_video_init(struct uvc_streaming *stream)
 		}
 	}
 
+	/* Prepare asynchronous work items. */
+	for_each_uvc_urb(uvc_urb, stream)
+		INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
+
 	return 0;
 }
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 7f884c60ae59..94accfa3c009 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -494,12 +494,30 @@ struct uvc_stats_stream {
 #define UVC_METATADA_BUF_SIZE 1024
 
 /**
+ * struct uvc_copy_op: Context structure to schedule asynchronous memcpy
+ *
+ * @buf: active buf object for this operation
+ * @dst: copy destination address
+ * @src: copy source address
+ * @len: copy length
+ */
+struct uvc_copy_op {
+	struct uvc_buffer *buf;
+	void *dst;
+	const __u8 *src;
+	size_t len;
+};
+
+/**
  * struct uvc_urb - URB context management structure
  *
  * @urb: the URB described by this context structure
  * @stream: UVC streaming context
  * @buffer: memory storage for the URB
  * @dma: DMA coherent addressing for the urb_buffer
+ * @async_operations: counter to indicate the number of copy operations
+ * @copy_operations: work descriptors for asynchronous copy operations
+ * @work: work queue entry for asynchronous decode
  */
 struct uvc_urb {
 	struct urb *urb;
@@ -507,6 +525,10 @@ struct uvc_urb {
 
 	char *buffer;
 	dma_addr_t dma;
+
+	unsigned int async_operations;
+	struct uvc_copy_op copy_operations[UVC_MAX_PACKETS];
+	struct work_struct work;
 };
 
 struct uvc_streaming {
@@ -539,6 +561,7 @@ struct uvc_streaming {
 	/* Buffers queue. */
 	unsigned int frozen : 1;
 	struct uvc_video_queue queue;
+	struct workqueue_struct *async_wq;
 	void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
 		       struct uvc_buffer *meta_buf);
 
@@ -592,6 +615,11 @@ struct uvc_streaming {
 	} clock;
 };
 
+#define for_each_uvc_urb(uvc_urb, uvc_streaming) \
+	for ((uvc_urb) = &(uvc_streaming)->uvc_urb[0]; \
+	     (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \
+	     ++(uvc_urb))
+
 struct uvc_device_info {
 	u32	quirks;
 	u32	meta_format;
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 08/10] media: uvcvideo: Split uvc_video_enable into two
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
                   ` (6 preceding siblings ...)
  2018-11-09 17:05 ` [PATCH v6 07/10] media: uvcvideo: Move decode processing to process context Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-11-10 22:02   ` Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 09/10] media: uvcvideo: Rename uvc_{un,}init_video() Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 10/10] media: uvcvideo: Utilise for_each_uvc_urb iterator Kieran Bingham
  9 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

uvc_video_enable() is used both to start and stop the video stream
object, however the single function entry point shares no code between
the two operations.

Split the function into two distinct calls, and rename to
uvc_video_start_streaming() and uvc_video_stop_streaming() as
appropriate.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_queue.c |  4 +-
 drivers/media/usb/uvc/uvc_video.c | 56 +++++++++++++++-----------------
 drivers/media/usb/uvc/uvcvideo.h  |  3 +-
 3 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 2752e386f1e8..d09b0c882938 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -176,7 +176,7 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	queue->buf_used = 0;
 
-	ret = uvc_video_enable(stream, 1);
+	ret = uvc_video_start_streaming(stream);
 	if (ret == 0)
 		return 0;
 
@@ -194,7 +194,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
 	lockdep_assert_irqs_enabled();
 
 	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
-		uvc_video_enable(uvc_queue_to_stream(queue), 0);
+		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
 
 	spin_lock_irq(&queue->irqlock);
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index e19bdf089cc4..cd67506dc696 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -2076,38 +2076,10 @@ int uvc_video_init(struct uvc_streaming *stream)
 	return 0;
 }
 
-/*
- * Enable or disable the video stream.
- */
-int uvc_video_enable(struct uvc_streaming *stream, int enable)
+int uvc_video_start_streaming(struct uvc_streaming *stream)
 {
 	int ret;
 
-	if (!enable) {
-		uvc_uninit_video(stream, 1);
-		if (stream->intf->num_altsetting > 1) {
-			usb_set_interface(stream->dev->udev,
-					  stream->intfnum, 0);
-		} else {
-			/* UVC doesn't specify how to inform a bulk-based device
-			 * when the video stream is stopped. Windows sends a
-			 * CLEAR_FEATURE(HALT) request to the video streaming
-			 * bulk endpoint, mimic the same behaviour.
-			 */
-			unsigned int epnum = stream->header.bEndpointAddress
-					   & USB_ENDPOINT_NUMBER_MASK;
-			unsigned int dir = stream->header.bEndpointAddress
-					 & USB_ENDPOINT_DIR_MASK;
-			unsigned int pipe;
-
-			pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
-			usb_clear_halt(stream->dev->udev, pipe);
-		}
-
-		uvc_video_clock_cleanup(stream);
-		return 0;
-	}
-
 	ret = uvc_video_clock_init(stream);
 	if (ret < 0)
 		return ret;
@@ -2130,3 +2102,29 @@ int uvc_video_enable(struct uvc_streaming *stream, int enable)
 
 	return ret;
 }
+
+int uvc_video_stop_streaming(struct uvc_streaming *stream)
+{
+	uvc_uninit_video(stream, 1);
+	if (stream->intf->num_altsetting > 1) {
+		usb_set_interface(stream->dev->udev,
+				  stream->intfnum, 0);
+	} else {
+		/* UVC doesn't specify how to inform a bulk-based device
+		 * when the video stream is stopped. Windows sends a
+		 * CLEAR_FEATURE(HALT) request to the video streaming
+		 * bulk endpoint, mimic the same behaviour.
+		 */
+		unsigned int epnum = stream->header.bEndpointAddress
+				   & USB_ENDPOINT_NUMBER_MASK;
+		unsigned int dir = stream->header.bEndpointAddress
+				 & USB_ENDPOINT_DIR_MASK;
+		unsigned int pipe;
+
+		pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
+		usb_clear_halt(stream->dev->udev, pipe);
+	}
+
+	uvc_video_clock_cleanup(stream);
+	return 0;
+}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 94accfa3c009..b1b895c67122 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -786,7 +786,8 @@ void uvc_mc_cleanup_entity(struct uvc_entity *entity);
 int uvc_video_init(struct uvc_streaming *stream);
 int uvc_video_suspend(struct uvc_streaming *stream);
 int uvc_video_resume(struct uvc_streaming *stream, int reset);
-int uvc_video_enable(struct uvc_streaming *stream, int enable);
+int uvc_video_start_streaming(struct uvc_streaming *stream);
+int uvc_video_stop_streaming(struct uvc_streaming *stream);
 int uvc_probe_video(struct uvc_streaming *stream,
 		    struct uvc_streaming_control *probe);
 int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 09/10] media: uvcvideo: Rename uvc_{un,}init_video()
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
                   ` (7 preceding siblings ...)
  2018-11-09 17:05 ` [PATCH v6 08/10] media: uvcvideo: Split uvc_video_enable into two Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  2018-11-09 17:15   ` Kieran Bingham
  2018-11-09 17:05 ` [PATCH v6 10/10] media: uvcvideo: Utilise for_each_uvc_urb iterator Kieran Bingham
  9 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

We have both uvc_init_video() and uvc_video_init() calls which can be
quite confusing to determine the process for each. Now that video
uvc_video_enable() has been renamed to uvc_video_start_streaming(),
adapt these calls to suit the new flow.

Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
uvc_video_stop().

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---

v6:
 - Append _transfer to {_stop,_start}

 drivers/media/usb/uvc/uvc_video.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index cd67506dc696..a81012c65280 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1641,7 +1641,8 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
 /*
  * Uninitialize isochronous/bulk URBs and free transfer buffers.
  */
-static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers)
+static void uvc_video_stop_transfer(struct uvc_streaming *stream,
+				    int free_buffers)
 {
 	struct uvc_urb *uvc_urb;
 
@@ -1718,7 +1719,7 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 
 		urb = usb_alloc_urb(npackets, gfp_flags);
 		if (urb == NULL) {
-			uvc_uninit_video(stream, 1);
+			uvc_video_stop_transfer(stream, 1);
 			return -ENOMEM;
 		}
 
@@ -1786,7 +1787,7 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
 
 		urb = usb_alloc_urb(0, gfp_flags);
 		if (urb == NULL) {
-			uvc_uninit_video(stream, 1);
+			uvc_video_stop_transfer(stream, 1);
 			return -ENOMEM;
 		}
 
@@ -1806,7 +1807,8 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
 /*
  * Initialize isochronous/bulk URBs and allocate transfer buffers.
  */
-static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
+static int uvc_video_start_transfer(struct uvc_streaming *stream,
+				    gfp_t gfp_flags)
 {
 	struct usb_interface *intf = stream->intf;
 	struct usb_host_endpoint *ep;
@@ -1894,7 +1896,7 @@ static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
 		if (ret < 0) {
 			uvc_printk(KERN_ERR, "Failed to submit URB %u "
 					"(%d).\n", i, ret);
-			uvc_uninit_video(stream, 1);
+			uvc_video_stop_transfer(stream, 1);
 			return ret;
 		}
 	}
@@ -1925,7 +1927,7 @@ int uvc_video_suspend(struct uvc_streaming *stream)
 		return 0;
 
 	stream->frozen = 1;
-	uvc_uninit_video(stream, 0);
+	uvc_video_stop_transfer(stream, 0);
 	usb_set_interface(stream->dev->udev, stream->intfnum, 0);
 	return 0;
 }
@@ -1961,7 +1963,7 @@ int uvc_video_resume(struct uvc_streaming *stream, int reset)
 	if (ret < 0)
 		return ret;
 
-	return uvc_init_video(stream, GFP_NOIO);
+	return uvc_video_start_transfer(stream, GFP_NOIO);
 }
 
 /* ------------------------------------------------------------------------
@@ -2089,7 +2091,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
 	if (ret < 0)
 		goto error_commit;
 
-	ret = uvc_init_video(stream, GFP_KERNEL);
+	ret = uvc_video_start_transfer(stream, GFP_KERNEL);
 	if (ret < 0)
 		goto error_video;
 
@@ -2105,7 +2107,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
 
 int uvc_video_stop_streaming(struct uvc_streaming *stream)
 {
-	uvc_uninit_video(stream, 1);
+	uvc_video_stop_transfer(stream, 1);
 	if (stream->intf->num_altsetting > 1) {
 		usb_set_interface(stream->dev->udev,
 				  stream->intfnum, 0);
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [PATCH v6 10/10] media: uvcvideo: Utilise for_each_uvc_urb iterator
  2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
                   ` (8 preceding siblings ...)
  2018-11-09 17:05 ` [PATCH v6 09/10] media: uvcvideo: Rename uvc_{un,}init_video() Kieran Bingham
@ 2018-11-09 17:05 ` Kieran Bingham
  9 siblings, 0 replies; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:05 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Kieran Bingham, Tomasz Figa,
	Keiichi Watanabe

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>

---

v6:
 - rename lone 'j' iterator to 'i'
 - Remove conversion which doesn't make sense due to needing the
   iterator value.

 drivers/media/usb/uvc/uvc_video.c | 46 ++++++++++++++------------------
 drivers/media/usb/uvc/uvcvideo.h  |  3 ++-
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a81012c65280..8d6d2fd8c74c 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;
@@ -1701,7 +1700,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, i;
 	u16 psize;
 	u32 size;
 
@@ -1714,9 +1714,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_transfer(stream, 1);
@@ -1739,9 +1737,9 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 		urb->number_of_packets = npackets;
 		urb->transfer_buffer_length = size;
 
-		for (j = 0; j < npackets; ++j) {
-			urb->iso_frame_desc[j].offset = j * psize;
-			urb->iso_frame_desc[j].length = psize;
+		for (i = 0; i < npackets; ++i) {
+			urb->iso_frame_desc[i].offset = i * psize;
+			urb->iso_frame_desc[i].length = psize;
 		}
 
 		uvc_urb->urb = urb;
@@ -1758,7 +1756,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;
 
@@ -1782,9 +1781,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_transfer(stream, 1);
@@ -1812,6 +1809,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
 {
 	struct usb_interface *intf = stream->intf;
 	struct usb_host_endpoint *ep;
+	struct uvc_urb *uvc_urb;
 	unsigned int i;
 	int ret;
 
@@ -1889,13 +1887,11 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
 		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_transfer(stream, 1);
 			return ret;
 		}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b1b895c67122..24bf1a934c59 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -620,6 +620,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;
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* Re: [PATCH v6 09/10] media: uvcvideo: Rename uvc_{un,}init_video()
  2018-11-09 17:05 ` [PATCH v6 09/10] media: uvcvideo: Rename uvc_{un,}init_video() Kieran Bingham
@ 2018-11-09 17:15   ` Kieran Bingham
  2018-12-04 13:05     ` Laurent Pinchart
  0 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2018-11-09 17:15 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Tomasz Figa, Keiichi Watanabe

Hi Laurent,

I'm sorry - I didn't update the commit message on this one.

On 09/11/2018 17:05, Kieran Bingham wrote:
> We have both uvc_init_video() and uvc_video_init() calls which can be
> quite confusing to determine the process for each. Now that video
> uvc_video_enable() has been renamed to uvc_video_start_streaming(),
> adapt these calls to suit the new flow.
> 
> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
> uvc_video_stop().
> 

Could you s/uvc_video_start/uvc_video_start_transfer/ and
s/uvc_video_stop/uvc_video_stop_transfer/ when applying please?

(Assuming you get to apply and don't find something else :D)
--
KB



> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> 
> v6:
>  - Append _transfer to {_stop,_start}
> 
>  drivers/media/usb/uvc/uvc_video.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index cd67506dc696..a81012c65280 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1641,7 +1641,8 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
>  /*
>   * Uninitialize isochronous/bulk URBs and free transfer buffers.
>   */
> -static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers)
> +static void uvc_video_stop_transfer(struct uvc_streaming *stream,
> +				    int free_buffers)
>  {
>  	struct uvc_urb *uvc_urb;
>  
> @@ -1718,7 +1719,7 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
>  
>  		urb = usb_alloc_urb(npackets, gfp_flags);
>  		if (urb == NULL) {
> -			uvc_uninit_video(stream, 1);
> +			uvc_video_stop_transfer(stream, 1);
>  			return -ENOMEM;
>  		}
>  
> @@ -1786,7 +1787,7 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
>  
>  		urb = usb_alloc_urb(0, gfp_flags);
>  		if (urb == NULL) {
> -			uvc_uninit_video(stream, 1);
> +			uvc_video_stop_transfer(stream, 1);
>  			return -ENOMEM;
>  		}
>  
> @@ -1806,7 +1807,8 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
>  /*
>   * Initialize isochronous/bulk URBs and allocate transfer buffers.
>   */
> -static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
> +static int uvc_video_start_transfer(struct uvc_streaming *stream,
> +				    gfp_t gfp_flags)
>  {
>  	struct usb_interface *intf = stream->intf;
>  	struct usb_host_endpoint *ep;
> @@ -1894,7 +1896,7 @@ static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
>  		if (ret < 0) {
>  			uvc_printk(KERN_ERR, "Failed to submit URB %u "
>  					"(%d).\n", i, ret);
> -			uvc_uninit_video(stream, 1);
> +			uvc_video_stop_transfer(stream, 1);
>  			return ret;
>  		}
>  	}
> @@ -1925,7 +1927,7 @@ int uvc_video_suspend(struct uvc_streaming *stream)
>  		return 0;
>  
>  	stream->frozen = 1;
> -	uvc_uninit_video(stream, 0);
> +	uvc_video_stop_transfer(stream, 0);
>  	usb_set_interface(stream->dev->udev, stream->intfnum, 0);
>  	return 0;
>  }
> @@ -1961,7 +1963,7 @@ int uvc_video_resume(struct uvc_streaming *stream, int reset)
>  	if (ret < 0)
>  		return ret;
>  
> -	return uvc_init_video(stream, GFP_NOIO);
> +	return uvc_video_start_transfer(stream, GFP_NOIO);
>  }
>  
>  /* ------------------------------------------------------------------------
> @@ -2089,7 +2091,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
>  	if (ret < 0)
>  		goto error_commit;
>  
> -	ret = uvc_init_video(stream, GFP_KERNEL);
> +	ret = uvc_video_start_transfer(stream, GFP_KERNEL);
>  	if (ret < 0)
>  		goto error_video;
>  
> @@ -2105,7 +2107,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
>  
>  int uvc_video_stop_streaming(struct uvc_streaming *stream)
>  {
> -	uvc_uninit_video(stream, 1);
> +	uvc_video_stop_transfer(stream, 1);
>  	if (stream->intf->num_altsetting > 1) {
>  		usb_set_interface(stream->dev->udev,
>  				  stream->intfnum, 0);
> 

-- 
Regards
--
Kieran

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH v6 05/10] media: uvcvideo: queue: Support asynchronous buffer handling
  2018-11-09 17:05 ` [PATCH v6 05/10] media: uvcvideo: queue: Support asynchronous buffer handling Kieran Bingham
@ 2018-11-10 16:30   ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2018-11-10 16:30 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: linux-media, Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky,
	Randy Dunlap, Philipp Zabel, Ezequiel Garcia, Tomasz Figa,
	Keiichi Watanabe

Hi Kieran,

Thank you for the patch.

On Friday, 9 November 2018 19:05:28 EET Kieran Bingham wrote:
> The buffer queue interface currently operates sequentially, processing
> buffers after they have fully completed.
> 
> In preparation for supporting parallel tasks operating on the buffers,
> we will need to support buffers being processed on multiple CPUs.
> 
> Adapt the uvc_queue_next_buffer() such that a reference count tracks the
> active use of the buffer, returning the buffer to the VB2 stack at
> completion.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> v5:
>  - uvc_queue_requeue() -> uvc_queue_buffer_requeue()
>  - Fix comment
> ---
>  drivers/media/usb/uvc/uvc_queue.c | 61 ++++++++++++++++++++++++++------
>  drivers/media/usb/uvc/uvcvideo.h  |  4 ++-
>  2 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index fa7059aab49a..2752e386f1e8 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -142,6 +142,7 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
> 
>  	spin_lock_irqsave(&queue->irqlock, flags);
>  	if (likely(!(queue->flags & UVC_QUEUE_DISCONNECTED))) {
> +		kref_init(&buf->ref);
>  		list_add_tail(&buf->queue, &queue->irqqueue);
>  	} else {
>  		/* If the device is disconnected return the buffer to userspace
> @@ -458,28 +459,66 @@ struct uvc_buffer *uvc_queue_get_current_buffer(struct
> uvc_video_queue *queue) return nextbuf;
>  }
> 
> -struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
> +/*
> + * uvc_queue_buffer_requeue: Requeue a buffer on our internal irqqueue
> + *
> + * Reuse a buffer through our internal queue without the need to 'prepare'.
> + * The buffer will be returned to userspace through the uvc_buffer_queue
> call if + * the device has been disconnected.
> + */
> +static void uvc_queue_buffer_requeue(struct uvc_video_queue *queue,
>  		struct uvc_buffer *buf)
>  {
> -	struct uvc_buffer *nextbuf;
> -	unsigned long flags;
> +	buf->error = 0;
> +	buf->state = UVC_BUF_STATE_QUEUED;
> +	buf->bytesused = 0;
> +	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
> +
> +	uvc_buffer_queue(&buf->buf.vb2_buf);
> +}
> +
> +static void uvc_queue_buffer_complete(struct kref *ref)
> +{
> +	struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref);
> +	struct vb2_buffer *vb = &buf->buf.vb2_buf;
> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> 
>  	if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
> -		buf->error = 0;
> -		buf->state = UVC_BUF_STATE_QUEUED;
> -		buf->bytesused = 0;
> -		vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
> -		return buf;
> +		uvc_queue_buffer_requeue(queue, buf);
> +		return;
>  	}
> 
> +	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
> +	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
> +	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
> +/*
> + * Release a reference on the buffer. Complete the buffer when the last
> + * reference is released.
> + */
> +void uvc_queue_buffer_release(struct uvc_buffer *buf)
> +{
> +	kref_put(&buf->ref, uvc_queue_buffer_complete);
> +}
> +
> +/*
> + * Remove this buffer from the queue. Lifetime will persist while async
> actions + * are still running (if any), and uvc_queue_buffer_release will
> give the buffer + * back to VB2 when all users have completed.
> + */
> +struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
> +		struct uvc_buffer *buf)
> +{
> +	struct uvc_buffer *nextbuf;
> +	unsigned long flags;
> +
>  	spin_lock_irqsave(&queue->irqlock, flags);
>  	list_del(&buf->queue);
>  	nextbuf = __uvc_queue_get_current_buffer(queue);
>  	spin_unlock_irqrestore(&queue->irqlock, flags);
> 
> -	buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
> -	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
> -	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
> +	uvc_queue_buffer_release(buf);
> 
>  	return nextbuf;
>  }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 16b8348f7ff0..7f884c60ae59 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -413,6 +413,9 @@ struct uvc_buffer {
>  	unsigned int bytesused;
> 
>  	u32 pts;
> +
> +	/* Asynchronous buffer handling. */
> +	struct kref ref;
>  };
> 
>  #define UVC_QUEUE_DISCONNECTED		(1 << 0)
> @@ -728,6 +731,7 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int
> disconnect);
> struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
> struct uvc_buffer *buf);
>  struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue
> *queue);
> +void uvc_queue_buffer_release(struct uvc_buffer *buf);
>  int uvc_queue_mmap(struct uvc_video_queue *queue,
>  		   struct vm_area_struct *vma);
>  __poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,


-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH v6 08/10] media: uvcvideo: Split uvc_video_enable into two
  2018-11-09 17:05 ` [PATCH v6 08/10] media: uvcvideo: Split uvc_video_enable into two Kieran Bingham
@ 2018-11-10 22:02   ` Kieran Bingham
  2018-12-04 12:59     ` Laurent Pinchart
  0 siblings, 1 reply; 18+ messages in thread
From: Kieran Bingham @ 2018-11-10 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media
  Cc: Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky, Randy Dunlap,
	Philipp Zabel, Ezequiel Garcia, Tomasz Figa, Keiichi Watanabe

Hi Laurent,

I see that you made changes to this patch before accepting it last time.

I'm afraid I haven't made those changes here, so please just cherry-pick
your previous incarnation. There are no changes here from me between v5,
and v6.

Regards
--
Kieran


On 09/11/2018 17:05, Kieran Bingham wrote:
> uvc_video_enable() is used both to start and stop the video stream
> object, however the single function entry point shares no code between
> the two operations.
> 
> Split the function into two distinct calls, and rename to
> uvc_video_start_streaming() and uvc_video_stop_streaming() as
> appropriate.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_queue.c |  4 +-
>  drivers/media/usb/uvc/uvc_video.c | 56 +++++++++++++++-----------------
>  drivers/media/usb/uvc/uvcvideo.h  |  3 +-
>  3 files changed, 31 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 2752e386f1e8..d09b0c882938 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -176,7 +176,7 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	queue->buf_used = 0;
>  
> -	ret = uvc_video_enable(stream, 1);
> +	ret = uvc_video_start_streaming(stream);
>  	if (ret == 0)
>  		return 0;
>  
> @@ -194,7 +194,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
>  	lockdep_assert_irqs_enabled();
>  
>  	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> -		uvc_video_enable(uvc_queue_to_stream(queue), 0);
> +		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>  
>  	spin_lock_irq(&queue->irqlock);
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index e19bdf089cc4..cd67506dc696 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -2076,38 +2076,10 @@ int uvc_video_init(struct uvc_streaming *stream)
>  	return 0;
>  }
>  
> -/*
> - * Enable or disable the video stream.
> - */
> -int uvc_video_enable(struct uvc_streaming *stream, int enable)
> +int uvc_video_start_streaming(struct uvc_streaming *stream)
>  {
>  	int ret;
>  
> -	if (!enable) {
> -		uvc_uninit_video(stream, 1);
> -		if (stream->intf->num_altsetting > 1) {
> -			usb_set_interface(stream->dev->udev,
> -					  stream->intfnum, 0);
> -		} else {
> -			/* UVC doesn't specify how to inform a bulk-based device
> -			 * when the video stream is stopped. Windows sends a
> -			 * CLEAR_FEATURE(HALT) request to the video streaming
> -			 * bulk endpoint, mimic the same behaviour.
> -			 */
> -			unsigned int epnum = stream->header.bEndpointAddress
> -					   & USB_ENDPOINT_NUMBER_MASK;
> -			unsigned int dir = stream->header.bEndpointAddress
> -					 & USB_ENDPOINT_DIR_MASK;
> -			unsigned int pipe;
> -
> -			pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> -			usb_clear_halt(stream->dev->udev, pipe);
> -		}
> -
> -		uvc_video_clock_cleanup(stream);
> -		return 0;
> -	}
> -
>  	ret = uvc_video_clock_init(stream);
>  	if (ret < 0)
>  		return ret;
> @@ -2130,3 +2102,29 @@ int uvc_video_enable(struct uvc_streaming *stream, int enable)
>  
>  	return ret;
>  }
> +
> +int uvc_video_stop_streaming(struct uvc_streaming *stream)
> +{
> +	uvc_uninit_video(stream, 1);
> +	if (stream->intf->num_altsetting > 1) {
> +		usb_set_interface(stream->dev->udev,
> +				  stream->intfnum, 0);
> +	} else {
> +		/* UVC doesn't specify how to inform a bulk-based device
> +		 * when the video stream is stopped. Windows sends a
> +		 * CLEAR_FEATURE(HALT) request to the video streaming
> +		 * bulk endpoint, mimic the same behaviour.
> +		 */
> +		unsigned int epnum = stream->header.bEndpointAddress
> +				   & USB_ENDPOINT_NUMBER_MASK;
> +		unsigned int dir = stream->header.bEndpointAddress
> +				 & USB_ENDPOINT_DIR_MASK;
> +		unsigned int pipe;
> +
> +		pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> +		usb_clear_halt(stream->dev->udev, pipe);
> +	}
> +
> +	uvc_video_clock_cleanup(stream);
> +	return 0;
> +}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 94accfa3c009..b1b895c67122 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -786,7 +786,8 @@ void uvc_mc_cleanup_entity(struct uvc_entity *entity);
>  int uvc_video_init(struct uvc_streaming *stream);
>  int uvc_video_suspend(struct uvc_streaming *stream);
>  int uvc_video_resume(struct uvc_streaming *stream, int reset);
> -int uvc_video_enable(struct uvc_streaming *stream, int enable);
> +int uvc_video_start_streaming(struct uvc_streaming *stream);
> +int uvc_video_stop_streaming(struct uvc_streaming *stream);
>  int uvc_probe_video(struct uvc_streaming *stream,
>  		    struct uvc_streaming_control *probe);
>  int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> 

-- 
Regards
--
Kieran

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH v6 06/10] media: uvcvideo: Abstract streaming object lifetime
  2018-11-09 17:05 ` [PATCH v6 06/10] media: uvcvideo: Abstract streaming object lifetime Kieran Bingham
@ 2018-12-04 12:51   ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2018-12-04 12:51 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: linux-media, Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky,
	Randy Dunlap, Philipp Zabel, Ezequiel Garcia, Tomasz Figa,
	Keiichi Watanabe

Hi Kieran,

Thank you for the patch.

On Friday, 9 November 2018 19:05:29 EET Kieran Bingham wrote:
> The streaming object is a key part of handling the UVC device. Although
> not critical, we are currently missing a call to destroy the mutex on
> clean up paths, and we are due to extend the objects complexity in the
> near future.
> 
> Facilitate easy management of a stream object by creating a pair of
> functions to handle creating and destroying the allocation. The new
> uvc_stream_delete() function also performs the missing mutex_destroy()
> operation.
> 
> Previously a failed streaming object allocation would cause
> uvc_parse_streaming() to return -EINVAL, which is inappropriate. If the
> constructor failes, we will instead return -ENOMEM.
> 
> While we're here, fix the trivial spelling error in the function banner
> of uvc_delete().
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 54 +++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 67bd58c6f397..afb44d1c9d04
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -396,6 +396,39 @@ static struct uvc_streaming *uvc_stream_by_id(struct
> uvc_device *dev, int id) }
> 
>  /* ------------------------------------------------------------------------
> + * Streaming Object Management
> + */
> +
> +static void uvc_stream_delete(struct uvc_streaming *stream)
> +{
> +	mutex_destroy(&stream->mutex);
> +
> +	usb_put_intf(stream->intf);
> +
> +	kfree(stream->format);
> +	kfree(stream->header.bmaControls);
> +	kfree(stream);
> +}
> +
> +static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
> +					    struct usb_interface *intf)
> +{
> +	struct uvc_streaming *stream;
> +
> +	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> +	if (stream == NULL)
> +		return NULL;
> +
> +	mutex_init(&stream->mutex);
> +
> +	stream->dev = dev;
> +	stream->intf = usb_get_intf(intf);
> +	stream->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
> +
> +	return stream;
> +}
> +
> +/* ------------------------------------------------------------------------
> * Descriptors parsing
>   */
> 
> @@ -687,17 +720,12 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> return -EINVAL;
>  	}
> 
> -	streaming = kzalloc(sizeof(*streaming), GFP_KERNEL);
> +	streaming = uvc_stream_new(dev, intf);
>  	if (streaming == NULL) {
>  		usb_driver_release_interface(&uvc_driver.driver, intf);
> -		return -EINVAL;
> +		return -ENOMEM;
>  	}
> 
> -	mutex_init(&streaming->mutex);
> -	streaming->dev = dev;
> -	streaming->intf = usb_get_intf(intf);
> -	streaming->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
> -
>  	/* The Pico iMage webcam has its class-specific interface descriptors
>  	 * after the endpoint descriptors.
>  	 */
> @@ -904,10 +932,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> 
>  error:
>  	usb_driver_release_interface(&uvc_driver.driver, intf);
> -	usb_put_intf(intf);
> -	kfree(streaming->format);
> -	kfree(streaming->header.bmaControls);
> -	kfree(streaming);
> +	uvc_stream_delete(streaming);
>  	return ret;
>  }
> 
> @@ -1815,7 +1840,7 @@ static int uvc_scan_device(struct uvc_device *dev)
>   * is released.
>   *
>   * As this function is called after or during disconnect(), all URBs have
> - * already been canceled by the USB core. There is no need to kill the
> + * already been cancelled by the USB core. There is no need to kill the
>   * interrupt URB manually.
>   */
>  static void uvc_delete(struct kref *kref)
> @@ -1853,10 +1878,7 @@ static void uvc_delete(struct kref *kref)
>  		streaming = list_entry(p, struct uvc_streaming, list);
>  		usb_driver_release_interface(&uvc_driver.driver,
>  			streaming->intf);
> -		usb_put_intf(streaming->intf);
> -		kfree(streaming->format);
> -		kfree(streaming->header.bmaControls);
> -		kfree(streaming);
> +		uvc_stream_delete(streaming);
>  	}
> 
>  	kfree(dev);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH v6 07/10] media: uvcvideo: Move decode processing to process context
  2018-11-09 17:05 ` [PATCH v6 07/10] media: uvcvideo: Move decode processing to process context Kieran Bingham
@ 2018-12-04 12:55   ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2018-12-04 12:55 UTC (permalink / raw)
  To: Kieran Bingham
  Cc: linux-media, Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky,
	Randy Dunlap, Philipp Zabel, Ezequiel Garcia, Tomasz Figa,
	Keiichi Watanabe

Hi Kieran,

Thank you for the patch.

On Friday, 9 November 2018 19:05:30 EET Kieran Bingham wrote:
> Newer high definition cameras, and cameras with multiple lenses such as
> the range of stereo-vision cameras now available have ever increasing
> data rates.
> 
> The inclusion of a variable length packet header in URB packets mean
> that we must memcpy the frame data out to our destination 'manually'.
> This can result in data rates of up to 2 gigabits per second being
> processed.
> 
> To improve efficiency, and maximise throughput, handle the URB decode
> processing through a work queue to move it from interrupt context, and
> allow multiple processors to work on URBs in parallel.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 
> ---
> v2:
>  - Lock full critical section of usb_submit_urb()
> 
> v3:
>  - Fix race on submitting uvc_video_decode_data_work() to work queue.
>  - Rename uvc_decode_op -> uvc_copy_op (Generic to encode/decode)
>  - Rename decodes -> copy_operations
>  - Don't queue work if there is no async task
>  - obtain copy op structure directly in uvc_video_decode_data()
>  - uvc_video_decode_data_work() -> uvc_video_copy_data_work()
> 
> v4:
>  - Provide for_each_uvc_urb()
>  - Simplify fix for shutdown race to flush queue before freeing URBs
>  - Rebase to v4.16-rc4 (linux-media/master) adjusting for metadata
>    conflicts.
> 
> v5:
>  - Rebase to media/v4.20-2
>  - Use GFP_KERNEL allocation in uvc_video_copy_data_work()
>  - Fix function documentation for uvc_video_copy_data_work()
>  - Add periods to the end of sentences
>  - Rename 'decode' variable to 'op' in uvc_video_decode_data()
>  - Move uvc_urb->async_operations initialisation to before use
>  - Move async workqueue to match uvc_streaming lifetime instead of
>    streamon/streamoff
> 
> v6:
>  - Utilise the new streaming object lifetime functions to perform
>    allocation and destruction of the async workqueue.
> 
>  drivers/media/usb/uvc/uvc_driver.c |  11 +++-
>  drivers/media/usb/uvc/uvc_video.c  | 104 +++++++++++++++++++++++-------
>  drivers/media/usb/uvc/uvcvideo.h   |  28 ++++++++-
>  3 files changed, 119 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index afb44d1c9d04..b62cbd800111
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -401,6 +401,9 @@ static struct uvc_streaming *uvc_stream_by_id(struct
> uvc_device *dev, int id)
> 
>  static void uvc_stream_delete(struct uvc_streaming *stream)
>  {
> +	if (stream->async_wq)
> +		destroy_workqueue(stream->async_wq);
> +
>  	mutex_destroy(&stream->mutex);
> 
>  	usb_put_intf(stream->intf);
> @@ -425,6 +428,14 @@ static struct uvc_streaming *uvc_stream_new(struct
> uvc_device *dev, stream->intf = usb_get_intf(intf);
>  	stream->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
> 
> +	/* Allocate a stream specific work queue for asynchronous tasks. */
> +	stream->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI,
> +					   0);
> +	if (!stream->async_wq) {
> +		uvc_stream_delete(stream);
> +		return NULL;
> +	}
> +
>  	return stream;
>  }
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 7a7779e1b466..e19bdf089cc4 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1094,21 +1094,54 @@ static int uvc_video_decode_start(struct
> uvc_streaming *stream, return data[0];
>  }
> 
> -static void uvc_video_decode_data(struct uvc_streaming *stream,
> +/*
> + * uvc_video_decode_data_work: Asynchronous memcpy processing
> + *
> + * Copy URB data to video buffers in process context, releasing buffer
> + * references and requeuing the URB when done.
> + */
> +static void uvc_video_copy_data_work(struct work_struct *work)
> +{
> +	struct uvc_urb *uvc_urb = container_of(work, struct uvc_urb, work);
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < uvc_urb->async_operations; i++) {
> +		struct uvc_copy_op *op = &uvc_urb->copy_operations[i];
> +
> +		memcpy(op->dst, op->src, op->len);
> +
> +		/* Release reference taken on this buffer. */
> +		uvc_queue_buffer_release(op->buf);
> +	}
> +
> +	ret = usb_submit_urb(uvc_urb->urb, GFP_KERNEL);
> +	if (ret < 0)
> +		uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
> +			   ret);
> +}
> +
> +static void uvc_video_decode_data(struct uvc_urb *uvc_urb,
>  		struct uvc_buffer *buf, const u8 *data, int len)
>  {
> -	unsigned int maxlen, nbytes;
> -	void *mem;
> +	unsigned int active_op = uvc_urb->async_operations;
> +	struct uvc_copy_op *op = &uvc_urb->copy_operations[active_op];
> +	unsigned int maxlen;
> 
>  	if (len <= 0)
>  		return;
> 
> -	/* Copy the video data to the buffer. */
>  	maxlen = buf->length - buf->bytesused;
> -	mem = buf->mem + buf->bytesused;
> -	nbytes = min((unsigned int)len, maxlen);
> -	memcpy(mem, data, nbytes);
> -	buf->bytesused += nbytes;
> +
> +	/* Take a buffer reference for async work. */
> +	kref_get(&buf->ref);
> +
> +	op->buf = buf;
> +	op->src = data;
> +	op->dst = buf->mem + buf->bytesused;
> +	op->len = min_t(unsigned int, len, maxlen);
> +
> +	buf->bytesused += op->len;
> 
>  	/* Complete the current frame if the buffer size was exceeded. */
>  	if (len > maxlen) {
> @@ -1116,6 +1149,8 @@ static void uvc_video_decode_data(struct uvc_streaming
> *stream, buf->error = 1;
>  		buf->state = UVC_BUF_STATE_READY;
>  	}
> +
> +	uvc_urb->async_operations++;
>  }
> 
>  static void uvc_video_decode_end(struct uvc_streaming *stream,
> @@ -1324,7 +1359,7 @@ static void uvc_video_decode_isoc(struct uvc_urb
> *uvc_urb, uvc_video_decode_meta(stream, meta_buf, mem, ret);
> 
>  		/* Decode the payload data. */
> -		uvc_video_decode_data(stream, buf, mem + ret,
> +		uvc_video_decode_data(uvc_urb, buf, mem + ret,
>  			urb->iso_frame_desc[i].actual_length - ret);
> 
>  		/* Process the header again. */
> @@ -1384,9 +1419,9 @@ static void uvc_video_decode_bulk(struct uvc_urb
> *uvc_urb, * sure buf is never dereferenced if NULL.
>  	 */
> 
> -	/* Process video data. */
> +	/* Prepare video data for processing. */
>  	if (!stream->bulk.skip_payload && buf != NULL)
> -		uvc_video_decode_data(stream, buf, mem, len);
> +		uvc_video_decode_data(uvc_urb, buf, mem, len);
> 
>  	/* Detect the payload end by a URB smaller than the maximum size (or
>  	 * a payload size equal to the maximum) and process the header again.
> @@ -1472,7 +1507,7 @@ static void uvc_video_complete(struct urb *urb)
>  		uvc_printk(KERN_WARNING, "Non-zero status (%d) in video "
>  			"completion handler.\n", urb->status);
>  		/* fall through */
> -	case -ENOENT:		/* usb_kill_urb() called. */
> +	case -ENOENT:		/* usb_poison_urb() called. */
>  		if (stream->frozen)
>  			return;
>  		/* fall through */
> @@ -1494,12 +1529,26 @@ static void uvc_video_complete(struct urb *urb)
>  		spin_unlock_irqrestore(&qmeta->irqlock, flags);
>  	}
> 
> +	/* Re-initialise the URB async work. */
> +	uvc_urb->async_operations = 0;
> +
> +	/*
> +	 * Process the URB headers, and optionally queue expensive memcpy tasks
> +	 * to be deferred to a work queue.
> +	 */
>  	stream->decode(uvc_urb, buf, buf_meta);
> 
> -	if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
> -		uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
> -			ret);
> +	/* If no async work is needed, resubmit the URB immediately. */
> +	if (!uvc_urb->async_operations) {
> +		ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
> +		if (ret < 0)
> +			uvc_printk(KERN_ERR,
> +				   "Failed to resubmit video URB (%d).\n",
> +				   ret);
> +		return;
>  	}
> +
> +	queue_work(stream->async_wq, &uvc_urb->work);
>  }
> 
>  /*
> @@ -1594,20 +1643,22 @@ static int uvc_alloc_urb_buffers(struct
> uvc_streaming *stream, */
>  static void uvc_uninit_video(struct uvc_streaming *stream, int
> free_buffers) {
> -	struct urb *urb;
> -	unsigned int i;
> +	struct uvc_urb *uvc_urb;
> 
>  	uvc_video_stats_stop(stream);
> 
> -	for (i = 0; i < UVC_URBS; ++i) {
> -		struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
> +	/*
> +	 * We must poison the URBs rather than kill them to ensure that even
> +	 * after the completion handler returns, any asynchronous workqueues
> +	 * will be prevented from resubmitting the URBs.
> +	 */
> +	for_each_uvc_urb(uvc_urb, stream)
> +		usb_poison_urb(uvc_urb->urb);
> 
> -		urb = uvc_urb->urb;
> -		if (urb == NULL)
> -			continue;
> +	flush_workqueue(stream->async_wq);
> 
> -		usb_kill_urb(urb);
> -		usb_free_urb(urb);
> +	for_each_uvc_urb(uvc_urb, stream) {
> +		usb_free_urb(uvc_urb->urb);
>  		uvc_urb->urb = NULL;
>  	}
> 
> @@ -1932,6 +1983,7 @@ int uvc_video_init(struct uvc_streaming *stream)
>  	struct uvc_streaming_control *probe = &stream->ctrl;
>  	struct uvc_format *format = NULL;
>  	struct uvc_frame *frame = NULL;
> +	struct uvc_urb *uvc_urb;
>  	unsigned int i;
>  	int ret;
> 
> @@ -2017,6 +2069,10 @@ int uvc_video_init(struct uvc_streaming *stream)
>  		}
>  	}
> 
> +	/* Prepare asynchronous work items. */
> +	for_each_uvc_urb(uvc_urb, stream)
> +		INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 7f884c60ae59..94accfa3c009 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -494,12 +494,30 @@ struct uvc_stats_stream {
>  #define UVC_METATADA_BUF_SIZE 1024
> 
>  /**
> + * struct uvc_copy_op: Context structure to schedule asynchronous memcpy
> + *
> + * @buf: active buf object for this operation
> + * @dst: copy destination address
> + * @src: copy source address
> + * @len: copy length
> + */
> +struct uvc_copy_op {
> +	struct uvc_buffer *buf;
> +	void *dst;
> +	const __u8 *src;
> +	size_t len;
> +};
> +
> +/**
>   * struct uvc_urb - URB context management structure
>   *
>   * @urb: the URB described by this context structure
>   * @stream: UVC streaming context
>   * @buffer: memory storage for the URB
>   * @dma: DMA coherent addressing for the urb_buffer
> + * @async_operations: counter to indicate the number of copy operations
> + * @copy_operations: work descriptors for asynchronous copy operations
> + * @work: work queue entry for asynchronous decode
>   */
>  struct uvc_urb {
>  	struct urb *urb;
> @@ -507,6 +525,10 @@ struct uvc_urb {
> 
>  	char *buffer;
>  	dma_addr_t dma;
> +
> +	unsigned int async_operations;
> +	struct uvc_copy_op copy_operations[UVC_MAX_PACKETS];
> +	struct work_struct work;
>  };
> 
>  struct uvc_streaming {
> @@ -539,6 +561,7 @@ struct uvc_streaming {
>  	/* Buffers queue. */
>  	unsigned int frozen : 1;
>  	struct uvc_video_queue queue;
> +	struct workqueue_struct *async_wq;
>  	void (*decode)(struct uvc_urb *uvc_urb, struct uvc_buffer *buf,
>  		       struct uvc_buffer *meta_buf);
> 
> @@ -592,6 +615,11 @@ struct uvc_streaming {
>  	} clock;
>  };
> 
> +#define for_each_uvc_urb(uvc_urb, uvc_streaming) \
> +	for ((uvc_urb) = &(uvc_streaming)->uvc_urb[0]; \
> +	     (uvc_urb) < &(uvc_streaming)->uvc_urb[UVC_URBS]; \
> +	     ++(uvc_urb))
> +
>  struct uvc_device_info {
>  	u32	quirks;
>  	u32	meta_format;


-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH v6 08/10] media: uvcvideo: Split uvc_video_enable into two
  2018-11-10 22:02   ` Kieran Bingham
@ 2018-12-04 12:59     ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2018-12-04 12:59 UTC (permalink / raw)
  To: kieran.bingham
  Cc: linux-media, Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky,
	Randy Dunlap, Philipp Zabel, Ezequiel Garcia, Tomasz Figa,
	Keiichi Watanabe

Hi Kieran,

On Sunday, 11 November 2018 00:02:39 EET Kieran Bingham wrote:
> Hi Laurent,
> 
> I see that you made changes to this patch before accepting it last time.
> 
> I'm afraid I haven't made those changes here, so please just cherry-pick
> your previous incarnation. There are no changes here from me between v5,
> and v6.

OK, I will do so. The two changes I made are both in 
uvc_video_stop_streaming(), turning the function into a void function and 
avoiding an unnecessarily line wrap.

> On 09/11/2018 17:05, Kieran Bingham wrote:
> > uvc_video_enable() is used both to start and stop the video stream
> > object, however the single function entry point shares no code between
> > the two operations.
> > 
> > Split the function into two distinct calls, and rename to
> > uvc_video_start_streaming() and uvc_video_stop_streaming() as
> > appropriate.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> > 
> >  drivers/media/usb/uvc/uvc_queue.c |  4 +-
> >  drivers/media/usb/uvc/uvc_video.c | 56 +++++++++++++++-----------------
> >  drivers/media/usb/uvc/uvcvideo.h  |  3 +-
> >  3 files changed, 31 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_queue.c
> > b/drivers/media/usb/uvc/uvc_queue.c index 2752e386f1e8..d09b0c882938
> > 100644
> > --- a/drivers/media/usb/uvc/uvc_queue.c
> > +++ b/drivers/media/usb/uvc/uvc_queue.c
> > @@ -176,7 +176,7 @@ static int uvc_start_streaming(struct vb2_queue *vq,
> > unsigned int count)> 
> >  	queue->buf_used = 0;
> > 
> > -	ret = uvc_video_enable(stream, 1);
> > +	ret = uvc_video_start_streaming(stream);
> > 
> >  	if (ret == 0)
> >  	
> >  		return 0;
> > 
> > @@ -194,7 +194,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
> > 
> >  	lockdep_assert_irqs_enabled();
> >  	
> >  	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> > 
> > -		uvc_video_enable(uvc_queue_to_stream(queue), 0);
> > +		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> > 
> >  	spin_lock_irq(&queue->irqlock);
> >  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index e19bdf089cc4..cd67506dc696
> > 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -2076,38 +2076,10 @@ int uvc_video_init(struct uvc_streaming *stream)
> > 
> >  	return 0;
> >  
> >  }
> > 
> > -/*
> > - * Enable or disable the video stream.
> > - */
> > -int uvc_video_enable(struct uvc_streaming *stream, int enable)
> > +int uvc_video_start_streaming(struct uvc_streaming *stream)
> > 
> >  {
> >  
> >  	int ret;
> > 
> > -	if (!enable) {
> > -		uvc_uninit_video(stream, 1);
> > -		if (stream->intf->num_altsetting > 1) {
> > -			usb_set_interface(stream->dev->udev,
> > -					  stream->intfnum, 0);
> > -		} else {
> > -			/* UVC doesn't specify how to inform a bulk-based device
> > -			 * when the video stream is stopped. Windows sends a
> > -			 * CLEAR_FEATURE(HALT) request to the video streaming
> > -			 * bulk endpoint, mimic the same behaviour.
> > -			 */
> > -			unsigned int epnum = stream->header.bEndpointAddress
> > -					   & USB_ENDPOINT_NUMBER_MASK;
> > -			unsigned int dir = stream->header.bEndpointAddress
> > -					 & USB_ENDPOINT_DIR_MASK;
> > -			unsigned int pipe;
> > -
> > -			pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > -			usb_clear_halt(stream->dev->udev, pipe);
> > -		}
> > -
> > -		uvc_video_clock_cleanup(stream);
> > -		return 0;
> > -	}
> > -
> > 
> >  	ret = uvc_video_clock_init(stream);
> >  	if (ret < 0)
> >  	
> >  		return ret;
> > 
> > @@ -2130,3 +2102,29 @@ int uvc_video_enable(struct uvc_streaming *stream,
> > int enable)> 
> >  	return ret;
> >  
> >  }
> > 
> > +
> > +int uvc_video_stop_streaming(struct uvc_streaming *stream)
> > +{
> > +	uvc_uninit_video(stream, 1);
> > +	if (stream->intf->num_altsetting > 1) {
> > +		usb_set_interface(stream->dev->udev,
> > +				  stream->intfnum, 0);
> > +	} else {
> > +		/* UVC doesn't specify how to inform a bulk-based device
> > +		 * when the video stream is stopped. Windows sends a
> > +		 * CLEAR_FEATURE(HALT) request to the video streaming
> > +		 * bulk endpoint, mimic the same behaviour.
> > +		 */
> > +		unsigned int epnum = stream->header.bEndpointAddress
> > +				   & USB_ENDPOINT_NUMBER_MASK;
> > +		unsigned int dir = stream->header.bEndpointAddress
> > +				 & USB_ENDPOINT_DIR_MASK;
> > +		unsigned int pipe;
> > +
> > +		pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > +		usb_clear_halt(stream->dev->udev, pipe);
> > +	}
> > +
> > +	uvc_video_clock_cleanup(stream);
> > +	return 0;
> > +}
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > b/drivers/media/usb/uvc/uvcvideo.h index 94accfa3c009..b1b895c67122
> > 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -786,7 +786,8 @@ void uvc_mc_cleanup_entity(struct uvc_entity *entity);
> > 
> >  int uvc_video_init(struct uvc_streaming *stream);
> >  int uvc_video_suspend(struct uvc_streaming *stream);
> >  int uvc_video_resume(struct uvc_streaming *stream, int reset);
> > 
> > -int uvc_video_enable(struct uvc_streaming *stream, int enable);
> > +int uvc_video_start_streaming(struct uvc_streaming *stream);
> > +int uvc_video_stop_streaming(struct uvc_streaming *stream);
> > 
> >  int uvc_probe_video(struct uvc_streaming *stream,
> >  
> >  		    struct uvc_streaming_control *probe);
> >  
> >  int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,


-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 18+ messages in thread
* Re: [PATCH v6 09/10] media: uvcvideo: Rename uvc_{un,}init_video()
  2018-11-09 17:15   ` Kieran Bingham
@ 2018-12-04 13:05     ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2018-12-04 13:05 UTC (permalink / raw)
  To: kieran.bingham
  Cc: linux-media, Guennadi Liakhovetski, Olivier BRAUN, Troy Kisky,
	Randy Dunlap, Philipp Zabel, Ezequiel Garcia, Tomasz Figa,
	Keiichi Watanabe

Hi Kieran,

Thank you for the patch.

On Friday, 9 November 2018 19:15:42 EET Kieran Bingham wrote:
> Hi Laurent,
> 
> I'm sorry - I didn't update the commit message on this one.
> 
> On 09/11/2018 17:05, Kieran Bingham wrote:
> > We have both uvc_init_video() and uvc_video_init() calls which can be
> > quite confusing to determine the process for each. Now that video
> > uvc_video_enable() has been renamed to uvc_video_start_streaming(),
> > adapt these calls to suit the new flow.
> > 
> > Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to
> > uvc_video_stop().
> 
> Could you s/uvc_video_start/uvc_video_start_transfer/ and
> s/uvc_video_stop/uvc_video_stop_transfer/ when applying please?
> 
> (Assuming you get to apply and don't find something else :D)

Will do.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > ---
> > 
> > v6:
> >  - Append _transfer to {_stop,_start}
> >  
> >  drivers/media/usb/uvc/uvc_video.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index cd67506dc696..a81012c65280
> > 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1641,7 +1641,8 @@ static int uvc_alloc_urb_buffers(struct
> > uvc_streaming *stream,
> >  /*
> >   * Uninitialize isochronous/bulk URBs and free transfer buffers.
> >   */
> > -static void uvc_uninit_video(struct uvc_streaming *stream, int
> > free_buffers)
> > +static void uvc_video_stop_transfer(struct uvc_streaming *stream,
> > +				    int free_buffers)
> >  {
> >  	struct uvc_urb *uvc_urb;
> > 
> > @@ -1718,7 +1719,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
> > *stream,
> > 
> >  		urb = usb_alloc_urb(npackets, gfp_flags);
> >  		if (urb == NULL) {
> > -			uvc_uninit_video(stream, 1);
> > +			uvc_video_stop_transfer(stream, 1);
> >  			return -ENOMEM;
> >  		}
> > 
> > @@ -1786,7 +1787,7 @@ static int uvc_init_video_bulk(struct uvc_streaming
> > *stream,
> > 
> >  		urb = usb_alloc_urb(0, gfp_flags);
> >  		if (urb == NULL) {
> > -			uvc_uninit_video(stream, 1);
> > +			uvc_video_stop_transfer(stream, 1);
> >  			return -ENOMEM;
> >  		}
> > 
> > @@ -1806,7 +1807,8 @@ static int uvc_init_video_bulk(struct uvc_streaming
> > *stream,
> >  /*
> >   * Initialize isochronous/bulk URBs and allocate transfer buffers.
> >   */
> > -static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
> > +static int uvc_video_start_transfer(struct uvc_streaming *stream,
> > +				    gfp_t gfp_flags)
 >  {
> >  	struct usb_interface *intf = stream->intf;
> >  	struct usb_host_endpoint *ep;
> > @@ -1894,7 +1896,7 @@ static int uvc_init_video(struct uvc_streaming
> > *stream, gfp_t gfp_flags)
> > 
> >  		if (ret < 0) {
> >  			uvc_printk(KERN_ERR, "Failed to submit URB %u "
> >  					"(%d).\n", i, ret);
> > -			uvc_uninit_video(stream, 1);
> > +			uvc_video_stop_transfer(stream, 1);
> >  			return ret;
 >  		}
> >  	}
> > @@ -1925,7 +1927,7 @@ int uvc_video_suspend(struct uvc_streaming *stream)
> >  		return 0;
> >  	
> >  	stream->frozen = 1;
> > -	uvc_uninit_video(stream, 0);
> > +	uvc_video_stop_transfer(stream, 0);
> >  	usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> >  	return 0;
> >  }
> > @@ -1961,7 +1963,7 @@ int uvc_video_resume(struct uvc_streaming *stream,
> > int reset)
> >  	
> >  	if (ret < 0)
> >  		return ret;
> > 
> > -	return uvc_init_video(stream, GFP_NOIO);
> > +	return uvc_video_start_transfer(stream, GFP_NOIO);
> >  }
> >  
> >  /* ----------------------------------------------------------------------
> > @@ -2089,7 +2091,7 @@ int uvc_video_start_streaming(struct uvc_streaming
> > *stream)
> >  	
> >  	if (ret < 0)
> >  		goto error_commit;
> > 
> > -	ret = uvc_init_video(stream, GFP_KERNEL);
> > +	ret = uvc_video_start_transfer(stream, GFP_KERNEL);
> >  	if (ret < 0)
> >  		goto error_video;
> > 
> > @@ -2105,7 +2107,7 @@ int uvc_video_start_streaming(struct uvc_streaming
> > *stream)
> > 
> >  int uvc_video_stop_streaming(struct uvc_streaming *stream)
> >  {
> > -	uvc_uninit_video(stream, 1);
> > +	uvc_video_stop_transfer(stream, 1);
> > 
> >  	if (stream->intf->num_altsetting > 1) {
> >  		usb_set_interface(stream->dev->udev,
> >  				  stream->intfnum, 0);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-12-04 13:05 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-09 17:05 [PATCH v6 00/10] Asynchronous UVC Kieran Bingham
2018-11-09 17:05 ` [PATCH v6 01/10] media: uvcvideo: Refactor URB descriptors Kieran Bingham
2018-11-09 17:05 ` [PATCH v6 02/10] media: uvcvideo: Convert decode functions to use new context structure Kieran Bingham
2018-11-09 17:05 ` [PATCH v6 03/10] media: uvcvideo: Protect queue internals with helper Kieran Bingham
2018-11-09 17:05 ` [PATCH v6 04/10] media: uvcvideo: queue: Simplify spin-lock usage Kieran Bingham
2018-11-09 17:05 ` [PATCH v6 05/10] media: uvcvideo: queue: Support asynchronous buffer handling Kieran Bingham
2018-11-10 16:30   ` Laurent Pinchart
2018-11-09 17:05 ` [PATCH v6 06/10] media: uvcvideo: Abstract streaming object lifetime Kieran Bingham
2018-12-04 12:51   ` Laurent Pinchart
2018-11-09 17:05 ` [PATCH v6 07/10] media: uvcvideo: Move decode processing to process context Kieran Bingham
2018-12-04 12:55   ` Laurent Pinchart
2018-11-09 17:05 ` [PATCH v6 08/10] media: uvcvideo: Split uvc_video_enable into two Kieran Bingham
2018-11-10 22:02   ` Kieran Bingham
2018-12-04 12:59     ` Laurent Pinchart
2018-11-09 17:05 ` [PATCH v6 09/10] media: uvcvideo: Rename uvc_{un,}init_video() Kieran Bingham
2018-11-09 17:15   ` Kieran Bingham
2018-12-04 13:05     ` Laurent Pinchart
2018-11-09 17:05 ` [PATCH v6 10/10] media: uvcvideo: Utilise for_each_uvc_urb iterator Kieran Bingham

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.