Re: [PATCH v1 12/19] uvcvideo: Reorganize next buffer handling.

2013-11-10 Thread Laurent Pinchart 
Hi Pawel,

Thank you for the patch.

On Friday 30 August 2013 11:17:11 Pawel Osciak wrote:
 Move getting the first buffer from the current queue to a uvc_queue function
 and out of the USB completion handler.

Could you please add a sentence explaining why the patch is needed ?

 Signed-off-by: Pawel Osciak posc...@chromium.org
 ---
  drivers/media/usb/uvc/uvc_isight.c |  6 --
  drivers/media/usb/uvc/uvc_queue.c  | 14 ++
  drivers/media/usb/uvc/uvc_video.c  | 29 -
  drivers/media/usb/uvc/uvcvideo.h   |  7 +++
  4 files changed, 33 insertions(+), 23 deletions(-)
 
 diff --git a/drivers/media/usb/uvc/uvc_isight.c
 b/drivers/media/usb/uvc/uvc_isight.c index 8510e72..ab01286 100644
 --- a/drivers/media/usb/uvc/uvc_isight.c
 +++ b/drivers/media/usb/uvc/uvc_isight.c
 @@ -99,10 +99,12 @@ 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)
 +void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream)
 {
   int ret, i;
 + struct uvc_buffer *buf;

Could you please move this on top of the previous line ?

 +
 + buf = uvc_queue_get_first_buf(stream-queue);
 
   for (i = 0; i  urb-number_of_packets; ++i) {
   if (urb-iso_frame_desc[i].status  0) {
 diff --git a/drivers/media/usb/uvc/uvc_queue.c
 b/drivers/media/usb/uvc/uvc_queue.c index cd962be..55d2670 100644
 --- a/drivers/media/usb/uvc/uvc_queue.c
 +++ b/drivers/media/usb/uvc/uvc_queue.c
 @@ -352,6 +352,20 @@ void uvc_queue_cancel(struct uvc_video_queue *queue,
 int disconnect) spin_unlock_irqrestore(queue-irqlock, flags);
  }
 
 +struct uvc_buffer *uvc_queue_get_first_buf(struct uvc_video_queue *queue)
 +{
 + struct uvc_buffer *buf = NULL;
 + unsigned long flags;
 +
 + 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);
 +
 + return buf;
 +}
 +
  struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
   struct uvc_buffer *buf)
  {
 diff --git a/drivers/media/usb/uvc/uvc_video.c
 b/drivers/media/usb/uvc/uvc_video.c index b4ebccd..2f9a5fa 100644
 --- a/drivers/media/usb/uvc/uvc_video.c
 +++ b/drivers/media/usb/uvc/uvc_video.c
 @@ -1193,11 +1193,11 @@ static int uvc_video_encode_data(struct
 uvc_streaming *stream, /*
   * Completion handler for video URBs.
   */
 -static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
 *stream, -struct uvc_buffer *buf)
 +static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
 *stream) {
   u8 *mem;
   int ret, i;
 + struct uvc_buffer *buf = NULL;

Same here (and below).

   for (i = 0; i  urb-number_of_packets; ++i) {
   if (urb-iso_frame_desc[i].status  0) {
 @@ -1211,6 +1211,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
 struct uvc_streaming *stream,
 
   /* Decode the payload header. */
   mem = urb-transfer_buffer + urb-iso_frame_desc[i].offset;
 + buf = uvc_queue_get_first_buf(stream-queue);

Can't this call be moved outside of the loop ?

   do {
   ret = uvc_video_decode_start(stream, buf, mem,
   urb-iso_frame_desc[i].actual_length);
 @@ -1241,11 +1242,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, -struct uvc_buffer *buf)
 +static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming
 *stream) {
   u8 *mem;
   int len, ret;
 + struct uvc_buffer *buf;
 
   /*
* Ignore ZLPs if they're not part of a frame, otherwise process them
 @@ -1258,6 +1259,8 @@ static void uvc_video_decode_bulk(struct urb *urb,
 struct uvc_streaming *stream, len = urb-actual_length;
   stream-bulk.payload_size += len;
 
 + buf = uvc_queue_get_first_buf(stream-queue);
 +
   /* If the URB is the first of its payload, decode and save the
* header.
*/
 @@ -1309,12 +1312,13 @@ 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, -struct uvc_buffer *buf)
 +static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming
 *stream) {
   u8 *mem = urb-transfer_buffer;
   int len = stream-urb_size, ret;
 + struct uvc_buffer *buf;
 
 + buf = uvc_queue_get_first_buf(stream-queue);
   if (buf == NULL) {
   urb-transfer_buffer_length = 0;
   return;
 @@ -1355,9 +1359,6 @@ static void uvc_video_encode_bulk(struct urb *urb,
 struct

[PATCH v1 12/19] uvcvideo: Reorganize next buffer handling.

2013-08-29 Thread Pawel Osciak 
Move getting the first buffer from the current queue to a uvc_queue function
and out of the USB completion handler.

Signed-off-by: Pawel Osciak posc...@chromium.org
---
 drivers/media/usb/uvc/uvc_isight.c |  6 --
 drivers/media/usb/uvc/uvc_queue.c  | 14 ++
 drivers/media/usb/uvc/uvc_video.c  | 29 -
 drivers/media/usb/uvc/uvcvideo.h   |  7 +++
 4 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_isight.c 
b/drivers/media/usb/uvc/uvc_isight.c
index 8510e72..ab01286 100644
--- a/drivers/media/usb/uvc/uvc_isight.c
+++ b/drivers/media/usb/uvc/uvc_isight.c
@@ -99,10 +99,12 @@ 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)
+void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream)
 {
int ret, i;
+   struct uvc_buffer *buf;
+
+   buf = uvc_queue_get_first_buf(stream-queue);
 
for (i = 0; i  urb-number_of_packets; ++i) {
if (urb-iso_frame_desc[i].status  0) {
diff --git a/drivers/media/usb/uvc/uvc_queue.c 
b/drivers/media/usb/uvc/uvc_queue.c
index cd962be..55d2670 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -352,6 +352,20 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int 
disconnect)
spin_unlock_irqrestore(queue-irqlock, flags);
 }
 
+struct uvc_buffer *uvc_queue_get_first_buf(struct uvc_video_queue *queue)
+{
+   struct uvc_buffer *buf = NULL;
+   unsigned long flags;
+
+   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);
+
+   return buf;
+}
+
 struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf)
 {
diff --git a/drivers/media/usb/uvc/uvc_video.c 
b/drivers/media/usb/uvc/uvc_video.c
index b4ebccd..2f9a5fa 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1193,11 +1193,11 @@ static int uvc_video_encode_data(struct uvc_streaming 
*stream,
 /*
  * Completion handler for video URBs.
  */
-static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming 
*stream,
-   struct uvc_buffer *buf)
+static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming 
*stream)
 {
u8 *mem;
int ret, i;
+   struct uvc_buffer *buf = NULL;
 
for (i = 0; i  urb-number_of_packets; ++i) {
if (urb-iso_frame_desc[i].status  0) {
@@ -1211,6 +1211,7 @@ static void uvc_video_decode_isoc(struct urb *urb, struct 
uvc_streaming *stream,
 
/* Decode the payload header. */
mem = urb-transfer_buffer + urb-iso_frame_desc[i].offset;
+   buf = uvc_queue_get_first_buf(stream-queue);
do {
ret = uvc_video_decode_start(stream, buf, mem,
urb-iso_frame_desc[i].actual_length);
@@ -1241,11 +1242,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,
-   struct uvc_buffer *buf)
+static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming 
*stream)
 {
u8 *mem;
int len, ret;
+   struct uvc_buffer *buf;
 
/*
 * Ignore ZLPs if they're not part of a frame, otherwise process them
@@ -1258,6 +1259,8 @@ static void uvc_video_decode_bulk(struct urb *urb, struct 
uvc_streaming *stream,
len = urb-actual_length;
stream-bulk.payload_size += len;
 
+   buf = uvc_queue_get_first_buf(stream-queue);
+
/* If the URB is the first of its payload, decode and save the
 * header.
 */
@@ -1309,12 +1312,13 @@ 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,
-   struct uvc_buffer *buf)
+static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming 
*stream)
 {
u8 *mem = urb-transfer_buffer;
int len = stream-urb_size, ret;
+   struct uvc_buffer *buf;
 
+   buf = uvc_queue_get_first_buf(stream-queue);
if (buf == NULL) {
urb-transfer_buffer_length = 0;
return;
@@ -1355,9 +1359,6 @@ 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_video_queue *queue = stream-queue;
-   struct uvc_buffer *buf = NULL;
-   unsigned long flags;