[media] pwc: Replace private buffer management code with videobuf2

Looking at the pwc buffer management code has made it clear to me it needed
some serious fixing. Not only was there a ton of code duplication even
internally to pwc (read and mmap wait for frame code was duplicated), the
code also was outright buggy. With the worst offender being dqbuf, which
just round robin returned all the mmap buffers, without paying any attention
to them being queued by the app with qbuf or not. And qbuf itself was a noop.

So I set out to fix this and already had some cleanups in place when
I read Jonathan Corbet's lwn article on videobuf2, this inspired me to just
rip out the buffer management code and replace it with videobuf2, greatly
reducing the amount of code, and fixing all bugs in one go:

Many thanks to Jonathan for the timely article on this !

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

drivers/media/video/pwc/Kconfig

@@ -1,6 +1,7 @@
 config USB_PWC
 	tristate "USB Philips Cameras"
 	depends on VIDEO_V4L2
+	select VIDEOBUF2_VMALLOC
 	---help---
 	  Say Y or M here if you want to use one of these Philips & OEM
 	  webcams:

drivers/media/video/pwc/pwc-ctrl.c

@@ -511,13 +511,9 @@ unsigned int pwc_get_fps(struct pwc_device *pdev, unsigned int index, unsigned i
 	return ret;
 }
 
