[PATCH 0/9] em28xx: refactor the frame data processing code

[PATCH 0/9] em28xx: refactor the frame data processing code

[Frank Schäfer]

This patch series refactors the frame data processing code in em28xx-video.c to
- reduce code duplication
- fix a bug in vbi data processing
- prepare for adding em25xx/em276x frame data processing support
- clean up the code and make it easier to understand

It applies on top of my previous patch series 
"em28xx: add support fur USB bulk transfers"
"em28xx: use common urb data copying function for vbi and non-vbi data streams"

The changes have been tested with the following devices:
- "SilverCrest 1.3 MPix webcam" (progressive, non-vbi)
- "Hauppauge HVR-900 (65008/A1C0)" (interlaced, vbi enabled and disabled)


Frank Schäfer (9):
  em28xx: refactor get_next_buf() and use it for vbi data, too
  em28xx: use common function for video and vbi buffer completion
  em28xx: remove obsolete field 'frame' from struct em28xx_buffer
  em28xx: move field 'pos' from struct em28xx_dmaqueue to struct
    em28xx_buffer
  em28xx: refactor VBI data processing code in em28xx_urb_data_copy()
  em28xx: move caching of pointer to vmalloc memory in videobuf to
    struct em28xx_buffer
  em28xx: em28xx_urb_data_copy(): move duplicate code for
    capture_type=0 and capture_type=2 to a function
  em28xx: move the em2710/em2750/em28xx specific frame data processing
    code to a separate function
  em28xx: clean up and unify functions em28xx_copy_vbi()
    em28xx_copy_video()

 drivers/media/usb/em28xx/em28xx-video.c |  374 ++++++++++++-------------------
 drivers/media/usb/em28xx/em28xx.h       |   12 +-
 2 Dateien geändert, 154 Zeilen hinzugefügt(+), 232 Zeilen entfernt(-)

-- 
1.7.10.4

[PATCH 1/9] em28xx: refactor get_next_buf() and use it for vbi data, too

[Frank Schäfer]

get_next_buf() and vbi_get_next_buf() do exactly the same just with a
different dma queue and buffer. Saving the new buffer pointer back to the
device struct in em28xx_urb_data_copy() instead of doing this from inside
these functions makes it possible to get rid of one of them.

Also refactor the function parameters and return type:
- pass a pointer to struct em28xx as parameter (instead of obtaining the
  pointer from the dma queue pointer with the container_of macro) like we do
  it in all other functions
- instead of using a pointer-pointer, return the pointer to the new buffer
  as return value of the function

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   58 ++++++++-----------------------
 1 Datei geändert, 15 Zeilen hinzugefügt(+), 43 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 6282d48..6acdfea 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -358,57 +358,26 @@ static inline void print_err_status(struct em28xx *dev,
 }
 
 /*
- * video-buf generic routine to get the next available buffer
+ * get the next available buffer from dma queue
  */
-static inline void get_next_buf(struct em28xx_dmaqueue *dma_q,
-					  struct em28xx_buffer **buf)
+static inline struct em28xx_buffer *get_next_buf(struct em28xx *dev,
+						 struct em28xx_dmaqueue *dma_q)
 {
-	struct em28xx *dev = container_of(dma_q, struct em28xx, vidq);
+	struct em28xx_buffer *buf;
 	char *outp;
 
 	if (list_empty(&dma_q->active)) {
 		em28xx_usbdbg("No active queue to serve\n");
-		dev->usb_ctl.vid_buf = NULL;
-		*buf = NULL;
-		return;
-	}
-
-	/* Get the next buffer */
-	*buf = list_entry(dma_q->active.next, struct em28xx_buffer, vb.queue);
-	/* Cleans up buffer - Useful for testing for frame/URB loss */
-	outp = videobuf_to_vmalloc(&(*buf)->vb);
-	memset(outp, 0, (*buf)->vb.size);
-
-	dev->usb_ctl.vid_buf = *buf;
-
-	return;
-}
-
-/*
- * video-buf generic routine to get the next available VBI buffer
- */
-static inline void vbi_get_next_buf(struct em28xx_dmaqueue *dma_q,
-				    struct em28xx_buffer **buf)
-{
-	struct em28xx *dev = container_of(dma_q, struct em28xx, vbiq);
-	char *outp;
-
-	if (list_empty(&dma_q->active)) {
-		em28xx_usbdbg("No active queue to serve\n");
-		dev->usb_ctl.vbi_buf = NULL;
-		*buf = NULL;
-		return;
+		return NULL;
 	}
 
 	/* Get the next buffer */
-	*buf = list_entry(dma_q->active.next, struct em28xx_buffer, vb.queue);
+	buf = list_entry(dma_q->active.next, struct em28xx_buffer, vb.queue);
 	/* Cleans up buffer - Useful for testing for frame/URB loss */
-	outp = videobuf_to_vmalloc(&(*buf)->vb);
-	memset(outp, 0x00, (*buf)->vb.size);
-
-	dev->usb_ctl.vbi_buf = *buf;
+	outp = videobuf_to_vmalloc(&buf->vb);
+	memset(outp, 0, buf->vb.size);
 
-	return;
+	return buf;
 }
 
 /* Processes and copies the URB data content (video and VBI data) */
@@ -518,7 +487,8 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 						vbi_buffer_filled(dev,
 								  vbi_dma_q,
 								  vbi_buf);
-					vbi_get_next_buf(vbi_dma_q, &vbi_buf);
+					vbi_buf = get_next_buf(dev, vbi_dma_q);
+					dev->usb_ctl.vbi_buf = vbi_buf;
 					if (vbi_buf == NULL)
 						vbioutp = NULL;
 					else
@@ -529,7 +499,8 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 				if (dev->vbi_read == 0) {
 					vbi_dma_q->pos = 0;
 					if (vbi_buf != NULL)
-						vbi_buf->top_field = dev->top_field;
+						vbi_buf->top_field
+						  = dev->top_field;
 				}
 
 				dev->vbi_read += len;