-#define BLACK_Y 0
-#define BLACK_U 128
-#define BLACK_V 128
-
 static void pwc_set_image_buffer_size(struct pwc_device *pdev)
 {
-	int i, factor = 0;
+	int factor = 0;
 
 	/* for V4L2_PIX_FMT_YUV420 */
 	switch (pdev->pixfmt) {
@@ -541,22 +537,9 @@ static void pwc_set_image_buffer_size(struct pwc_device *pdev)
 	 */
 	pdev->offset.x = ((pdev->view.x - pdev->image.x) / 2) & 0xFFFC;
 	pdev->offset.y = ((pdev->view.y - pdev->image.y) / 2) & 0xFFFE;
-
-	/* Fill buffers with black colors */
-	for (i = 0; i < pwc_mbufs; i++) {
-		unsigned char *p = pdev->image_data + pdev->images[i].offset;
-		memset(p, BLACK_Y, pdev->view.x * pdev->view.y);
-		p += pdev->view.x * pdev->view.y;
-		memset(p, BLACK_U, pdev->view.x * pdev->view.y/4);
-		p += pdev->view.x * pdev->view.y/4;
-		memset(p, BLACK_V, pdev->view.x * pdev->view.y/4);
-	}
 }
 
-
-
 /* BRIGHTNESS */
-
 int pwc_get_brightness(struct pwc_device *pdev)
 {
 	char buf;

drivers/media/video/pwc/pwc-misc.c

@@ -126,8 +126,4 @@ void pwc_construct(struct pwc_device *pdev)
 	pdev->pixfmt = V4L2_PIX_FMT_YUV420; /* default */
 	pdev->view_min.size = pdev->view_min.x * pdev->view_min.y;
 	pdev->view_max.size = pdev->view_max.x * pdev->view_max.y;
-	/* length of image, in YUV format; always allocate enough memory. */
-	pdev->len_per_image = PAGE_ALIGN((pdev->abs_max.x * pdev->abs_max.y * 3) / 2);
 }
-
-

drivers/media/video/pwc/pwc-uncompress.c

@@ -34,17 +34,14 @@
 #include "pwc-dec1.h"
 #include "pwc-dec23.h"
 
-int pwc_decompress(struct pwc_device *pdev)
+int pwc_decompress(struct pwc_device *pdev, struct pwc_frame_buf *fbuf)
 {
-	struct pwc_frame_buf *fbuf;
 	int n, line, col, stride;
 	void *yuv, *image;
 	u16 *src;
 	u16 *dsty, *dstu, *dstv;
 
-	fbuf = pdev->read_frame;
+	image = vb2_plane_vaddr(&fbuf->vb, 0);
-	image  = pdev->image_data;
-	image += pdev->images[pdev->fill_image].offset;
 
 	yuv = fbuf->data + pdev->frame_header_size;  /* Skip header */
 
@@ -59,9 +56,13 @@ int pwc_decompress(struct pwc_device *pdev)
 			 * determine this using the type of the webcam */
 		memcpy(raw_frame->cmd, pdev->cmd_buf, 4);
 		memcpy(raw_frame+1, yuv, pdev->frame_size);
+		vb2_set_plane_payload(&fbuf->vb, 0,
+			pdev->frame_size + sizeof(struct pwc_raw_frame));
 		return 0;
 	}
 
+	vb2_set_plane_payload(&fbuf->vb, 0, pdev->view.size);
+
 	if (pdev->vbandlength == 0) {
 		/* Uncompressed mode.
 		 * We copy the data into the output buffer, using the viewport

drivers/media/video/pwc/pwc-v4l.c

@@ -309,7 +309,7 @@ static int pwc_vidioc_set_fmt(struct pwc_device *pdev, struct v4l2_format *f)
 	    pixelformat != V4L2_PIX_FMT_PWC2)
 		return -EINVAL;
 
-	if (pdev->iso_init)
+	if (vb2_is_streaming(&pdev->vb_queue))
 		return -EBUSY;
 
 	PWC_DEBUG_IOCTL("Trying to set format to: width=%d height=%d fps=%d "
@@ -673,150 +673,47 @@ static int pwc_s_fmt_vid_cap(struct file *file, void *fh, struct v4l2_format *f)
 	return pwc_vidioc_set_fmt(pdev, f);
 }
 
-static int pwc_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *rb)
+static int pwc_reqbufs(struct file *file, void *fh,
+		       struct v4l2_requestbuffers *rb)
 {
-	int nbuffers;
+	struct pwc_device *pdev = video_drvdata(file);
 
-	PWC_DEBUG_IOCTL("ioctl(VIDIOC_REQBUFS) count=%d\n", rb->count);
+	return vb2_reqbufs(&pdev->vb_queue, rb);
-	if (rb->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		return -EINVAL;
-	if (rb->memory != V4L2_MEMORY_MMAP)
-		return -EINVAL;
-
-	nbuffers = rb->count;
-	if (nbuffers < 2)
-		nbuffers = 2;
-	else if (nbuffers > pwc_mbufs)
-		nbuffers = pwc_mbufs;
-	/* Force to use our # of buffers */
-	rb->count = pwc_mbufs;
-	return 0;
 }
 
 static int pwc_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 {
 	struct pwc_device *pdev = video_drvdata(file);
-	int index;
 
-	PWC_DEBUG_IOCTL("ioctl(VIDIOC_QUERYBUF) index=%d\n", buf->index);
+	return vb2_querybuf(&pdev->vb_queue, buf);
-	if (buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
-		PWC_DEBUG_IOCTL("ioctl(VIDIOC_QUERYBUF) Bad type\n");
-		return -EINVAL;
-	}
-	index = buf->index;
-	if (index < 0 || index >= pwc_mbufs) {
-		PWC_DEBUG_IOCTL("ioctl(VIDIOC_QUERYBUF) Bad index %d\n", buf->index);
-		return -EINVAL;
-	}
-
-	buf->m.offset = index * pdev->len_per_image;
-	if (pdev->pixfmt != V4L2_PIX_FMT_YUV420)
-		buf->bytesused = pdev->frame_size + sizeof(struct pwc_raw_frame);
-	else
-		buf->bytesused = pdev->view.size;
-	buf->field = V4L2_FIELD_NONE;
-	buf->memory = V4L2_MEMORY_MMAP;
-	/*buf->flags = V4L2_BUF_FLAG_MAPPED;*/
-	buf->length = pdev->len_per_image;
-
-	PWC_DEBUG_READ("VIDIOC_QUERYBUF: index=%d\n", buf->index);
-	PWC_DEBUG_READ("VIDIOC_QUERYBUF: m.offset=%d\n", buf->m.offset);
-	PWC_DEBUG_READ("VIDIOC_QUERYBUF: bytesused=%d\n", buf->bytesused);
-
-	return 0;
 }
 
 static int pwc_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 {
-	PWC_DEBUG_IOCTL("ioctl(VIDIOC_QBUF) index=%d\n", buf->index);
+	struct pwc_device *pdev = video_drvdata(file);
-	if (buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		return -EINVAL;
-	if (buf->memory != V4L2_MEMORY_MMAP)
-		return -EINVAL;
-	if (buf->index >= pwc_mbufs)
-		return -EINVAL;
 
-	buf->flags |= V4L2_BUF_FLAG_QUEUED;
+	return vb2_qbuf(&pdev->vb_queue, buf);
-	buf->flags &= ~V4L2_BUF_FLAG_DONE;
-
-	return 0;
 }
 
 static int pwc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 {
-	DECLARE_WAITQUEUE(wait, current);
 	struct pwc_device *pdev = video_drvdata(file);
-	int ret;
-
-	PWC_DEBUG_IOCTL("ioctl(VIDIOC_DQBUF)\n");
-
-	if (buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		return -EINVAL;
-
-	add_wait_queue(&pdev->frameq, &wait);
-	while (pdev->full_frames == NULL) {
-		if (pdev->error_status) {
-			remove_wait_queue(&pdev->frameq, &wait);
-			set_current_state(TASK_RUNNING);
-			return -pdev->error_status;
-		}
-
-		if (signal_pending(current)) {
-			remove_wait_queue(&pdev->frameq, &wait);
-			set_current_state(TASK_RUNNING);
-			return -ERESTARTSYS;
-		}
-		mutex_unlock(&pdev->modlock);
-		schedule();
-		set_current_state(TASK_INTERRUPTIBLE);
-		mutex_lock(&pdev->modlock);
-	}
-	remove_wait_queue(&pdev->frameq, &wait);
-	set_current_state(TASK_RUNNING);
-
-	PWC_DEBUG_IOCTL("VIDIOC_DQBUF: frame ready.\n");
-	/* Decompress data in pdev->images[pdev->fill_image] */
-	ret = pwc_handle_frame(pdev);
-	if (ret)
-		return ret;
-	PWC_DEBUG_IOCTL("VIDIOC_DQBUF: after pwc_handle_frame\n");
-
-	buf->index = pdev->fill_image;
-	if (pdev->pixfmt != V4L2_PIX_FMT_YUV420)
-		buf->bytesused = pdev->frame_size + sizeof(struct pwc_raw_frame);
-	else
-		buf->bytesused = pdev->view.size;
-	buf->flags = V4L2_BUF_FLAG_MAPPED;
-	buf->field = V4L2_FIELD_NONE;
-	do_gettimeofday(&buf->timestamp);
-	buf->sequence = 0;
-	buf->memory = V4L2_MEMORY_MMAP;
-	buf->m.offset = pdev->fill_image * pdev->len_per_image;
-	buf->length = pdev->len_per_image;
-	pwc_next_image(pdev);
-
-	PWC_DEBUG_IOCTL("VIDIOC_DQBUF: buf->index=%d\n", buf->index);
-	PWC_DEBUG_IOCTL("VIDIOC_DQBUF: buf->length=%d\n", buf->length);
-	PWC_DEBUG_IOCTL("VIDIOC_DQBUF: m.offset=%d\n", buf->m.offset);
-	PWC_DEBUG_IOCTL("VIDIOC_DQBUF: bytesused=%d\n", buf->bytesused);
-	PWC_DEBUG_IOCTL("VIDIOC_DQBUF: leaving\n");
-	return 0;
 
+	return vb2_dqbuf(&pdev->vb_queue, buf, file->f_flags & O_NONBLOCK);
 }
 
 static int pwc_streamon(struct file *file, void *fh, enum v4l2_buf_type i)
 {
 	struct pwc_device *pdev = video_drvdata(file);
 
-	return pwc_isoc_init(pdev);
+	return vb2_streamon(&pdev->vb_queue, i);
 }
 
 static int pwc_streamoff(struct file *file, void *fh, enum v4l2_buf_type i)
 {
 	struct pwc_device *pdev = video_drvdata(file);
 
-	pwc_isoc_cleanup(pdev);
+	return vb2_streamoff(&pdev->vb_queue, i);
-	return 0;
 }
 
 static int pwc_enum_framesizes(struct file *file, void *fh,

drivers/media/video/pwc/pwc.h

@@ -36,6 +36,7 @@
 #include <linux/videodev2.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ioctl.h>
+#include <media/videobuf2-vmalloc.h>
 #ifdef CONFIG_USB_PWC_INPUT_EVDEV
 #include <linux/input.h>
 #endif
@@ -112,18 +113,17 @@
 #define FRAME_LOWMARK 5
 
 /* Size and number of buffers for the ISO pipe. */
-#define MAX_ISO_BUFS		2
+#define MAX_ISO_BUFS		3
 #define ISO_FRAMES_PER_DESC	10
 #define ISO_MAX_FRAME_SIZE	960
 #define ISO_BUFFER_SIZE 	(ISO_FRAMES_PER_DESC * ISO_MAX_FRAME_SIZE)
 
-/* Frame buffers: contains compressed or uncompressed video data. */
-#define MAX_FRAMES		5
 /* Maximum size after decompression is 640x480 YUV data, 1.5 * 640 * 480 */
 #define PWC_FRAME_SIZE 		(460800 + TOUCAM_HEADER_SIZE + TOUCAM_TRAILER_SIZE)
 
-/* Absolute maximum number of buffers available for mmap() */
+/* Absolute minimum and maximum number of buffers available for mmap() */
-#define MAX_IMAGES 		10
+#define MIN_FRAMES		2
+#define MAX_FRAMES		16
 
 /* Some macros to quickly find the type of a webcam */
 #define DEVICE_USE_CODEC1(x) ((x)<675)
@@ -143,16 +143,10 @@ struct pwc_iso_buf
 /* intermediate buffers with raw data from the USB cam */
 struct pwc_frame_buf
 {
-   void *data;
+	struct vb2_buffer vb;	/* common v4l buffer stuff -- must be first */
-   volatile int filled;		/* number of bytes filled */
+	struct list_head list;
-   struct pwc_frame_buf *next;	/* list */
+	void *data;
-};
+	int filled;		/* number of bytes filled */
-
-/* additionnal informations used when dealing image between kernel and userland */
-struct pwc_imgbuf
-{
-	unsigned long offset;	/* offset of this buffer in the big array of image_data */
-	int   vma_use_count;	/* count the number of time this memory is mapped */
 };
 
 struct pwc_device
@@ -177,8 +171,6 @@ struct pwc_device
    int vframes, vsize;		/* frames-per-second & size (see PSZ_*) */
    int pixfmt;			/* pixelformat: V4L2_PIX_FMT_YUV420 or raw: _PWC1, _PWC2 */
    int vframe_count;		/* received frames */
-   int vframes_dumped; 		/* counter for dumped frames */
-   int vframes_error;		/* frames received in error */
    int vmax_packet_size;	/* USB maxpacket size */
    int vlast_packet_size;	/* for frame synchronisation */
    int visoc_errors;		/* number of contiguous ISOC errors */
@@ -192,35 +184,29 @@ struct pwc_device
    int cmd_len;
    unsigned char cmd_buf[13];
 
-   /* The image acquisition requires 3 to 4 steps:
-      1. data is gathered in short packets from the USB controller
-      2. data is synchronized and packed into a frame buffer
-      3a. in case data is compressed, decompress it directly into image buffer
-      3b. in case data is uncompressed, copy into image buffer with viewport
-      4. data is transferred to the user process
-
-      Note that MAX_ISO_BUFS != MAX_FRAMES != MAX_IMAGES....
-      We have in effect a back-to-back-double-buffer system.
-    */
-   /* 1: isoc */
    struct pwc_iso_buf sbuf[MAX_ISO_BUFS];
    char iso_init;
 
-   /* 2: frame */
+	/* videobuf2 queue and queued buffers list */
-   struct pwc_frame_buf *fbuf;	/* all frames */
+	struct vb2_queue vb_queue;
-   struct pwc_frame_buf *empty_frames, *empty_frames_tail;	/* all empty frames */
+	struct list_head queued_bufs;
-   struct pwc_frame_buf *full_frames, *full_frames_tail;	/* all filled frames */
+	spinlock_t queued_bufs_lock;
-   struct pwc_frame_buf *fill_frame;	/* frame currently being filled */
+
-   struct pwc_frame_buf *read_frame;	/* frame currently read by user process */
+	/*
+	 * Frame currently being filled, this only gets touched by the
+	 * isoc urb complete handler, and by stream start / stop since
+	 * start / stop touch it before / after starting / killing the urbs
+	 * no locking is needed around this
+	 */
+	struct pwc_frame_buf *fill_buf;
+
    int frame_header_size, frame_trailer_size;
    int frame_size;
    int frame_total_size; /* including header & trailer */
    int drop_frames;
 
-   /* 3: decompression */
    void *decompress_data;		/* private data for decompression engine */
 
-   /* 4: image */
    /* We have an 'image' and a 'view', where 'image' is the fixed-size image
       as delivered by the camera, and 'view' is the size requested by the
       program. The camera image is centered in this viewport, laced with
@@ -232,15 +218,7 @@ struct pwc_device
    struct pwc_coord image, view;	/* image and viewport size */
    struct pwc_coord offset;		/* offset within the viewport */
 
-   void *image_data;			/* total buffer, which is subdivided into ... */
-   struct pwc_imgbuf images[MAX_IMAGES];/* ...several images... */
-   int fill_image;			/* ...which are rotated. */
-   int len_per_image;			/* length per image */
-   int image_read_pos;			/* In case we read data in pieces, keep track of were we are in the imagebuffer */
-   int image_used[MAX_IMAGES];		/* For MCAPTURE and SYNC */
-
    struct mutex modlock;		/* to prevent races in video_open(), etc */
-   spinlock_t ptrlock;			/* for manipulating the buffer pointers */
 
    /*** motorized pan/tilt feature */
    struct pwc_mpt_range angle_range;
@@ -253,7 +231,6 @@ struct pwc_device
 #endif
 
    /*** Misc. data ***/
-   wait_queue_head_t frameq;		/* When waiting for a frame to finish... */
 #if PWC_INT_PIPE
    void *usb_int_handler;		/* for the interrupt endpoint */
 #endif
@@ -263,13 +240,6 @@ struct pwc_device
 #ifdef CONFIG_USB_PWC_DEBUG
 extern int pwc_trace;
 #endif
-extern int pwc_mbufs;
-
-/** functions in pwc-if.c */
-int pwc_handle_frame(struct pwc_device *pdev);
-void pwc_next_image(struct pwc_device *pdev);
-int pwc_isoc_init(struct pwc_device *pdev);
-void pwc_isoc_cleanup(struct pwc_device *pdev);
 
 /** Functions in pwc-misc.c */
 /* sizes in pixels */
@@ -334,6 +304,6 @@ extern const struct v4l2_ioctl_ops pwc_ioctl_ops;
 
 /** pwc-uncompress.c */
 /* Expand frame to image, possibly including decompression. Uses read_frame and fill_image */
-extern int pwc_decompress(struct pwc_device *pdev);
+int pwc_decompress(struct pwc_device *pdev, struct pwc_frame_buf *fbuf);
 
 #endif