@@ -553,7 +524,8 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 			if (dev->progressive || dev->top_field) {
 				if (buf != NULL)
 					buffer_filled(dev, dma_q, buf);
-				get_next_buf(dma_q, &buf);
+				buf = get_next_buf(dev, dma_q);
+				dev->usb_ctl.vid_buf = buf;
 				if (buf == NULL)
 					outp = NULL;
 				else
-- 
1.7.10.4

[PATCH 2/9] em28xx: use common function for video and vbi buffer completion

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   33 +++++--------------------------
 1 Datei geändert, 5 Zeilen hinzugefügt(+), 28 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 6acdfea..71a3181 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -154,36 +154,15 @@ static struct v4l2_queryctrl ac97_qctrl[] = {
    ------------------------------------------------------------------*/
 
 /*
- * Announces that a buffer were filled and request the next
+ * Finish the current buffer
  */
-static inline void buffer_filled(struct em28xx *dev,
-				  struct em28xx_dmaqueue *dma_q,
-				  struct em28xx_buffer *buf)
+static inline void finish_buffer(struct em28xx *dev,
+				 struct em28xx_buffer *buf)
 {
-	/* Advice that buffer was filled */
 	em28xx_usbdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
 	buf->vb.state = VIDEOBUF_DONE;
 	buf->vb.field_count++;
 	do_gettimeofday(&buf->vb.ts);
-
-	dev->usb_ctl.vid_buf = NULL;
-
-	list_del(&buf->vb.queue);
-	wake_up(&buf->vb.done);
-}
-
-static inline void vbi_buffer_filled(struct em28xx *dev,
-				     struct em28xx_dmaqueue *dma_q,
-				     struct em28xx_buffer *buf)
-{
-	/* Advice that buffer was filled */
-	em28xx_usbdbg("[%p/%d] wakeup\n", buf, buf->vb.i);
-	buf->vb.state = VIDEOBUF_DONE;
-	buf->vb.field_count++;
-	do_gettimeofday(&buf->vb.ts);
-
-	dev->usb_ctl.vbi_buf = NULL;
-
 	list_del(&buf->vb.queue);
 	wake_up(&buf->vb.done);
 }
@@ -484,9 +463,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 				if (dev->vbi_read == 0 && dev->top_field) {
 					/* Brand new frame */
 					if (vbi_buf != NULL)
-						vbi_buffer_filled(dev,
-								  vbi_dma_q,
-								  vbi_buf);
+						finish_buffer(dev, vbi_buf);
 					vbi_buf = get_next_buf(dev, vbi_dma_q);
 					dev->usb_ctl.vbi_buf = vbi_buf;
 					if (vbi_buf == NULL)
@@ -523,7 +500,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 			dev->capture_type = 2;
 			if (dev->progressive || dev->top_field) {
 				if (buf != NULL)
-					buffer_filled(dev, dma_q, buf);
+					finish_buffer(dev, buf);
 				buf = get_next_buf(dev, dma_q);
 				dev->usb_ctl.vid_buf = buf;
 				if (buf == NULL)
-- 
1.7.10.4

[PATCH 3/9] em28xx: remove obsolete field 'frame' from struct em28xx_buffer

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx.h |    1 -
 1 Datei geändert, 1 Zeile entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 304896d..b3d72a9 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -252,7 +252,6 @@ struct em28xx_buffer {
 	/* common v4l buffer stuff -- must be first */
 	struct videobuf_buffer vb;
 
-	struct list_head frame;
 	int top_field;
 };
 
-- 
1.7.10.4

[PATCH 4/9] em28xx: move field 'pos' from struct em28xx_dmaqueue to struct em28xx_buffer

[Frank Schäfer]

This field is used to keep track of the current memory position in the buffer,
not in the dma queue, so move it to right place.
This also allows us to get rid of the struct em28xx_dmaqueue pointer parameter
in functions em28xx_copy_video() and em28xx_copy_vbi().

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   53 ++++++++++++++-----------------
 drivers/media/usb/em28xx/em28xx.h       |    8 +++--
 2 Dateien geändert, 29 Zeilen hinzugefügt(+), 32 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 71a3181..ef81d62 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -171,7 +171,6 @@ static inline void finish_buffer(struct em28xx *dev,
  * Identify the buffer header type and properly handles
  */
 static void em28xx_copy_video(struct em28xx *dev,
-			      struct em28xx_dmaqueue  *dma_q,
 			      struct em28xx_buffer *buf,
 			      unsigned char *p,
 			      unsigned char *outp, unsigned long len)
@@ -180,8 +179,8 @@ static void em28xx_copy_video(struct em28xx *dev,
 	int  linesdone, currlinedone, offset, lencopy, remain;
 	int bytesperline = dev->width << 1;
 
-	if (dma_q->pos + len > buf->vb.size)
-		len = buf->vb.size - dma_q->pos;
+	if (buf->pos + len > buf->vb.size)
+		len = buf->vb.size - buf->pos;
 
 	startread = p;
 	remain = len;
@@ -191,8 +190,8 @@ static void em28xx_copy_video(struct em28xx *dev,
 	else /* interlaced mode, even nr. of lines */
 		fieldstart = outp + bytesperline;
 
-	linesdone = dma_q->pos / bytesperline;
-	currlinedone = dma_q->pos % bytesperline;
+	linesdone = buf->pos / bytesperline;
+	currlinedone = buf->pos % bytesperline;
 
 	if (dev->progressive)
 		offset = linesdone * bytesperline + currlinedone;
@@ -244,14 +243,13 @@ static void em28xx_copy_video(struct em28xx *dev,
 		remain -= lencopy;
 	}
 
-	dma_q->pos += len;
+	buf->pos += len;
 }
 
 static void em28xx_copy_vbi(struct em28xx *dev,
-			      struct em28xx_dmaqueue  *dma_q,
-			      struct em28xx_buffer *buf,
-			      unsigned char *p,
-			      unsigned char *outp, unsigned long len)
+			    struct em28xx_buffer *buf,
+			    unsigned char *p,
+			    unsigned char *outp, unsigned long len)
 {
 	void *startwrite, *startread;
 	int  offset;
@@ -263,10 +261,6 @@ static void em28xx_copy_vbi(struct em28xx *dev,
 	}
 	bytesperline = dev->vbi_width;
 
-	if (dma_q == NULL) {
-		em28xx_usbdbg("dma_q is null\n");
-		return;
-	}
 	if (buf == NULL) {
 		return;
 	}
@@ -279,13 +273,13 @@ static void em28xx_copy_vbi(struct em28xx *dev,
 		return;
 	}
 
-	if (dma_q->pos + len > buf->vb.size)
-		len = buf->vb.size - dma_q->pos;
+	if (buf->pos + len > buf->vb.size)
+		len = buf->vb.size - buf->pos;
 
 	startread = p;
 
-	startwrite = outp + dma_q->pos;
-	offset = dma_q->pos;
+	startwrite = outp + buf->pos;
+	offset = buf->pos;
 
 	/* Make sure the bottom field populates the second half of the frame */
 	if (buf->top_field == 0) {
@@ -294,7 +288,7 @@ static void em28xx_copy_vbi(struct em28xx *dev,
 	}
 
 	memcpy(startwrite, startread, len);
-	dma_q->pos += len;
+	buf->pos += len;
 }
 
 static inline void print_err_status(struct em28xx *dev,
@@ -355,6 +349,7 @@ static inline struct em28xx_buffer *get_next_buf(struct em28xx *dev,
 	/* Cleans up buffer - Useful for testing for frame/URB loss */
 	outp = videobuf_to_vmalloc(&buf->vb);
 	memset(outp, 0, buf->vb.size);
+	buf->pos = 0;
 
 	return buf;
 }
@@ -474,22 +469,22 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 				}
 
 				if (dev->vbi_read == 0) {
-					vbi_dma_q->pos = 0;
-					if (vbi_buf != NULL)
+					if (vbi_buf != NULL) {
 						vbi_buf->top_field
 						  = dev->top_field;
+						vbi_buf->pos = 0;
+					}
 				}
 
 				dev->vbi_read += len;
-				em28xx_copy_vbi(dev, vbi_dma_q, vbi_buf, p,
-						vbioutp, len);
+				em28xx_copy_vbi(dev, vbi_buf, p, vbioutp, len);
 			} else {
 				/* Some of this frame is VBI data and some is
 				   video data */
 				int vbi_data_len = vbi_size - dev->vbi_read;
 				dev->vbi_read += vbi_data_len;
-				em28xx_copy_vbi(dev, vbi_dma_q, vbi_buf, p,
-						vbioutp, vbi_data_len);
+				em28xx_copy_vbi(dev, vbi_buf, p, vbioutp,
+						vbi_data_len);
 				dev->capture_type = 1;
 				p += vbi_data_len;
 				len -= vbi_data_len;
@@ -508,14 +503,14 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 				else
 					outp = videobuf_to_vmalloc(&buf->vb);
 			}
-			if (buf != NULL)
+			if (buf != NULL) {
 				buf->top_field = dev->top_field;
-
-			dma_q->pos = 0;
+				buf->pos = 0;
+			}
 		}
 
 		if (buf != NULL && dev->capture_type == 2 && len > 0)
-			em28xx_copy_video(dev, dma_q, buf, p, outp, len);
+			em28xx_copy_video(dev, buf, p, outp, len);
 	}
 	return rc;
 }
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index b3d72a9..7507aa6 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -253,15 +253,17 @@ struct em28xx_buffer {
 	struct videobuf_buffer vb;
 
 	int top_field;
+
+	/* counter to control buffer fill */
+	unsigned int pos;
+	/* NOTE; in interlaced mode, this value is reset to zero at
+	 * the start of each new field (not frame !)		   */
 };
 
 struct em28xx_dmaqueue {
 	struct list_head       active;
 
 	wait_queue_head_t          wq;
-
-	/* Counters to control buffer fill */
-	int                        pos;
 };
 
 /* inputs */
-- 
1.7.10.4

[PATCH 5/9] em28xx: refactor VBI data processing code in em28xx_urb_data_copy()

[Frank Schäfer]

When a new frame header is detected in em28xx_urb_data_copy() and the data
packet contains both, VBI data and video data, the prevoius VBI buffer doesn't
get finished and is overwritten with the new VBI data.
This bug is not triggered with isochronous USB transfers, because the data
packetes are much smaller than the VBI data size.
But when using USB bulk transfers, the whole data of an URB is treated as
single packet, which is usually much larger then the VBI data size.

Refactor the VBI data processing code to fix this bug, but also to simplify the
code and make it similar to the video data processing code part (which allows
further code abstraction/unification in the future).

The changes have been tested with device "Hauppauge HVR-900".

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   77 +++++++++++++++----------------
 1 Datei geändert, 36 Zeilen hinzugefügt(+), 41 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index ef81d62..70bc562 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -418,8 +418,9 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 		}
 
 		/* capture type 0 = vbi start
-		   capture type 1 = video start
-		   capture type 2 = video in progress */
+		   capture type 1 = vbi in progress
+		   capture type 2 = video start
+		   capture type 3 = video in progress */
 		len = actual_length;
 		if (len >= 4) {
 			/* NOTE: headers are always 4 bytes and
@@ -438,7 +439,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 				len -= 4;
 			} else if (p[0] == 0x22 && p[1] == 0x5a) {
 				/* start video */
-				dev->capture_type = 1;
+				dev->capture_type = 2;
 				dev->top_field = !(p[2] & 1);
 				p += 4;
 				len -= 4;
@@ -448,51 +449,45 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 		 * have no continuation header */
 
 		if (dev->capture_type == 0) {
+			dev->capture_type = 1;
+			if (dev->top_field) { /* Brand new frame */
+				if (vbi_buf != NULL)
+					finish_buffer(dev, vbi_buf);
+				vbi_buf = get_next_buf(dev, vbi_dma_q);
+				dev->usb_ctl.vbi_buf = vbi_buf;
+				if (vbi_buf == NULL)
+					vbioutp = NULL;
+				else
+					vbioutp =
+					  videobuf_to_vmalloc(&vbi_buf->vb);
+			}
+			if (vbi_buf != NULL) {
+				vbi_buf->top_field = dev->top_field;
+				vbi_buf->pos = 0;
+			}
+		}
+
+		if (dev->capture_type == 1) {
 			int vbi_size = dev->vbi_width * dev->vbi_height;
-			if (dev->vbi_read >= vbi_size) {
-				/* We've already read all the VBI data, so
-				   treat the rest as video */
-				em28xx_usbdbg("dev->vbi_read > vbi_size\n");
-			} else if ((dev->vbi_read + len) < vbi_size) {
-				/* This entire frame is VBI data */
-				if (dev->vbi_read == 0 && dev->top_field) {
-					/* Brand new frame */
-					if (vbi_buf != NULL)
-						finish_buffer(dev, vbi_buf);
-					vbi_buf = get_next_buf(dev, vbi_dma_q);
-					dev->usb_ctl.vbi_buf = vbi_buf;
-					if (vbi_buf == NULL)
-						vbioutp = NULL;
-					else
-						vbioutp = videobuf_to_vmalloc(
-							&vbi_buf->vb);
-				}
-
-				if (dev->vbi_read == 0) {
-					if (vbi_buf != NULL) {
-						vbi_buf->top_field
-						  = dev->top_field;
-						vbi_buf->pos = 0;
-					}
-				}
-
-				dev->vbi_read += len;
-				em28xx_copy_vbi(dev, vbi_buf, p, vbioutp, len);
-			} else {
-				/* Some of this frame is VBI data and some is
-				   video data */
-				int vbi_data_len = vbi_size - dev->vbi_read;
-				dev->vbi_read += vbi_data_len;
+			int vbi_data_len = ((dev->vbi_read + len) > vbi_size) ?
+					   (vbi_size - dev->vbi_read) : len;
+
+			/* Copy VBI data */
+			if (vbi_buf != NULL)
 				em28xx_copy_vbi(dev, vbi_buf, p, vbioutp,
 						vbi_data_len);
-				dev->capture_type = 1;
+			dev->vbi_read += vbi_data_len;
+
+			if (vbi_data_len < len) {
+				/* Continue with copying video data */
+				dev->capture_type = 2;
 				p += vbi_data_len;
 				len -= vbi_data_len;
 			}
 		}
 
-		if (dev->capture_type == 1) {
-			dev->capture_type = 2;
+		if (dev->capture_type == 2) {
+			dev->capture_type = 3;
 			if (dev->progressive || dev->top_field) {
 				if (buf != NULL)
 					finish_buffer(dev, buf);
@@ -509,7 +504,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 			}
 		}
 
-		if (buf != NULL && dev->capture_type == 2 && len > 0)
+		if (buf != NULL && dev->capture_type == 3 && len > 0)
 			em28xx_copy_video(dev, buf, p, outp, len);
 	}
 	return rc;
-- 
1.7.10.4

[PATCH 6/9] em28xx: move caching of pointer to vmalloc memory in videobuf to struct em28xx_buffer

[Frank Schäfer]

In the current code em28xx_urb_data_copy() caches the pointer to the vmalloc
memory in videobuf locally.
The alternative would be to call videobuf_to_vmalloc() for each processed USB
data packet (isoc USB transfers => 64 times per URB) in the em28xx_copy_*()
functions.

With the next commits, the data processing code will be split into functions
for serveral reasons:
- em28xx_urb_data_copy() is generally way to long, making it less readable
- there is code duplication between VBI and video data processing
- support for em25xx data processing (uses a different header and frame
  end signaling mechanism) will be added

This would require extensive usage of pointer-pointers, which usually makes the
code less readable and prone to bugs.

The better solution is to cache the pointer in struct em28xx_buffer.
This also improves consistency, because we already track the buffer fill count there.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   29 +++++++++--------------------
 drivers/media/usb/em28xx/em28xx.h       |    3 +++
 2 Dateien geändert, 12 Zeilen hinzugefügt(+), 20 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 70bc562..60df756 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -173,11 +173,12 @@ static inline void finish_buffer(struct em28xx *dev,
 static void em28xx_copy_video(struct em28xx *dev,
 			      struct em28xx_buffer *buf,
 			      unsigned char *p,
-			      unsigned char *outp, unsigned long len)
+			      unsigned long len)
 {
 	void *fieldstart, *startwrite, *startread;
 	int  linesdone, currlinedone, offset, lencopy, remain;
 	int bytesperline = dev->width << 1;
+	unsigned char *outp = buf->vb_buf;
 
 	if (buf->pos + len > buf->vb.size)
 		len = buf->vb.size - buf->pos;
@@ -249,11 +250,12 @@ static void em28xx_copy_video(struct em28xx *dev,
 static void em28xx_copy_vbi(struct em28xx *dev,
 			    struct em28xx_buffer *buf,
 			    unsigned char *p,
-			    unsigned char *outp, unsigned long len)
+			    unsigned long len)
 {
 	void *startwrite, *startread;
 	int  offset;
 	int bytesperline;
+	unsigned char *outp;
 
 	if (dev == NULL) {
 		em28xx_usbdbg("dev is null\n");
@@ -268,6 +270,7 @@ static void em28xx_copy_vbi(struct em28xx *dev,
 		em28xx_usbdbg("p is null\n");
 		return;
 	}
+	outp = buf->vb_buf;
 	if (outp == NULL) {
 		em28xx_usbdbg("outp is null\n");
 		return;
@@ -350,6 +353,7 @@ static inline struct em28xx_buffer *get_next_buf(struct em28xx *dev,
 	outp = videobuf_to_vmalloc(&buf->vb);
 	memset(outp, 0, buf->vb.size);
 	buf->pos = 0;
+	buf->vb_buf = outp;
 
 	return buf;
 }
@@ -362,7 +366,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 	struct em28xx_dmaqueue  *vbi_dma_q = &dev->vbiq;
 	int xfer_bulk, num_packets, i, rc = 1;
 	unsigned int actual_length, len = 0;
-	unsigned char *p, *outp = NULL, *vbioutp = NULL;
+	unsigned char *p;
 
 	if (!dev)
 		return 0;
@@ -376,12 +380,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 	xfer_bulk = usb_pipebulk(urb->pipe);
 
 	buf = dev->usb_ctl.vid_buf;
-	if (buf != NULL)
-		outp = videobuf_to_vmalloc(&buf->vb);
-
 	vbi_buf = dev->usb_ctl.vbi_buf;
-	if (vbi_buf != NULL)
-		vbioutp = videobuf_to_vmalloc(&vbi_buf->vb);
 
 	if (xfer_bulk) /* bulk */
 		num_packets = 1;
@@ -455,11 +454,6 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 					finish_buffer(dev, vbi_buf);
 				vbi_buf = get_next_buf(dev, vbi_dma_q);
 				dev->usb_ctl.vbi_buf = vbi_buf;
-				if (vbi_buf == NULL)
-					vbioutp = NULL;
-				else
-					vbioutp =
-					  videobuf_to_vmalloc(&vbi_buf->vb);
 			}
 			if (vbi_buf != NULL) {
 				vbi_buf->top_field = dev->top_field;
@@ -474,8 +468,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 
 			/* Copy VBI data */
 			if (vbi_buf != NULL)
-				em28xx_copy_vbi(dev, vbi_buf, p, vbioutp,
-						vbi_data_len);
+				em28xx_copy_vbi(dev, vbi_buf, p, vbi_data_len);
 			dev->vbi_read += vbi_data_len;
 
 			if (vbi_data_len < len) {
@@ -493,10 +486,6 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 					finish_buffer(dev, buf);
 				buf = get_next_buf(dev, dma_q);
 				dev->usb_ctl.vid_buf = buf;
-				if (buf == NULL)
-					outp = NULL;
-				else
-					outp = videobuf_to_vmalloc(&buf->vb);
 			}
 			if (buf != NULL) {
 				buf->top_field = dev->top_field;
@@ -505,7 +494,7 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 		}
 
 		if (buf != NULL && dev->capture_type == 3 && len > 0)
-			em28xx_copy_video(dev, buf, p, outp, len);
+			em28xx_copy_video(dev, buf, p, len);
 	}
 	return rc;
 }
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 7507aa6..062841e 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -258,6 +258,9 @@ struct em28xx_buffer {
 	unsigned int pos;
 	/* NOTE; in interlaced mode, this value is reset to zero at
 	 * the start of each new field (not frame !)		   */
+
+	/* pointer to vmalloc memory address in vb */
+	char *vb_buf;
 };
 
 struct em28xx_dmaqueue {
-- 
1.7.10.4

[PATCH 7/9] em28xx: em28xx_urb_data_copy(): move duplicate code for capture_type=0 and capture_type=2 to a function

[Frank Schäfer]

Reduce code duplication by moving the duplicate code for dev->capture_type=0
(vbi start) and dev->capture_type=2 (video start) to a function.
The same function will also be called by the (not yet existing) em25xx frame
data processing code.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   45 +++++++++++++++++--------------
 1 Datei geändert, 25 Zeilen hinzugefügt(+), 20 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 60df756..61c7321 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -358,6 +358,27 @@ static inline struct em28xx_buffer *get_next_buf(struct em28xx *dev,
 	return buf;
 }
 
+/*
+ * Finish the current buffer if completed and prepare for the next field
+ */
+static struct em28xx_buffer *
+finish_field_prepare_next(struct em28xx *dev,
+			  struct em28xx_buffer *buf,
+			  struct em28xx_dmaqueue *dma_q)
+{
+	if (dev->progressive || dev->top_field) { /* Brand new frame */
+		if (buf != NULL)
+			finish_buffer(dev, buf);
+		buf = get_next_buf(dev, dma_q);
+	}
+	if (buf != NULL) {
+		buf->top_field = dev->top_field;
+		buf->pos = 0;
+	}
+
+	return buf;
+}
+
 /* Processes and copies the URB data content (video and VBI data) */
 static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 {
@@ -448,17 +469,9 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 		 * have no continuation header */
 
 		if (dev->capture_type == 0) {
+			vbi_buf = finish_field_prepare_next(dev, vbi_buf, vbi_dma_q);
+			dev->usb_ctl.vbi_buf = vbi_buf;
 			dev->capture_type = 1;
-			if (dev->top_field) { /* Brand new frame */
-				if (vbi_buf != NULL)
-					finish_buffer(dev, vbi_buf);
-				vbi_buf = get_next_buf(dev, vbi_dma_q);
-				dev->usb_ctl.vbi_buf = vbi_buf;
-			}
-			if (vbi_buf != NULL) {
-				vbi_buf->top_field = dev->top_field;
-				vbi_buf->pos = 0;
-			}
 		}
 
 		if (dev->capture_type == 1) {
@@ -480,17 +493,9 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 		}
 
 		if (dev->capture_type == 2) {
+			buf = finish_field_prepare_next(dev, buf, dma_q);
+			dev->usb_ctl.vid_buf = buf;
 			dev->capture_type = 3;
-			if (dev->progressive || dev->top_field) {
-				if (buf != NULL)
-					finish_buffer(dev, buf);
-				buf = get_next_buf(dev, dma_q);
-				dev->usb_ctl.vid_buf = buf;
-			}
-			if (buf != NULL) {
-				buf->top_field = dev->top_field;
-				buf->pos = 0;
-			}
 		}
 
 		if (buf != NULL && dev->capture_type == 3 && len > 0)
-- 
1.7.10.4

[PATCH 8/9] em28xx: move the em2710/em2750/em28xx specific frame data processing code to a separate function

[Frank Schäfer]

em28xx_urb_data_copy() actually consists of two parts:
USB urb processing (checks, data extraction) and frame data packet processing.
Move the latter to a separate function and call it from em28xx_urb_data_copy()
for each data packet.
The em25xx, em2760, em2765 (and likely em277x) chip variants are using a
different frame data format, for which support will be added later with
another function.

This reduces the size of em28xx_urb_data_copy() and makes the code much more
readable. While we're at it, clean up the code a bit (rename some variables to
something more meaningful, improve some comments etc.)

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |  170 ++++++++++++++++---------------
 1 Datei geändert, 90 Zeilen hinzugefügt(+), 80 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 61c7321..fb40d0b 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -379,15 +379,90 @@ finish_field_prepare_next(struct em28xx *dev,
 	return buf;
 }
 
-/* Processes and copies the URB data content (video and VBI data) */
-static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
+/*
+ * Process data packet according to the em2710/em2750/em28xx frame data format
+ */
+static inline void process_frame_data_em28xx(struct em28xx *dev,
+					     unsigned char *data_pkt,
+					     unsigned int  data_len)
 {
-	struct em28xx_buffer    *buf, *vbi_buf;
+	struct em28xx_buffer    *buf = dev->usb_ctl.vid_buf;
+	struct em28xx_buffer    *vbi_buf = dev->usb_ctl.vbi_buf;
 	struct em28xx_dmaqueue  *dma_q = &dev->vidq;
 	struct em28xx_dmaqueue  *vbi_dma_q = &dev->vbiq;
-	int xfer_bulk, num_packets, i, rc = 1;
-	unsigned int actual_length, len = 0;
-	unsigned char *p;
+
+	/* capture type 0 = vbi start
+	   capture type 1 = vbi in progress
+	   capture type 2 = video start
+	   capture type 3 = video in progress */
+	if (data_len >= 4) {
+		/* NOTE: Headers are always 4 bytes and
+		 * never split across packets */
+		if (data_pkt[0] == 0x88 && data_pkt[1] == 0x88 &&
+		    data_pkt[2] == 0x88 && data_pkt[3] == 0x88) {
+			/* Continuation */
+			data_pkt += 4;
+			data_len -= 4;
+		} else if (data_pkt[0] == 0x33 && data_pkt[1] == 0x95) {
+			/* Field start (VBI mode) */
+			dev->capture_type = 0;
+			dev->vbi_read = 0;
+			em28xx_usbdbg("VBI START HEADER !!!\n");
+			dev->top_field = !(data_pkt[2] & 1);
+			data_pkt += 4;
+			data_len -= 4;
+		} else if (data_pkt[0] == 0x22 && data_pkt[1] == 0x5a) {
+			/* Field start (VBI disabled) */
+			dev->capture_type = 2;
+			em28xx_usbdbg("VIDEO START HEADER !!!\n");
+			dev->top_field = !(data_pkt[2] & 1);
+			data_pkt += 4;
+			data_len -= 4;
+		}
+	}
+	/* NOTE: With bulk transfers, intermediate data packets
+	 * have no continuation header */
+
+	if (dev->capture_type == 0) {
+		vbi_buf = finish_field_prepare_next(dev, vbi_buf, vbi_dma_q);
+		dev->usb_ctl.vbi_buf = vbi_buf;
+		dev->capture_type = 1;
+	}
+
+	if (dev->capture_type == 1) {
+		int vbi_size = dev->vbi_width * dev->vbi_height;
+		int vbi_data_len = ((dev->vbi_read + data_len) > vbi_size) ?
+				   (vbi_size - dev->vbi_read) : data_len;
+
+		/* Copy VBI data */
+		if (vbi_buf != NULL)
+			em28xx_copy_vbi(dev, vbi_buf, data_pkt, vbi_data_len);
+		dev->vbi_read += vbi_data_len;
+
+		if (vbi_data_len < data_len) {
+			/* Continue with copying video data */
+			dev->capture_type = 2;
+			data_pkt += vbi_data_len;
+			data_len -= vbi_data_len;
+		}
+	}
+
+	if (dev->capture_type == 2) {
+		buf = finish_field_prepare_next(dev, buf, dma_q);
+		dev->usb_ctl.vid_buf = buf;
+		dev->capture_type = 3;
+	}
+
+	if (dev->capture_type == 3 && buf != NULL && data_len > 0)
+		em28xx_copy_video(dev, buf, data_pkt, data_len);
+}
+
+/* Processes and copies the URB data content (video and VBI data) */
+static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
+{
+	int xfer_bulk, num_packets, i;
+	unsigned char *usb_data_pkt;
+	unsigned int usb_data_len;
 
 	if (!dev)
 		return 0;
@@ -400,9 +475,6 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 
 	xfer_bulk = usb_pipebulk(urb->pipe);
 
-	buf = dev->usb_ctl.vid_buf;
-	vbi_buf = dev->usb_ctl.vbi_buf;
-
 	if (xfer_bulk) /* bulk */
 		num_packets = 1;
 	else /* isoc */
@@ -410,9 +482,9 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 
 	for (i = 0; i < num_packets; i++) {
 		if (xfer_bulk) { /* bulk */
-			actual_length = urb->actual_length;
+			usb_data_len = urb->actual_length;
 
-			p = urb->transfer_buffer;
+			usb_data_pkt = urb->transfer_buffer;
 		} else { /* isoc */
 			if (urb->iso_frame_desc[i].status < 0) {
 				print_err_status(dev, i,
@@ -421,87 +493,25 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 					continue;
 			}
 
-			actual_length = urb->iso_frame_desc[i].actual_length;
-			if (actual_length > dev->max_pkt_size) {
+			usb_data_len = urb->iso_frame_desc[i].actual_length;
+			if (usb_data_len > dev->max_pkt_size) {
 				em28xx_usbdbg("packet bigger than packet size");
 				continue;
 			}
 
-			p = urb->transfer_buffer +
-			    urb->iso_frame_desc[i].offset;
+			usb_data_pkt = urb->transfer_buffer +
+				       urb->iso_frame_desc[i].offset;
 		}
 
-		if (actual_length == 0) {
+		if (usb_data_len == 0) {
 			/* NOTE: happens very often with isoc transfers */
 			/* em28xx_usbdbg("packet %d is empty",i); - spammy */
 			continue;
 		}
 
-		/* capture type 0 = vbi start
-		   capture type 1 = vbi in progress
-		   capture type 2 = video start
-		   capture type 3 = video in progress */
-		len = actual_length;
-		if (len >= 4) {
-			/* NOTE: headers are always 4 bytes and
-			 * never split across packets */
-			if (p[0] == 0x33 && p[1] == 0x95) {
-				dev->capture_type = 0;
-				dev->vbi_read = 0;
-				em28xx_usbdbg("VBI START HEADER!!!\n");
-				dev->top_field = !(p[2] & 1);
-				p += 4;
-				len -= 4;
-			} else if (p[0] == 0x88 && p[1] == 0x88 &&
-				   p[2] == 0x88 && p[3] == 0x88) {
-				/* continuation */
-				p += 4;
-				len -= 4;
-			} else if (p[0] == 0x22 && p[1] == 0x5a) {
-				/* start video */
-				dev->capture_type = 2;
-				dev->top_field = !(p[2] & 1);
-				p += 4;
-				len -= 4;
-			}
-		}
-		/* NOTE: with bulk transfers, intermediate data packets
-		 * have no continuation header */
-
-		if (dev->capture_type == 0) {
-			vbi_buf = finish_field_prepare_next(dev, vbi_buf, vbi_dma_q);
-			dev->usb_ctl.vbi_buf = vbi_buf;
-			dev->capture_type = 1;
-		}
-
-		if (dev->capture_type == 1) {
-			int vbi_size = dev->vbi_width * dev->vbi_height;
-			int vbi_data_len = ((dev->vbi_read + len) > vbi_size) ?
-					   (vbi_size - dev->vbi_read) : len;
-
-			/* Copy VBI data */
-			if (vbi_buf != NULL)
-				em28xx_copy_vbi(dev, vbi_buf, p, vbi_data_len);
-			dev->vbi_read += vbi_data_len;
-
-			if (vbi_data_len < len) {
-				/* Continue with copying video data */
-				dev->capture_type = 2;
-				p += vbi_data_len;
-				len -= vbi_data_len;
-			}
-		}
-
-		if (dev->capture_type == 2) {
-			buf = finish_field_prepare_next(dev, buf, dma_q);
-			dev->usb_ctl.vid_buf = buf;
-			dev->capture_type = 3;
-		}
-
-		if (buf != NULL && dev->capture_type == 3 && len > 0)
-			em28xx_copy_video(dev, buf, p, len);
+		process_frame_data_em28xx(dev, usb_data_pkt, usb_data_len);
 	}
-	return rc;
+	return 1;
 }
 
 
-- 
1.7.10.4

[PATCH 9/9] em28xx: clean up and unify functions em28xx_copy_vbi() em28xx_copy_video()

[Frank Schäfer]

The code in em28xx_vbi_copy can be simplified a lot.
Also rename some variables to something more meaningful and fix+add the
function descriptions.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c |   65 ++++++++++---------------------
 1 Datei geändert, 20 Zeilen hinzugefügt(+), 45 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index fb40d0b..bd168de 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -168,28 +168,27 @@ static inline void finish_buffer(struct em28xx *dev,
 }
 
 /*
- * Identify the buffer header type and properly handles
+ * Copy picture data from USB buffer to videobuf buffer
  */
 static void em28xx_copy_video(struct em28xx *dev,
 			      struct em28xx_buffer *buf,
-			      unsigned char *p,
+			      unsigned char *usb_buf,
 			      unsigned long len)
 {
 	void *fieldstart, *startwrite, *startread;
 	int  linesdone, currlinedone, offset, lencopy, remain;
 	int bytesperline = dev->width << 1;
-	unsigned char *outp = buf->vb_buf;
 
 	if (buf->pos + len > buf->vb.size)
 		len = buf->vb.size - buf->pos;
 
-	startread = p;
+	startread = usb_buf;
 	remain = len;
 
 	if (dev->progressive || buf->top_field)
-		fieldstart = outp;
+		fieldstart = buf->vb_buf;
 	else /* interlaced mode, even nr. of lines */
-		fieldstart = outp + bytesperline;
+		fieldstart = buf->vb_buf + bytesperline;
 
 	linesdone = buf->pos / bytesperline;
 	currlinedone = buf->pos % bytesperline;
@@ -203,11 +202,12 @@ static void em28xx_copy_video(struct em28xx *dev,
 	lencopy = bytesperline - currlinedone;
 	lencopy = lencopy > remain ? remain : lencopy;
 
-	if ((char *)startwrite + lencopy > (char *)outp + buf->vb.size) {
+	if ((char *)startwrite + lencopy > (char *)buf->vb_buf + buf->vb.size) {
 		em28xx_usbdbg("Overflow of %zi bytes past buffer end (1)\n",
 			      ((char *)startwrite + lencopy) -
-			      ((char *)outp + buf->vb.size));
-		remain = (char *)outp + buf->vb.size - (char *)startwrite;
+			      ((char *)buf->vb_buf + buf->vb.size));
+		remain = (char *)buf->vb_buf + buf->vb.size -
+			 (char *)startwrite;
 		lencopy = remain;
 	}
 	if (lencopy <= 0)
@@ -227,13 +227,13 @@ static void em28xx_copy_video(struct em28xx *dev,
 		else
 			lencopy = bytesperline;
 
-		if ((char *)startwrite + lencopy > (char *)outp +
+		if ((char *)startwrite + lencopy > (char *)buf->vb_buf +
 		    buf->vb.size) {
 			em28xx_usbdbg("Overflow of %zi bytes past buffer end"
 				       "(2)\n",
 				       ((char *)startwrite + lencopy) -
-				       ((char *)outp + buf->vb.size));
-			lencopy = remain = (char *)outp + buf->vb.size -
+				       ((char *)buf->vb_buf + buf->vb.size));
+			lencopy = remain = (char *)buf->vb_buf + buf->vb.size -
 					   (char *)startwrite;
 		}
 		if (lencopy <= 0)
@@ -247,50 +247,25 @@ static void em28xx_copy_video(struct em28xx *dev,
 	buf->pos += len;
 }
 
+/*
+ * Copy VBI data from USB buffer to videobuf buffer
+ */
 static void em28xx_copy_vbi(struct em28xx *dev,
 			    struct em28xx_buffer *buf,
-			    unsigned char *p,
+			    unsigned char *usb_buf,
 			    unsigned long len)
 {
-	void *startwrite, *startread;
-	int  offset;
-	int bytesperline;
-	unsigned char *outp;
-
-	if (dev == NULL) {
-		em28xx_usbdbg("dev is null\n");
-		return;
-	}
-	bytesperline = dev->vbi_width;
-
-	if (buf == NULL) {
-		return;
-	}
-	if (p == NULL) {
-		em28xx_usbdbg("p is null\n");
-		return;
-	}
-	outp = buf->vb_buf;
-	if (outp == NULL) {
-		em28xx_usbdbg("outp is null\n");
-		return;
-	}
+	unsigned int offset;
 
 	if (buf->pos + len > buf->vb.size)
 		len = buf->vb.size - buf->pos;
 
-	startread = p;
-
-	startwrite = outp + buf->pos;
 	offset = buf->pos;
-
 	/* Make sure the bottom field populates the second half of the frame */
-	if (buf->top_field == 0) {
-		startwrite += bytesperline * dev->vbi_height;
-		offset += bytesperline * dev->vbi_height;
-	}
+	if (buf->top_field == 0)
+		offset += dev->vbi_width * dev->vbi_height;
 
-	memcpy(startwrite, startread, len);
+	memcpy(buf->vb_buf + offset, usb_buf, len);
 	buf->pos += len;
 }
 
-- 
1.7.10.4

Re: [PATCH 0/9] em28xx: refactor the frame data processing code

[Frank Schäfer]

Am 13.12.2012 18:36, schrieb Devin Heitmueller:
> On Sat, Dec 8, 2012 at 10:31 AM, Frank Schäfer
> <fschaefer.oss@googlemail.com> wrote:
>> This patch series refactors the frame data processing code in em28xx-video.c to
>> - reduce code duplication
>> - fix a bug in vbi data processing
>> - prepare for adding em25xx/em276x frame data processing support
>> - clean up the code and make it easier to understand
> Hi Frank,
>
> Do you have these patches in a git tree somewhere that I can do a git
> pull from?  If not then that's fine - I'll just save off the patches
> and apply them by hand.

No, I have no public git tree.

> I've basically got your patches, fixes Hans did for v4l2 compliance,
> and I've got a tree that converts the driver to videobuf2 (which
> obviously heavily conflicts with the URB handler cleanup you did).
> Plan is to suck them all into a single tree, deal with the merge
> issues, then issue a pull request to Mauro.

Ahhh, videobuf2 !
Good to know, because I've put this on my TODO list... ;)
Yes, there will likely be heavy merge conflicts...
In which tree are the videobuf2 patches ?

Regards,
Frank

>
> Cheers,
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com

Re: [PATCH 0/9] em28xx: refactor the frame data processing code

[Devin Heitmueller]

On Thu, Dec 13, 2012 at 12:56 PM, Frank Schäfer
<fschaefer.oss@googlemail.com> wrote:
> Am 13.12.2012 18:36, schrieb Devin Heitmueller:
>> On Sat, Dec 8, 2012 at 10:31 AM, Frank Schäfer
>> <fschaefer.oss@googlemail.com> wrote:
>>> This patch series refactors the frame data processing code in em28xx-video.c to
>>> - reduce code duplication
>>> - fix a bug in vbi data processing
>>> - prepare for adding em25xx/em276x frame data processing support
>>> - clean up the code and make it easier to understand
>> Hi Frank,
>>
>> Do you have these patches in a git tree somewhere that I can do a git
>> pull from?  If not then that's fine - I'll just save off the patches
>> and apply them by hand.
>
> No, I have no public git tree.
>
>> I've basically got your patches, fixes Hans did for v4l2 compliance,
>> and I've got a tree that converts the driver to videobuf2 (which
>> obviously heavily conflicts with the URB handler cleanup you did).
>> Plan is to suck them all into a single tree, deal with the merge
>> issues, then issue a pull request to Mauro.
>
> Ahhh, videobuf2 !
> Good to know, because I've put this on my TODO list... ;)

It's harder than it looks.  There are currently no other devices
ported to vb2 which have VBI and/or radio devices.  Hence I have to
refactor the locking a bit (since the URB callback feeds two different
VB2 queues).  In other words, there's no other driver to look at as a
model and copy the business logic from.

> Yes, there will likely be heavy merge conflicts...
> In which tree are the videobuf2 patches ?

It's in a private tree right now, and it doesn't support VBI
currently.  Once I've setup a public tree with yours and Hans changes,
I'll start merging in my changes.

Obviously it would be great for you to test with your webcam and make
sure I didn't break anything along the way.

I've also got changes to support V4L2_FIELD_SEQ_TB, which is needed in
order to take the output and feed to certain hardware deinterlacers.
In reality this is pretty much just a matter of treating the video
data as progressive but changing the field type indicator.

I'm generally pretty easy to find in #linuxtv or #v4l if you want to
discuss further.

Cheers,

Devin

--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 0/9] em28xx: refactor the frame data processing code

[Frank Schäfer]

Am 13.12.2012 19:16, schrieb Devin Heitmueller:
> On Thu, Dec 13, 2012 at 12:56 PM, Frank Schäfer
> <fschaefer.oss@googlemail.com> wrote:
>> Am 13.12.2012 18:36, schrieb Devin Heitmueller:
>>> On Sat, Dec 8, 2012 at 10:31 AM, Frank Schäfer
>>> <fschaefer.oss@googlemail.com> wrote:
>>>> This patch series refactors the frame data processing code in em28xx-video.c to
>>>> - reduce code duplication
>>>> - fix a bug in vbi data processing
>>>> - prepare for adding em25xx/em276x frame data processing support
>>>> - clean up the code and make it easier to understand
>>> Hi Frank,
>>>
>>> Do you have these patches in a git tree somewhere that I can do a git
>>> pull from?  If not then that's fine - I'll just save off the patches
>>> and apply them by hand.
>> No, I have no public git tree.
>>
>>> I've basically got your patches, fixes Hans did for v4l2 compliance,
>>> and I've got a tree that converts the driver to videobuf2 (which
>>> obviously heavily conflicts with the URB handler cleanup you did).
>>> Plan is to suck them all into a single tree, deal with the merge
>>> issues, then issue a pull request to Mauro.
>> Ahhh, videobuf2 !
>> Good to know, because I've put this on my TODO list... ;)
> It's harder than it looks.  There are currently no other devices
> ported to vb2 which have VBI and/or radio devices.  Hence I have to
> refactor the locking a bit (since the URB callback feeds two different
> VB2 queues).  In other words, there's no other driver to look at as a
> model and copy the business logic from.

Yeah, that's one of the reasons why I decided to do it later ;)

>
>> Yes, there will likely be heavy merge conflicts...
>> In which tree are the videobuf2 patches ?
> It's in a private tree right now, and it doesn't support VBI
> currently.  Once I've setup a public tree with yours and Hans changes,
> I'll start merging in my changes.

I suggest to do the conversion on top of my patches, as they should make
things much easier for you.
I unified the handling of the VBI and video buffers, leaving just a few
common functions dealing with the videobuf stuff.

In any case, we should develop against a common tree with a minimum
number of pending patches.
And we should coordinate development.
I don't work on further changes of the frame processing stuff at the moment.
Some I2C fixes/changes will be next. After that, I will try to fix
support for remote controls with external IR IC (connected via i2c).

> Obviously it would be great for you to test with your webcam and make
> sure I didn't break anything along the way.

Sure, I will be glad to test your changes.

> I've also got changes to support V4L2_FIELD_SEQ_TB, which is needed in
> order to take the output and feed to certain hardware deinterlacers.
> In reality this is pretty much just a matter of treating the video
> data as progressive but changing the field type indicator.

Ok, so I assume most of the changes will happen in em28xx_copy_video().
Maybe we can then use a common copy function for video and VBI. Placing
the field data sequentially in the videobuf is what we already do with
the VBI data in em28xx_copy_vbi()

Regards,
Frank

> I'm generally pretty easy to find in #linuxtv or #v4l if you want to
> discuss further.
>
> Cheers,
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com

Re: [PATCH 0/9] em28xx: refactor the frame data processing code

[Devin Heitmueller]

>>> Yes, there will likely be heavy merge conflicts...
>>> In which tree are the videobuf2 patches ?
>> It's in a private tree right now, and it doesn't support VBI
>> currently.  Once I've setup a public tree with yours and Hans changes,
>> I'll start merging in my changes.
>
> I suggest to do the conversion on top of my patches, as they should make
> things much easier for you.
> I unified the handling of the VBI and video buffers, leaving just a few
> common functions dealing with the videobuf stuff.

Yup, that's exactly what I had planned.

> In any case, we should develop against a common tree with a minimum
> number of pending patches.
> And we should coordinate development.
> I don't work on further changes of the frame processing stuff at the moment.
> Some I2C fixes/changes will be next. After that, I will try to fix
> support for remote controls with external IR IC (connected via i2c).
>
>> Obviously it would be great for you to test with your webcam and make
>> sure I didn't break anything along the way.
>
> Sure, I will be glad to test your changes.
>
>> I've also got changes to support V4L2_FIELD_SEQ_TB, which is needed in
>> order to take the output and feed to certain hardware deinterlacers.
>> In reality this is pretty much just a matter of treating the video
>> data as progressive but changing the field type indicator.
>
> Ok, so I assume most of the changes will happen in em28xx_copy_video().

The changes really are all over the tree because it's not just vb2
support but also support for v4l2_fh, which means every ioctl() has a
change to its arguments, and there is no longer an open/close call
implemented.  Also significant impact on the locking model.

> Maybe we can then use a common copy function for video and VBI. Placing
> the field data sequentially in the videobuf is what we already do with
> the VBI data in em28xx_copy_vbi()

Let's get something that works, at which point we can tune/optimize as needed.

Devin

--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 0/9] em28xx: refactor the frame data processing code

[Frank Schäfer]

Am 14.12.2012 16:29, schrieb Devin Heitmueller:
>>>> Yes, there will likely be heavy merge conflicts...
>>>> In which tree are the videobuf2 patches ?
>>> It's in a private tree right now, and it doesn't support VBI
>>> currently.  Once I've setup a public tree with yours and Hans changes,
>>> I'll start merging in my changes.
>> I suggest to do the conversion on top of my patches, as they should make
>> things much easier for you.
>> I unified the handling of the VBI and video buffers, leaving just a few
>> common functions dealing with the videobuf stuff.
> Yup, that's exactly what I had planned.
>
>> In any case, we should develop against a common tree with a minimum
>> number of pending patches.
>> And we should coordinate development.
>> I don't work on further changes of the frame processing stuff at the moment.
>> Some I2C fixes/changes will be next. After that, I will try to fix
>> support for remote controls with external IR IC (connected via i2c).
>>
>>> Obviously it would be great for you to test with your webcam and make
>>> sure I didn't break anything along the way.
>> Sure, I will be glad to test your changes.
>>
>>> I've also got changes to support V4L2_FIELD_SEQ_TB, which is needed in
>>> order to take the output and feed to certain hardware deinterlacers.
>>> In reality this is pretty much just a matter of treating the video
>>> data as progressive but changing the field type indicator.
>> Ok, so I assume most of the changes will happen in em28xx_copy_video().
> The changes really are all over the tree because it's not just vb2
> support but also support for v4l2_fh, which means every ioctl() has a
> change to its arguments, and there is no longer an open/close call
> implemented.  Also significant impact on the locking model.

Ok. Sounds like a lot of fun... ;)

If the changes are all over the tree, we will likely get more collisions.
So we should both make our changes public as soon as possible.

>
>> Maybe we can then use a common copy function for video and VBI. Placing
>> the field data sequentially in the videobuf is what we already do with
>> the VBI data in em28xx_copy_vbi()
> Let's get something that works, at which point we can tune/optimize as needed.

I agree.

Frank

>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com