Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Hans Verkuil]

NACK.

For two reasons: first of all it is not signed off by Andy Walls, the cx18
maintainer. I know he has had other things on his plate recently which is
probably why he hasn't had the chance to review this.

Secondly, while doing a quick scan myself I noticed that this code does a
conversion from UYVY format to YUYV *in the driver*. Format conversion is
not allowed in the kernel, we have libv4lconvert for that. So at the minimum
this conversion code must be removed first.

Regards,

	Hans

On Monday, May 02, 2011 19:17:57 Mauro Carvalho Chehab wrote:
> This is an automatic generated email to let you know that the following patch were queued at the 
> http://git.linuxtv.org/media_tree.git tree:
> 
> Subject: [media] cx18: mmap() support for raw YUV video capture
> Author:  Steven Toth <stoth@kernellabs.com>
> Date:    Wed Apr 6 08:32:56 2011 -0300
> 
> Add support for mmap method streaming of raw YUV video on cx18-based
> hardware, in addition to the existing support for read() streaming of
> raw YUV and MPEG-2 encoded video.
> 
> [simon.farnsworth@onelan.co.uk: I forward-ported this from Steven's original work,
>  done under contract to ONELAN. The original code is at
>  http://www.kernellabs.com/hg/~stoth/cx18-videobuf]
> 
> Signed-off-by: Steven Toth <stoth@kernellabs.com>
> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
>  drivers/media/video/cx18/Kconfig        |    2 +
>  drivers/media/video/cx18/cx18-driver.h  |   25 ++++
>  drivers/media/video/cx18/cx18-fileops.c |  214 +++++++++++++++++++++++++++++++
>  drivers/media/video/cx18/cx18-fileops.h |    2 +
>  drivers/media/video/cx18/cx18-ioctl.c   |  136 ++++++++++++++++++--
>  drivers/media/video/cx18/cx18-mailbox.c |   70 ++++++++++
>  drivers/media/video/cx18/cx18-streams.c |   23 ++++
>  drivers/media/video/cx18/cx23418.h      |    6 +
>  8 files changed, 466 insertions(+), 12 deletions(-)
> 
> ---
> 
> http://git.linuxtv.org/media_tree.git?a=commitdiff;h=fa8f1381764d83222333cb67b8d93b9cb1605bf3
> 
> diff --git a/drivers/media/video/cx18/Kconfig b/drivers/media/video/cx18/Kconfig
> index d788ad6..9c23202 100644
> --- a/drivers/media/video/cx18/Kconfig
> +++ b/drivers/media/video/cx18/Kconfig
> @@ -2,6 +2,8 @@ config VIDEO_CX18
>  	tristate "Conexant cx23418 MPEG encoder support"
>  	depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
>  	select I2C_ALGOBIT
> +	select VIDEOBUF_DVB
> +	select VIDEOBUF_VMALLOC
>  	depends on RC_CORE
>  	select VIDEO_TUNER
>  	select VIDEO_TVEEPROM
> diff --git a/drivers/media/video/cx18/cx18-driver.h b/drivers/media/video/cx18/cx18-driver.h
> index b86a740..70e1e04 100644
> --- a/drivers/media/video/cx18/cx18-driver.h
> +++ b/drivers/media/video/cx18/cx18-driver.h
> @@ -65,6 +65,10 @@
>  #include "dvb_net.h"
>  #include "dvbdev.h"
>  
> +/* Videobuf / YUV support */
> +#include <media/videobuf-core.h>
> +#include <media/videobuf-vmalloc.h>
> +
>  #ifndef CONFIG_PCI
>  #  error "This driver requires kernel PCI support."
>  #endif
> @@ -403,6 +407,23 @@ struct cx18_stream {
>  	struct cx18_queue q_idle;	/* idle - not in rotation */
>  
>  	struct work_struct out_work_order;
> +
> +	/* Videobuf for YUV video */
> +	u32 pixelformat;
> +	struct list_head vb_capture;    /* video capture queue */
> +	spinlock_t vb_lock;
> +	struct v4l2_framebuffer fbuf;
> +	v4l2_std_id tvnorm; /* selected tv norm */
> +	struct timer_list vb_timeout;
> +	int vbwidth;
> +	int vbheight;
> +};
> +
> +struct cx18_videobuf_buffer {
> +	/* Common video buffer sub-system struct */
> +	struct videobuf_buffer vb;
> +	v4l2_std_id tvnorm; /* selected tv norm */
> +	u32 bytes_used;
>  };
>  
>  struct cx18_open_id {
> @@ -410,6 +431,10 @@ struct cx18_open_id {
>  	u32 open_id;
>  	int type;
>  	struct cx18 *cx;
> +
> +	struct videobuf_queue vbuf_q;
> +	spinlock_t s_lock; /* Protect vbuf_q */
> +	enum v4l2_buf_type vb_type;
>  };
>  
>  static inline struct cx18_open_id *fh2id(struct v4l2_fh *fh)
> diff --git a/drivers/media/video/cx18/cx18-fileops.c b/drivers/media/video/cx18/cx18-fileops.c
> index e9802d9..c74eafd 100644
> --- a/drivers/media/video/cx18/cx18-fileops.c
> +++ b/drivers/media/video/cx18/cx18-fileops.c
> @@ -597,6 +597,13 @@ ssize_t cx18_v4l2_read(struct file *filp, char __user *buf, size_t count,
>  	mutex_unlock(&cx->serialize_lock);
>  	if (rc)
>  		return rc;
> +
> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
> +		return videobuf_read_stream(&id->vbuf_q, buf, count, pos, 0,
> +			filp->f_flags & O_NONBLOCK);
> +	}
> +
>  	return cx18_read_pos(s, buf, count, pos, filp->f_flags & O_NONBLOCK);
>  }
>  
> @@ -622,6 +629,11 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp, poll_table *wait)
>  		CX18_DEBUG_FILE("Encoder poll started capture\n");
>  	}
>  
> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
> +		return videobuf_poll_stream(filp, &id->vbuf_q, wait);
> +	}
> +
>  	/* add stream's waitq to the poll list */
>  	CX18_DEBUG_HI_FILE("Encoder poll\n");
>  	poll_wait(filp, &s->waitq, wait);
> @@ -633,6 +645,58 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp, poll_table *wait)
>  	return 0;
>  }
>  
> +int cx18_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	struct cx18_open_id *id = file->private_data;
> +	struct cx18 *cx = id->cx;
> +	struct cx18_stream *s = &cx->streams[id->type];
> +	int eof = test_bit(CX18_F_S_STREAMOFF, &s->s_flags);
> +
> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
> +
> +		/* Start a capture if there is none */
> +		if (!eof && !test_bit(CX18_F_S_STREAMING, &s->s_flags)) {
> +			int rc;
> +
> +			mutex_lock(&cx->serialize_lock);
> +			rc = cx18_start_capture(id);
> +			mutex_unlock(&cx->serialize_lock);
> +			if (rc) {
> +				CX18_DEBUG_INFO(
> +					"Could not start capture for %s (%d)\n",
> +					s->name, rc);
> +				return -EINVAL;
> +			}
> +			CX18_DEBUG_FILE("Encoder poll started capture\n");
> +		}
> +
> +		return videobuf_mmap_mapper(&id->vbuf_q, vma);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +void cx18_vb_timeout(unsigned long data)
> +{
> +	struct cx18_stream *s = (struct cx18_stream *)data;
> +	struct cx18_videobuf_buffer *buf;
> +	unsigned long flags;
> +
> +	/* Return all of the buffers in error state, so the vbi/vid inode
> +	 * can return from blocking.
> +	 */
> +	spin_lock_irqsave(&s->vb_lock, flags);
> +	while (!list_empty(&s->vb_capture)) {
> +		buf = list_entry(s->vb_capture.next,
> +			struct cx18_videobuf_buffer, vb.queue);
> +		list_del(&buf->vb.queue);
> +		buf->vb.state = VIDEOBUF_ERROR;
> +		wake_up(&buf->vb.done);
> +	}
> +	spin_unlock_irqrestore(&s->vb_lock, flags);
> +}
> +
>  void cx18_stop_capture(struct cx18_open_id *id, int gop_end)
>  {
>  	struct cx18 *cx = id->cx;
> @@ -716,12 +780,150 @@ int cx18_v4l2_close(struct file *filp)
>  		cx18_release_stream(s);
>  	} else {
>  		cx18_stop_capture(id, 0);
> +		if (id->type == CX18_ENC_STREAM_TYPE_YUV)
> +			videobuf_mmap_free(&id->vbuf_q);
>  	}
>  	kfree(id);
>  	mutex_unlock(&cx->serialize_lock);
>  	return 0;
>  }
>  
> +void cx18_dma_free(struct videobuf_queue *q,
> +	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
> +{
> +	videobuf_waiton(q, &buf->vb, 0, 0);
> +	videobuf_vmalloc_free(&buf->vb);
> +	buf->vb.state = VIDEOBUF_NEEDS_INIT;
> +}
> +
> +static int cx18_prepare_buffer(struct videobuf_queue *q,
> +	struct cx18_stream *s,
> +	struct cx18_videobuf_buffer *buf,
> +	u32 pixelformat,
> +	unsigned int width, unsigned int height,
> +	enum v4l2_field field)
> +{
> +	int rc = 0;
> +
> +	/* check settings */
> +	buf->bytes_used = 0;
> +
> +	if ((width  < 48) || (height < 32))
> +		return -EINVAL;
> +
> +	buf->vb.size = (width * height * 16 /*fmt->depth*/) >> 3;
> +	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
> +		return -EINVAL;
> +
> +	/* alloc + fill struct (if changed) */
> +	if (buf->vb.width != width || buf->vb.height != height ||
> +	    buf->vb.field != field || s->pixelformat != pixelformat ||
> +	    buf->tvnorm != s->tvnorm) {
> +
> +		buf->vb.width  = width;
> +		buf->vb.height = height;
> +		buf->vb.field  = field;
> +		buf->tvnorm    = s->tvnorm;
> +		s->pixelformat = pixelformat;
> +
> +		cx18_dma_free(q, s, buf);
> +	}
> +
> +	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
> +		return -EINVAL;
> +
> +	if (buf->vb.field == 0)
> +		buf->vb.field = V4L2_FIELD_INTERLACED;
> +
> +	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
> +		buf->vb.width  = width;
> +		buf->vb.height = height;
> +		buf->vb.field  = field;
> +		buf->tvnorm    = s->tvnorm;
> +		s->pixelformat = pixelformat;
> +
> +		rc = videobuf_iolock(q, &buf->vb, &s->fbuf);
> +		if (rc != 0)
> +			goto fail;
> +	}
> +	buf->vb.state = VIDEOBUF_PREPARED;
> +	return 0;
> +
> +fail:
> +	cx18_dma_free(q, s, buf);
> +	return rc;
> +
> +}
> +
> +#define VB_MIN_BUFFERS 32
> +#define VB_MIN_BUFSIZE 0x208000
> +
> +static int buffer_setup(struct videobuf_queue *q,
> +	unsigned int *count, unsigned int *size)
> +{
> +	struct cx18_open_id *id = q->priv_data;
> +	struct cx18 *cx = id->cx;
> +	struct cx18_stream *s = &cx->streams[id->type];
> +
> +	*size = 2 * s->vbwidth * s->vbheight;
> +	if (*count == 0)
> +		*count = VB_MIN_BUFFERS;
> +
> +	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
> +		(*count)--;
> +
> +	q->field = V4L2_FIELD_INTERLACED;
> +	q->last = V4L2_FIELD_INTERLACED;
> +
> +	return 0;
> +}
> +
> +static int buffer_prepare(struct videobuf_queue *q,
> +	struct videobuf_buffer *vb,
> +	enum v4l2_field field)
> +{
> +	struct cx18_videobuf_buffer *buf =
> +		container_of(vb, struct cx18_videobuf_buffer, vb);
> +	struct cx18_open_id *id = q->priv_data;
> +	struct cx18 *cx = id->cx;
> +	struct cx18_stream *s = &cx->streams[id->type];
> +
> +	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
> +		s->vbwidth, s->vbheight, field);
> +}
> +
> +static void buffer_release(struct videobuf_queue *q,
> +	struct videobuf_buffer *vb)
> +{
> +	struct cx18_videobuf_buffer *buf =
> +		container_of(vb, struct cx18_videobuf_buffer, vb);
> +	struct cx18_open_id *id = q->priv_data;
> +	struct cx18 *cx = id->cx;
> +	struct cx18_stream *s = &cx->streams[id->type];
> +
> +	cx18_dma_free(q, s, buf);
> +}
> +
> +static void buffer_queue(struct videobuf_queue *q, struct videobuf_buffer *vb)
> +{
> +	struct cx18_videobuf_buffer *buf =
> +		container_of(vb, struct cx18_videobuf_buffer, vb);
> +	struct cx18_open_id *id = q->priv_data;
> +	struct cx18 *cx = id->cx;
> +	struct cx18_stream *s = &cx->streams[id->type];
> +
> +	buf->vb.state = VIDEOBUF_QUEUED;
> +
> +	list_add_tail(&buf->vb.queue, &s->vb_capture);
> +}
> +
> +static struct videobuf_queue_ops cx18_videobuf_qops = {
> +	.buf_setup    = buffer_setup,
> +	.buf_prepare  = buffer_prepare,
> +	.buf_queue    = buffer_queue,
> +	.buf_release  = buffer_release,
> +};
> +
>  static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
>  {
>  	struct cx18 *cx = s->cx;
> @@ -740,6 +942,9 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
>  	item->cx = cx;
>  	item->type = s->type;
>  
> +	spin_lock_init(&item->s_lock);
> +	item->vb_type = 0;
> +
>  	item->open_id = cx->open_id++;
>  	filp->private_data = &item->fh;
>  
> @@ -774,6 +979,15 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
>  		/* Done! Unmute and continue. */
>  		cx18_unmute(cx);
>  	}
> +	if (item->type == CX18_ENC_STREAM_TYPE_YUV) {
> +		item->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +		videobuf_queue_vmalloc_init(&item->vbuf_q, &cx18_videobuf_qops,
> +			&cx->pci_dev->dev, &item->s_lock,
> +			V4L2_BUF_TYPE_VIDEO_CAPTURE,
> +			V4L2_FIELD_INTERLACED,
> +			sizeof(struct cx18_videobuf_buffer),
> +			item, &cx->serialize_lock);
> +	}
>  	v4l2_fh_add(&item->fh);
>  	return 0;
>  }
> diff --git a/drivers/media/video/cx18/cx18-fileops.h b/drivers/media/video/cx18/cx18-fileops.h
> index 5c8fcb8..b9e5110 100644
> --- a/drivers/media/video/cx18/cx18-fileops.h
> +++ b/drivers/media/video/cx18/cx18-fileops.h
> @@ -33,6 +33,8 @@ int cx18_start_capture(struct cx18_open_id *id);
>  void cx18_stop_capture(struct cx18_open_id *id, int gop_end);
>  void cx18_mute(struct cx18 *cx);
>  void cx18_unmute(struct cx18 *cx);
> +int cx18_v4l2_mmap(struct file *file, struct vm_area_struct *vma);
> +void cx18_vb_timeout(unsigned long data);
>  
>  /* Shared with cx18-alsa module */
>  int cx18_claim_stream(struct cx18_open_id *id, int type);
> diff --git a/drivers/media/video/cx18/cx18-ioctl.c b/drivers/media/video/cx18/cx18-ioctl.c
> index 4f041c0..777d726 100644
> --- a/drivers/media/video/cx18/cx18-ioctl.c
> +++ b/drivers/media/video/cx18/cx18-ioctl.c
> @@ -41,6 +41,18 @@
>  #include <media/tveeprom.h>
>  #include <media/v4l2-chip-ident.h>
>  
> +static struct v4l2_fmtdesc formats[] = {
> +	{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
> +	  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
> +	},
> +	{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
> +	  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
> +	},
> +	{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
> +	  "YUYV 4:2:2", V4L2_PIX_FMT_YUYV, { 0, 0, 0, 0 }
> +	},
> +};
> +
>  u16 cx18_service2vbi(int type)
>  {
>  	switch (type) {
> @@ -150,6 +162,7 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh,
>  {
>  	struct cx18_open_id *id = fh2id(fh);
>  	struct cx18 *cx = id->cx;
> +	struct cx18_stream *s = &cx->streams[id->type];
>  	struct v4l2_pix_format *pixfmt = &fmt->fmt.pix;
>  
>  	pixfmt->width = cx->cxhdl.width;
> @@ -158,7 +171,7 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh,
>  	pixfmt->field = V4L2_FIELD_INTERLACED;
>  	pixfmt->priv = 0;
>  	if (id->type == CX18_ENC_STREAM_TYPE_YUV) {
> -		pixfmt->pixelformat = V4L2_PIX_FMT_HM12;
> +		pixfmt->pixelformat = s->pixelformat;
>  		/* YUV size is (Y=(h*720) + UV=(h*(720/2))) */
>  		pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
>  		pixfmt->bytesperline = 720;
> @@ -237,7 +250,6 @@ static int cx18_try_fmt_vid_cap(struct file *file, void *fh,
>  	h = min(h, cx->is_50hz ? 576 : 480);
>  	h = max(h, min_h);
>  
> -	cx18_g_fmt_vid_cap(file, fh, fmt);
>  	fmt->fmt.pix.width = w;
>  	fmt->fmt.pix.height = h;
>  	return 0;
> @@ -274,6 +286,7 @@ static int cx18_s_fmt_vid_cap(struct file *file, void *fh,
>  	struct cx18_open_id *id = fh2id(fh);
>  	struct cx18 *cx = id->cx;
>  	struct v4l2_mbus_framefmt mbus_fmt;
> +	struct cx18_stream *s = &cx->streams[id->type];
>  	int ret;
>  	int w, h;
>  
> @@ -283,6 +296,10 @@ static int cx18_s_fmt_vid_cap(struct file *file, void *fh,
>  	w = fmt->fmt.pix.width;
>  	h = fmt->fmt.pix.height;
>  
> +	s->pixelformat = fmt->fmt.pix.pixelformat;
> +	s->vbheight = h;
> +	s->vbwidth = w;
> +
>  	if (cx->cxhdl.width == w && cx->cxhdl.height == h)
>  		return 0;
>  
> @@ -540,16 +557,7 @@ static int cx18_g_crop(struct file *file, void *fh, struct v4l2_crop *crop)
>  static int cx18_enum_fmt_vid_cap(struct file *file, void *fh,
>  					struct v4l2_fmtdesc *fmt)
>  {
> -	static struct v4l2_fmtdesc formats[] = {
> -		{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
> -		  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
> -		},
> -		{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
> -		  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
> -		}
> -	};
> -
> -	if (fmt->index > 1)
> +	if (fmt->index > ARRAY_SIZE(formats) - 1)
>  		return -EINVAL;
>  	*fmt = formats[fmt->index];
>  	return 0;
> @@ -863,6 +871,104 @@ static int cx18_g_enc_index(struct file *file, void *fh,
>  	return 0;
>  }
>  
> +static struct videobuf_queue *cx18_vb_queue(struct cx18_open_id *id)
> +{
> +	struct videobuf_queue *q = NULL;
> +
> +	switch (id->vb_type) {
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +		q = &id->vbuf_q;
> +		break;
> +	case V4L2_BUF_TYPE_VBI_CAPTURE:
> +		break;
> +	default:
> +		break;
> +	}
> +	return q;
> +}
> +
> +static int cx18_streamon(struct file *file, void *priv,
> +	enum v4l2_buf_type type)
> +{
> +	struct cx18_open_id *id = file->private_data;
> +	struct cx18 *cx = id->cx;
> +	struct cx18_stream *s = &cx->streams[id->type];
> +
> +	/* Start the hardware only if we're the video device */
> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> +		return -EINVAL;
> +
> +	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
> +		return -EINVAL;
> +
> +	/* Establish a buffer timeout */
> +	mod_timer(&s->vb_timeout, jiffies + (HZ * 2));
> +
> +	return videobuf_streamon(cx18_vb_queue(id));
> +}
> +
> +static int cx18_streamoff(struct file *file, void *priv,
> +	enum v4l2_buf_type type)
> +{
> +	struct cx18_open_id *id = file->private_data;
> +
> +	/* Start the hardware only if we're the video device */
> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> +		return -EINVAL;
> +
> +	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
> +		return -EINVAL;
> +
> +	return videobuf_streamoff(cx18_vb_queue(id));
> +}
> +
> +static int cx18_reqbufs(struct file *file, void *priv,
> +	struct v4l2_requestbuffers *rb)
> +{
> +	struct cx18_open_id *id = file->private_data;
> +
> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> +		return -EINVAL;
> +
> +	return videobuf_reqbufs(cx18_vb_queue(id), rb);
> +}
> +
> +static int cx18_querybuf(struct file *file, void *priv,
> +	struct v4l2_buffer *b)
> +{
> +	struct cx18_open_id *id = file->private_data;
> +
> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> +		return -EINVAL;
> +
> +	return videobuf_querybuf(cx18_vb_queue(id), b);
> +}
> +
> +static int cx18_qbuf(struct file *file, void *priv, struct v4l2_buffer *b)
> +{
> +	struct cx18_open_id *id = file->private_data;
> +
> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> +		return -EINVAL;
> +
> +	return videobuf_qbuf(cx18_vb_queue(id), b);
> +}
> +
> +static int cx18_dqbuf(struct file *file, void *priv, struct v4l2_buffer *b)
> +{
> +	struct cx18_open_id *id = file->private_data;
> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> +		return -EINVAL;
> +
> +	return videobuf_dqbuf(cx18_vb_queue(id), b, file->f_flags & O_NONBLOCK);
> +}
> +
>  static int cx18_encoder_cmd(struct file *file, void *fh,
>  				struct v4l2_encoder_cmd *enc)
>  {
> @@ -1081,6 +1187,12 @@ static const struct v4l2_ioctl_ops cx18_ioctl_ops = {
>  	.vidioc_s_register              = cx18_s_register,
>  #endif
>  	.vidioc_default                 = cx18_default,
> +	.vidioc_streamon                = cx18_streamon,
> +	.vidioc_streamoff               = cx18_streamoff,
> +	.vidioc_reqbufs                 = cx18_reqbufs,
> +	.vidioc_querybuf                = cx18_querybuf,
> +	.vidioc_qbuf                    = cx18_qbuf,
> +	.vidioc_dqbuf                   = cx18_dqbuf,
>  };
>  
>  void cx18_set_funcs(struct video_device *vdev)
> diff --git a/drivers/media/video/cx18/cx18-mailbox.c b/drivers/media/video/cx18/cx18-mailbox.c
> index 9605d54..d4d8873 100644
> --- a/drivers/media/video/cx18/cx18-mailbox.c
> +++ b/drivers/media/video/cx18/cx18-mailbox.c
> @@ -81,6 +81,7 @@ static const struct cx18_api_info api_info[] = {
>  	API_ENTRY(CPU, CX18_CPU_SET_SLICED_VBI_PARAM,           0),
>  	API_ENTRY(CPU, CX18_CPU_SET_USERDATA_PLACE_HOLDER,      0),
>  	API_ENTRY(CPU, CX18_CPU_GET_ENC_PTS,                    0),
> +	API_ENTRY(CPU, CX18_CPU_SET_VFC_PARAM,                  0),
>  	API_ENTRY(CPU, CX18_CPU_DE_SET_MDL_ACK,			0),
>  	API_ENTRY(CPU, CX18_CPU_DE_SET_MDL,			API_FAST),
>  	API_ENTRY(CPU, CX18_CPU_DE_RELEASE_MDL,			API_SLOW),
> @@ -158,6 +159,72 @@ static void cx18_mdl_send_to_dvb(struct cx18_stream *s, struct cx18_mdl *mdl)
>  	}
>  }
>  
> +static void cx18_mdl_send_to_videobuf(struct cx18_stream *s,
> +	struct cx18_mdl *mdl)
> +{
> +	struct cx18_videobuf_buffer *vb_buf;
> +	struct cx18_buffer *buf;
> +	u8 *p, u;
> +	u32 offset = 0;
> +	int dispatch = 0;
> +	int i;
> +
> +	if (mdl->bytesused == 0)
> +		return;
> +
> +	/* Acquire a videobuf buffer, clone to and and release it */
> +	spin_lock(&s->vb_lock);
> +	if (list_empty(&s->vb_capture))
> +		goto out;
> +
> +	vb_buf = list_entry(s->vb_capture.next, struct cx18_videobuf_buffer,
> +		vb.queue);
> +
> +	p = videobuf_to_vmalloc(&vb_buf->vb);
> +	if (!p)
> +		goto out;
> +
> +	offset = vb_buf->bytes_used;
> +	list_for_each_entry(buf, &mdl->buf_list, list) {
> +		if (buf->bytesused == 0)
> +			break;
> +
> +		if ((offset + buf->bytesused) <= vb_buf->vb.bsize) {
> +			memcpy(p + offset, buf->buf, buf->bytesused);
> +			offset += buf->bytesused;
> +			vb_buf->bytes_used += buf->bytesused;
> +		}
> +	}
> +
> +	/* If we've filled the buffer as per the callers res then dispatch it */
> +	if (vb_buf->bytes_used >= (vb_buf->vb.width * vb_buf->vb.height * 2)) {
> +		dispatch = 1;
> +		vb_buf->bytes_used = 0;
> +	}
> +
> +	/* */
> +	if (dispatch) {
> +
> +		if (s->pixelformat == V4L2_PIX_FMT_YUYV) {
> +			/* UYVY to YUYV */
> +			for (i = 0; i < (720 * 480 * 2); i += 2) {
> +				u = *(p + i);
> +				*(p + i) = *(p + i + 1);
> +				*(p + i + 1) = u;
> +			}
> +		}
> +
> +		do_gettimeofday(&vb_buf->vb.ts);
> +		list_del(&vb_buf->vb.queue);
> +		vb_buf->vb.state = VIDEOBUF_DONE;
> +		wake_up(&vb_buf->vb.done);
> +	}
> +
> +	mod_timer(&s->vb_timeout, jiffies + (HZ / 10));
> +
> +out:
> +	spin_unlock(&s->vb_lock);
> +}
>  
>  static void cx18_mdl_send_to_alsa(struct cx18 *cx, struct cx18_stream *s,
>  				  struct cx18_mdl *mdl)
> @@ -263,6 +330,9 @@ static void epu_dma_done(struct cx18 *cx, struct cx18_in_work_order *order)
>  			} else {
>  				cx18_enqueue(s, mdl, &s->q_full);
>  			}
> +		} else if (s->type == CX18_ENC_STREAM_TYPE_YUV) {
> +			cx18_mdl_send_to_videobuf(s, mdl);
> +			cx18_enqueue(s, mdl, &s->q_free);
>  		} else {
>  			cx18_enqueue(s, mdl, &s->q_full);
>  			if (s->type == CX18_ENC_STREAM_TYPE_IDX)
> diff --git a/drivers/media/video/cx18/cx18-streams.c b/drivers/media/video/cx18/cx18-streams.c
> index c6e2ca3..53f5e4f 100644
> --- a/drivers/media/video/cx18/cx18-streams.c
> +++ b/drivers/media/video/cx18/cx18-streams.c
> @@ -44,6 +44,7 @@ static struct v4l2_file_operations cx18_v4l2_enc_fops = {
>  	.unlocked_ioctl = cx18_v4l2_ioctl,
>  	.release = cx18_v4l2_close,
>  	.poll = cx18_v4l2_enc_poll,
> +	.mmap = cx18_v4l2_mmap,
>  };
>  
>  /* offset from 0 to register ts v4l2 minors on */
> @@ -132,6 +133,15 @@ static void cx18_stream_init(struct cx18 *cx, int type)
>  	cx18_queue_init(&s->q_idle);
>  
>  	INIT_WORK(&s->out_work_order, cx18_out_work_handler);
> +
> +	INIT_LIST_HEAD(&s->vb_capture);
> +	s->vb_timeout.function = cx18_vb_timeout;
> +	s->vb_timeout.data = (unsigned long)s;
> +	init_timer(&s->vb_timeout);
> +	spin_lock_init(&s->vb_lock);
> +
> +	/* Assume the previous pixel default */
> +	s->pixelformat = V4L2_PIX_FMT_HM12;
>  }
>  
>  static int cx18_prep_dev(struct cx18 *cx, int type)
> @@ -721,6 +731,19 @@ int cx18_start_v4l2_encode_stream(struct cx18_stream *s)
>  		    test_bit(CX18_F_I_RADIO_USER, &cx->i_flags))
>  			cx18_vapi(cx, CX18_CPU_SET_VIDEO_MUTE, 2, s->handle,
>  			  (v4l2_ctrl_g_ctrl(cx->cxhdl.video_mute_yuv) << 8) | 1);
> +
> +		/* Enable the Video Format Converter for UYVY 4:2:2 support,
> +		 * rather than the default HM12 Macroblovk 4:2:0 support.
> +		 */
> +		if (captype == CAPTURE_CHANNEL_TYPE_YUV) {
> +			if (s->pixelformat == V4L2_PIX_FMT_YUYV)
> +				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
> +					s->handle, 1);
> +			else
> +				/* If in doubt, default to HM12 */
> +				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
> +					s->handle, 0);
> +		}
>  	}
>  
>  	if (atomic_read(&cx->tot_capturing) == 0) {
> diff --git a/drivers/media/video/cx18/cx23418.h b/drivers/media/video/cx18/cx23418.h
> index 935f557..767a8d2 100644
> --- a/drivers/media/video/cx18/cx23418.h
> +++ b/drivers/media/video/cx18/cx23418.h
> @@ -342,6 +342,12 @@
>     ReturnCode */
>  #define CX18_CPU_GET_ENC_PTS			(CPU_CMD_MASK_CAPTURE | 0x0022)
>  
> +/* Description: Set VFC parameters
> +   IN[0] - task handle
> +   IN[1] - VFC enable flag, 1 - enable, 0 - disable
> +*/
> +#define CX18_CPU_SET_VFC_PARAM                  (CPU_CMD_MASK_CAPTURE | 0x0023)
> +
>  /* Below is the list of commands related to the data exchange */
>  #define CPU_CMD_MASK_DE 			(CPU_CMD_MASK | 0x040000)
>  
> 
> _______________________________________________
> linuxtv-commits mailing list
> linuxtv-commits@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits
> 
> 

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Devin Heitmueller]

On Mon, May 2, 2011 at 3:11 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> NACK.
>
> For two reasons: first of all it is not signed off by Andy Walls, the cx18
> maintainer. I know he has had other things on his plate recently which is
> probably why he hasn't had the chance to review this.
>
> Secondly, while doing a quick scan myself I noticed that this code does a
> conversion from UYVY format to YUYV *in the driver*. Format conversion is
> not allowed in the kernel, we have libv4lconvert for that. So at the minimum
> this conversion code must be removed first.

Hi Hans,

Cutting the code that does UYVY to YUYV shouldn't be a problem, since
there are other devices which only support UYVY and thus applications
do support the format (the HVR-950q for one).  Should just need to
remove the offending code block and adjust the advertised formats
list.

That said, Andy hasn't provided any feedback onlist at all, which is a
bit disconcerting (and probably calls for "why won't Andy comment?"
instead of an arbitrary NACK).

I did speak to Andy about this patch series several months ago, and he
was generally not in favor of it because he was planning on converting
to videobuf2.  While I agree this would be good in the long term, this
patch provides a great deal of value in the meantime, and I've always
been a fan of the notion that "perfect is the enemy of good".  Who
knows when we'll actually see a videobuf2 conversion, and this patch
doesn't really prevent any of that from happening.

I would hate to see yet another situation where a solution stays
out-of-tree for years because of some totally awesome better approach
which might possibly get integrated at some unknown point in the
future.

In other words, let's get this merged in (sans the UYVY/YUYV
conversion), and if/when Andy eventually does a videobuf2 conversion,
then we will all rejoice.  Actually, nobody except us driver
developers will rejoice since it's an internal architecture change
which provides no user-visible value.

Devin

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

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Mauro Carvalho Chehab]

Em 02-05-2011 16:11, Hans Verkuil escreveu:
> NACK.
> 
> For two reasons: first of all it is not signed off by Andy Walls, the cx18
> maintainer. I know he has had other things on his plate recently which is
> probably why he hasn't had the chance to review this.
> 
> Secondly, while doing a quick scan myself I noticed that this code does a
> conversion from UYVY format to YUYV *in the driver*. Format conversion is
> not allowed in the kernel, we have libv4lconvert for that. So at the minimum
> this conversion code must be removed first.

Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or
andy were against it, why none of you commented it there?

Now that the patch were committed, I won't revert it without a very good reason.

With respect to the "conversion from UYVY format to YUYV", a simple patch could
fix it, instead of removing the entire patchset.

Steven/Simon,
could you please work on such change?

Thanks,
Mauro.

> 
> Regards,
> 
> 	Hans
> 
> On Monday, May 02, 2011 19:17:57 Mauro Carvalho Chehab wrote:
>> This is an automatic generated email to let you know that the following patch were queued at the 
>> http://git.linuxtv.org/media_tree.git tree:
>>
>> Subject: [media] cx18: mmap() support for raw YUV video capture
>> Author:  Steven Toth <stoth@kernellabs.com>
>> Date:    Wed Apr 6 08:32:56 2011 -0300
>>
>> Add support for mmap method streaming of raw YUV video on cx18-based
>> hardware, in addition to the existing support for read() streaming of
>> raw YUV and MPEG-2 encoded video.
>>
>> [simon.farnsworth@onelan.co.uk: I forward-ported this from Steven's original work,
>>  done under contract to ONELAN. The original code is at
>>  http://www.kernellabs.com/hg/~stoth/cx18-videobuf]
>>
>> Signed-off-by: Steven Toth <stoth@kernellabs.com>
>> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>
>>  drivers/media/video/cx18/Kconfig        |    2 +
>>  drivers/media/video/cx18/cx18-driver.h  |   25 ++++
>>  drivers/media/video/cx18/cx18-fileops.c |  214 +++++++++++++++++++++++++++++++
>>  drivers/media/video/cx18/cx18-fileops.h |    2 +
>>  drivers/media/video/cx18/cx18-ioctl.c   |  136 ++++++++++++++++++--
>>  drivers/media/video/cx18/cx18-mailbox.c |   70 ++++++++++
>>  drivers/media/video/cx18/cx18-streams.c |   23 ++++
>>  drivers/media/video/cx18/cx23418.h      |    6 +
>>  8 files changed, 466 insertions(+), 12 deletions(-)
>>
>> ---
>>
>> http://git.linuxtv.org/media_tree.git?a=commitdiff;h=fa8f1381764d83222333cb67b8d93b9cb1605bf3
>>
>> diff --git a/drivers/media/video/cx18/Kconfig b/drivers/media/video/cx18/Kconfig
>> index d788ad6..9c23202 100644
>> --- a/drivers/media/video/cx18/Kconfig
>> +++ b/drivers/media/video/cx18/Kconfig
>> @@ -2,6 +2,8 @@ config VIDEO_CX18
>>  	tristate "Conexant cx23418 MPEG encoder support"
>>  	depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
>>  	select I2C_ALGOBIT
>> +	select VIDEOBUF_DVB
>> +	select VIDEOBUF_VMALLOC
>>  	depends on RC_CORE
>>  	select VIDEO_TUNER
>>  	select VIDEO_TVEEPROM
>> diff --git a/drivers/media/video/cx18/cx18-driver.h b/drivers/media/video/cx18/cx18-driver.h
>> index b86a740..70e1e04 100644
>> --- a/drivers/media/video/cx18/cx18-driver.h
>> +++ b/drivers/media/video/cx18/cx18-driver.h
>> @@ -65,6 +65,10 @@
>>  #include "dvb_net.h"
>>  #include "dvbdev.h"
>>  
>> +/* Videobuf / YUV support */
>> +#include <media/videobuf-core.h>
>> +#include <media/videobuf-vmalloc.h>
>> +
>>  #ifndef CONFIG_PCI
>>  #  error "This driver requires kernel PCI support."
>>  #endif
>> @@ -403,6 +407,23 @@ struct cx18_stream {
>>  	struct cx18_queue q_idle;	/* idle - not in rotation */
>>  
>>  	struct work_struct out_work_order;
>> +
>> +	/* Videobuf for YUV video */
>> +	u32 pixelformat;
>> +	struct list_head vb_capture;    /* video capture queue */
>> +	spinlock_t vb_lock;
>> +	struct v4l2_framebuffer fbuf;
>> +	v4l2_std_id tvnorm; /* selected tv norm */
>> +	struct timer_list vb_timeout;
>> +	int vbwidth;
>> +	int vbheight;
>> +};
>> +
>> +struct cx18_videobuf_buffer {
>> +	/* Common video buffer sub-system struct */
>> +	struct videobuf_buffer vb;
>> +	v4l2_std_id tvnorm; /* selected tv norm */
>> +	u32 bytes_used;
>>  };
>>  
>>  struct cx18_open_id {
>> @@ -410,6 +431,10 @@ struct cx18_open_id {
>>  	u32 open_id;
>>  	int type;
>>  	struct cx18 *cx;
>> +
>> +	struct videobuf_queue vbuf_q;
>> +	spinlock_t s_lock; /* Protect vbuf_q */
>> +	enum v4l2_buf_type vb_type;
>>  };
>>  
>>  static inline struct cx18_open_id *fh2id(struct v4l2_fh *fh)
>> diff --git a/drivers/media/video/cx18/cx18-fileops.c b/drivers/media/video/cx18/cx18-fileops.c
>> index e9802d9..c74eafd 100644
>> --- a/drivers/media/video/cx18/cx18-fileops.c
>> +++ b/drivers/media/video/cx18/cx18-fileops.c
>> @@ -597,6 +597,13 @@ ssize_t cx18_v4l2_read(struct file *filp, char __user *buf, size_t count,
>>  	mutex_unlock(&cx->serialize_lock);
>>  	if (rc)
>>  		return rc;
>> +
>> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
>> +		return videobuf_read_stream(&id->vbuf_q, buf, count, pos, 0,
>> +			filp->f_flags & O_NONBLOCK);
>> +	}
>> +
>>  	return cx18_read_pos(s, buf, count, pos, filp->f_flags & O_NONBLOCK);
>>  }
>>  
>> @@ -622,6 +629,11 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp, poll_table *wait)
>>  		CX18_DEBUG_FILE("Encoder poll started capture\n");
>>  	}
>>  
>> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
>> +		return videobuf_poll_stream(filp, &id->vbuf_q, wait);
>> +	}
>> +
>>  	/* add stream's waitq to the poll list */
>>  	CX18_DEBUG_HI_FILE("Encoder poll\n");
>>  	poll_wait(filp, &s->waitq, wait);
>> @@ -633,6 +645,58 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp, poll_table *wait)
>>  	return 0;
>>  }
>>  
>> +int cx18_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> +	struct cx18_open_id *id = file->private_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>> +	int eof = test_bit(CX18_F_S_STREAMOFF, &s->s_flags);
>> +
>> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
>> +
>> +		/* Start a capture if there is none */
>> +		if (!eof && !test_bit(CX18_F_S_STREAMING, &s->s_flags)) {
>> +			int rc;
>> +
>> +			mutex_lock(&cx->serialize_lock);
>> +			rc = cx18_start_capture(id);
>> +			mutex_unlock(&cx->serialize_lock);
>> +			if (rc) {
>> +				CX18_DEBUG_INFO(
>> +					"Could not start capture for %s (%d)\n",
>> +					s->name, rc);
>> +				return -EINVAL;
>> +			}
>> +			CX18_DEBUG_FILE("Encoder poll started capture\n");
>> +		}
>> +
>> +		return videobuf_mmap_mapper(&id->vbuf_q, vma);
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +void cx18_vb_timeout(unsigned long data)
>> +{
>> +	struct cx18_stream *s = (struct cx18_stream *)data;
>> +	struct cx18_videobuf_buffer *buf;
>> +	unsigned long flags;
>> +
>> +	/* Return all of the buffers in error state, so the vbi/vid inode
>> +	 * can return from blocking.
>> +	 */
>> +	spin_lock_irqsave(&s->vb_lock, flags);
>> +	while (!list_empty(&s->vb_capture)) {
>> +		buf = list_entry(s->vb_capture.next,
>> +			struct cx18_videobuf_buffer, vb.queue);
>> +		list_del(&buf->vb.queue);
>> +		buf->vb.state = VIDEOBUF_ERROR;
>> +		wake_up(&buf->vb.done);
>> +	}
>> +	spin_unlock_irqrestore(&s->vb_lock, flags);
>> +}
>> +
>>  void cx18_stop_capture(struct cx18_open_id *id, int gop_end)
>>  {
>>  	struct cx18 *cx = id->cx;
>> @@ -716,12 +780,150 @@ int cx18_v4l2_close(struct file *filp)
>>  		cx18_release_stream(s);
>>  	} else {
>>  		cx18_stop_capture(id, 0);
>> +		if (id->type == CX18_ENC_STREAM_TYPE_YUV)
>> +			videobuf_mmap_free(&id->vbuf_q);
>>  	}
>>  	kfree(id);
>>  	mutex_unlock(&cx->serialize_lock);
>>  	return 0;
>>  }
>>  
>> +void cx18_dma_free(struct videobuf_queue *q,
>> +	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
>> +{
>> +	videobuf_waiton(q, &buf->vb, 0, 0);
>> +	videobuf_vmalloc_free(&buf->vb);
>> +	buf->vb.state = VIDEOBUF_NEEDS_INIT;
>> +}
>> +
>> +static int cx18_prepare_buffer(struct videobuf_queue *q,
>> +	struct cx18_stream *s,
>> +	struct cx18_videobuf_buffer *buf,
>> +	u32 pixelformat,
>> +	unsigned int width, unsigned int height,
>> +	enum v4l2_field field)
>> +{
>> +	int rc = 0;
>> +
>> +	/* check settings */
>> +	buf->bytes_used = 0;
>> +
>> +	if ((width  < 48) || (height < 32))
>> +		return -EINVAL;
>> +
>> +	buf->vb.size = (width * height * 16 /*fmt->depth*/) >> 3;
>> +	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>> +		return -EINVAL;
>> +
>> +	/* alloc + fill struct (if changed) */
>> +	if (buf->vb.width != width || buf->vb.height != height ||
>> +	    buf->vb.field != field || s->pixelformat != pixelformat ||
>> +	    buf->tvnorm != s->tvnorm) {
>> +
>> +		buf->vb.width  = width;
>> +		buf->vb.height = height;
>> +		buf->vb.field  = field;
>> +		buf->tvnorm    = s->tvnorm;
>> +		s->pixelformat = pixelformat;
>> +
>> +		cx18_dma_free(q, s, buf);
>> +	}
>> +
>> +	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>> +		return -EINVAL;
>> +
>> +	if (buf->vb.field == 0)
>> +		buf->vb.field = V4L2_FIELD_INTERLACED;
>> +
>> +	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
>> +		buf->vb.width  = width;
>> +		buf->vb.height = height;
>> +		buf->vb.field  = field;
>> +		buf->tvnorm    = s->tvnorm;
>> +		s->pixelformat = pixelformat;
>> +
>> +		rc = videobuf_iolock(q, &buf->vb, &s->fbuf);
>> +		if (rc != 0)
>> +			goto fail;
>> +	}
>> +	buf->vb.state = VIDEOBUF_PREPARED;
>> +	return 0;
>> +
>> +fail:
>> +	cx18_dma_free(q, s, buf);
>> +	return rc;
>> +
>> +}
>> +
>> +#define VB_MIN_BUFFERS 32
>> +#define VB_MIN_BUFSIZE 0x208000
>> +
>> +static int buffer_setup(struct videobuf_queue *q,
>> +	unsigned int *count, unsigned int *size)
>> +{
>> +	struct cx18_open_id *id = q->priv_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>> +
>> +	*size = 2 * s->vbwidth * s->vbheight;
>> +	if (*count == 0)
>> +		*count = VB_MIN_BUFFERS;
>> +
>> +	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
>> +		(*count)--;
>> +
>> +	q->field = V4L2_FIELD_INTERLACED;
>> +	q->last = V4L2_FIELD_INTERLACED;
>> +
>> +	return 0;
>> +}
>> +
>> +static int buffer_prepare(struct videobuf_queue *q,
>> +	struct videobuf_buffer *vb,
>> +	enum v4l2_field field)
>> +{
>> +	struct cx18_videobuf_buffer *buf =
>> +		container_of(vb, struct cx18_videobuf_buffer, vb);
>> +	struct cx18_open_id *id = q->priv_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>> +
>> +	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
>> +		s->vbwidth, s->vbheight, field);
>> +}
>> +
>> +static void buffer_release(struct videobuf_queue *q,
>> +	struct videobuf_buffer *vb)
>> +{
>> +	struct cx18_videobuf_buffer *buf =
>> +		container_of(vb, struct cx18_videobuf_buffer, vb);
>> +	struct cx18_open_id *id = q->priv_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>> +
>> +	cx18_dma_free(q, s, buf);
>> +}
>> +
>> +static void buffer_queue(struct videobuf_queue *q, struct videobuf_buffer *vb)
>> +{
>> +	struct cx18_videobuf_buffer *buf =
>> +		container_of(vb, struct cx18_videobuf_buffer, vb);
>> +	struct cx18_open_id *id = q->priv_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>> +
>> +	buf->vb.state = VIDEOBUF_QUEUED;
>> +
>> +	list_add_tail(&buf->vb.queue, &s->vb_capture);
>> +}
>> +
>> +static struct videobuf_queue_ops cx18_videobuf_qops = {
>> +	.buf_setup    = buffer_setup,
>> +	.buf_prepare  = buffer_prepare,
>> +	.buf_queue    = buffer_queue,
>> +	.buf_release  = buffer_release,
>> +};
>> +
>>  static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
>>  {
>>  	struct cx18 *cx = s->cx;
>> @@ -740,6 +942,9 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
>>  	item->cx = cx;
>>  	item->type = s->type;
>>  
>> +	spin_lock_init(&item->s_lock);
>> +	item->vb_type = 0;
>> +
>>  	item->open_id = cx->open_id++;
>>  	filp->private_data = &item->fh;
>>  
>> @@ -774,6 +979,15 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
>>  		/* Done! Unmute and continue. */
>>  		cx18_unmute(cx);
>>  	}
>> +	if (item->type == CX18_ENC_STREAM_TYPE_YUV) {
>> +		item->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +		videobuf_queue_vmalloc_init(&item->vbuf_q, &cx18_videobuf_qops,
>> +			&cx->pci_dev->dev, &item->s_lock,
>> +			V4L2_BUF_TYPE_VIDEO_CAPTURE,
>> +			V4L2_FIELD_INTERLACED,
>> +			sizeof(struct cx18_videobuf_buffer),
>> +			item, &cx->serialize_lock);
>> +	}
>>  	v4l2_fh_add(&item->fh);
>>  	return 0;
>>  }
>> diff --git a/drivers/media/video/cx18/cx18-fileops.h b/drivers/media/video/cx18/cx18-fileops.h
>> index 5c8fcb8..b9e5110 100644
>> --- a/drivers/media/video/cx18/cx18-fileops.h
>> +++ b/drivers/media/video/cx18/cx18-fileops.h
>> @@ -33,6 +33,8 @@ int cx18_start_capture(struct cx18_open_id *id);
>>  void cx18_stop_capture(struct cx18_open_id *id, int gop_end);
>>  void cx18_mute(struct cx18 *cx);
>>  void cx18_unmute(struct cx18 *cx);
>> +int cx18_v4l2_mmap(struct file *file, struct vm_area_struct *vma);
>> +void cx18_vb_timeout(unsigned long data);
>>  
>>  /* Shared with cx18-alsa module */
>>  int cx18_claim_stream(struct cx18_open_id *id, int type);
>> diff --git a/drivers/media/video/cx18/cx18-ioctl.c b/drivers/media/video/cx18/cx18-ioctl.c
>> index 4f041c0..777d726 100644
>> --- a/drivers/media/video/cx18/cx18-ioctl.c
>> +++ b/drivers/media/video/cx18/cx18-ioctl.c
>> @@ -41,6 +41,18 @@
>>  #include <media/tveeprom.h>
>>  #include <media/v4l2-chip-ident.h>
>>  
>> +static struct v4l2_fmtdesc formats[] = {
>> +	{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>> +	  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
>> +	},
>> +	{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
>> +	  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
>> +	},
>> +	{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>> +	  "YUYV 4:2:2", V4L2_PIX_FMT_YUYV, { 0, 0, 0, 0 }
>> +	},
>> +};
>> +
>>  u16 cx18_service2vbi(int type)
>>  {
>>  	switch (type) {
>> @@ -150,6 +162,7 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh,
>>  {
>>  	struct cx18_open_id *id = fh2id(fh);
>>  	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>>  	struct v4l2_pix_format *pixfmt = &fmt->fmt.pix;
>>  
>>  	pixfmt->width = cx->cxhdl.width;
>> @@ -158,7 +171,7 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh,
>>  	pixfmt->field = V4L2_FIELD_INTERLACED;
>>  	pixfmt->priv = 0;
>>  	if (id->type == CX18_ENC_STREAM_TYPE_YUV) {
>> -		pixfmt->pixelformat = V4L2_PIX_FMT_HM12;
>> +		pixfmt->pixelformat = s->pixelformat;
>>  		/* YUV size is (Y=(h*720) + UV=(h*(720/2))) */
>>  		pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
>>  		pixfmt->bytesperline = 720;
>> @@ -237,7 +250,6 @@ static int cx18_try_fmt_vid_cap(struct file *file, void *fh,
>>  	h = min(h, cx->is_50hz ? 576 : 480);
>>  	h = max(h, min_h);
>>  
>> -	cx18_g_fmt_vid_cap(file, fh, fmt);
>>  	fmt->fmt.pix.width = w;
>>  	fmt->fmt.pix.height = h;
>>  	return 0;
>> @@ -274,6 +286,7 @@ static int cx18_s_fmt_vid_cap(struct file *file, void *fh,
>>  	struct cx18_open_id *id = fh2id(fh);
>>  	struct cx18 *cx = id->cx;
>>  	struct v4l2_mbus_framefmt mbus_fmt;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>>  	int ret;
>>  	int w, h;
>>  
>> @@ -283,6 +296,10 @@ static int cx18_s_fmt_vid_cap(struct file *file, void *fh,
>>  	w = fmt->fmt.pix.width;
>>  	h = fmt->fmt.pix.height;
>>  
>> +	s->pixelformat = fmt->fmt.pix.pixelformat;
>> +	s->vbheight = h;
>> +	s->vbwidth = w;
>> +
>>  	if (cx->cxhdl.width == w && cx->cxhdl.height == h)
>>  		return 0;
>>  
>> @@ -540,16 +557,7 @@ static int cx18_g_crop(struct file *file, void *fh, struct v4l2_crop *crop)
>>  static int cx18_enum_fmt_vid_cap(struct file *file, void *fh,
>>  					struct v4l2_fmtdesc *fmt)
>>  {
>> -	static struct v4l2_fmtdesc formats[] = {
>> -		{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>> -		  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
>> -		},
>> -		{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
>> -		  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
>> -		}
>> -	};
>> -
>> -	if (fmt->index > 1)
>> +	if (fmt->index > ARRAY_SIZE(formats) - 1)
>>  		return -EINVAL;
>>  	*fmt = formats[fmt->index];
>>  	return 0;
>> @@ -863,6 +871,104 @@ static int cx18_g_enc_index(struct file *file, void *fh,
>>  	return 0;
>>  }
>>  
>> +static struct videobuf_queue *cx18_vb_queue(struct cx18_open_id *id)
>> +{
>> +	struct videobuf_queue *q = NULL;
>> +
>> +	switch (id->vb_type) {
>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>> +		q = &id->vbuf_q;
>> +		break;
>> +	case V4L2_BUF_TYPE_VBI_CAPTURE:
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +	return q;
>> +}
>> +
>> +static int cx18_streamon(struct file *file, void *priv,
>> +	enum v4l2_buf_type type)
>> +{
>> +	struct cx18_open_id *id = file->private_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>> +
>> +	/* Start the hardware only if we're the video device */
>> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +		return -EINVAL;
>> +
>> +	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
>> +		return -EINVAL;
>> +
>> +	/* Establish a buffer timeout */
>> +	mod_timer(&s->vb_timeout, jiffies + (HZ * 2));
>> +
>> +	return videobuf_streamon(cx18_vb_queue(id));
>> +}
>> +
>> +static int cx18_streamoff(struct file *file, void *priv,
>> +	enum v4l2_buf_type type)
>> +{
>> +	struct cx18_open_id *id = file->private_data;
>> +
>> +	/* Start the hardware only if we're the video device */
>> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +		return -EINVAL;
>> +
>> +	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
>> +		return -EINVAL;
>> +
>> +	return videobuf_streamoff(cx18_vb_queue(id));
>> +}
>> +
>> +static int cx18_reqbufs(struct file *file, void *priv,
>> +	struct v4l2_requestbuffers *rb)
>> +{
>> +	struct cx18_open_id *id = file->private_data;
>> +
>> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +		return -EINVAL;
>> +
>> +	return videobuf_reqbufs(cx18_vb_queue(id), rb);
>> +}
>> +
>> +static int cx18_querybuf(struct file *file, void *priv,
>> +	struct v4l2_buffer *b)
>> +{
>> +	struct cx18_open_id *id = file->private_data;
>> +
>> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +		return -EINVAL;
>> +
>> +	return videobuf_querybuf(cx18_vb_queue(id), b);
>> +}
>> +
>> +static int cx18_qbuf(struct file *file, void *priv, struct v4l2_buffer *b)
>> +{
>> +	struct cx18_open_id *id = file->private_data;
>> +
>> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +		return -EINVAL;
>> +
>> +	return videobuf_qbuf(cx18_vb_queue(id), b);
>> +}
>> +
>> +static int cx18_dqbuf(struct file *file, void *priv, struct v4l2_buffer *b)
>> +{
>> +	struct cx18_open_id *id = file->private_data;
>> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +		return -EINVAL;
>> +
>> +	return videobuf_dqbuf(cx18_vb_queue(id), b, file->f_flags & O_NONBLOCK);
>> +}
>> +
>>  static int cx18_encoder_cmd(struct file *file, void *fh,
>>  				struct v4l2_encoder_cmd *enc)
>>  {
>> @@ -1081,6 +1187,12 @@ static const struct v4l2_ioctl_ops cx18_ioctl_ops = {
>>  	.vidioc_s_register              = cx18_s_register,
>>  #endif
>>  	.vidioc_default                 = cx18_default,
>> +	.vidioc_streamon                = cx18_streamon,
>> +	.vidioc_streamoff               = cx18_streamoff,
>> +	.vidioc_reqbufs                 = cx18_reqbufs,
>> +	.vidioc_querybuf                = cx18_querybuf,
>> +	.vidioc_qbuf                    = cx18_qbuf,
>> +	.vidioc_dqbuf                   = cx18_dqbuf,
>>  };
>>  
>>  void cx18_set_funcs(struct video_device *vdev)
>> diff --git a/drivers/media/video/cx18/cx18-mailbox.c b/drivers/media/video/cx18/cx18-mailbox.c
>> index 9605d54..d4d8873 100644
>> --- a/drivers/media/video/cx18/cx18-mailbox.c
>> +++ b/drivers/media/video/cx18/cx18-mailbox.c
>> @@ -81,6 +81,7 @@ static const struct cx18_api_info api_info[] = {
>>  	API_ENTRY(CPU, CX18_CPU_SET_SLICED_VBI_PARAM,           0),
>>  	API_ENTRY(CPU, CX18_CPU_SET_USERDATA_PLACE_HOLDER,      0),
>>  	API_ENTRY(CPU, CX18_CPU_GET_ENC_PTS,                    0),
>> +	API_ENTRY(CPU, CX18_CPU_SET_VFC_PARAM,                  0),
>>  	API_ENTRY(CPU, CX18_CPU_DE_SET_MDL_ACK,			0),
>>  	API_ENTRY(CPU, CX18_CPU_DE_SET_MDL,			API_FAST),
>>  	API_ENTRY(CPU, CX18_CPU_DE_RELEASE_MDL,			API_SLOW),
>> @@ -158,6 +159,72 @@ static void cx18_mdl_send_to_dvb(struct cx18_stream *s, struct cx18_mdl *mdl)
>>  	}
>>  }
>>  
>> +static void cx18_mdl_send_to_videobuf(struct cx18_stream *s,
>> +	struct cx18_mdl *mdl)
>> +{
>> +	struct cx18_videobuf_buffer *vb_buf;
>> +	struct cx18_buffer *buf;
>> +	u8 *p, u;
>> +	u32 offset = 0;
>> +	int dispatch = 0;
>> +	int i;
>> +
>> +	if (mdl->bytesused == 0)
>> +		return;
>> +
>> +	/* Acquire a videobuf buffer, clone to and and release it */
>> +	spin_lock(&s->vb_lock);
>> +	if (list_empty(&s->vb_capture))
>> +		goto out;
>> +
>> +	vb_buf = list_entry(s->vb_capture.next, struct cx18_videobuf_buffer,
>> +		vb.queue);
>> +
>> +	p = videobuf_to_vmalloc(&vb_buf->vb);
>> +	if (!p)
>> +		goto out;
>> +
>> +	offset = vb_buf->bytes_used;
>> +	list_for_each_entry(buf, &mdl->buf_list, list) {
>> +		if (buf->bytesused == 0)
>> +			break;
>> +
>> +		if ((offset + buf->bytesused) <= vb_buf->vb.bsize) {
>> +			memcpy(p + offset, buf->buf, buf->bytesused);
>> +			offset += buf->bytesused;
>> +			vb_buf->bytes_used += buf->bytesused;
>> +		}
>> +	}
>> +
>> +	/* If we've filled the buffer as per the callers res then dispatch it */
>> +	if (vb_buf->bytes_used >= (vb_buf->vb.width * vb_buf->vb.height * 2)) {
>> +		dispatch = 1;
>> +		vb_buf->bytes_used = 0;
>> +	}
>> +
>> +	/* */
>> +	if (dispatch) {
>> +
>> +		if (s->pixelformat == V4L2_PIX_FMT_YUYV) {
>> +			/* UYVY to YUYV */
>> +			for (i = 0; i < (720 * 480 * 2); i += 2) {
>> +				u = *(p + i);
>> +				*(p + i) = *(p + i + 1);
>> +				*(p + i + 1) = u;
>> +			}
>> +		}
>> +
>> +		do_gettimeofday(&vb_buf->vb.ts);
>> +		list_del(&vb_buf->vb.queue);
>> +		vb_buf->vb.state = VIDEOBUF_DONE;
>> +		wake_up(&vb_buf->vb.done);
>> +	}
>> +
>> +	mod_timer(&s->vb_timeout, jiffies + (HZ / 10));
>> +
>> +out:
>> +	spin_unlock(&s->vb_lock);
>> +}
>>  
>>  static void cx18_mdl_send_to_alsa(struct cx18 *cx, struct cx18_stream *s,
>>  				  struct cx18_mdl *mdl)
>> @@ -263,6 +330,9 @@ static void epu_dma_done(struct cx18 *cx, struct cx18_in_work_order *order)
>>  			} else {
>>  				cx18_enqueue(s, mdl, &s->q_full);
>>  			}
>> +		} else if (s->type == CX18_ENC_STREAM_TYPE_YUV) {
>> +			cx18_mdl_send_to_videobuf(s, mdl);
>> +			cx18_enqueue(s, mdl, &s->q_free);
>>  		} else {
>>  			cx18_enqueue(s, mdl, &s->q_full);
>>  			if (s->type == CX18_ENC_STREAM_TYPE_IDX)
>> diff --git a/drivers/media/video/cx18/cx18-streams.c b/drivers/media/video/cx18/cx18-streams.c
>> index c6e2ca3..53f5e4f 100644
>> --- a/drivers/media/video/cx18/cx18-streams.c
>> +++ b/drivers/media/video/cx18/cx18-streams.c
>> @@ -44,6 +44,7 @@ static struct v4l2_file_operations cx18_v4l2_enc_fops = {
>>  	.unlocked_ioctl = cx18_v4l2_ioctl,
>>  	.release = cx18_v4l2_close,
>>  	.poll = cx18_v4l2_enc_poll,
>> +	.mmap = cx18_v4l2_mmap,
>>  };
>>  
>>  /* offset from 0 to register ts v4l2 minors on */
>> @@ -132,6 +133,15 @@ static void cx18_stream_init(struct cx18 *cx, int type)
>>  	cx18_queue_init(&s->q_idle);
>>  
>>  	INIT_WORK(&s->out_work_order, cx18_out_work_handler);
>> +
>> +	INIT_LIST_HEAD(&s->vb_capture);
>> +	s->vb_timeout.function = cx18_vb_timeout;
>> +	s->vb_timeout.data = (unsigned long)s;
>> +	init_timer(&s->vb_timeout);
>> +	spin_lock_init(&s->vb_lock);
>> +
>> +	/* Assume the previous pixel default */
>> +	s->pixelformat = V4L2_PIX_FMT_HM12;
>>  }
>>  
>>  static int cx18_prep_dev(struct cx18 *cx, int type)
>> @@ -721,6 +731,19 @@ int cx18_start_v4l2_encode_stream(struct cx18_stream *s)
>>  		    test_bit(CX18_F_I_RADIO_USER, &cx->i_flags))
>>  			cx18_vapi(cx, CX18_CPU_SET_VIDEO_MUTE, 2, s->handle,
>>  			  (v4l2_ctrl_g_ctrl(cx->cxhdl.video_mute_yuv) << 8) | 1);
>> +
>> +		/* Enable the Video Format Converter for UYVY 4:2:2 support,
>> +		 * rather than the default HM12 Macroblovk 4:2:0 support.
>> +		 */
>> +		if (captype == CAPTURE_CHANNEL_TYPE_YUV) {
>> +			if (s->pixelformat == V4L2_PIX_FMT_YUYV)
>> +				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
>> +					s->handle, 1);
>> +			else
>> +				/* If in doubt, default to HM12 */
>> +				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
>> +					s->handle, 0);
>> +		}
>>  	}
>>  
>>  	if (atomic_read(&cx->tot_capturing) == 0) {
>> diff --git a/drivers/media/video/cx18/cx23418.h b/drivers/media/video/cx18/cx23418.h
>> index 935f557..767a8d2 100644
>> --- a/drivers/media/video/cx18/cx23418.h
>> +++ b/drivers/media/video/cx18/cx23418.h
>> @@ -342,6 +342,12 @@
>>     ReturnCode */
>>  #define CX18_CPU_GET_ENC_PTS			(CPU_CMD_MASK_CAPTURE | 0x0022)
>>  
>> +/* Description: Set VFC parameters
>> +   IN[0] - task handle
>> +   IN[1] - VFC enable flag, 1 - enable, 0 - disable
>> +*/
>> +#define CX18_CPU_SET_VFC_PARAM                  (CPU_CMD_MASK_CAPTURE | 0x0023)
>> +
>>  /* Below is the list of commands related to the data exchange */
>>  #define CPU_CMD_MASK_DE 			(CPU_CMD_MASK | 0x040000)
>>  
>>
>> _______________________________________________
>> linuxtv-commits mailing list
>> linuxtv-commits@linuxtv.org
>> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Devin Heitmueller]

On Mon, May 2, 2011 at 3:35 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 02-05-2011 16:11, Hans Verkuil escreveu:
>> NACK.
>>
>> For two reasons: first of all it is not signed off by Andy Walls, the cx18
>> maintainer. I know he has had other things on his plate recently which is
>> probably why he hasn't had the chance to review this.
>>
>> Secondly, while doing a quick scan myself I noticed that this code does a
>> conversion from UYVY format to YUYV *in the driver*. Format conversion is
>> not allowed in the kernel, we have libv4lconvert for that. So at the minimum
>> this conversion code must be removed first.
>
> Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or
> andy were against it, why none of you commented it there?
>
> Now that the patch were committed, I won't revert it without a very good reason.
>
> With respect to the "conversion from UYVY format to YUYV", a simple patch could
> fix it, instead of removing the entire patchset.
>
> Steven/Simon,
> could you please work on such change?

Simon,

If you're willing to do a bit of work to actually prepare the patch
and test the results, I can walk you through pretty much exactly what
needs to change (basically you just need to remove one block of code
and change a #define).

Steven has been really busy with other stuff, so I don't think we
should count on his participation in this process.

Devin


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

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Hans Verkuil]

On Monday, May 02, 2011 21:35:45 Mauro Carvalho Chehab wrote:
> Em 02-05-2011 16:11, Hans Verkuil escreveu:
> > NACK.
> > 
> > For two reasons: first of all it is not signed off by Andy Walls, the cx18
> > maintainer. I know he has had other things on his plate recently which is
> > probably why he hasn't had the chance to review this.
> > 
> > Secondly, while doing a quick scan myself I noticed that this code does a
> > conversion from UYVY format to YUYV *in the driver*. Format conversion is
> > not allowed in the kernel, we have libv4lconvert for that. So at the minimum
> > this conversion code must be removed first.
> 
> Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or
> andy were against it, why none of you commented it there?

It was merged without *asking* Andy. I know he has had some private stuff to
deal with this month so I wasn't surprised that he hadn't reviewed it yet.

It would have been nice if he was reminded first of this patch. It's a
fairly substantial change that also has user-visible implications. The simple
fact is that this patch has not been reviewed and as a former cx18 maintainer
I think that it needs a review first.

If someone had asked and Andy wouldn't have been able to review, then I'd have
jumped in and would have reviewed it.

Andy, I hope you can look at it, but if not, then let me know and I'll do a
more in-depth review rather than just the simple scan I did now.

> Now that the patch were committed, I won't revert it without a very good reason.
> 
> With respect to the "conversion from UYVY format to YUYV", a simple patch could
> fix it, instead of removing the entire patchset.

No, please remove the patchset because I have found two other issues:
 
The patch adds this field:

	struct v4l2_framebuffer fbuf;

This is not needed, videobuf_iolock can be called with a NULL pointer instead
of &fbuf.

The patch also adds tvnorm fields, but never sets s->tvnorm. And it's
pointless anyway since you can't change tvnorm while streaming.

Given that I've found three things now without even trying suggests to me that
it is too soon to commit this. Sorry.

Regards,

	Hans

> 
> Thanks,
> Mauro.
> 
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > On Monday, May 02, 2011 19:17:57 Mauro Carvalho Chehab wrote:
> >> This is an automatic generated email to let you know that the following patch were queued at the 
> >> http://git.linuxtv.org/media_tree.git tree:
> >>
> >> Subject: [media] cx18: mmap() support for raw YUV video capture
> >> Author:  Steven Toth <stoth@kernellabs.com>
> >> Date:    Wed Apr 6 08:32:56 2011 -0300
> >>
> >> Add support for mmap method streaming of raw YUV video on cx18-based
> >> hardware, in addition to the existing support for read() streaming of
> >> raw YUV and MPEG-2 encoded video.
> >>
> >> [simon.farnsworth@onelan.co.uk: I forward-ported this from Steven's original work,
> >>  done under contract to ONELAN. The original code is at
> >>  http://www.kernellabs.com/hg/~stoth/cx18-videobuf]
> >>
> >> Signed-off-by: Steven Toth <stoth@kernellabs.com>
> >> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> >>
> >>  drivers/media/video/cx18/Kconfig        |    2 +
> >>  drivers/media/video/cx18/cx18-driver.h  |   25 ++++
> >>  drivers/media/video/cx18/cx18-fileops.c |  214 +++++++++++++++++++++++++++++++
> >>  drivers/media/video/cx18/cx18-fileops.h |    2 +
> >>  drivers/media/video/cx18/cx18-ioctl.c   |  136 ++++++++++++++++++--
> >>  drivers/media/video/cx18/cx18-mailbox.c |   70 ++++++++++
> >>  drivers/media/video/cx18/cx18-streams.c |   23 ++++
> >>  drivers/media/video/cx18/cx23418.h      |    6 +
> >>  8 files changed, 466 insertions(+), 12 deletions(-)
> >>
> >> ---
> >>
> >> http://git.linuxtv.org/media_tree.git?a=commitdiff;h=fa8f1381764d83222333cb67b8d93b9cb1605bf3
> >>
> >> diff --git a/drivers/media/video/cx18/Kconfig b/drivers/media/video/cx18/Kconfig
> >> index d788ad6..9c23202 100644
> >> --- a/drivers/media/video/cx18/Kconfig
> >> +++ b/drivers/media/video/cx18/Kconfig
> >> @@ -2,6 +2,8 @@ config VIDEO_CX18
> >>  	tristate "Conexant cx23418 MPEG encoder support"
> >>  	depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
> >>  	select I2C_ALGOBIT
> >> +	select VIDEOBUF_DVB
> >> +	select VIDEOBUF_VMALLOC
> >>  	depends on RC_CORE
> >>  	select VIDEO_TUNER
> >>  	select VIDEO_TVEEPROM
> >> diff --git a/drivers/media/video/cx18/cx18-driver.h b/drivers/media/video/cx18/cx18-driver.h
> >> index b86a740..70e1e04 100644
> >> --- a/drivers/media/video/cx18/cx18-driver.h
> >> +++ b/drivers/media/video/cx18/cx18-driver.h
> >> @@ -65,6 +65,10 @@
> >>  #include "dvb_net.h"
> >>  #include "dvbdev.h"
> >>  
> >> +/* Videobuf / YUV support */
> >> +#include <media/videobuf-core.h>
> >> +#include <media/videobuf-vmalloc.h>
> >> +
> >>  #ifndef CONFIG_PCI
> >>  #  error "This driver requires kernel PCI support."
> >>  #endif
> >> @@ -403,6 +407,23 @@ struct cx18_stream {
> >>  	struct cx18_queue q_idle;	/* idle - not in rotation */
> >>  
> >>  	struct work_struct out_work_order;
> >> +
> >> +	/* Videobuf for YUV video */
> >> +	u32 pixelformat;
> >> +	struct list_head vb_capture;    /* video capture queue */
> >> +	spinlock_t vb_lock;
> >> +	struct v4l2_framebuffer fbuf;
> >> +	v4l2_std_id tvnorm; /* selected tv norm */
> >> +	struct timer_list vb_timeout;
> >> +	int vbwidth;
> >> +	int vbheight;
> >> +};
> >> +
> >> +struct cx18_videobuf_buffer {
> >> +	/* Common video buffer sub-system struct */
> >> +	struct videobuf_buffer vb;
> >> +	v4l2_std_id tvnorm; /* selected tv norm */
> >> +	u32 bytes_used;
> >>  };
> >>  
> >>  struct cx18_open_id {
> >> @@ -410,6 +431,10 @@ struct cx18_open_id {
> >>  	u32 open_id;
> >>  	int type;
> >>  	struct cx18 *cx;
> >> +
> >> +	struct videobuf_queue vbuf_q;
> >> +	spinlock_t s_lock; /* Protect vbuf_q */
> >> +	enum v4l2_buf_type vb_type;
> >>  };
> >>  
> >>  static inline struct cx18_open_id *fh2id(struct v4l2_fh *fh)
> >> diff --git a/drivers/media/video/cx18/cx18-fileops.c b/drivers/media/video/cx18/cx18-fileops.c
> >> index e9802d9..c74eafd 100644
> >> --- a/drivers/media/video/cx18/cx18-fileops.c
> >> +++ b/drivers/media/video/cx18/cx18-fileops.c
> >> @@ -597,6 +597,13 @@ ssize_t cx18_v4l2_read(struct file *filp, char __user *buf, size_t count,
> >>  	mutex_unlock(&cx->serialize_lock);
> >>  	if (rc)
> >>  		return rc;
> >> +
> >> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
> >> +		return videobuf_read_stream(&id->vbuf_q, buf, count, pos, 0,
> >> +			filp->f_flags & O_NONBLOCK);
> >> +	}
> >> +
> >>  	return cx18_read_pos(s, buf, count, pos, filp->f_flags & O_NONBLOCK);
> >>  }
> >>  
> >> @@ -622,6 +629,11 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp, poll_table *wait)
> >>  		CX18_DEBUG_FILE("Encoder poll started capture\n");
> >>  	}
> >>  
> >> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
> >> +		return videobuf_poll_stream(filp, &id->vbuf_q, wait);
> >> +	}
> >> +
> >>  	/* add stream's waitq to the poll list */
> >>  	CX18_DEBUG_HI_FILE("Encoder poll\n");
> >>  	poll_wait(filp, &s->waitq, wait);
> >> @@ -633,6 +645,58 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp, poll_table *wait)
> >>  	return 0;
> >>  }
> >>  
> >> +int cx18_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
> >> +{
> >> +	struct cx18_open_id *id = file->private_data;
> >> +	struct cx18 *cx = id->cx;
> >> +	struct cx18_stream *s = &cx->streams[id->type];
> >> +	int eof = test_bit(CX18_F_S_STREAMOFF, &s->s_flags);
> >> +
> >> +	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
> >> +
> >> +		/* Start a capture if there is none */
> >> +		if (!eof && !test_bit(CX18_F_S_STREAMING, &s->s_flags)) {
> >> +			int rc;
> >> +
> >> +			mutex_lock(&cx->serialize_lock);
> >> +			rc = cx18_start_capture(id);
> >> +			mutex_unlock(&cx->serialize_lock);
> >> +			if (rc) {
> >> +				CX18_DEBUG_INFO(
> >> +					"Could not start capture for %s (%d)\n",
> >> +					s->name, rc);
> >> +				return -EINVAL;
> >> +			}
> >> +			CX18_DEBUG_FILE("Encoder poll started capture\n");
> >> +		}
> >> +
> >> +		return videobuf_mmap_mapper(&id->vbuf_q, vma);
> >> +	}
> >> +
> >> +	return -EINVAL;
> >> +}
> >> +
> >> +void cx18_vb_timeout(unsigned long data)
> >> +{
> >> +	struct cx18_stream *s = (struct cx18_stream *)data;
> >> +	struct cx18_videobuf_buffer *buf;
> >> +	unsigned long flags;
> >> +
> >> +	/* Return all of the buffers in error state, so the vbi/vid inode
> >> +	 * can return from blocking.
> >> +	 */
> >> +	spin_lock_irqsave(&s->vb_lock, flags);
> >> +	while (!list_empty(&s->vb_capture)) {
> >> +		buf = list_entry(s->vb_capture.next,
> >> +			struct cx18_videobuf_buffer, vb.queue);
> >> +		list_del(&buf->vb.queue);
> >> +		buf->vb.state = VIDEOBUF_ERROR;
> >> +		wake_up(&buf->vb.done);
> >> +	}
> >> +	spin_unlock_irqrestore(&s->vb_lock, flags);
> >> +}
> >> +
> >>  void cx18_stop_capture(struct cx18_open_id *id, int gop_end)
> >>  {
> >>  	struct cx18 *cx = id->cx;
> >> @@ -716,12 +780,150 @@ int cx18_v4l2_close(struct file *filp)
> >>  		cx18_release_stream(s);
> >>  	} else {
> >>  		cx18_stop_capture(id, 0);
> >> +		if (id->type == CX18_ENC_STREAM_TYPE_YUV)
> >> +			videobuf_mmap_free(&id->vbuf_q);
> >>  	}
> >>  	kfree(id);
> >>  	mutex_unlock(&cx->serialize_lock);
> >>  	return 0;
> >>  }
> >>  
> >> +void cx18_dma_free(struct videobuf_queue *q,
> >> +	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
> >> +{
> >> +	videobuf_waiton(q, &buf->vb, 0, 0);
> >> +	videobuf_vmalloc_free(&buf->vb);
> >> +	buf->vb.state = VIDEOBUF_NEEDS_INIT;
> >> +}
> >> +
> >> +static int cx18_prepare_buffer(struct videobuf_queue *q,
> >> +	struct cx18_stream *s,
> >> +	struct cx18_videobuf_buffer *buf,
> >> +	u32 pixelformat,
> >> +	unsigned int width, unsigned int height,
> >> +	enum v4l2_field field)
> >> +{
> >> +	int rc = 0;
> >> +
> >> +	/* check settings */
> >> +	buf->bytes_used = 0;
> >> +
> >> +	if ((width  < 48) || (height < 32))
> >> +		return -EINVAL;
> >> +
> >> +	buf->vb.size = (width * height * 16 /*fmt->depth*/) >> 3;
> >> +	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
> >> +		return -EINVAL;
> >> +
> >> +	/* alloc + fill struct (if changed) */
> >> +	if (buf->vb.width != width || buf->vb.height != height ||
> >> +	    buf->vb.field != field || s->pixelformat != pixelformat ||
> >> +	    buf->tvnorm != s->tvnorm) {
> >> +
> >> +		buf->vb.width  = width;
> >> +		buf->vb.height = height;
> >> +		buf->vb.field  = field;
> >> +		buf->tvnorm    = s->tvnorm;
> >> +		s->pixelformat = pixelformat;
> >> +
> >> +		cx18_dma_free(q, s, buf);
> >> +	}
> >> +
> >> +	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
> >> +		return -EINVAL;
> >> +
> >> +	if (buf->vb.field == 0)
> >> +		buf->vb.field = V4L2_FIELD_INTERLACED;
> >> +
> >> +	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
> >> +		buf->vb.width  = width;
> >> +		buf->vb.height = height;
> >> +		buf->vb.field  = field;
> >> +		buf->tvnorm    = s->tvnorm;
> >> +		s->pixelformat = pixelformat;
> >> +
> >> +		rc = videobuf_iolock(q, &buf->vb, &s->fbuf);
> >> +		if (rc != 0)
> >> +			goto fail;
> >> +	}
> >> +	buf->vb.state = VIDEOBUF_PREPARED;
> >> +	return 0;
> >> +
> >> +fail:
> >> +	cx18_dma_free(q, s, buf);
> >> +	return rc;
> >> +
> >> +}
> >> +
> >> +#define VB_MIN_BUFFERS 32
> >> +#define VB_MIN_BUFSIZE 0x208000
> >> +
> >> +static int buffer_setup(struct videobuf_queue *q,
> >> +	unsigned int *count, unsigned int *size)
> >> +{
> >> +	struct cx18_open_id *id = q->priv_data;
> >> +	struct cx18 *cx = id->cx;
> >> +	struct cx18_stream *s = &cx->streams[id->type];
> >> +
> >> +	*size = 2 * s->vbwidth * s->vbheight;
> >> +	if (*count == 0)
> >> +		*count = VB_MIN_BUFFERS;
> >> +
> >> +	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
> >> +		(*count)--;
> >> +
> >> +	q->field = V4L2_FIELD_INTERLACED;
> >> +	q->last = V4L2_FIELD_INTERLACED;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int buffer_prepare(struct videobuf_queue *q,
> >> +	struct videobuf_buffer *vb,
> >> +	enum v4l2_field field)
> >> +{
> >> +	struct cx18_videobuf_buffer *buf =
> >> +		container_of(vb, struct cx18_videobuf_buffer, vb);
> >> +	struct cx18_open_id *id = q->priv_data;
> >> +	struct cx18 *cx = id->cx;
> >> +	struct cx18_stream *s = &cx->streams[id->type];
> >> +
> >> +	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
> >> +		s->vbwidth, s->vbheight, field);
> >> +}
> >> +
> >> +static void buffer_release(struct videobuf_queue *q,
> >> +	struct videobuf_buffer *vb)
> >> +{
> >> +	struct cx18_videobuf_buffer *buf =
> >> +		container_of(vb, struct cx18_videobuf_buffer, vb);
> >> +	struct cx18_open_id *id = q->priv_data;
> >> +	struct cx18 *cx = id->cx;
> >> +	struct cx18_stream *s = &cx->streams[id->type];
> >> +
> >> +	cx18_dma_free(q, s, buf);
> >> +}
> >> +
> >> +static void buffer_queue(struct videobuf_queue *q, struct videobuf_buffer *vb)
> >> +{
> >> +	struct cx18_videobuf_buffer *buf =
> >> +		container_of(vb, struct cx18_videobuf_buffer, vb);
> >> +	struct cx18_open_id *id = q->priv_data;
> >> +	struct cx18 *cx = id->cx;
> >> +	struct cx18_stream *s = &cx->streams[id->type];
> >> +
> >> +	buf->vb.state = VIDEOBUF_QUEUED;
> >> +
> >> +	list_add_tail(&buf->vb.queue, &s->vb_capture);
> >> +}
> >> +
> >> +static struct videobuf_queue_ops cx18_videobuf_qops = {
> >> +	.buf_setup    = buffer_setup,
> >> +	.buf_prepare  = buffer_prepare,
> >> +	.buf_queue    = buffer_queue,
> >> +	.buf_release  = buffer_release,
> >> +};
> >> +
> >>  static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
> >>  {
> >>  	struct cx18 *cx = s->cx;
> >> @@ -740,6 +942,9 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
> >>  	item->cx = cx;
> >>  	item->type = s->type;
> >>  
> >> +	spin_lock_init(&item->s_lock);
> >> +	item->vb_type = 0;
> >> +
> >>  	item->open_id = cx->open_id++;
> >>  	filp->private_data = &item->fh;
> >>  
> >> @@ -774,6 +979,15 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
> >>  		/* Done! Unmute and continue. */
> >>  		cx18_unmute(cx);
> >>  	}
> >> +	if (item->type == CX18_ENC_STREAM_TYPE_YUV) {
> >> +		item->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +		videobuf_queue_vmalloc_init(&item->vbuf_q, &cx18_videobuf_qops,
> >> +			&cx->pci_dev->dev, &item->s_lock,
> >> +			V4L2_BUF_TYPE_VIDEO_CAPTURE,
> >> +			V4L2_FIELD_INTERLACED,
> >> +			sizeof(struct cx18_videobuf_buffer),
> >> +			item, &cx->serialize_lock);
> >> +	}
> >>  	v4l2_fh_add(&item->fh);
> >>  	return 0;
> >>  }
> >> diff --git a/drivers/media/video/cx18/cx18-fileops.h b/drivers/media/video/cx18/cx18-fileops.h
> >> index 5c8fcb8..b9e5110 100644
> >> --- a/drivers/media/video/cx18/cx18-fileops.h
> >> +++ b/drivers/media/video/cx18/cx18-fileops.h
> >> @@ -33,6 +33,8 @@ int cx18_start_capture(struct cx18_open_id *id);
> >>  void cx18_stop_capture(struct cx18_open_id *id, int gop_end);
> >>  void cx18_mute(struct cx18 *cx);
> >>  void cx18_unmute(struct cx18 *cx);
> >> +int cx18_v4l2_mmap(struct file *file, struct vm_area_struct *vma);
> >> +void cx18_vb_timeout(unsigned long data);
> >>  
> >>  /* Shared with cx18-alsa module */
> >>  int cx18_claim_stream(struct cx18_open_id *id, int type);
> >> diff --git a/drivers/media/video/cx18/cx18-ioctl.c b/drivers/media/video/cx18/cx18-ioctl.c
> >> index 4f041c0..777d726 100644
> >> --- a/drivers/media/video/cx18/cx18-ioctl.c
> >> +++ b/drivers/media/video/cx18/cx18-ioctl.c
> >> @@ -41,6 +41,18 @@
> >>  #include <media/tveeprom.h>
> >>  #include <media/v4l2-chip-ident.h>
> >>  
> >> +static struct v4l2_fmtdesc formats[] = {
> >> +	{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
> >> +	  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
> >> +	},
> >> +	{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
> >> +	  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
> >> +	},
> >> +	{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
> >> +	  "YUYV 4:2:2", V4L2_PIX_FMT_YUYV, { 0, 0, 0, 0 }
> >> +	},
> >> +};
> >> +
> >>  u16 cx18_service2vbi(int type)
> >>  {
> >>  	switch (type) {
> >> @@ -150,6 +162,7 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh,
> >>  {
> >>  	struct cx18_open_id *id = fh2id(fh);
> >>  	struct cx18 *cx = id->cx;
> >> +	struct cx18_stream *s = &cx->streams[id->type];
> >>  	struct v4l2_pix_format *pixfmt = &fmt->fmt.pix;
> >>  
> >>  	pixfmt->width = cx->cxhdl.width;
> >> @@ -158,7 +171,7 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh,
> >>  	pixfmt->field = V4L2_FIELD_INTERLACED;
> >>  	pixfmt->priv = 0;
> >>  	if (id->type == CX18_ENC_STREAM_TYPE_YUV) {
> >> -		pixfmt->pixelformat = V4L2_PIX_FMT_HM12;
> >> +		pixfmt->pixelformat = s->pixelformat;
> >>  		/* YUV size is (Y=(h*720) + UV=(h*(720/2))) */
> >>  		pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
> >>  		pixfmt->bytesperline = 720;
> >> @@ -237,7 +250,6 @@ static int cx18_try_fmt_vid_cap(struct file *file, void *fh,
> >>  	h = min(h, cx->is_50hz ? 576 : 480);
> >>  	h = max(h, min_h);
> >>  
> >> -	cx18_g_fmt_vid_cap(file, fh, fmt);
> >>  	fmt->fmt.pix.width = w;
> >>  	fmt->fmt.pix.height = h;
> >>  	return 0;
> >> @@ -274,6 +286,7 @@ static int cx18_s_fmt_vid_cap(struct file *file, void *fh,
> >>  	struct cx18_open_id *id = fh2id(fh);
> >>  	struct cx18 *cx = id->cx;
> >>  	struct v4l2_mbus_framefmt mbus_fmt;
> >> +	struct cx18_stream *s = &cx->streams[id->type];
> >>  	int ret;
> >>  	int w, h;
> >>  
> >> @@ -283,6 +296,10 @@ static int cx18_s_fmt_vid_cap(struct file *file, void *fh,
> >>  	w = fmt->fmt.pix.width;
> >>  	h = fmt->fmt.pix.height;
> >>  
> >> +	s->pixelformat = fmt->fmt.pix.pixelformat;
> >> +	s->vbheight = h;
> >> +	s->vbwidth = w;
> >> +
> >>  	if (cx->cxhdl.width == w && cx->cxhdl.height == h)
> >>  		return 0;
> >>  
> >> @@ -540,16 +557,7 @@ static int cx18_g_crop(struct file *file, void *fh, struct v4l2_crop *crop)
> >>  static int cx18_enum_fmt_vid_cap(struct file *file, void *fh,
> >>  					struct v4l2_fmtdesc *fmt)
> >>  {
> >> -	static struct v4l2_fmtdesc formats[] = {
> >> -		{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
> >> -		  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
> >> -		},
> >> -		{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
> >> -		  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
> >> -		}
> >> -	};
> >> -
> >> -	if (fmt->index > 1)
> >> +	if (fmt->index > ARRAY_SIZE(formats) - 1)
> >>  		return -EINVAL;
> >>  	*fmt = formats[fmt->index];
> >>  	return 0;
> >> @@ -863,6 +871,104 @@ static int cx18_g_enc_index(struct file *file, void *fh,
> >>  	return 0;
> >>  }
> >>  
> >> +static struct videobuf_queue *cx18_vb_queue(struct cx18_open_id *id)
> >> +{
> >> +	struct videobuf_queue *q = NULL;
> >> +
> >> +	switch (id->vb_type) {
> >> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> >> +		q = &id->vbuf_q;
> >> +		break;
> >> +	case V4L2_BUF_TYPE_VBI_CAPTURE:
> >> +		break;
> >> +	default:
> >> +		break;
> >> +	}
> >> +	return q;
> >> +}
> >> +
> >> +static int cx18_streamon(struct file *file, void *priv,
> >> +	enum v4l2_buf_type type)
> >> +{
> >> +	struct cx18_open_id *id = file->private_data;
> >> +	struct cx18 *cx = id->cx;
> >> +	struct cx18_stream *s = &cx->streams[id->type];
> >> +
> >> +	/* Start the hardware only if we're the video device */
> >> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> >> +		return -EINVAL;
> >> +
> >> +	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
> >> +		return -EINVAL;
> >> +
> >> +	/* Establish a buffer timeout */
> >> +	mod_timer(&s->vb_timeout, jiffies + (HZ * 2));
> >> +
> >> +	return videobuf_streamon(cx18_vb_queue(id));
> >> +}
> >> +
> >> +static int cx18_streamoff(struct file *file, void *priv,
> >> +	enum v4l2_buf_type type)
> >> +{
> >> +	struct cx18_open_id *id = file->private_data;
> >> +
> >> +	/* Start the hardware only if we're the video device */
> >> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> >> +		return -EINVAL;
> >> +
> >> +	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
> >> +		return -EINVAL;
> >> +
> >> +	return videobuf_streamoff(cx18_vb_queue(id));
> >> +}
> >> +
> >> +static int cx18_reqbufs(struct file *file, void *priv,
> >> +	struct v4l2_requestbuffers *rb)
> >> +{
> >> +	struct cx18_open_id *id = file->private_data;
> >> +
> >> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> >> +		return -EINVAL;
> >> +
> >> +	return videobuf_reqbufs(cx18_vb_queue(id), rb);
> >> +}
> >> +
> >> +static int cx18_querybuf(struct file *file, void *priv,
> >> +	struct v4l2_buffer *b)
> >> +{
> >> +	struct cx18_open_id *id = file->private_data;
> >> +
> >> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> >> +		return -EINVAL;
> >> +
> >> +	return videobuf_querybuf(cx18_vb_queue(id), b);
> >> +}
> >> +
> >> +static int cx18_qbuf(struct file *file, void *priv, struct v4l2_buffer *b)
> >> +{
> >> +	struct cx18_open_id *id = file->private_data;
> >> +
> >> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> >> +		return -EINVAL;
> >> +
> >> +	return videobuf_qbuf(cx18_vb_queue(id), b);
> >> +}
> >> +
> >> +static int cx18_dqbuf(struct file *file, void *priv, struct v4l2_buffer *b)
> >> +{
> >> +	struct cx18_open_id *id = file->private_data;
> >> +	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> >> +		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> >> +		return -EINVAL;
> >> +
> >> +	return videobuf_dqbuf(cx18_vb_queue(id), b, file->f_flags & O_NONBLOCK);
> >> +}
> >> +
> >>  static int cx18_encoder_cmd(struct file *file, void *fh,
> >>  				struct v4l2_encoder_cmd *enc)
> >>  {
> >> @@ -1081,6 +1187,12 @@ static const struct v4l2_ioctl_ops cx18_ioctl_ops = {
> >>  	.vidioc_s_register              = cx18_s_register,
> >>  #endif
> >>  	.vidioc_default                 = cx18_default,
> >> +	.vidioc_streamon                = cx18_streamon,
> >> +	.vidioc_streamoff               = cx18_streamoff,
> >> +	.vidioc_reqbufs                 = cx18_reqbufs,
> >> +	.vidioc_querybuf                = cx18_querybuf,
> >> +	.vidioc_qbuf                    = cx18_qbuf,
> >> +	.vidioc_dqbuf                   = cx18_dqbuf,
> >>  };
> >>  
> >>  void cx18_set_funcs(struct video_device *vdev)
> >> diff --git a/drivers/media/video/cx18/cx18-mailbox.c b/drivers/media/video/cx18/cx18-mailbox.c
> >> index 9605d54..d4d8873 100644
> >> --- a/drivers/media/video/cx18/cx18-mailbox.c
> >> +++ b/drivers/media/video/cx18/cx18-mailbox.c
> >> @@ -81,6 +81,7 @@ static const struct cx18_api_info api_info[] = {
> >>  	API_ENTRY(CPU, CX18_CPU_SET_SLICED_VBI_PARAM,           0),
> >>  	API_ENTRY(CPU, CX18_CPU_SET_USERDATA_PLACE_HOLDER,      0),
> >>  	API_ENTRY(CPU, CX18_CPU_GET_ENC_PTS,                    0),
> >> +	API_ENTRY(CPU, CX18_CPU_SET_VFC_PARAM,                  0),
> >>  	API_ENTRY(CPU, CX18_CPU_DE_SET_MDL_ACK,			0),
> >>  	API_ENTRY(CPU, CX18_CPU_DE_SET_MDL,			API_FAST),
> >>  	API_ENTRY(CPU, CX18_CPU_DE_RELEASE_MDL,			API_SLOW),
> >> @@ -158,6 +159,72 @@ static void cx18_mdl_send_to_dvb(struct cx18_stream *s, struct cx18_mdl *mdl)
> >>  	}
> >>  }
> >>  
> >> +static void cx18_mdl_send_to_videobuf(struct cx18_stream *s,
> >> +	struct cx18_mdl *mdl)
> >> +{
> >> +	struct cx18_videobuf_buffer *vb_buf;
> >> +	struct cx18_buffer *buf;
> >> +	u8 *p, u;
> >> +	u32 offset = 0;
> >> +	int dispatch = 0;
> >> +	int i;
> >> +
> >> +	if (mdl->bytesused == 0)
> >> +		return;
> >> +
> >> +	/* Acquire a videobuf buffer, clone to and and release it */
> >> +	spin_lock(&s->vb_lock);
> >> +	if (list_empty(&s->vb_capture))
> >> +		goto out;
> >> +
> >> +	vb_buf = list_entry(s->vb_capture.next, struct cx18_videobuf_buffer,
> >> +		vb.queue);
> >> +
> >> +	p = videobuf_to_vmalloc(&vb_buf->vb);
> >> +	if (!p)
> >> +		goto out;
> >> +
> >> +	offset = vb_buf->bytes_used;
> >> +	list_for_each_entry(buf, &mdl->buf_list, list) {
> >> +		if (buf->bytesused == 0)
> >> +			break;
> >> +
> >> +		if ((offset + buf->bytesused) <= vb_buf->vb.bsize) {
> >> +			memcpy(p + offset, buf->buf, buf->bytesused);
> >> +			offset += buf->bytesused;
> >> +			vb_buf->bytes_used += buf->bytesused;
> >> +		}
> >> +	}
> >> +
> >> +	/* If we've filled the buffer as per the callers res then dispatch it */
> >> +	if (vb_buf->bytes_used >= (vb_buf->vb.width * vb_buf->vb.height * 2)) {
> >> +		dispatch = 1;
> >> +		vb_buf->bytes_used = 0;
> >> +	}
> >> +
> >> +	/* */
> >> +	if (dispatch) {
> >> +
> >> +		if (s->pixelformat == V4L2_PIX_FMT_YUYV) {
> >> +			/* UYVY to YUYV */
> >> +			for (i = 0; i < (720 * 480 * 2); i += 2) {
> >> +				u = *(p + i);
> >> +				*(p + i) = *(p + i + 1);
> >> +				*(p + i + 1) = u;
> >> +			}
> >> +		}
> >> +
> >> +		do_gettimeofday(&vb_buf->vb.ts);
> >> +		list_del(&vb_buf->vb.queue);
> >> +		vb_buf->vb.state = VIDEOBUF_DONE;
> >> +		wake_up(&vb_buf->vb.done);
> >> +	}
> >> +
> >> +	mod_timer(&s->vb_timeout, jiffies + (HZ / 10));
> >> +
> >> +out:
> >> +	spin_unlock(&s->vb_lock);
> >> +}
> >>  
> >>  static void cx18_mdl_send_to_alsa(struct cx18 *cx, struct cx18_stream *s,
> >>  				  struct cx18_mdl *mdl)
> >> @@ -263,6 +330,9 @@ static void epu_dma_done(struct cx18 *cx, struct cx18_in_work_order *order)
> >>  			} else {
> >>  				cx18_enqueue(s, mdl, &s->q_full);
> >>  			}
> >> +		} else if (s->type == CX18_ENC_STREAM_TYPE_YUV) {
> >> +			cx18_mdl_send_to_videobuf(s, mdl);
> >> +			cx18_enqueue(s, mdl, &s->q_free);
> >>  		} else {
> >>  			cx18_enqueue(s, mdl, &s->q_full);
> >>  			if (s->type == CX18_ENC_STREAM_TYPE_IDX)
> >> diff --git a/drivers/media/video/cx18/cx18-streams.c b/drivers/media/video/cx18/cx18-streams.c
> >> index c6e2ca3..53f5e4f 100644
> >> --- a/drivers/media/video/cx18/cx18-streams.c
> >> +++ b/drivers/media/video/cx18/cx18-streams.c
> >> @@ -44,6 +44,7 @@ static struct v4l2_file_operations cx18_v4l2_enc_fops = {
> >>  	.unlocked_ioctl = cx18_v4l2_ioctl,
> >>  	.release = cx18_v4l2_close,
> >>  	.poll = cx18_v4l2_enc_poll,
> >> +	.mmap = cx18_v4l2_mmap,
> >>  };
> >>  
> >>  /* offset from 0 to register ts v4l2 minors on */
> >> @@ -132,6 +133,15 @@ static void cx18_stream_init(struct cx18 *cx, int type)
> >>  	cx18_queue_init(&s->q_idle);
> >>  
> >>  	INIT_WORK(&s->out_work_order, cx18_out_work_handler);
> >> +
> >> +	INIT_LIST_HEAD(&s->vb_capture);
> >> +	s->vb_timeout.function = cx18_vb_timeout;
> >> +	s->vb_timeout.data = (unsigned long)s;
> >> +	init_timer(&s->vb_timeout);
> >> +	spin_lock_init(&s->vb_lock);
> >> +
> >> +	/* Assume the previous pixel default */
> >> +	s->pixelformat = V4L2_PIX_FMT_HM12;
> >>  }
> >>  
> >>  static int cx18_prep_dev(struct cx18 *cx, int type)
> >> @@ -721,6 +731,19 @@ int cx18_start_v4l2_encode_stream(struct cx18_stream *s)
> >>  		    test_bit(CX18_F_I_RADIO_USER, &cx->i_flags))
> >>  			cx18_vapi(cx, CX18_CPU_SET_VIDEO_MUTE, 2, s->handle,
> >>  			  (v4l2_ctrl_g_ctrl(cx->cxhdl.video_mute_yuv) << 8) | 1);
> >> +
> >> +		/* Enable the Video Format Converter for UYVY 4:2:2 support,
> >> +		 * rather than the default HM12 Macroblovk 4:2:0 support.
> >> +		 */
> >> +		if (captype == CAPTURE_CHANNEL_TYPE_YUV) {
> >> +			if (s->pixelformat == V4L2_PIX_FMT_YUYV)
> >> +				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
> >> +					s->handle, 1);
> >> +			else
> >> +				/* If in doubt, default to HM12 */
> >> +				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
> >> +					s->handle, 0);
> >> +		}
> >>  	}
> >>  
> >>  	if (atomic_read(&cx->tot_capturing) == 0) {
> >> diff --git a/drivers/media/video/cx18/cx23418.h b/drivers/media/video/cx18/cx23418.h
> >> index 935f557..767a8d2 100644
> >> --- a/drivers/media/video/cx18/cx23418.h
> >> +++ b/drivers/media/video/cx18/cx23418.h
> >> @@ -342,6 +342,12 @@
> >>     ReturnCode */
> >>  #define CX18_CPU_GET_ENC_PTS			(CPU_CMD_MASK_CAPTURE | 0x0022)
> >>  
> >> +/* Description: Set VFC parameters
> >> +   IN[0] - task handle
> >> +   IN[1] - VFC enable flag, 1 - enable, 0 - disable
> >> +*/
> >> +#define CX18_CPU_SET_VFC_PARAM                  (CPU_CMD_MASK_CAPTURE | 0x0023)
> >> +
> >>  /* Below is the list of commands related to the data exchange */
> >>  #define CPU_CMD_MASK_DE 			(CPU_CMD_MASK | 0x040000)
> >>  
> >>
> >> _______________________________________________
> >> linuxtv-commits mailing list
> >> linuxtv-commits@linuxtv.org
> >> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits
> >>
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Devin Heitmueller]

On Mon, May 2, 2011 at 4:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> It was merged without *asking* Andy. I know he has had some private stuff to
> deal with this month so I wasn't surprised that he hadn't reviewed it yet.
>
> It would have been nice if he was reminded first of this patch. It's a
> fairly substantial change that also has user-visible implications. The simple
> fact is that this patch has not been reviewed and as a former cx18 maintainer
> I think that it needs a review first.
>
> If someone had asked and Andy wouldn't have been able to review, then I'd have
> jumped in and would have reviewed it.
>
> Andy, I hope you can look at it, but if not, then let me know and I'll do a
> more in-depth review rather than just the simple scan I did now.
>
>> Now that the patch were committed, I won't revert it without a very good reason.
>>
>> With respect to the "conversion from UYVY format to YUYV", a simple patch could
>> fix it, instead of removing the entire patchset.
>
> No, please remove the patchset because I have found two other issues:
>
> The patch adds this field:
>
>        struct v4l2_framebuffer fbuf;
>
> This is not needed, videobuf_iolock can be called with a NULL pointer instead
> of &fbuf.
>
> The patch also adds tvnorm fields, but never sets s->tvnorm. And it's
> pointless anyway since you can't change tvnorm while streaming.
>
> Given that I've found three things now without even trying suggests to me that
> it is too soon to commit this. Sorry.
>
> Regards,
>
>        Hans

Indeed comments/review are always welcome, although it would have been
great if it had happened a month ago.  It's the maintainer's
responsibility to review patches, and if he has issues to raise them
in a timely manner.  If he doesn't care enough or is too busy to
publicly say "hold off on this" for whatever reason, then you can
hardly blame Mauro for merging it.

Likewise, I know there have indeed been cases in the past where code
got upstream that caused regressions (in fact, you have personally
been responsible for some of these if I recall).

Let's not throw the baby out with the bathwater.  If there are real
structural issues with the patch, then let's get them fixed.  But if
we're just talking about a few minor "unused variable" type of
aesthetic issues, then that shouldn't constitute reverting the commit.
 Do your review, and if an additional patch is needed with a half
dozen removals of dead/unused code, then so be it.

We're not talking about an untested board profile submitted by some
random user.  We're talking about a patch written by someone highly
familiar with the chipset and it's *working code* that has been
running in production for almost a year.

Devin

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

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Hans Verkuil]

On Monday, May 02, 2011 22:59:09 Devin Heitmueller wrote:
> On Mon, May 2, 2011 at 4:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > It was merged without *asking* Andy. I know he has had some private stuff to
> > deal with this month so I wasn't surprised that he hadn't reviewed it yet.
> >
> > It would have been nice if he was reminded first of this patch. It's a
> > fairly substantial change that also has user-visible implications. The simple
> > fact is that this patch has not been reviewed and as a former cx18 maintainer
> > I think that it needs a review first.
> >
> > If someone had asked and Andy wouldn't have been able to review, then I'd have
> > jumped in and would have reviewed it.
> >
> > Andy, I hope you can look at it, but if not, then let me know and I'll do a
> > more in-depth review rather than just the simple scan I did now.
> >
> >> Now that the patch were committed, I won't revert it without a very good reason.
> >>
> >> With respect to the "conversion from UYVY format to YUYV", a simple patch could
> >> fix it, instead of removing the entire patchset.
> >
> > No, please remove the patchset because I have found two other issues:
> >
> > The patch adds this field:
> >
> >        struct v4l2_framebuffer fbuf;
> >
> > This is not needed, videobuf_iolock can be called with a NULL pointer instead
> > of &fbuf.
> >
> > The patch also adds tvnorm fields, but never sets s->tvnorm. And it's
> > pointless anyway since you can't change tvnorm while streaming.
> >
> > Given that I've found three things now without even trying suggests to me that
> > it is too soon to commit this. Sorry.
> >
> > Regards,
> >
> >        Hans
> 
> Indeed comments/review are always welcome, although it would have been
> great if it had happened a month ago.  It's the maintainer's
> responsibility to review patches, and if he has issues to raise them
> in a timely manner.  If he doesn't care enough or is too busy to
> publicly say "hold off on this" for whatever reason, then you can
> hardly blame Mauro for merging it.

It's also a good idea if the author of a patch pings the list if there
has been no feedback after one or two weeks. It's easy to forget patches,
people can be on vacation, be sick, or in the case of Andy, have a family
emergency.

> Likewise, I know there have indeed been cases in the past where code
> got upstream that caused regressions (in fact, you have personally
> been responsible for some of these if I recall).
> 
> Let's not throw the baby out with the bathwater.  If there are real
> structural issues with the patch, then let's get them fixed.  But if
> we're just talking about a few minor "unused variable" type of
> aesthetic issues, then that shouldn't constitute reverting the commit.
>  Do your review, and if an additional patch is needed with a half
> dozen removals of dead/unused code, then so be it.

Well, one structural thing I am not at all happy about (but it is Andy's
call) is that it uses videobuf instead of vb2. Since this patch only deals
with YUV it shouldn't be hard to use vb2. The problem with videobuf is that
it violates the V4L2 spec in several places so I would prefer not to use
videobuf in cx18. If only because converting cx18 to vb2 later will change
the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is
something I would like to avoid if possible.

I know that Andy started work on vb2 in cx18 for all stream types (not just
YUV). I have no idea of the current state of that work. But it might be a
good starting point to use this patch and convert it to vb2. Later Andy can
add vb2 support for the other stream types.

> We're not talking about an untested board profile submitted by some
> random user.  We're talking about a patch written by someone highly
> familiar with the chipset and it's *working code* that has been
> running in production for almost a year.

It's not about that, it's about merging something substantial without the SoB
of the maintainer and without asking the maintainer.

I'm not blaming anyone, it's just a miscommunication. What should happen with
this patch is up to Andy.

Regards,

	Hans

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Mauro Carvalho Chehab]

Em 02-05-2011 18:31, Hans Verkuil escreveu:
> On Monday, May 02, 2011 22:59:09 Devin Heitmueller wrote:
>> On Mon, May 2, 2011 at 4:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> It was merged without *asking* Andy. I know he has had some private stuff to
>>> deal with this month so I wasn't surprised that he hadn't reviewed it yet.
>>>
>>> It would have been nice if he was reminded first of this patch. It's a
>>> fairly substantial change that also has user-visible implications. The simple
>>> fact is that this patch has not been reviewed and as a former cx18 maintainer
>>> I think that it needs a review first.
>>>
>>> If someone had asked and Andy wouldn't have been able to review, then I'd have
>>> jumped in and would have reviewed it.
>>>
>>> Andy, I hope you can look at it, but if not, then let me know and I'll do a
>>> more in-depth review rather than just the simple scan I did now.
>>>
>>>> Now that the patch were committed, I won't revert it without a very good reason.
>>>>
>>>> With respect to the "conversion from UYVY format to YUYV", a simple patch could
>>>> fix it, instead of removing the entire patchset.
>>>
>>> No, please remove the patchset because I have found two other issues:
>>>
>>> The patch adds this field:
>>>
>>>        struct v4l2_framebuffer fbuf;
>>>
>>> This is not needed, videobuf_iolock can be called with a NULL pointer instead
>>> of &fbuf.
>>>
>>> The patch also adds tvnorm fields, but never sets s->tvnorm. And it's
>>> pointless anyway since you can't change tvnorm while streaming.
>>>
>>> Given that I've found three things now without even trying suggests to me that
>>> it is too soon to commit this. Sorry.
>>>
>>> Regards,
>>>
>>>        Hans
>>
>> Indeed comments/review are always welcome, although it would have been
>> great if it had happened a month ago.  It's the maintainer's
>> responsibility to review patches, and if he has issues to raise them
>> in a timely manner.  If he doesn't care enough or is too busy to
>> publicly say "hold off on this" for whatever reason, then you can
>> hardly blame Mauro for merging it.
> 
> It's also a good idea if the author of a patch pings the list if there
> has been no feedback after one or two weeks. It's easy to forget patches,
> people can be on vacation, be sick, or in the case of Andy, have a family
> emergency.

Do you know if/when he will be available to discuss about it?

>> Likewise, I know there have indeed been cases in the past where code
>> got upstream that caused regressions (in fact, you have personally
>> been responsible for some of these if I recall).
>>
>> Let's not throw the baby out with the bathwater.  If there are real
>> structural issues with the patch, then let's get them fixed.  But if
>> we're just talking about a few minor "unused variable" type of
>> aesthetic issues, then that shouldn't constitute reverting the commit.
>>  Do your review, and if an additional patch is needed with a half
>> dozen removals of dead/unused code, then so be it.
> 
> Well, one structural thing I am not at all happy about (but it is Andy's
> call) is that it uses videobuf instead of vb2. Since this patch only deals
> with YUV it shouldn't be hard to use vb2. The problem with videobuf is that
> it violates the V4L2 spec in several places so I would prefer not to use
> videobuf in cx18. If only because converting cx18 to vb2 later will change
> the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is
> something I would like to avoid if possible.

Userspace applications works fine with vb1. Even if vb1 is not fully v4l2 compliant,
it is a de-facto standard[1]. 

In other words, any change of behaviour for stream I/O controls that would cause
applications that rely on vb1 way to break should be reverted on vb2, as otherwise
it would be a regression. I'm assuming that this is not the case for vb2. 

However, as I was not able to test it yet, as the saa7134 port were incomplete, and 
didn't work, and I was not able yet to test a stream with a Samsung device, partially
due to my lack of time, I'm not sure if all relevant applications work properly with 
vb2.

> I know that Andy started work on vb2 in cx18 for all stream types (not just
> YUV). I have no idea of the current state of that work. But it might be a
> good starting point to use this patch and convert it to vb2. Later Andy can
> add vb2 support for the other stream types.

When he's done with his work, it is only a matter of replacing one code by the
other.

>> We're not talking about an untested board profile submitted by some
>> random user.  We're talking about a patch written by someone highly
>> familiar with the chipset and it's *working code* that has been
>> running in production for almost a year.
> 
> It's not about that, it's about merging something substantial without the SoB
> of the maintainer and without asking the maintainer.
> 
> I'm not blaming anyone, it's just a miscommunication. What should happen with
> this patch is up to Andy.

Let's hear Andy's review on the patchset.

Thanks,
Mauro.

[1] http://en.wikipedia.org/wiki/De_facto

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Andy Walls]

Hi All,

Ah crud, what a mess.  Where to begin...?

Where have I been:

On 30 March 2011, my 8-year-old son was diagnosed with Necrotizing
Fasciitis caused by Invasive Group A Streptococcous - otherwise known as
"Flesh-eating bacteria":

http://en.wikipedia.org/wiki/Necrotizing_fasciitis
http://www.ncbi.nlm.nih.gov/pubmedhealth/PMH0002415/

By the grace of God, my son was diagnosed very early.  He only lost the
fascia on his left side and one lymph node - damage essentially
unnoticable to anyone, including my son himself.  His recovery progress
is excellent and he is now back to his normal life. Yay! \O/

Naturally, Linux driver development disappeared from my mind during the
extended hospital stay, multiple surgeries, and post-hospitalization
recovery.

As always; yard-work, house-work, work-work, choir practice, kids'
sports, kids' after school clubs, and kids' instrument lessons also
consume my time.




History of this patch:

1. Steven wrote the bulk of it 10 months ago:

	http://www.kernellabs.com/hg/~stoth/cx18-videobuf/

2. At Steven's request, I took a day and reviewed it on July 10 2010 and
provide comments off-list.  (I will provide them in a follow up to Mauro
Devin and Hans).

3. The patch languished as Steven didn't have time to make the fixes and
neither did I.

4. Videobuf2 came along as did good documentation on the deficiencies of
videobuf1:

http://linuxtv.org/downloads/presentations/summit_jun_2010/20100614-v4l2_summit-videobuf.pdf
http://linuxtv.org/downloads/presentations/summit_jun_2010/Videobuf_Helsinki_June2010.pdf
http://lwn.net/Articles/415883/

5. I started independent work to implement videobuf2 for YUV and
actually using zero-copy.  My progress is very slow.

http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18-vb2-proto
http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18_39

6. Simon submits the patches to the list as one big patch.

7. Off-list I forward the same 5 emails of comments to Simon as I
provided in #2 to Steven.

8. Simon addresses most of the comments and provides a revised patch
off-list asking for review.  I haven't had time to look at it.

9. Mauro commits the original patch that Simon submitted to the list.


My thoughts:

1. I don't want to stop progress, so I did not NACK this patch.  I don't
exactly like the patch either, so I didn't ACK it.

2. At a minimum someone needs to review Simon's revised patch that tried
to address my comments.  That patch has to be better than this one.
Hans has already noticed a few of the bugs I pointed out to Steven and
Simon.

3. I value that this patch has been tested, but I am guessing the
use-case was limited.  The toughest cx18 use-cases involve a lot of
concurrency - multiple stream captures (MPEG, VBI, YUV, PCM) on multiple
boards (3 or 4).  I had to do a lot of work with the driver to get that
concurrency reliable and performing well.  Has this been tested post-BKL
removal?  Have screen sizes other than the full-screen size been tested?

4. I do not like using videobuf(1) for this.  Videobuf(1) is a buggy
dead-end IMO.  I will NACK any patch that tries to fix anything due to
videobuf(1) related problems introduced into cx18 by this patch.
There's no point in throwing too much effort into fixing what would
likely be unfixable.

5. When I am done with my videobuf2 stuff for cx18, I will essentially
revert this one and add in my new implementation after sufficient
testing.  Though given the amount of time I have for this, maybe the
last HVR-1600 will be dead before then.


Summary:

1. I'm not going to fix any YUV related problems merging this patch
causes.  It's the YUV stream of an MPEG capture card that's more
expensive than a simple frame grabber.  (I've only heard of it being
used for live play of video games and of course for Simon's
application.)

2. I'd at least like Simon's revised patch to be merged instead, to fix
the known deficincies in this one.

3. If merging this patch, means a change to videobuf2 in the future is
not allowed, than I'd prefer to NACK the patch that introduces
videobuf(1) into cx18.


Regards,
Andy



On Mon, 2011-05-02 at 23:31 +0200, Hans Verkuil wrote:
> On Monday, May 02, 2011 22:59:09 Devin Heitmueller wrote:
> > On Mon, May 2, 2011 at 4:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > It was merged without *asking* Andy. I know he has had some private stuff to
> > > deal with this month so I wasn't surprised that he hadn't reviewed it yet.
> > >
> > > It would have been nice if he was reminded first of this patch. It's a
> > > fairly substantial change that also has user-visible implications. The simple
> > > fact is that this patch has not been reviewed and as a former cx18 maintainer
> > > I think that it needs a review first.
> > >
> > > If someone had asked and Andy wouldn't have been able to review, then I'd have
> > > jumped in and would have reviewed it.
> > >
> > > Andy, I hope you can look at it, but if not, then let me know and I'll do a
> > > more in-depth review rather than just the simple scan I did now.
> > >
> > >> Now that the patch were committed, I won't revert it without a very good reason.
> > >>
> > >> With respect to the "conversion from UYVY format to YUYV", a simple patch could
> > >> fix it, instead of removing the entire patchset.
> > >
> > > No, please remove the patchset because I have found two other issues:
> > >
> > > The patch adds this field:
> > >
> > >        struct v4l2_framebuffer fbuf;
> > >
> > > This is not needed, videobuf_iolock can be called with a NULL pointer instead
> > > of &fbuf.
> > >
> > > The patch also adds tvnorm fields, but never sets s->tvnorm. And it's
> > > pointless anyway since you can't change tvnorm while streaming.
> > >
> > > Given that I've found three things now without even trying suggests to me that
> > > it is too soon to commit this. Sorry.
> > >
> > > Regards,
> > >
> > >        Hans
> > 
> > Indeed comments/review are always welcome, although it would have been
> > great if it had happened a month ago.  It's the maintainer's
> > responsibility to review patches, and if he has issues to raise them
> > in a timely manner.  If he doesn't care enough or is too busy to
> > publicly say "hold off on this" for whatever reason, then you can
> > hardly blame Mauro for merging it.
> 
> It's also a good idea if the author of a patch pings the list if there
> has been no feedback after one or two weeks. It's easy to forget patches,
> people can be on vacation, be sick, or in the case of Andy, have a family
> emergency.
> 
> > Likewise, I know there have indeed been cases in the past where code
> > got upstream that caused regressions (in fact, you have personally
> > been responsible for some of these if I recall).
> > 
> > Let's not throw the baby out with the bathwater.  If there are real
> > structural issues with the patch, then let's get them fixed.  But if
> > we're just talking about a few minor "unused variable" type of
> > aesthetic issues, then that shouldn't constitute reverting the commit.
> >  Do your review, and if an additional patch is needed with a half
> > dozen removals of dead/unused code, then so be it.
> 
> Well, one structural thing I am not at all happy about (but it is Andy's
> call) is that it uses videobuf instead of vb2. Since this patch only deals
> with YUV it shouldn't be hard to use vb2. The problem with videobuf is that
> it violates the V4L2 spec in several places so I would prefer not to use
> videobuf in cx18. If only because converting cx18 to vb2 later will change
> the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is
> something I would like to avoid if possible.
> 
> I know that Andy started work on vb2 in cx18 for all stream types (not just
> YUV). I have no idea of the current state of that work. But it might be a
> good starting point to use this patch and convert it to vb2. Later Andy can
> add vb2 support for the other stream types.
> 
> > We're not talking about an untested board profile submitted by some
> > random user.  We're talking about a patch written by someone highly
> > familiar with the chipset and it's *working code* that has been
> > running in production for almost a year.
> 
> It's not about that, it's about merging something substantial without the SoB
> of the maintainer and without asking the maintainer.
> 
> I'm not blaming anyone, it's just a miscommunication. What should happen with
> this patch is up to Andy.
> 
> Regards,
> 
> 	Hans

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Mauro Carvalho Chehab]

Em 02-05-2011 23:40, Andy Walls escreveu:
> Hi All,
> 
> Ah crud, what a mess.  Where to begin...?
> 
> Where have I been:
> 
> On 30 March 2011, my 8-year-old son was diagnosed with Necrotizing
> Fasciitis caused by Invasive Group A Streptococcous - otherwise known as
> "Flesh-eating bacteria":
> 
> http://en.wikipedia.org/wiki/Necrotizing_fasciitis
> http://www.ncbi.nlm.nih.gov/pubmedhealth/PMH0002415/

Sorry to hear about that!

> By the grace of God, my son was diagnosed very early.  He only lost the
> fascia on his left side and one lymph node - damage essentially
> unnoticable to anyone, including my son himself.  His recovery progress
> is excellent and he is now back to his normal life. Yay! \O/

Good! I hope him to fully recover from the disease. All the best for you
and your wife. Our hearts are with you.

> Naturally, Linux driver development disappeared from my mind during the
> extended hospital stay, multiple surgeries, and post-hospitalization
> recovery.
> 
> As always; yard-work, house-work, work-work, choir practice, kids'
> sports, kids' after school clubs, and kids' instrument lessons also
> consume my time.
> 

Completely understandable.

> History of this patch:
> 
> 1. Steven wrote the bulk of it 10 months ago:
> 
> 	http://www.kernellabs.com/hg/~stoth/cx18-videobuf/
> 
> 2. At Steven's request, I took a day and reviewed it on July 10 2010 and
> provide comments off-list.  (I will provide them in a follow up to Mauro
> Devin and Hans).

Thanks! 

Next time, please answer it publicly, or if the patch author submitted
it in priv for a good reason, please c/c me on the review, in order to
warn me that you have some restrictions about a patch.

> 3. The patch languished as Steven didn't have time to make the fixes and
> neither did I.
> 
> 4. Videobuf2 came along as did good documentation on the deficiencies of
> videobuf1:
> 
> http://linuxtv.org/downloads/presentations/summit_jun_2010/20100614-v4l2_summit-videobuf.pdf
> http://linuxtv.org/downloads/presentations/summit_jun_2010/Videobuf_Helsinki_June2010.pdf
> http://lwn.net/Articles/415883/
> 
> 5. I started independent work to implement videobuf2 for YUV and
> actually using zero-copy.  My progress is very slow.
> 
> http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18-vb2-proto
> http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18_39
> 
> 6. Simon submits the patches to the list as one big patch.
> 
> 7. Off-list I forward the same 5 emails of comments to Simon as I
> provided in #2 to Steven.

In this case, as Simon had opened the source code already for the patch,
the better would be if you had made a public statement about your nack.
I always review the ML before applying a patch.

> 8. Simon addresses most of the comments and provides a revised patch
> off-list asking for review.  I haven't had time to look at it.
> 
> 9. Mauro commits the original patch that Simon submitted to the list.
> 
> 
> My thoughts:
> 
> 1. I don't want to stop progress, so I did not NACK this patch.  I don't
> exactly like the patch either, so I didn't ACK it.
> 
> 2. At a minimum someone needs to review Simon's revised patch that tried
> to address my comments.  That patch has to be better than this one.
> Hans has already noticed a few of the bugs I pointed out to Steven and
> Simon.
> 
> 3. I value that this patch has been tested, but I am guessing the
> use-case was limited.  The toughest cx18 use-cases involve a lot of
> concurrency - multiple stream captures (MPEG, VBI, YUV, PCM) on multiple
> boards (3 or 4).  I had to do a lot of work with the driver to get that
> concurrency reliable and performing well.  Has this been tested post-BKL
> removal?  Have screen sizes other than the full-screen size been tested?
> 
> 4. I do not like using videobuf(1) for this.  Videobuf(1) is a buggy
> dead-end IMO.  I will NACK any patch that tries to fix anything due to
> videobuf(1) related problems introduced into cx18 by this patch.
> There's no point in throwing too much effort into fixing what would
> likely be unfixable.
> 
> 5. When I am done with my videobuf2 stuff for cx18, I will essentially
> revert this one and add in my new implementation after sufficient
> testing.  Though given the amount of time I have for this, maybe the
> last HVR-1600 will be dead before then.
> 
> 
> Summary:
> 
> 1. I'm not going to fix any YUV related problems merging this patch
> causes.  It's the YUV stream of an MPEG capture card that's more
> expensive than a simple frame grabber.  (I've only heard of it being
> used for live play of video games and of course for Simon's
> application.)
> 
> 2. I'd at least like Simon's revised patch to be merged instead, to fix
> the known deficincies in this one.

IMO, the proper workflow would be that Simon should send his changes, as
a diff patch against the current one. We can all review it, based on the
comments you sent in priv and fix it.

As it seems that that the patch offers a subset of the desired features
that you're planning with your approach, maybe the better would be to add
a CONFIG var to enable YUV support, stating that such feature is experimental.

> 3. If merging this patch, means a change to videobuf2 in the future is
> not allowed, than I'd prefer to NACK the patch that introduces
> videobuf(1) into cx18.

The addition of VB1 first doesn't imply that VB2 would be acked or nacked.

In any case, the first non-embedded VB2 driver will need a very careful
review, to be sure that they won't break any userspace applications. 

On embedded hardware, only a limited set of applications are supported, and they
are patched and bundled together with the hardware, so there's not much concern
about userspace apps breakage.

However, on non-embedded hardware, we should be sure that no regressions to
existing applications will happen. So, the better would be if the first VB2 
non-embedded driver to be a full-featured V4L2 board (e. g. saa7134 or bttv, 
as they support all types of video buffer userspace API's, including overlay
mode), allowing us to test if VB2 is really following the specs (both the
"de facto" and "de jure" specs).

After having one full-featured driver ported to VB2, the other driver conversions
and usages of VB2 will depend mostly on driver maintainer's desire and enough tests.

Cheers,
Mauro.

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Hans Verkuil]

On Tuesday, May 03, 2011 05:28:02 Mauro Carvalho Chehab wrote:
> Em 02-05-2011 23:40, Andy Walls escreveu:
> > Hi All,
> > 
> > Ah crud, what a mess.  Where to begin...?
> > 
> > Where have I been:
> > 
> > On 30 March 2011, my 8-year-old son was diagnosed with Necrotizing
> > Fasciitis caused by Invasive Group A Streptococcous - otherwise known as
> > "Flesh-eating bacteria":
> > 
> > http://en.wikipedia.org/wiki/Necrotizing_fasciitis
> > http://www.ncbi.nlm.nih.gov/pubmedhealth/PMH0002415/
> 
> Sorry to hear about that!
> 
> > By the grace of God, my son was diagnosed very early.  He only lost the
> > fascia on his left side and one lymph node - damage essentially
> > unnoticable to anyone, including my son himself.  His recovery progress
> > is excellent and he is now back to his normal life. Yay! \O/
> 
> Good! I hope him to fully recover from the disease. All the best for you
> and your wife. Our hearts are with you.
> 
> > Naturally, Linux driver development disappeared from my mind during the
> > extended hospital stay, multiple surgeries, and post-hospitalization
> > recovery.
> > 
> > As always; yard-work, house-work, work-work, choir practice, kids'
> > sports, kids' after school clubs, and kids' instrument lessons also
> > consume my time.
> > 
> 
> Completely understandable.
> 
> > History of this patch:
> > 
> > 1. Steven wrote the bulk of it 10 months ago:
> > 
> > 	http://www.kernellabs.com/hg/~stoth/cx18-videobuf/
> > 
> > 2. At Steven's request, I took a day and reviewed it on July 10 2010 and
> > provide comments off-list.  (I will provide them in a follow up to Mauro
> > Devin and Hans).
> 
> Thanks! 
> 
> Next time, please answer it publicly, or if the patch author submitted
> it in priv for a good reason, please c/c me on the review, in order to
> warn me that you have some restrictions about a patch.
> 
> > 3. The patch languished as Steven didn't have time to make the fixes and
> > neither did I.
> > 
> > 4. Videobuf2 came along as did good documentation on the deficiencies of
> > videobuf1:
> > 
> > http://linuxtv.org/downloads/presentations/summit_jun_2010/20100614-v4l2_summit-videobuf.pdf
> > http://linuxtv.org/downloads/presentations/summit_jun_2010/Videobuf_Helsinki_June2010.pdf
> > http://lwn.net/Articles/415883/
> > 
> > 5. I started independent work to implement videobuf2 for YUV and
> > actually using zero-copy.  My progress is very slow.
> > 
> > http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18-vb2-proto
> > http://git.linuxtv.org/awalls/media_tree.git?a=shortlog;h=refs/heads/cx18_39
> > 
> > 6. Simon submits the patches to the list as one big patch.
> > 
> > 7. Off-list I forward the same 5 emails of comments to Simon as I
> > provided in #2 to Steven.
> 
> In this case, as Simon had opened the source code already for the patch,
> the better would be if you had made a public statement about your nack.
> I always review the ML before applying a patch.
> 
> > 8. Simon addresses most of the comments and provides a revised patch
> > off-list asking for review.  I haven't had time to look at it.
> > 
> > 9. Mauro commits the original patch that Simon submitted to the list.
> > 
> > 
> > My thoughts:
> > 
> > 1. I don't want to stop progress, so I did not NACK this patch.  I don't
> > exactly like the patch either, so I didn't ACK it.
> > 
> > 2. At a minimum someone needs to review Simon's revised patch that tried
> > to address my comments.  That patch has to be better than this one.
> > Hans has already noticed a few of the bugs I pointed out to Steven and
> > Simon.
> > 
> > 3. I value that this patch has been tested, but I am guessing the
> > use-case was limited.  The toughest cx18 use-cases involve a lot of
> > concurrency - multiple stream captures (MPEG, VBI, YUV, PCM) on multiple
> > boards (3 or 4).  I had to do a lot of work with the driver to get that
> > concurrency reliable and performing well.  Has this been tested post-BKL
> > removal?  Have screen sizes other than the full-screen size been tested?
> > 
> > 4. I do not like using videobuf(1) for this.  Videobuf(1) is a buggy
> > dead-end IMO.  I will NACK any patch that tries to fix anything due to
> > videobuf(1) related problems introduced into cx18 by this patch.
> > There's no point in throwing too much effort into fixing what would
> > likely be unfixable.
> > 
> > 5. When I am done with my videobuf2 stuff for cx18, I will essentially
> > revert this one and add in my new implementation after sufficient
> > testing.  Though given the amount of time I have for this, maybe the
> > last HVR-1600 will be dead before then.
> > 
> > 
> > Summary:
> > 
> > 1. I'm not going to fix any YUV related problems merging this patch
> > causes.  It's the YUV stream of an MPEG capture card that's more
> > expensive than a simple frame grabber.  (I've only heard of it being
> > used for live play of video games and of course for Simon's
> > application.)
> > 
> > 2. I'd at least like Simon's revised patch to be merged instead, to fix
> > the known deficincies in this one.
> 
> IMO, the proper workflow would be that Simon should send his changes, as
> a diff patch against the current one. We can all review it, based on the
> comments you sent in priv and fix it.

I disagree. The proper workflow in this particular instance is to revert the
patch, have Simon post the revised patch to the list and have it reviewed on
the list.

As Andy noticed, in this particular case the whole procedure was a mess due
to completely understandable reasons. Nobody is to blame, it's just one of
those things that happens.

Reading through the comments Andy made regarding this patch it is clear to
me that there are too many issues with this patch.

Anyway, I stand by my NACK.

> As it seems that that the patch offers a subset of the desired features
> that you're planning with your approach, maybe the better would be to add
> a CONFIG var to enable YUV support, stating that such feature is experimental.
> 
> > 3. If merging this patch, means a change to videobuf2 in the future is
> > not allowed, than I'd prefer to NACK the patch that introduces
> > videobuf(1) into cx18.
> 
> The addition of VB1 first doesn't imply that VB2 would be acked or nacked.
> 
> In any case, the first non-embedded VB2 driver will need a very careful
> review, to be sure that they won't break any userspace applications. 
> On embedded hardware, only a limited set of applications are supported, and they
> are patched and bundled together with the hardware, so there's not much concern
> about userspace apps breakage.
> 
> However, on non-embedded hardware, we should be sure that no regressions to
> existing applications will happen. So, the better would be if the first VB2 
> non-embedded driver to be a full-featured V4L2 board (e. g. saa7134 or bttv, 
> as they support all types of video buffer userspace API's, including overlay
> mode), allowing us to test if VB2 is really following the specs (both the
> "de facto" and "de jure" specs).

I fail to see why we can't implement vb2 in cx18. And vb2 has been tested
extensively already with respect to the spec. vivi is using it, and I'm doing
a lot of testing with that driver.

Note that the current set of drivers behaves different already depending on
whether videobuf is used or not. Drivers like UVC follow the spec, drivers
based on videobuf don't. It's a big freakin' mess.

Regards,

	Hans
 
> After having one full-featured driver ported to VB2, the other driver conversions
> and usages of VB2 will depend mostly on driver maintainer's desire and enough tests.
> 
> Cheers,
> Mauro.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Mauro Carvalho Chehab]

Em 03-05-2011 02:15, Hans Verkuil escreveu:
> On Tuesday, May 03, 2011 05:28:02 Mauro Carvalho Chehab wrote:
>>> 2. I'd at least like Simon's revised patch to be merged instead, to fix
>>> the known deficincies in this one.
>>
>> IMO, the proper workflow would be that Simon should send his changes, as
>> a diff patch against the current one. We can all review it, based on the
>> comments you sent in priv and fix it.
> 
> I disagree. The proper workflow in this particular instance is to revert the
> patch, have Simon post the revised patch to the list and have it reviewed on
> the list.
> 
> As Andy noticed, in this particular case the whole procedure was a mess due
> to completely understandable reasons. Nobody is to blame, it's just one of
> those things that happens.
> 
> Reading through the comments Andy made regarding this patch it is clear to
> me that there are too many issues with this patch.
> 
> Anyway, I stand by my NACK.

I won't do anything before seeing the new version. Reverting a patch is painful,
as it means that I need to take extra care when sending upstream, and I'm having
enough headaches with all patchwork issues. I won't do it, except if we can't
have this fixed before the end of the next merge window.

>> As it seems that that the patch offers a subset of the desired features
>> that you're planning with your approach, maybe the better would be to add
>> a CONFIG var to enable YUV support, stating that such feature is experimental.
>>
>>> 3. If merging this patch, means a change to videobuf2 in the future is
>>> not allowed, than I'd prefer to NACK the patch that introduces
>>> videobuf(1) into cx18.
>>
>> The addition of VB1 first doesn't imply that VB2 would be acked or nacked.
>>
>> In any case, the first non-embedded VB2 driver will need a very careful
>> review, to be sure that they won't break any userspace applications. 
>> On embedded hardware, only a limited set of applications are supported, and they
>> are patched and bundled together with the hardware, so there's not much concern
>> about userspace apps breakage.
>>
>> However, on non-embedded hardware, we should be sure that no regressions to
>> existing applications will happen. So, the better would be if the first VB2 
>> non-embedded driver to be a full-featured V4L2 board (e. g. saa7134 or bttv, 
>> as they support all types of video buffer userspace API's, including overlay
>> mode), allowing us to test if VB2 is really following the specs (both the
>> "de facto" and "de jure" specs).
> 
> I fail to see why we can't implement vb2 in cx18.

Where did I said that?  Please, don't understand me wrong, nor put words into my mouth.
All I said is that vb2 is not enough tested.

> And vb2 has been tested
> extensively already with respect to the spec. vivi is using it, and I'm doing
> a lot of testing with that driver.

There are two specs envolved here: the V4L2 API spec and the by practice spec,
used by userspace apps developers when they write their stuff. It is a Linux
requirement that Kernel changes should not break existing applications (no
regressions). This basically means that the "by practice" spec should not be
broken.

I'm not saying that vb2 broke it. All I'm saying is that we don't have enough tests.
vivi is nice to test new features, but only developers use it, and on a restricted
environment. Embedded drivers also use it also on a restricted environment, were
just one application is used, and such application is developed (or patched) to
work for an specific piece of hardware.

I really doubt that, except by people that work with embedded devices, people tried
to use vb2 into a real environment. For example, on the early days of videobuf
split into vb sub-drivers, kernel OOPSes/Panic's were noticed when channels were 
changed, because hardware DMA engine restarts on some hardware, and this caused
some race conditions.

So, before applying vb2 to a driver that will be used by the existing applications,
a wide range of tests with the applications are needed.

> Note that the current set of drivers behaves different already depending on
> whether videobuf is used or not. Drivers like UVC follow the spec, drivers
> based on videobuf don't. It's a big freakin' mess.

The long term solution is to merge all vb stuff into one solution, and I have
good hope that vb2 will be such solution. But before doing that, we need to be
sure that vb2 will work with all kinds of situations covered by vb1, uvc-vb,
gspca-vb, etc. We'll get there one day, but we should not put the cart before 
the horse.

The proper way of doing it is to do convert a ful-featured v4l2 driver that works
fine with vb1 and test it, after its conversion to vb2, if all situations are
covered by vb2, and it it works properly with the existing applications, fixing
it until it works properly. After having one of such drivers properly working,
we can migrate the others to vb2.

Doing the opposite way by adding new drivers to vb2 without even knowing if it
is compliant with the "status quo" is risky and will just add more entropy, as
vb2 is currently just one more vb implementation, that it is still not known to
work for all cases, just like the other existing ones.

Mauro.

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Devin Heitmueller]

Hi Andy,

On Mon, May 2, 2011 at 10:40 PM, Andy Walls <awalls@md.metrocast.net> wrote:
> Hi All,
>
> Ah crud, what a mess.  Where to begin...?
>
> Where have I been:
>
> On 30 March 2011, my 8-year-old son was diagnosed with Necrotizing
> Fasciitis caused by Invasive Group A Streptococcous - otherwise known as
> "Flesh-eating bacteria":
>
> http://en.wikipedia.org/wiki/Necrotizing_fasciitis
> http://www.ncbi.nlm.nih.gov/pubmedhealth/PMH0002415/
>
> By the grace of God, my son was diagnosed very early.  He only lost the
> fascia on his left side and one lymph node - damage essentially
> unnoticable to anyone, including my son himself.  His recovery progress
> is excellent and he is now back to his normal life. Yay! \O/

I am very sorry to hear about this, and am happy to hear he is doing
better now.  My thoughts go out to you and your family.

> Naturally, Linux driver development disappeared from my mind during the
> extended hospital stay, multiple surgeries, and post-hospitalization
> recovery.
>
> As always; yard-work, house-work, work-work, choir practice, kids'
> sports, kids' after school clubs, and kids' instrument lessons also
> consume my time.

Lest we not forget our relative priorities.  LinuxTV isn't saving the
world: it's making it easier for people to watch television.  I think
everyone here would be appalled if your priorities were reversed just
for the benefit of fixing TV tuners.

> History of this patch:
<snip>

> My thoughts:
>
> 1. I don't want to stop progress, so I did not NACK this patch.  I don't
> exactly like the patch either, so I didn't ACK it.

I can certainly appreciate this, as I've been in this situation myself
more than once.

> 2. At a minimum someone needs to review Simon's revised patch that tried
> to address my comments.  That patch has to be better than this one.
> Hans has already noticed a few of the bugs I pointed out to Steven and
> Simon.

Admittedly it's unfortunate that we didn't know there was a newer
version of the patch, and it would definitely be a shame to see
Simon's incremental work go to waste.  That said, it would be nice to
perhaps see the incremental improvements separated out into a separate
patch from Steven's original, so we can understand what constitutes
the original work versus what were the cleanup/improvements made by
Simon.

> 3. I value that this patch has been tested, but I am guessing the
> use-case was limited.  The toughest cx18 use-cases involve a lot of
> concurrency - multiple stream captures (MPEG, VBI, YUV, PCM) on multiple
> boards (3 or 4).  I had to do a lot of work with the driver to get that
> concurrency reliable and performing well.  Has this been tested post-BKL
> removal?  Have screen sizes other than the full-screen size been tested?

Good questions.  Simon?

> 4. I do not like using videobuf(1) for this.  Videobuf(1) is a buggy
> dead-end IMO.  I will NACK any patch that tries to fix anything due to
> videobuf(1) related problems introduced into cx18 by this patch.
> There's no point in throwing too much effort into fixing what would
> likely be unfixable.

I don't think we're trying to make videobuf1 into something it's not.
The goal here is feature parity with other VB1 based devices.  In
fact, it's not even that:  it's just being able to meet the userland
API requirements related to providing video frames instead of a stream
of YUV bytes.

> 5. When I am done with my videobuf2 stuff for cx18, I will essentially
> revert this one and add in my new implementation after sufficient
> testing.  Though given the amount of time I have for this, maybe the
> last HVR-1600 will be dead before then.

I don't see why anyone would have any objection to this provided it
doesn't result in any regression in functionality.  The only concerns
I would have were if the VB2 cutover was submitted before fully baked,
resulting in some loss of existing functionality that was considered
important to the user base.  And of course the more practical concern
that it's unclear even by your own admission when/if this would
actually happen.

> Summary:
>
> 1. I'm not going to fix any YUV related problems merging this patch
> causes.  It's the YUV stream of an MPEG capture card that's more
> expensive than a simple frame grabber.  (I've only heard of it being
> used for live play of video games and of course for Simon's
> application.)

Seems reasonable.  To my knowledge nobody has been using the cx18 YUV
support anyway other than Simon, which is what prompted his
contributions in the first place.  In fact in the long term I would be
perfectly happy to see /dev/video32 disappear entirely since it just
causes confusion for regular users trying to figure out what video
device to choose.

> 2. I'd at least like Simon's revised patch to be merged instead, to fix
> the known deficincies in this one.

Agreed.

> 3. If merging this patch, means a change to videobuf2 in the future is
> not allowed, than I'd prefer to NACK the patch that introduces
> videobuf(1) into cx18.

I cannot imagine any case where moving the driver to VB2 would be
blocked by having put in this patch.  I think everybody agrees that
VB2 is the long-term solution.  It's just a question of who incurs the
cost of conversion and at what point in time.

Devin

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

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Devin Heitmueller]

On Mon, May 2, 2011 at 5:31 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> It's also a good idea if the author of a patch pings the list if there
> has been no feedback after one or two weeks. It's easy to forget patches,
> people can be on vacation, be sick, or in the case of Andy, have a family
> emergency.

In principle I agree with you, and I was actually surprised to hear it
was merged.  That said, what's done is done so we need to focus on
where to go from here.

>> Likewise, I know there have indeed been cases in the past where code
>> got upstream that caused regressions (in fact, you have personally
>> been responsible for some of these if I recall).
>>
>> Let's not throw the baby out with the bathwater.  If there are real
>> structural issues with the patch, then let's get them fixed.  But if
>> we're just talking about a few minor "unused variable" type of
>> aesthetic issues, then that shouldn't constitute reverting the commit.
>>  Do your review, and if an additional patch is needed with a half
>> dozen removals of dead/unused code, then so be it.
>
> Well, one structural thing I am not at all happy about (but it is Andy's
> call) is that it uses videobuf instead of vb2. Since this patch only deals
> with YUV it shouldn't be hard to use vb2. The problem with videobuf is that
> it violates the V4L2 spec in several places so I would prefer not to use
> videobuf in cx18. If only because converting cx18 to vb2 later will change
> the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is
> something I would like to avoid if possible.
>
> I know that Andy started work on vb2 in cx18 for all stream types (not just
> YUV). I have no idea of the current state of that work. But it might be a
> good starting point to use this patch and convert it to vb2. Later Andy can
> add vb2 support for the other stream types.

Sure Hans.  Let me just dig into my collection of 30+ products and
grab one that has already been converted to VB2 which I can use as a
reference for porting.  Should be simple enough...

cx88: nope
cx23885: nope
cx18: nope
ivtv: nope
em28xx: nope
au0828: nope
pvrusb2: nope
cx231xx: nope
saa7134: nope
saa7164: nope
tm6010: nope
dib0700: nope
bttv: nope

Oh wait, you mean that there aren't *any* non-embedded drivers that
currently implement VB2?  Vivi is the *only* example, and it's not
even real hardware so who knows what issues with the architecture we
might run into?

And exactly what real-world applications has VB2 been validated
against?  Any apps that aren't just a test harness or written by an
SOC vendor making it work against their one piece of embedded
hardware?  Any consumer apps?  Mplayer?  VLC?  Kaffeine?  tvtime?
XawTV?  MeTV?  MythTV?

VB2 may be the future of buffering models and it may actually be
better in the long-run, but if you want to see adoption outside of the
SOC space then you need to prove that it works against real hardware
that isn't an SOC, and demonstrate that it doesn't cause regressions
in real-world applications that people are using today.

Let's talk about what's going to happen in the real world:  the first
guy who actually ports one of the above drivers to VB2 is going to run
into bugs.  He's going to have to work with you to shake out those
bugs.  And it wouldn't surprise me if it exposes some bugs in some
existing applications, which are going to have to be fixed too.  In
the end we'll eventually end up in a better situation, but the cost
will be non-trivial and it will be incurred by people who don't really
give a damn about VB2 since it has little end-user visible benefit.

If you had ported any of the above drivers to the VB2 framework and
demonstrated that it works with existing applications without
modifications, then I think everybody here would breathe much easier.
But the current state today is "experimental, not implemented in any
consumer products or validated in any real-world usage outside of
SOC".

Asking us to be the "guinea pig" for this new framework just because
cx18 is the most recent driver to get a videobuf related patch just
isn't appropriate.

Devin

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

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Hans Verkuil]

On Tuesday, May 03, 2011 14:49:43 Devin Heitmueller wrote:
> On Mon, May 2, 2011 at 5:31 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > It's also a good idea if the author of a patch pings the list if there
> > has been no feedback after one or two weeks. It's easy to forget patches,
> > people can be on vacation, be sick, or in the case of Andy, have a family
> > emergency.
> 
> In principle I agree with you, and I was actually surprised to hear it
> was merged.  That said, what's done is done so we need to focus on
> where to go from here.
> 
> >> Likewise, I know there have indeed been cases in the past where code
> >> got upstream that caused regressions (in fact, you have personally
> >> been responsible for some of these if I recall).
> >>
> >> Let's not throw the baby out with the bathwater.  If there are real
> >> structural issues with the patch, then let's get them fixed.  But if
> >> we're just talking about a few minor "unused variable" type of
> >> aesthetic issues, then that shouldn't constitute reverting the commit.
> >>  Do your review, and if an additional patch is needed with a half
> >> dozen removals of dead/unused code, then so be it.
> >
> > Well, one structural thing I am not at all happy about (but it is Andy's
> > call) is that it uses videobuf instead of vb2. Since this patch only deals
> > with YUV it shouldn't be hard to use vb2. The problem with videobuf is 
that
> > it violates the V4L2 spec in several places so I would prefer not to use
> > videobuf in cx18. If only because converting cx18 to vb2 later will change
> > the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is
> > something I would like to avoid if possible.
> >
> > I know that Andy started work on vb2 in cx18 for all stream types (not 
just
> > YUV). I have no idea of the current state of that work. But it might be a
> > good starting point to use this patch and convert it to vb2. Later Andy 
can
> > add vb2 support for the other stream types.
> 
> Sure Hans.  Let me just dig into my collection of 30+ products and
> grab one that has already been converted to VB2 which I can use as a
> reference for porting.  Should be simple enough...
> 
> cx88: nope
> cx23885: nope
> cx18: nope
> ivtv: nope
> em28xx: nope
> au0828: nope
> pvrusb2: nope
> cx231xx: nope
> saa7134: nope
> saa7164: nope
> tm6010: nope
> dib0700: nope
> bttv: nope
> 
> Oh wait, you mean that there aren't *any* non-embedded drivers that
> currently implement VB2?  Vivi is the *only* example, and it's not
> even real hardware so who knows what issues with the architecture we
> might run into?
> 
> And exactly what real-world applications has VB2 been validated
> against?  Any apps that aren't just a test harness or written by an
> SOC vendor making it work against their one piece of embedded
> hardware?  Any consumer apps?  Mplayer?  VLC?  Kaffeine?  tvtime?
> XawTV?  MeTV?  MythTV?
> 
> VB2 may be the future of buffering models and it may actually be
> better in the long-run, but if you want to see adoption outside of the
> SOC space then you need to prove that it works against real hardware
> that isn't an SOC, and demonstrate that it doesn't cause regressions
> in real-world applications that people are using today.
> 
> Let's talk about what's going to happen in the real world:  the first
> guy who actually ports one of the above drivers to VB2 is going to run
> into bugs.  He's going to have to work with you to shake out those
> bugs.  And it wouldn't surprise me if it exposes some bugs in some
> existing applications, which are going to have to be fixed too.  In
> the end we'll eventually end up in a better situation, but the cost
> will be non-trivial and it will be incurred by people who don't really
> give a damn about VB2 since it has little end-user visible benefit.
> 
> If you had ported any of the above drivers to the VB2 framework and
> demonstrated that it works with existing applications without
> modifications, then I think everybody here would breathe much easier.
> But the current state today is "experimental, not implemented in any
> consumer products or validated in any real-world usage outside of
> SOC".
> 
> Asking us to be the "guinea pig" for this new framework just because
> cx18 is the most recent driver to get a videobuf related patch just
> isn't appropriate.

I don't get it.

What better non-embedded driver to implement vb2 in than one that doesn't yet 
do stream I/O? The risks of breaking anything are much smaller and it would be 
a good 'gentle introduction' to vb2. Also, it prevents the unnecessary 
overhead of having to replace videobuf in the future in cx18.

The problem is no doubt different agendas. You want to have your code 
upstreamed. I want to have code upstreamed that uses the latest frameworks.
And the only way to prove that vb2 works is to use it. Saying "it's unproven, 
so let's not use it" is silly. The right approach IMHO is to implement it in 
new drivers, and ensure that the author(s) of the framework give high priority 
to fixing any issues that may surface.

Anyway, converting bttv to vb2 is steadily getting higher on my TODO list. 
Unfortunately there is still a large number of other items that are also on 
that list. I'd love to have more time for this, and things actually may 
improve in the future, but not any time soon :-(

Regards,

	Hans

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Simon Farnsworth]

On Tuesday 3 May 2011, Hans Verkuil <hansverk@cisco.com> wrote:
> On Tuesday, May 03, 2011 14:49:43 Devin Heitmueller wrote:
> > Asking us to be the "guinea pig" for this new framework just because
> > cx18 is the most recent driver to get a videobuf related patch just
> > isn't appropriate.
> 
> I don't get it.
> 
> What better non-embedded driver to implement vb2 in than one that doesn't
> yet do stream I/O? The risks of breaking anything are much smaller and it
> would be a good 'gentle introduction' to vb2. Also, it prevents the
> unnecessary overhead of having to replace videobuf in the future in cx18.
> 
Just for everyone's information; I've been caught up in other issues here, so 
I'm unlikely to get onto changing videobuf to vb2 in this code in the next 
week or so.

However, if someone else had just enough free time to convert the existing 
patch for me, I can free up enough time to test with our application and with 
GStreamer, to confirm that the conversion works adequately.
-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Mauro Carvalho Chehab]

Em 03-05-2011 10:59, Hans Verkuil escreveu:
> On Tuesday, May 03, 2011 14:49:43 Devin Heitmueller wrote:

> What better non-embedded driver to implement vb2 in than one that doesn't yet 
> do stream I/O? The risks of breaking anything are much smaller and it would be 
> a good 'gentle introduction' to vb2. 

The risk is there even on this case: existing applications should work with vb2.
Also, you're discussing about something that we don't have: there's no vb2 patches
for cx18 yet.

> Also, it prevents the unnecessary 
> overhead of having to replace videobuf in the future in cx18.

This overhead already exists, as a vb1 solution is there and there's no vb2 solution
yet.

> The problem is no doubt different agendas. You want to have your code 
> upstreamed. I want to have code upstreamed that uses the latest frameworks.

>From my side, I'm more concerned if vb2 will really support all memory modes that
vb1 already supports, on both kernelspace and userspace API's. I'm not confident
about that yet, and before starting spreading a solution that we don't know for sure
that it will work on non-embedded devices, with similar or better performance than vb1, 
we need to fully test it with one complete driver, before testing on vb subsets, 
in order to fix architectural design problems (if is there any). Before that, 
porting any non-embedded driver to vb2 is premature.

> And the only way to prove that vb2 works is to use it. Saying "it's unproven, 
> so let's not use it" is silly. 

Yes, and nobody said otherwise.

> The right approach IMHO is to implement it in 
> new drivers, and ensure that the author(s) of the framework give high priority 
> to fixing any issues that may surface.

This is where we diverge: while a "pure api/application compliance" might work
with a new driver, you can't compare performance if you don't have the very same 
driver using vb1 against the same driver using vb2. Even for de-facto API compliance
tests, if you find something not working with some application and a new driver, it
is harder to point the fingers if the issue is at VB2 or at the new driver, as a
new driver is, per definition: NEW. So, it is untested.

On the other hand, if you take an existing drivers, convert it to VB2, test it with
some compliant tool and it works, and test with application X and it breaks, you
know for sure that the error is at VB2.

> Anyway, converting bttv to vb2 is steadily getting higher on my TODO list. 
> Unfortunately there is still a large number of other items that are also on 
> that list. I'd love to have more time for this, and things actually may 
> improve in the future, but not any time soon :-(
> 
> Regards,
> 
> 	Hans

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Hans Verkuil]

On Tuesday, May 03, 2011 17:03:13 Mauro Carvalho Chehab wrote:
> Em 03-05-2011 10:59, Hans Verkuil escreveu:
> > On Tuesday, May 03, 2011 14:49:43 Devin Heitmueller wrote:
> 
> > What better non-embedded driver to implement vb2 in than one that doesn't yet 
> > do stream I/O? The risks of breaking anything are much smaller and it would be 
> > a good 'gentle introduction' to vb2. 
> 
> The risk is there even on this case: existing applications should work with vb2.
> Also, you're discussing about something that we don't have: there's no vb2 patches
> for cx18 yet.
> 
> > Also, it prevents the unnecessary 
> > overhead of having to replace videobuf in the future in cx18.
> 
> This overhead already exists, as a vb1 solution is there and there's no vb2 solution
> yet.
> 
> > The problem is no doubt different agendas. You want to have your code 
> > upstreamed. I want to have code upstreamed that uses the latest frameworks.
> 
> From my side, I'm more concerned if vb2 will really support all memory modes that
> vb1 already supports, on both kernelspace and userspace API's. I'm not confident
> about that yet, and before starting spreading a solution that we don't know for sure
> that it will work on non-embedded devices, with similar or better performance than vb1, 
> we need to fully test it with one complete driver, before testing on vb subsets, 
> in order to fix architectural design problems (if is there any). Before that, 
> porting any non-embedded driver to vb2 is premature.
> 
> > And the only way to prove that vb2 works is to use it. Saying "it's unproven, 
> > so let's not use it" is silly. 
> 
> Yes, and nobody said otherwise.
> 
> > The right approach IMHO is to implement it in 
> > new drivers, and ensure that the author(s) of the framework give high priority 
> > to fixing any issues that may surface.
> 
> This is where we diverge: while a "pure api/application compliance" might work
> with a new driver, you can't compare performance if you don't have the very same 
> driver using vb1 against the same driver using vb2. Even for de-facto API compliance
> tests, if you find something not working with some application and a new driver, it
> is harder to point the fingers if the issue is at VB2 or at the new driver, as a
> new driver is, per definition: NEW. So, it is untested.
> 
> On the other hand, if you take an existing drivers, convert it to VB2, test it with
> some compliant tool and it works, and test with application X and it breaks, you
> know for sure that the error is at VB2.

It sounds great, but the problem is that there is little incentive to convert existing
drivers to vb2. Take cx18: someone obviously wants to get this code in. So if you
require vb2 then there is an incentive to do that work. If the current version is
accepted, then the incentive to move to vb2 is gone. If you are lucky you can find
developers like Andy who might look at it. Look at how long it took to get rid of
V4L1. Or how long it takes to convert drivers to video_ioctl2? Or the control
framework?

It is much easier to put such requirements on incoming code. Once it is in it can
take a very long time to convert code to a newer framework.

Regards,

	Hans

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Simon Farnsworth]

On Monday 2 May 2011, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Em 02-05-2011 16:11, Hans Verkuil escreveu:
> > NACK.
> > 
> > For two reasons: first of all it is not signed off by Andy Walls, the
> > cx18 maintainer. I know he has had other things on his plate recently
> > which is probably why he hasn't had the chance to review this.
> > 
> > Secondly, while doing a quick scan myself I noticed that this code does a
> > conversion from UYVY format to YUYV *in the driver*. Format conversion is
> > not allowed in the kernel, we have libv4lconvert for that. So at the
> > minimum this conversion code must be removed first.
> 
> Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or
> andy were against it, why none of you commented it there?
> 
> Now that the patch were committed, I won't revert it without a very good
> reason.
> 
> With respect to the "conversion from UYVY format to YUYV", a simple patch
> could fix it, instead of removing the entire patchset.
> 
> Steven/Simon,
> could you please work on such change?
> 
I received feedback, which I've been working on, and have converted to a new 
patch against the baseline tree without this patch applied. I can obviously 
rebase (thanks, git!) so that it applies on top of this patch. It massively 
cleans up the code, fixes a bug, and removes the in-kernel format conversion 
(we use libv4l here anyway, so it's not needed)

I have one more work item, requested by Andy and Hans, and that's to convert 
just the YUV capture from videobuf to vb2, so that when Andy's got spare time 
again, it'll be easier for him to convert the whole driver. I've been delayed 
on this by other work committments, but I do have this on my schedule.

How do you want me to proceed?
-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/

Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture

[Mauro Carvalho Chehab]

Em 03-05-2011 06:03, Simon Farnsworth escreveu:
> On Monday 2 May 2011, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>> Em 02-05-2011 16:11, Hans Verkuil escreveu:
>>> NACK.
>>>
>>> For two reasons: first of all it is not signed off by Andy Walls, the
>>> cx18 maintainer. I know he has had other things on his plate recently
>>> which is probably why he hasn't had the chance to review this.
>>>
>>> Secondly, while doing a quick scan myself I noticed that this code does a
>>> conversion from UYVY format to YUYV *in the driver*. Format conversion is
>>> not allowed in the kernel, we have libv4lconvert for that. So at the
>>> minimum this conversion code must be removed first.
>>
>> Patch is there at the ML since Apr, 6 and nobody acked/nacked it. If you or
>> andy were against it, why none of you commented it there?
>>
>> Now that the patch were committed, I won't revert it without a very good
>> reason.
>>
>> With respect to the "conversion from UYVY format to YUYV", a simple patch
>> could fix it, instead of removing the entire patchset.
>>
>> Steven/Simon,
>> could you please work on such change?
>>
> I received feedback, which I've been working on, and have converted to a new 
> patch against the baseline tree without this patch applied. I can obviously 
> rebase (thanks, git!) so that it applies on top of this patch. It massively 
> cleans up the code, fixes a bug, and removes the in-kernel format conversion 
> (we use libv4l here anyway, so it's not needed)
> 
> I have one more work item, requested by Andy and Hans, and that's to convert 
> just the YUV capture from videobuf to vb2, so that when Andy's got spare time 
> again, it'll be easier for him to convert the whole driver. I've been delayed 
> on this by other work committments, but I do have this on my schedule.
> 
> How do you want me to proceed?

Please send a rebased version.

Thanks,
Mauro.

[PATCH] cx18: Clean up mmap() support for raw YUV

[Simon Farnsworth]

The initial version of this patch (commit
d5976931639176bb6777755d96b9f8d959f79e9e) had some issues:

 * It didn't correctly calculate the size of the YUV buffer for 4:2:2,
   resulting in capture sometimes being offset by 1/3rd of a picture.

 * There were a lot of variables duplicating information the driver
   already knew, which have been removed.

 * There was an in-kernel format conversion - libv4l can do this one,
   and is the right place to do format conversions anyway.

 * Some magic numbers weren't properly explained.

Fix all these issues, leaving just the move from videobuf to videobuf2
to do.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
 drivers/media/video/cx18/Kconfig        |    1 -
 drivers/media/video/cx18/cx18-driver.h  |    8 +-
 drivers/media/video/cx18/cx18-fileops.c |  167 +++----------------------------
 drivers/media/video/cx18/cx18-ioctl.c   |   84 +++++++++------
 drivers/media/video/cx18/cx18-mailbox.c |   17 +---
 drivers/media/video/cx18/cx18-streams.c |  160 ++++++++++++++++++++++++++++-
 6 files changed, 225 insertions(+), 212 deletions(-)

diff --git a/drivers/media/video/cx18/Kconfig b/drivers/media/video/cx18/Kconfig
index 9c23202..53b3c77 100644
--- a/drivers/media/video/cx18/Kconfig
+++ b/drivers/media/video/cx18/Kconfig
@@ -2,7 +2,6 @@ config VIDEO_CX18
 	tristate "Conexant cx23418 MPEG encoder support"
 	depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
 	select I2C_ALGOBIT
-	select VIDEOBUF_DVB
 	select VIDEOBUF_VMALLOC
 	depends on RC_CORE
 	select VIDEO_TUNER
diff --git a/drivers/media/video/cx18/cx18-driver.h b/drivers/media/video/cx18/cx18-driver.h
index 70e1e04..0864272 100644
--- a/drivers/media/video/cx18/cx18-driver.h
+++ b/drivers/media/video/cx18/cx18-driver.h
@@ -412,11 +412,11 @@ struct cx18_stream {
 	u32 pixelformat;
 	struct list_head vb_capture;    /* video capture queue */
 	spinlock_t vb_lock;
-	struct v4l2_framebuffer fbuf;
-	v4l2_std_id tvnorm; /* selected tv norm */
 	struct timer_list vb_timeout;
-	int vbwidth;
-	int vbheight;
+
+	struct videobuf_queue vbuf_q;
+	spinlock_t vbuf_q_lock; /* Protect vbuf_q */
+	enum v4l2_buf_type vb_type;
 };
 
 struct cx18_videobuf_buffer {
diff --git a/drivers/media/video/cx18/cx18-fileops.c b/drivers/media/video/cx18/cx18-fileops.c
index c74eafd..6609222 100644
--- a/drivers/media/video/cx18/cx18-fileops.c
+++ b/drivers/media/video/cx18/cx18-fileops.c
@@ -598,9 +598,9 @@ ssize_t cx18_v4l2_read(struct file *filp, char __user *buf, size_t count,
 	if (rc)
 		return rc;
 
-	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
-		return videobuf_read_stream(&id->vbuf_q, buf, count, pos, 0,
+		return videobuf_read_stream(&s->vbuf_q, buf, count, pos, 0,
 			filp->f_flags & O_NONBLOCK);
 	}
 
@@ -629,9 +629,13 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp, poll_table *wait)
 		CX18_DEBUG_FILE("Encoder poll started capture\n");
 	}
 
-	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
-		return videobuf_poll_stream(filp, &id->vbuf_q, wait);
+		int videobuf_poll = videobuf_poll_stream(filp, &s->vbuf_q, wait);
+                if (eof && videobuf_poll == POLLERR)
+                        return POLLHUP;
+                else
+                        return videobuf_poll;
 	}
 
 	/* add stream's waitq to the poll list */
@@ -652,7 +656,7 @@ int cx18_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
 	struct cx18_stream *s = &cx->streams[id->type];
 	int eof = test_bit(CX18_F_S_STREAMOFF, &s->s_flags);
 
-	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
 
 		/* Start a capture if there is none */
@@ -668,10 +672,10 @@ int cx18_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
 					s->name, rc);
 				return -EINVAL;
 			}
-			CX18_DEBUG_FILE("Encoder poll started capture\n");
+			CX18_DEBUG_FILE("Encoder mmap started capture\n");
 		}
 
-		return videobuf_mmap_mapper(&id->vbuf_q, vma);
+		return videobuf_mmap_mapper(&s->vbuf_q, vma);
 	}
 
 	return -EINVAL;
@@ -788,142 +792,6 @@ int cx18_v4l2_close(struct file *filp)
 	return 0;
 }
 
-void cx18_dma_free(struct videobuf_queue *q,
-	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
-{
-	videobuf_waiton(q, &buf->vb, 0, 0);
-	videobuf_vmalloc_free(&buf->vb);
-	buf->vb.state = VIDEOBUF_NEEDS_INIT;
-}
-
-static int cx18_prepare_buffer(struct videobuf_queue *q,
-	struct cx18_stream *s,
-	struct cx18_videobuf_buffer *buf,
-	u32 pixelformat,
-	unsigned int width, unsigned int height,
-	enum v4l2_field field)
-{
-	int rc = 0;
-
-	/* check settings */
-	buf->bytes_used = 0;
-
-	if ((width  < 48) || (height < 32))
-		return -EINVAL;
-
-	buf->vb.size = (width * height * 16 /*fmt->depth*/) >> 3;
-	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
-		return -EINVAL;
-
-	/* alloc + fill struct (if changed) */
-	if (buf->vb.width != width || buf->vb.height != height ||
-	    buf->vb.field != field || s->pixelformat != pixelformat ||
-	    buf->tvnorm != s->tvnorm) {
-
-		buf->vb.width  = width;
-		buf->vb.height = height;
-		buf->vb.field  = field;
-		buf->tvnorm    = s->tvnorm;
-		s->pixelformat = pixelformat;
-
-		cx18_dma_free(q, s, buf);
-	}
-
-	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
-		return -EINVAL;
-
-	if (buf->vb.field == 0)
-		buf->vb.field = V4L2_FIELD_INTERLACED;
-
-	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
-		buf->vb.width  = width;
-		buf->vb.height = height;
-		buf->vb.field  = field;
-		buf->tvnorm    = s->tvnorm;
-		s->pixelformat = pixelformat;
-
-		rc = videobuf_iolock(q, &buf->vb, &s->fbuf);
-		if (rc != 0)
-			goto fail;
-	}
-	buf->vb.state = VIDEOBUF_PREPARED;
-	return 0;
-
-fail:
-	cx18_dma_free(q, s, buf);
-	return rc;
-
-}
-
-#define VB_MIN_BUFFERS 32
-#define VB_MIN_BUFSIZE 0x208000
-
-static int buffer_setup(struct videobuf_queue *q,
-	unsigned int *count, unsigned int *size)
-{
-	struct cx18_open_id *id = q->priv_data;
-	struct cx18 *cx = id->cx;
-	struct cx18_stream *s = &cx->streams[id->type];
-
-	*size = 2 * s->vbwidth * s->vbheight;
-	if (*count == 0)
-		*count = VB_MIN_BUFFERS;
-
-	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
-		(*count)--;
-
-	q->field = V4L2_FIELD_INTERLACED;
-	q->last = V4L2_FIELD_INTERLACED;
-
-	return 0;
-}
-
-static int buffer_prepare(struct videobuf_queue *q,
-	struct videobuf_buffer *vb,
-	enum v4l2_field field)
-{
-	struct cx18_videobuf_buffer *buf =
-		container_of(vb, struct cx18_videobuf_buffer, vb);
-	struct cx18_open_id *id = q->priv_data;
-	struct cx18 *cx = id->cx;
-	struct cx18_stream *s = &cx->streams[id->type];
-
-	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
-		s->vbwidth, s->vbheight, field);
-}
-
-static void buffer_release(struct videobuf_queue *q,
-	struct videobuf_buffer *vb)
-{
-	struct cx18_videobuf_buffer *buf =
-		container_of(vb, struct cx18_videobuf_buffer, vb);
-	struct cx18_open_id *id = q->priv_data;
-	struct cx18 *cx = id->cx;
-	struct cx18_stream *s = &cx->streams[id->type];
-
-	cx18_dma_free(q, s, buf);
-}
-
-static void buffer_queue(struct videobuf_queue *q, struct videobuf_buffer *vb)
-{
-	struct cx18_videobuf_buffer *buf =
-		container_of(vb, struct cx18_videobuf_buffer, vb);
-	struct cx18_open_id *id = q->priv_data;
-	struct cx18 *cx = id->cx;
-	struct cx18_stream *s = &cx->streams[id->type];
-
-	buf->vb.state = VIDEOBUF_QUEUED;
-
-	list_add_tail(&buf->vb.queue, &s->vb_capture);
-}
-
-static struct videobuf_queue_ops cx18_videobuf_qops = {
-	.buf_setup    = buffer_setup,
-	.buf_prepare  = buffer_prepare,
-	.buf_queue    = buffer_queue,
-	.buf_release  = buffer_release,
-};
-
 static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
 {
 	struct cx18 *cx = s->cx;
@@ -942,8 +810,8 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
 	item->cx = cx;
 	item->type = s->type;
 
-	spin_lock_init(&item->s_lock);
-	item->vb_type = 0;
+	spin_lock_init(&s->vbuf_q_lock);
+	s->vb_type = 0;
 
 	item->open_id = cx->open_id++;
 	filp->private_data = &item->fh;
@@ -979,15 +847,6 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
 		/* Done! Unmute and continue. */
 		cx18_unmute(cx);
 	}
-	if (item->type == CX18_ENC_STREAM_TYPE_YUV) {
-		item->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-		videobuf_queue_vmalloc_init(&item->vbuf_q, &cx18_videobuf_qops,
-			&cx->pci_dev->dev, &item->s_lock,
-			V4L2_BUF_TYPE_VIDEO_CAPTURE,
-			V4L2_FIELD_INTERLACED,
-			sizeof(struct cx18_videobuf_buffer),
-			item, &cx->serialize_lock);
-	}
 	v4l2_fh_add(&item->fh);
 	return 0;
 }
diff --git a/drivers/media/video/cx18/cx18-ioctl.c b/drivers/media/video/cx18/cx18-ioctl.c
index 777d726..1933d4d 100644
--- a/drivers/media/video/cx18/cx18-ioctl.c
+++ b/drivers/media/video/cx18/cx18-ioctl.c
@@ -41,18 +41,6 @@
 #include <media/tveeprom.h>
 #include <media/v4l2-chip-ident.h>
 
-static struct v4l2_fmtdesc formats[] = {
-	{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
-	  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
-	},
-	{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
-	  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
-	},
-	{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
-	  "YUYV 4:2:2", V4L2_PIX_FMT_YUYV, { 0, 0, 0, 0 }
-	},
-};
-
 u16 cx18_service2vbi(int type)
 {
 	switch (type) {
@@ -172,8 +160,12 @@ static int cx18_g_fmt_vid_cap(struct file *file, void *fh,
 	pixfmt->priv = 0;
 	if (id->type == CX18_ENC_STREAM_TYPE_YUV) {
 		pixfmt->pixelformat = s->pixelformat;
-		/* YUV size is (Y=(h*720) + UV=(h*(720/2))) */
-		pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
+		/* HM12 YUV size is (Y=(h*720) + UV=(h*(720/2)))
+		   UYUV YUV size is (Y=(h*720) + UV=(h*(720))) */
+		if (s->pixelformat == V4L2_PIX_FMT_HM12)
+			pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
+		else
+			pixfmt->sizeimage = pixfmt->height * 720 * 2;
 		pixfmt->bytesperline = 720;
 	} else {
 		pixfmt->pixelformat = V4L2_PIX_FMT_MPEG;
@@ -296,16 +288,15 @@ static int cx18_s_fmt_vid_cap(struct file *file, void *fh,
 	w = fmt->fmt.pix.width;
 	h = fmt->fmt.pix.height;
 
-	s->pixelformat = fmt->fmt.pix.pixelformat;
-	s->vbheight = h;
-	s->vbwidth = w;
-
-	if (cx->cxhdl.width == w && cx->cxhdl.height == h)
+	if (cx->cxhdl.width == w && cx->cxhdl.height == h &&
+	    s->pixelformat == fmt->fmt.pix.pixelformat)
 		return 0;
 
 	if (atomic_read(&cx->ana_capturing) > 0)
 		return -EBUSY;
 
+	s->pixelformat = fmt->fmt.pix.pixelformat;
+
 	mbus_fmt.width = cx->cxhdl.width = w;
 	mbus_fmt.height = cx->cxhdl.height = h;
 	mbus_fmt.code = V4L2_MBUS_FMT_FIXED;
@@ -557,6 +548,18 @@ static int cx18_g_crop(struct file *file, void *fh, struct v4l2_crop *crop)
 static int cx18_enum_fmt_vid_cap(struct file *file, void *fh,
 					struct v4l2_fmtdesc *fmt)
 {
+	static const struct v4l2_fmtdesc formats[] = {
+		{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
+		  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
+		},
+		{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
+		  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
+		},
+		{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
+		  "UYVY 4:2:2", V4L2_PIX_FMT_UYVY, { 0, 0, 0, 0 }
+		},
+	};
+
 	if (fmt->index > ARRAY_SIZE(formats) - 1)
 		return -EINVAL;
 	*fmt = formats[fmt->index];
@@ -874,10 +877,12 @@ static int cx18_g_enc_index(struct file *file, void *fh,
 static struct videobuf_queue *cx18_vb_queue(struct cx18_open_id *id)
 {
 	struct videobuf_queue *q = NULL;
+	struct cx18 *cx = id->cx;
+	struct cx18_stream *s = &cx->streams[id->type];
 
-	switch (id->vb_type) {
+	switch (s->vb_type) {
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
-		q = &id->vbuf_q;
+		q = &s->vbuf_q;
 		break;
 	case V4L2_BUF_TYPE_VBI_CAPTURE:
 		break;
@@ -895,15 +900,15 @@ static int cx18_streamon(struct file *file, void *priv,
 	struct cx18_stream *s = &cx->streams[id->type];
 
 	/* Start the hardware only if we're the video device */
-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
 		return -EINVAL;
 
 	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
 		return -EINVAL;
 
 	/* Establish a buffer timeout */
-	mod_timer(&s->vb_timeout, jiffies + (HZ * 2));
+	mod_timer(&s->vb_timeout, msecs_to_jiffies(2000) + jiffies);
 
 	return videobuf_streamon(cx18_vb_queue(id));
 }
@@ -912,10 +917,12 @@ static int cx18_streamoff(struct file *file, void *priv,
 	enum v4l2_buf_type type)
 {
 	struct cx18_open_id *id = file->private_data;
+	struct cx18 *cx = id->cx;
+	struct cx18_stream *s = &cx->streams[id->type];
 
 	/* Start the hardware only if we're the video device */
-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
 		return -EINVAL;
 
 	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
@@ -928,9 +935,11 @@ static int cx18_reqbufs(struct file *file, void *priv,
 	struct v4l2_requestbuffers *rb)
 {
 	struct cx18_open_id *id = file->private_data;
+	struct cx18 *cx = id->cx;
+	struct cx18_stream *s = &cx->streams[id->type];
 
-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
 		return -EINVAL;
 
 	return videobuf_reqbufs(cx18_vb_queue(id), rb);
@@ -940,9 +949,11 @@ static int cx18_querybuf(struct file *file, void *priv,
 	struct v4l2_buffer *b)
 {
 	struct cx18_open_id *id = file->private_data;
+	struct cx18 *cx = id->cx;
+	struct cx18_stream *s = &cx->streams[id->type];
 
-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
 		return -EINVAL;
 
 	return videobuf_querybuf(cx18_vb_queue(id), b);
@@ -951,9 +962,11 @@ static int cx18_querybuf(struct file *file, void *priv,
 static int cx18_qbuf(struct file *file, void *priv, struct v4l2_buffer *b)
 {
 	struct cx18_open_id *id = file->private_data;
+	struct cx18 *cx = id->cx;
+	struct cx18_stream *s = &cx->streams[id->type];
 
-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
 		return -EINVAL;
 
 	return videobuf_qbuf(cx18_vb_queue(id), b);
@@ -962,8 +975,11 @@ static int cx18_qbuf(struct file *file, void *priv, struct v4l2_buffer *b)
 static int cx18_dqbuf(struct file *file, void *priv, struct v4l2_buffer *b)
 {
 	struct cx18_open_id *id = file->private_data;
-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
+	struct cx18 *cx = id->cx;
+	struct cx18_stream *s = &cx->streams[id->type];
+
+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
 		return -EINVAL;
 
 	return videobuf_dqbuf(cx18_vb_queue(id), b, file->f_flags & O_NONBLOCK);
diff --git a/drivers/media/video/cx18/cx18-mailbox.c b/drivers/media/video/cx18/cx18-mailbox.c
index d4d8873..5ecae93 100644
--- a/drivers/media/video/cx18/cx18-mailbox.c
+++ b/drivers/media/video/cx18/cx18-mailbox.c
@@ -177,7 +177,7 @@ static void cx18_mdl_send_to_videobuf(struct cx18_stream *s,
 	if (list_empty(&s->vb_capture))
 		goto out;
 
-	vb_buf = list_entry(s->vb_capture.next, struct cx18_videobuf_buffer,
+	vb_buf = list_first_entry(&s->vb_capture, struct cx18_videobuf_buffer,
 		vb.queue);
 
 	p = videobuf_to_vmalloc(&vb_buf->vb);
@@ -202,25 +202,14 @@ static void cx18_mdl_send_to_videobuf(struct cx18_stream *s,
 		vb_buf->bytes_used = 0;
 	}
 
-	/* */
 	if (dispatch) {
-
-		if (s->pixelformat == V4L2_PIX_FMT_YUYV) {
-			/* UYVY to YUYV */
-			for (i = 0; i < (720 * 480 * 2); i += 2) {
-				u = *(p + i);
-				*(p + i) = *(p + i + 1);
-				*(p + i + 1) = u;
-			}
-		}
-
-		do_gettimeofday(&vb_buf->vb.ts);
+		ktime_get_ts(&vb_buf->vb.ts);
 		list_del(&vb_buf->vb.queue);
 		vb_buf->vb.state = VIDEOBUF_DONE;
 		wake_up(&vb_buf->vb.done);
 	}
 
-	mod_timer(&s->vb_timeout, jiffies + (HZ / 10));
+	mod_timer(&s->vb_timeout, msecs_to_jiffies(2000) + jiffies);
 
 out:
 	spin_unlock(&s->vb_lock);
diff --git a/drivers/media/video/cx18/cx18-streams.c b/drivers/media/video/cx18/cx18-streams.c
index 53f5e4f..24c9688 100644
--- a/drivers/media/video/cx18/cx18-streams.c
+++ b/drivers/media/video/cx18/cx18-streams.c
@@ -98,6 +98,141 @@ static struct {
 	},
 };
 
+
+void cx18_dma_free(struct videobuf_queue *q,
+	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
+{
+	videobuf_waiton(q, &buf->vb, 0, 0);
+	videobuf_vmalloc_free(&buf->vb);
+	buf->vb.state = VIDEOBUF_NEEDS_INIT;
+}
+
+static int cx18_prepare_buffer(struct videobuf_queue *q,
+	struct cx18_stream *s,
+	struct cx18_videobuf_buffer *buf,
+	u32 pixelformat,
+	unsigned int width, unsigned int height,
+	enum v4l2_field field)
+{
+        struct cx18 *cx = s->cx;
+	int rc = 0;
+
+	/* check settings */
+	buf->bytes_used = 0;
+
+	if ((width  < 48) || (height < 32))
+		return -EINVAL;
+
+	buf->vb.size = (width * height * 2);
+	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
+		return -EINVAL;
+
+	/* alloc + fill struct (if changed) */
+	if (buf->vb.width != width || buf->vb.height != height ||
+	    buf->vb.field != field || s->pixelformat != pixelformat ||
+	    buf->tvnorm != cx->std) {
+
+		buf->vb.width  = width;
+		buf->vb.height = height;
+		buf->vb.field  = field;
+		buf->tvnorm    = cx->std;
+		s->pixelformat = pixelformat;
+
+		cx18_dma_free(q, s, buf);
+	}
+
+	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
+		return -EINVAL;
+
+	if (buf->vb.field == 0)
+		buf->vb.field = V4L2_FIELD_INTERLACED;
+
+	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
+		buf->vb.width  = width;
+		buf->vb.height = height;
+		buf->vb.field  = field;
+		buf->tvnorm    = cx->std;
+		s->pixelformat = pixelformat;
+
+		rc = videobuf_iolock(q, &buf->vb, NULL);
+		if (rc != 0)
+			goto fail;
+	}
+	buf->vb.state = VIDEOBUF_PREPARED;
+	return 0;
+
+fail:
+	cx18_dma_free(q, s, buf);
+	return rc;
+
+}
+
+/* VB_MIN_BUFSIZE is lcm(1440 * 480, 1440 * 576)
+   1440 is a single line of 4:2:2 YUV at 720 luma samples wide
+*/
+#define VB_MIN_BUFFERS 32
+#define VB_MIN_BUFSIZE 4147200
+
+static int buffer_setup(struct videobuf_queue *q,
+	unsigned int *count, unsigned int *size)
+{
+	struct cx18_stream *s = q->priv_data;
+	struct cx18 *cx = s->cx;
+
+	*size = 2 * cx->cxhdl.width * cx->cxhdl.height;
+	if (*count == 0)
+		*count = VB_MIN_BUFFERS;
+
+	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
+		(*count)--;
+
+	q->field = V4L2_FIELD_INTERLACED;
+	q->last = V4L2_FIELD_INTERLACED;
+
+	return 0;
+}
+
+static int buffer_prepare(struct videobuf_queue *q,
+	struct videobuf_buffer *vb,
+	enum v4l2_field field)
+{
+	struct cx18_videobuf_buffer *buf =
+		container_of(vb, struct cx18_videobuf_buffer, vb);
+	struct cx18_stream *s = q->priv_data;
+	struct cx18 *cx = s->cx;
+
+	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
+		cx->cxhdl.width, cx->cxhdl.height, field);
+}
+
+static void buffer_release(struct videobuf_queue *q,
+	struct videobuf_buffer *vb)
+{
+	struct cx18_videobuf_buffer *buf =
+		container_of(vb, struct cx18_videobuf_buffer, vb);
+	struct cx18_stream *s = q->priv_data;
+
+	cx18_dma_free(q, s, buf);
+}
+
+static void buffer_queue(struct videobuf_queue *q, struct videobuf_buffer *vb)
+{
+	struct cx18_videobuf_buffer *buf =
+		container_of(vb, struct cx18_videobuf_buffer, vb);
+	struct cx18_stream *s = q->priv_data;
+
+	buf->vb.state = VIDEOBUF_QUEUED;
+
+	list_add_tail(&buf->vb.queue, &s->vb_capture);
+}
+
+static struct videobuf_queue_ops cx18_videobuf_qops = {
+	.buf_setup    = buffer_setup,
+	.buf_prepare  = buffer_prepare,
+	.buf_queue    = buffer_queue,
+	.buf_release  = buffer_release,
+};
+
 static void cx18_stream_init(struct cx18 *cx, int type)
 {
 	struct cx18_stream *s = &cx->streams[type];
@@ -139,9 +274,18 @@ static void cx18_stream_init(struct cx18 *cx, int type)
 	s->vb_timeout.data = (unsigned long)s;
 	init_timer(&s->vb_timeout);
 	spin_lock_init(&s->vb_lock);
-
-	/* Assume the previous pixel default */
-	s->pixelformat = V4L2_PIX_FMT_HM12;
+	if (type == CX18_ENC_STREAM_TYPE_YUV) {
+		s->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		videobuf_queue_vmalloc_init(&s->vbuf_q, &cx18_videobuf_qops,
+			&cx->pci_dev->dev, &s->vbuf_q_lock,
+			V4L2_BUF_TYPE_VIDEO_CAPTURE,
+			V4L2_FIELD_INTERLACED,
+			sizeof(struct cx18_videobuf_buffer),
+			s, &cx->serialize_lock);
+
+		/* Assume the previous pixel default */
+		s->pixelformat = V4L2_PIX_FMT_HM12;
+	}
 }
 
 static int cx18_prep_dev(struct cx18 *cx, int type)
@@ -374,6 +518,9 @@ void cx18_streams_cleanup(struct cx18 *cx, int unregister)
 		if (vdev == NULL)
 			continue;
 
+		if (type == CX18_ENC_STREAM_TYPE_YUV)
+			videobuf_mmap_free(&cx->streams[type].vbuf_q);
+
 		cx18_stream_free(&cx->streams[type]);
 
 		/* Unregister or release device */
@@ -583,7 +730,10 @@ static void cx18_stream_configure_mdls(struct cx18_stream *s)
 		 * Set the MDL size to the exact size needed for one frame.
 		 * Use enough buffers per MDL to cover the MDL size
 		 */
-		s->mdl_size = 720 * s->cx->cxhdl.height * 3 / 2;
+		if (s->pixelformat == V4L2_PIX_FMT_HM12)
+			s->mdl_size = 720 * s->cx->cxhdl.height * 3 / 2;
+		else
+			s->mdl_size = 720 * s->cx->cxhdl.height * 2;
 		s->bufs_per_mdl = s->mdl_size / s->buf_size;
 		if (s->mdl_size % s->buf_size)
 			s->bufs_per_mdl++;
@@ -736,7 +886,7 @@ int cx18_start_v4l2_encode_stream(struct cx18_stream *s)
 		 * rather than the default HM12 Macroblovk 4:2:0 support.
 		 */
 		if (captype == CAPTURE_CHANNEL_TYPE_YUV) {
-			if (s->pixelformat == V4L2_PIX_FMT_YUYV)
+			if (s->pixelformat == V4L2_PIX_FMT_UYVY)
 				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
 					s->handle, 1);
 			else
-- 
1.7.4

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Hans Verkuil]

Hi Simon,

On Tuesday, May 03, 2011 13:57:40 Simon Farnsworth wrote:
> The initial version of this patch (commit
> d5976931639176bb6777755d96b9f8d959f79e9e) had some issues:
> 
>  * It didn't correctly calculate the size of the YUV buffer for 4:2:2,
>    resulting in capture sometimes being offset by 1/3rd of a picture.
> 
>  * There were a lot of variables duplicating information the driver
>    already knew, which have been removed.
> 
>  * There was an in-kernel format conversion - libv4l can do this one,
>    and is the right place to do format conversions anyway.
> 
>  * Some magic numbers weren't properly explained.
> 
> Fix all these issues, leaving just the move from videobuf to videobuf2
> to do.
> 
> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>

I just wanted to thank you for your work. I hope I never gave the impression
that the whole discussion had anything to do with you. You were just unlucky
enough to trigger a 'to merge or not to merge' and a 'to vb2 or not to vb2'
discussion through no fault of your own.

Just thought I should mention that. I would definitely like to see cx18
working with tvtime and it is valuable work you are doing.

Regards,

	Hans

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Andy Walls]

Simon Farnsworth <simon.farnsworth@onelan.co.uk> wrote:

>The initial version of this patch (commit
>d5976931639176bb6777755d96b9f8d959f79e9e) had some issues:
>
> * It didn't correctly calculate the size of the YUV buffer for 4:2:2,
>   resulting in capture sometimes being offset by 1/3rd of a picture.
>
> * There were a lot of variables duplicating information the driver
>   already knew, which have been removed.
>
> * There was an in-kernel format conversion - libv4l can do this one,
>   and is the right place to do format conversions anyway.
>
> * Some magic numbers weren't properly explained.
>
>Fix all these issues, leaving just the move from videobuf to videobuf2
>to do.
>
>Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
>---
> drivers/media/video/cx18/Kconfig        |    1 -
> drivers/media/video/cx18/cx18-driver.h  |    8 +-
>drivers/media/video/cx18/cx18-fileops.c |  167
>+++----------------------------
> drivers/media/video/cx18/cx18-ioctl.c   |   84 +++++++++------
> drivers/media/video/cx18/cx18-mailbox.c |   17 +---
>drivers/media/video/cx18/cx18-streams.c |  160
>++++++++++++++++++++++++++++-
> 6 files changed, 225 insertions(+), 212 deletions(-)
>
>diff --git a/drivers/media/video/cx18/Kconfig
>b/drivers/media/video/cx18/Kconfig
>index 9c23202..53b3c77 100644
>--- a/drivers/media/video/cx18/Kconfig
>+++ b/drivers/media/video/cx18/Kconfig
>@@ -2,7 +2,6 @@ config VIDEO_CX18
> 	tristate "Conexant cx23418 MPEG encoder support"
> 	depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
> 	select I2C_ALGOBIT
>-	select VIDEOBUF_DVB
> 	select VIDEOBUF_VMALLOC
> 	depends on RC_CORE
> 	select VIDEO_TUNER
>diff --git a/drivers/media/video/cx18/cx18-driver.h
>b/drivers/media/video/cx18/cx18-driver.h
>index 70e1e04..0864272 100644
>--- a/drivers/media/video/cx18/cx18-driver.h
>+++ b/drivers/media/video/cx18/cx18-driver.h
>@@ -412,11 +412,11 @@ struct cx18_stream {
> 	u32 pixelformat;
> 	struct list_head vb_capture;    /* video capture queue */
> 	spinlock_t vb_lock;
>-	struct v4l2_framebuffer fbuf;
>-	v4l2_std_id tvnorm; /* selected tv norm */
> 	struct timer_list vb_timeout;
>-	int vbwidth;
>-	int vbheight;
>+
>+	struct videobuf_queue vbuf_q;
>+	spinlock_t vbuf_q_lock; /* Protect vbuf_q */
>+	enum v4l2_buf_type vb_type;
> };
> 
> struct cx18_videobuf_buffer {
>diff --git a/drivers/media/video/cx18/cx18-fileops.c
>b/drivers/media/video/cx18/cx18-fileops.c
>index c74eafd..6609222 100644
>--- a/drivers/media/video/cx18/cx18-fileops.c
>+++ b/drivers/media/video/cx18/cx18-fileops.c
>@@ -598,9 +598,9 @@ ssize_t cx18_v4l2_read(struct file *filp, char
>__user *buf, size_t count,
> 	if (rc)
> 		return rc;
> 
>-	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
>-		return videobuf_read_stream(&id->vbuf_q, buf, count, pos, 0,
>+		return videobuf_read_stream(&s->vbuf_q, buf, count, pos, 0,
> 			filp->f_flags & O_NONBLOCK);
> 	}
> 
>@@ -629,9 +629,13 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp,
>poll_table *wait)
> 		CX18_DEBUG_FILE("Encoder poll started capture\n");
> 	}
> 
>-	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
>-		return videobuf_poll_stream(filp, &id->vbuf_q, wait);
>+		int videobuf_poll = videobuf_poll_stream(filp, &s->vbuf_q, wait);
>+                if (eof && videobuf_poll == POLLERR)
>+                        return POLLHUP;
>+                else
>+                        return videobuf_poll;
> 	}
> 
> 	/* add stream's waitq to the poll list */
>@@ -652,7 +656,7 @@ int cx18_v4l2_mmap(struct file *file, struct
>vm_area_struct *vma)
> 	struct cx18_stream *s = &cx->streams[id->type];
> 	int eof = test_bit(CX18_F_S_STREAMOFF, &s->s_flags);
> 
>-	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
> 
> 		/* Start a capture if there is none */
>@@ -668,10 +672,10 @@ int cx18_v4l2_mmap(struct file *file, struct
>vm_area_struct *vma)
> 					s->name, rc);
> 				return -EINVAL;
> 			}
>-			CX18_DEBUG_FILE("Encoder poll started capture\n");
>+			CX18_DEBUG_FILE("Encoder mmap started capture\n");
> 		}
> 
>-		return videobuf_mmap_mapper(&id->vbuf_q, vma);
>+		return videobuf_mmap_mapper(&s->vbuf_q, vma);
> 	}
> 
> 	return -EINVAL;
>@@ -788,142 +792,6 @@ int cx18_v4l2_close(struct file *filp)
> 	return 0;
> }
> 
>-void cx18_dma_free(struct videobuf_queue *q,
>-	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
>-{
>-	videobuf_waiton(q, &buf->vb, 0, 0);
>-	videobuf_vmalloc_free(&buf->vb);
>-	buf->vb.state = VIDEOBUF_NEEDS_INIT;
>-}
>-
>-static int cx18_prepare_buffer(struct videobuf_queue *q,
>-	struct cx18_stream *s,
>-	struct cx18_videobuf_buffer *buf,
>-	u32 pixelformat,
>-	unsigned int width, unsigned int height,
>-	enum v4l2_field field)
>-{
>-	int rc = 0;
>-
>-	/* check settings */
>-	buf->bytes_used = 0;
>-
>-	if ((width  < 48) || (height < 32))
>-		return -EINVAL;
>-
>-	buf->vb.size = (width * height * 16 /*fmt->depth*/) >> 3;
>-	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>-		return -EINVAL;
>-
>-	/* alloc + fill struct (if changed) */
>-	if (buf->vb.width != width || buf->vb.height != height ||
>-	    buf->vb.field != field || s->pixelformat != pixelformat ||
>-	    buf->tvnorm != s->tvnorm) {
>-
>-		buf->vb.width  = width;
>-		buf->vb.height = height;
>-		buf->vb.field  = field;
>-		buf->tvnorm    = s->tvnorm;
>-		s->pixelformat = pixelformat;
>-
>-		cx18_dma_free(q, s, buf);
>-	}
>-
>-	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>-		return -EINVAL;
>-
>-	if (buf->vb.field == 0)
>-		buf->vb.field = V4L2_FIELD_INTERLACED;
>-
>-	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
>-		buf->vb.width  = width;
>-		buf->vb.height = height;
>-		buf->vb.field  = field;
>-		buf->tvnorm    = s->tvnorm;
>-		s->pixelformat = pixelformat;
>-
>-		rc = videobuf_iolock(q, &buf->vb, &s->fbuf);
>-		if (rc != 0)
>-			goto fail;
>-	}
>-	buf->vb.state = VIDEOBUF_PREPARED;
>-	return 0;
>-
>-fail:
>-	cx18_dma_free(q, s, buf);
>-	return rc;
>-
>-}
>-
>-#define VB_MIN_BUFFERS 32
>-#define VB_MIN_BUFSIZE 0x208000
>-
>-static int buffer_setup(struct videobuf_queue *q,
>-	unsigned int *count, unsigned int *size)
>-{
>-	struct cx18_open_id *id = q->priv_data;
>-	struct cx18 *cx = id->cx;
>-	struct cx18_stream *s = &cx->streams[id->type];
>-
>-	*size = 2 * s->vbwidth * s->vbheight;
>-	if (*count == 0)
>-		*count = VB_MIN_BUFFERS;
>-
>-	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
>-		(*count)--;
>-
>-	q->field = V4L2_FIELD_INTERLACED;
>-	q->last = V4L2_FIELD_INTERLACED;
>-
>-	return 0;
>-}
>-
>-static int buffer_prepare(struct videobuf_queue *q,
>-	struct videobuf_buffer *vb,
>-	enum v4l2_field field)
>-{
>-	struct cx18_videobuf_buffer *buf =
>-		container_of(vb, struct cx18_videobuf_buffer, vb);
>-	struct cx18_open_id *id = q->priv_data;
>-	struct cx18 *cx = id->cx;
>-	struct cx18_stream *s = &cx->streams[id->type];
>-
>-	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
>-		s->vbwidth, s->vbheight, field);
>-}
>-
>-static void buffer_release(struct videobuf_queue *q,
>-	struct videobuf_buffer *vb)
>-{
>-	struct cx18_videobuf_buffer *buf =
>-		container_of(vb, struct cx18_videobuf_buffer, vb);
>-	struct cx18_open_id *id = q->priv_data;
>-	struct cx18 *cx = id->cx;
>-	struct cx18_stream *s = &cx->streams[id->type];
>-
>-	cx18_dma_free(q, s, buf);
>-}
>-
>-static void buffer_queue(struct videobuf_queue *q, struct
>videobuf_buffer *vb)
>-{
>-	struct cx18_videobuf_buffer *buf =
>-		container_of(vb, struct cx18_videobuf_buffer, vb);
>-	struct cx18_open_id *id = q->priv_data;
>-	struct cx18 *cx = id->cx;
>-	struct cx18_stream *s = &cx->streams[id->type];
>-
>-	buf->vb.state = VIDEOBUF_QUEUED;
>-
>-	list_add_tail(&buf->vb.queue, &s->vb_capture);
>-}
>-
>-static struct videobuf_queue_ops cx18_videobuf_qops = {
>-	.buf_setup    = buffer_setup,
>-	.buf_prepare  = buffer_prepare,
>-	.buf_queue    = buffer_queue,
>-	.buf_release  = buffer_release,
>-};
>-
>static int cx18_serialized_open(struct cx18_stream *s, struct file
>*filp)
> {
> 	struct cx18 *cx = s->cx;
>@@ -942,8 +810,8 @@ static int cx18_serialized_open(struct cx18_stream
>*s, struct file *filp)
> 	item->cx = cx;
> 	item->type = s->type;
> 
>-	spin_lock_init(&item->s_lock);
>-	item->vb_type = 0;
>+	spin_lock_init(&s->vbuf_q_lock);
>+	s->vb_type = 0;
> 
> 	item->open_id = cx->open_id++;
> 	filp->private_data = &item->fh;
>@@ -979,15 +847,6 @@ static int cx18_serialized_open(struct cx18_stream
>*s, struct file *filp)
> 		/* Done! Unmute and continue. */
> 		cx18_unmute(cx);
> 	}
>-	if (item->type == CX18_ENC_STREAM_TYPE_YUV) {
>-		item->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>-		videobuf_queue_vmalloc_init(&item->vbuf_q, &cx18_videobuf_qops,
>-			&cx->pci_dev->dev, &item->s_lock,
>-			V4L2_BUF_TYPE_VIDEO_CAPTURE,
>-			V4L2_FIELD_INTERLACED,
>-			sizeof(struct cx18_videobuf_buffer),
>-			item, &cx->serialize_lock);
>-	}
> 	v4l2_fh_add(&item->fh);
> 	return 0;
> }
>diff --git a/drivers/media/video/cx18/cx18-ioctl.c
>b/drivers/media/video/cx18/cx18-ioctl.c
>index 777d726..1933d4d 100644
>--- a/drivers/media/video/cx18/cx18-ioctl.c
>+++ b/drivers/media/video/cx18/cx18-ioctl.c
>@@ -41,18 +41,6 @@
> #include <media/tveeprom.h>
> #include <media/v4l2-chip-ident.h>
> 
>-static struct v4l2_fmtdesc formats[] = {
>-	{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>-	  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
>-	},
>-	{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
>-	  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
>-	},
>-	{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>-	  "YUYV 4:2:2", V4L2_PIX_FMT_YUYV, { 0, 0, 0, 0 }
>-	},
>-};
>-
> u16 cx18_service2vbi(int type)
> {
> 	switch (type) {
>@@ -172,8 +160,12 @@ static int cx18_g_fmt_vid_cap(struct file *file,
>void *fh,
> 	pixfmt->priv = 0;
> 	if (id->type == CX18_ENC_STREAM_TYPE_YUV) {
> 		pixfmt->pixelformat = s->pixelformat;
>-		/* YUV size is (Y=(h*720) + UV=(h*(720/2))) */
>-		pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
>+		/* HM12 YUV size is (Y=(h*720) + UV=(h*(720/2)))
>+		   UYUV YUV size is (Y=(h*720) + UV=(h*(720))) */
>+		if (s->pixelformat == V4L2_PIX_FMT_HM12)
>+			pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
>+		else
>+			pixfmt->sizeimage = pixfmt->height * 720 * 2;
> 		pixfmt->bytesperline = 720;
> 	} else {
> 		pixfmt->pixelformat = V4L2_PIX_FMT_MPEG;
>@@ -296,16 +288,15 @@ static int cx18_s_fmt_vid_cap(struct file *file,
>void *fh,
> 	w = fmt->fmt.pix.width;
> 	h = fmt->fmt.pix.height;
> 
>-	s->pixelformat = fmt->fmt.pix.pixelformat;
>-	s->vbheight = h;
>-	s->vbwidth = w;
>-
>-	if (cx->cxhdl.width == w && cx->cxhdl.height == h)
>+	if (cx->cxhdl.width == w && cx->cxhdl.height == h &&
>+	    s->pixelformat == fmt->fmt.pix.pixelformat)
> 		return 0;
> 
> 	if (atomic_read(&cx->ana_capturing) > 0)
> 		return -EBUSY;
> 
>+	s->pixelformat = fmt->fmt.pix.pixelformat;
>+
> 	mbus_fmt.width = cx->cxhdl.width = w;
> 	mbus_fmt.height = cx->cxhdl.height = h;
> 	mbus_fmt.code = V4L2_MBUS_FMT_FIXED;
>@@ -557,6 +548,18 @@ static int cx18_g_crop(struct file *file, void
>*fh, struct v4l2_crop *crop)
> static int cx18_enum_fmt_vid_cap(struct file *file, void *fh,
> 					struct v4l2_fmtdesc *fmt)
> {
>+	static const struct v4l2_fmtdesc formats[] = {
>+		{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>+		  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
>+		},
>+		{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
>+		  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
>+		},
>+		{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>+		  "UYVY 4:2:2", V4L2_PIX_FMT_UYVY, { 0, 0, 0, 0 }
>+		},
>+	};
>+
> 	if (fmt->index > ARRAY_SIZE(formats) - 1)
> 		return -EINVAL;
> 	*fmt = formats[fmt->index];
>@@ -874,10 +877,12 @@ static int cx18_g_enc_index(struct file *file,
>void *fh,
> static struct videobuf_queue *cx18_vb_queue(struct cx18_open_id *id)
> {
> 	struct videobuf_queue *q = NULL;
>+	struct cx18 *cx = id->cx;
>+	struct cx18_stream *s = &cx->streams[id->type];
> 
>-	switch (id->vb_type) {
>+	switch (s->vb_type) {
> 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>-		q = &id->vbuf_q;
>+		q = &s->vbuf_q;
> 		break;
> 	case V4L2_BUF_TYPE_VBI_CAPTURE:
> 		break;
>@@ -895,15 +900,15 @@ static int cx18_streamon(struct file *file, void
>*priv,
> 	struct cx18_stream *s = &cx->streams[id->type];
> 
> 	/* Start the hardware only if we're the video device */
>-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> 		return -EINVAL;
> 
> 	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
> 		return -EINVAL;
> 
> 	/* Establish a buffer timeout */
>-	mod_timer(&s->vb_timeout, jiffies + (HZ * 2));
>+	mod_timer(&s->vb_timeout, msecs_to_jiffies(2000) + jiffies);
> 
> 	return videobuf_streamon(cx18_vb_queue(id));
> }
>@@ -912,10 +917,12 @@ static int cx18_streamoff(struct file *file, void
>*priv,
> 	enum v4l2_buf_type type)
> {
> 	struct cx18_open_id *id = file->private_data;
>+	struct cx18 *cx = id->cx;
>+	struct cx18_stream *s = &cx->streams[id->type];
> 
> 	/* Start the hardware only if we're the video device */
>-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> 		return -EINVAL;
> 
> 	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
>@@ -928,9 +935,11 @@ static int cx18_reqbufs(struct file *file, void
>*priv,
> 	struct v4l2_requestbuffers *rb)
> {
> 	struct cx18_open_id *id = file->private_data;
>+	struct cx18 *cx = id->cx;
>+	struct cx18_stream *s = &cx->streams[id->type];
> 
>-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> 		return -EINVAL;
> 
> 	return videobuf_reqbufs(cx18_vb_queue(id), rb);
>@@ -940,9 +949,11 @@ static int cx18_querybuf(struct file *file, void
>*priv,
> 	struct v4l2_buffer *b)
> {
> 	struct cx18_open_id *id = file->private_data;
>+	struct cx18 *cx = id->cx;
>+	struct cx18_stream *s = &cx->streams[id->type];
> 
>-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> 		return -EINVAL;
> 
> 	return videobuf_querybuf(cx18_vb_queue(id), b);
>@@ -951,9 +962,11 @@ static int cx18_querybuf(struct file *file, void
>*priv,
>static int cx18_qbuf(struct file *file, void *priv, struct v4l2_buffer
>*b)
> {
> 	struct cx18_open_id *id = file->private_data;
>+	struct cx18 *cx = id->cx;
>+	struct cx18_stream *s = &cx->streams[id->type];
> 
>-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> 		return -EINVAL;
> 
> 	return videobuf_qbuf(cx18_vb_queue(id), b);
>@@ -962,8 +975,11 @@ static int cx18_qbuf(struct file *file, void
>*priv, struct v4l2_buffer *b)
>static int cx18_dqbuf(struct file *file, void *priv, struct v4l2_buffer
>*b)
> {
> 	struct cx18_open_id *id = file->private_data;
>-	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>-		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>+	struct cx18 *cx = id->cx;
>+	struct cx18_stream *s = &cx->streams[id->type];
>+
>+	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>+		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
> 		return -EINVAL;
> 
>	return videobuf_dqbuf(cx18_vb_queue(id), b, file->f_flags &
>O_NONBLOCK);
>diff --git a/drivers/media/video/cx18/cx18-mailbox.c
>b/drivers/media/video/cx18/cx18-mailbox.c
>index d4d8873..5ecae93 100644
>--- a/drivers/media/video/cx18/cx18-mailbox.c
>+++ b/drivers/media/video/cx18/cx18-mailbox.c
>@@ -177,7 +177,7 @@ static void cx18_mdl_send_to_videobuf(struct
>cx18_stream *s,
> 	if (list_empty(&s->vb_capture))
> 		goto out;
> 
>-	vb_buf = list_entry(s->vb_capture.next, struct cx18_videobuf_buffer,
>+	vb_buf = list_first_entry(&s->vb_capture, struct
>cx18_videobuf_buffer,
> 		vb.queue);
> 
> 	p = videobuf_to_vmalloc(&vb_buf->vb);
>@@ -202,25 +202,14 @@ static void cx18_mdl_send_to_videobuf(struct
>cx18_stream *s,
> 		vb_buf->bytes_used = 0;
> 	}
> 
>-	/* */
> 	if (dispatch) {
>-
>-		if (s->pixelformat == V4L2_PIX_FMT_YUYV) {
>-			/* UYVY to YUYV */
>-			for (i = 0; i < (720 * 480 * 2); i += 2) {
>-				u = *(p + i);
>-				*(p + i) = *(p + i + 1);
>-				*(p + i + 1) = u;
>-			}
>-		}
>-
>-		do_gettimeofday(&vb_buf->vb.ts);
>+		ktime_get_ts(&vb_buf->vb.ts);
> 		list_del(&vb_buf->vb.queue);
> 		vb_buf->vb.state = VIDEOBUF_DONE;
> 		wake_up(&vb_buf->vb.done);
> 	}
> 
>-	mod_timer(&s->vb_timeout, jiffies + (HZ / 10));
>+	mod_timer(&s->vb_timeout, msecs_to_jiffies(2000) + jiffies);
> 
> out:
> 	spin_unlock(&s->vb_lock);
>diff --git a/drivers/media/video/cx18/cx18-streams.c
>b/drivers/media/video/cx18/cx18-streams.c
>index 53f5e4f..24c9688 100644
>--- a/drivers/media/video/cx18/cx18-streams.c
>+++ b/drivers/media/video/cx18/cx18-streams.c
>@@ -98,6 +98,141 @@ static struct {
> 	},
> };
> 
>+
>+void cx18_dma_free(struct videobuf_queue *q,
>+	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
>+{
>+	videobuf_waiton(q, &buf->vb, 0, 0);
>+	videobuf_vmalloc_free(&buf->vb);
>+	buf->vb.state = VIDEOBUF_NEEDS_INIT;
>+}
>+
>+static int cx18_prepare_buffer(struct videobuf_queue *q,
>+	struct cx18_stream *s,
>+	struct cx18_videobuf_buffer *buf,
>+	u32 pixelformat,
>+	unsigned int width, unsigned int height,
>+	enum v4l2_field field)
>+{
>+        struct cx18 *cx = s->cx;
>+	int rc = 0;
>+
>+	/* check settings */
>+	buf->bytes_used = 0;
>+
>+	if ((width  < 48) || (height < 32))
>+		return -EINVAL;
>+
>+	buf->vb.size = (width * height * 2);
>+	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>+		return -EINVAL;
>+
>+	/* alloc + fill struct (if changed) */
>+	if (buf->vb.width != width || buf->vb.height != height ||
>+	    buf->vb.field != field || s->pixelformat != pixelformat ||
>+	    buf->tvnorm != cx->std) {
>+
>+		buf->vb.width  = width;
>+		buf->vb.height = height;
>+		buf->vb.field  = field;
>+		buf->tvnorm    = cx->std;
>+		s->pixelformat = pixelformat;
>+
>+		cx18_dma_free(q, s, buf);
>+	}
>+
>+	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>+		return -EINVAL;
>+
>+	if (buf->vb.field == 0)
>+		buf->vb.field = V4L2_FIELD_INTERLACED;
>+
>+	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
>+		buf->vb.width  = width;
>+		buf->vb.height = height;
>+		buf->vb.field  = field;
>+		buf->tvnorm    = cx->std;
>+		s->pixelformat = pixelformat;
>+
>+		rc = videobuf_iolock(q, &buf->vb, NULL);
>+		if (rc != 0)
>+			goto fail;
>+	}
>+	buf->vb.state = VIDEOBUF_PREPARED;
>+	return 0;
>+
>+fail:
>+	cx18_dma_free(q, s, buf);
>+	return rc;
>+
>+}
>+
>+/* VB_MIN_BUFSIZE is lcm(1440 * 480, 1440 * 576)
>+   1440 is a single line of 4:2:2 YUV at 720 luma samples wide
>+*/
>+#define VB_MIN_BUFFERS 32
>+#define VB_MIN_BUFSIZE 4147200
>+
>+static int buffer_setup(struct videobuf_queue *q,
>+	unsigned int *count, unsigned int *size)
>+{
>+	struct cx18_stream *s = q->priv_data;
>+	struct cx18 *cx = s->cx;
>+
>+	*size = 2 * cx->cxhdl.width * cx->cxhdl.height;
>+	if (*count == 0)
>+		*count = VB_MIN_BUFFERS;
>+
>+	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
>+		(*count)--;
>+
>+	q->field = V4L2_FIELD_INTERLACED;
>+	q->last = V4L2_FIELD_INTERLACED;
>+
>+	return 0;
>+}
>+
>+static int buffer_prepare(struct videobuf_queue *q,
>+	struct videobuf_buffer *vb,
>+	enum v4l2_field field)
>+{
>+	struct cx18_videobuf_buffer *buf =
>+		container_of(vb, struct cx18_videobuf_buffer, vb);
>+	struct cx18_stream *s = q->priv_data;
>+	struct cx18 *cx = s->cx;
>+
>+	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
>+		cx->cxhdl.width, cx->cxhdl.height, field);
>+}
>+
>+static void buffer_release(struct videobuf_queue *q,
>+	struct videobuf_buffer *vb)
>+{
>+	struct cx18_videobuf_buffer *buf =
>+		container_of(vb, struct cx18_videobuf_buffer, vb);
>+	struct cx18_stream *s = q->priv_data;
>+
>+	cx18_dma_free(q, s, buf);
>+}
>+
>+static void buffer_queue(struct videobuf_queue *q, struct
>videobuf_buffer *vb)
>+{
>+	struct cx18_videobuf_buffer *buf =
>+		container_of(vb, struct cx18_videobuf_buffer, vb);
>+	struct cx18_stream *s = q->priv_data;
>+
>+	buf->vb.state = VIDEOBUF_QUEUED;
>+
>+	list_add_tail(&buf->vb.queue, &s->vb_capture);
>+}
>+
>+static struct videobuf_queue_ops cx18_videobuf_qops = {
>+	.buf_setup    = buffer_setup,
>+	.buf_prepare  = buffer_prepare,
>+	.buf_queue    = buffer_queue,
>+	.buf_release  = buffer_release,
>+};
>+
> static void cx18_stream_init(struct cx18 *cx, int type)
> {
> 	struct cx18_stream *s = &cx->streams[type];
>@@ -139,9 +274,18 @@ static void cx18_stream_init(struct cx18 *cx, int
>type)
> 	s->vb_timeout.data = (unsigned long)s;
> 	init_timer(&s->vb_timeout);
> 	spin_lock_init(&s->vb_lock);
>-
>-	/* Assume the previous pixel default */
>-	s->pixelformat = V4L2_PIX_FMT_HM12;
>+	if (type == CX18_ENC_STREAM_TYPE_YUV) {
>+		s->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>+		videobuf_queue_vmalloc_init(&s->vbuf_q, &cx18_videobuf_qops,
>+			&cx->pci_dev->dev, &s->vbuf_q_lock,
>+			V4L2_BUF_TYPE_VIDEO_CAPTURE,
>+			V4L2_FIELD_INTERLACED,
>+			sizeof(struct cx18_videobuf_buffer),
>+			s, &cx->serialize_lock);
>+
>+		/* Assume the previous pixel default */
>+		s->pixelformat = V4L2_PIX_FMT_HM12;
>+	}
> }
> 
> static int cx18_prep_dev(struct cx18 *cx, int type)
>@@ -374,6 +518,9 @@ void cx18_streams_cleanup(struct cx18 *cx, int
>unregister)
> 		if (vdev == NULL)
> 			continue;
> 
>+		if (type == CX18_ENC_STREAM_TYPE_YUV)
>+			videobuf_mmap_free(&cx->streams[type].vbuf_q);
>+
> 		cx18_stream_free(&cx->streams[type]);
> 
> 		/* Unregister or release device */
>@@ -583,7 +730,10 @@ static void cx18_stream_configure_mdls(struct
>cx18_stream *s)
> 		 * Set the MDL size to the exact size needed for one frame.
> 		 * Use enough buffers per MDL to cover the MDL size
> 		 */
>-		s->mdl_size = 720 * s->cx->cxhdl.height * 3 / 2;
>+		if (s->pixelformat == V4L2_PIX_FMT_HM12)
>+			s->mdl_size = 720 * s->cx->cxhdl.height * 3 / 2;
>+		else
>+			s->mdl_size = 720 * s->cx->cxhdl.height * 2;
> 		s->bufs_per_mdl = s->mdl_size / s->buf_size;
> 		if (s->mdl_size % s->buf_size)
> 			s->bufs_per_mdl++;
>@@ -736,7 +886,7 @@ int cx18_start_v4l2_encode_stream(struct
>cx18_stream *s)
> 		 * rather than the default HM12 Macroblovk 4:2:0 support.
> 		 */
> 		if (captype == CAPTURE_CHANNEL_TYPE_YUV) {
>-			if (s->pixelformat == V4L2_PIX_FMT_YUYV)
>+			if (s->pixelformat == V4L2_PIX_FMT_UYVY)
> 				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
> 					s->handle, 1);
> 			else
>-- 
>1.7.4

Simon,

If these two changes are going in, please also bump the driver version to 1.5.0 in cx18-version.c.  These changes are significant enough perturbation.

End users are going to look to driver version 1.4.1 as the first version for proper analog tuner support of the HVR1600 model 74351.

Mauro,

Is cx18 v1.4.1 with HVR1600 model 74351 analog tuner support, without these mmap() changes going to be visible in kernel version .39 ?

Regards,
Andy

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Mauro Carvalho Chehab]

Em 03-05-2011 19:51, Andy Walls escreveu:
> Simon Farnsworth <simon.farnsworth@onelan.co.uk> wrote:
> 
>> The initial version of this patch (commit
>> d5976931639176bb6777755d96b9f8d959f79e9e) had some issues:
>>
>> * It didn't correctly calculate the size of the YUV buffer for 4:2:2,
>>   resulting in capture sometimes being offset by 1/3rd of a picture.
>>
>> * There were a lot of variables duplicating information the driver
>>   already knew, which have been removed.
>>
>> * There was an in-kernel format conversion - libv4l can do this one,
>>   and is the right place to do format conversions anyway.
>>
>> * Some magic numbers weren't properly explained.
>>
>> Fix all these issues, leaving just the move from videobuf to videobuf2
>> to do.
>>
>> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
>> ---
>> drivers/media/video/cx18/Kconfig        |    1 -
>> drivers/media/video/cx18/cx18-driver.h  |    8 +-
>> drivers/media/video/cx18/cx18-fileops.c |  167
>> +++----------------------------
>> drivers/media/video/cx18/cx18-ioctl.c   |   84 +++++++++------
>> drivers/media/video/cx18/cx18-mailbox.c |   17 +---
>> drivers/media/video/cx18/cx18-streams.c |  160
>> ++++++++++++++++++++++++++++-
>> 6 files changed, 225 insertions(+), 212 deletions(-)
>>
>> diff --git a/drivers/media/video/cx18/Kconfig
>> b/drivers/media/video/cx18/Kconfig
>> index 9c23202..53b3c77 100644
>> --- a/drivers/media/video/cx18/Kconfig
>> +++ b/drivers/media/video/cx18/Kconfig
>> @@ -2,7 +2,6 @@ config VIDEO_CX18
>> 	tristate "Conexant cx23418 MPEG encoder support"
>> 	depends on VIDEO_V4L2 && DVB_CORE && PCI && I2C && EXPERIMENTAL
>> 	select I2C_ALGOBIT
>> -	select VIDEOBUF_DVB
>> 	select VIDEOBUF_VMALLOC
>> 	depends on RC_CORE
>> 	select VIDEO_TUNER
>> diff --git a/drivers/media/video/cx18/cx18-driver.h
>> b/drivers/media/video/cx18/cx18-driver.h
>> index 70e1e04..0864272 100644
>> --- a/drivers/media/video/cx18/cx18-driver.h
>> +++ b/drivers/media/video/cx18/cx18-driver.h
>> @@ -412,11 +412,11 @@ struct cx18_stream {
>> 	u32 pixelformat;
>> 	struct list_head vb_capture;    /* video capture queue */
>> 	spinlock_t vb_lock;
>> -	struct v4l2_framebuffer fbuf;
>> -	v4l2_std_id tvnorm; /* selected tv norm */
>> 	struct timer_list vb_timeout;
>> -	int vbwidth;
>> -	int vbheight;
>> +
>> +	struct videobuf_queue vbuf_q;
>> +	spinlock_t vbuf_q_lock; /* Protect vbuf_q */
>> +	enum v4l2_buf_type vb_type;
>> };
>>
>> struct cx18_videobuf_buffer {
>> diff --git a/drivers/media/video/cx18/cx18-fileops.c
>> b/drivers/media/video/cx18/cx18-fileops.c
>> index c74eafd..6609222 100644
>> --- a/drivers/media/video/cx18/cx18-fileops.c
>> +++ b/drivers/media/video/cx18/cx18-fileops.c
>> @@ -598,9 +598,9 @@ ssize_t cx18_v4l2_read(struct file *filp, char
>> __user *buf, size_t count,
>> 	if (rc)
>> 		return rc;
>>
>> -	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
>> -		return videobuf_read_stream(&id->vbuf_q, buf, count, pos, 0,
>> +		return videobuf_read_stream(&s->vbuf_q, buf, count, pos, 0,
>> 			filp->f_flags & O_NONBLOCK);
>> 	}
>>
>> @@ -629,9 +629,13 @@ unsigned int cx18_v4l2_enc_poll(struct file *filp,
>> poll_table *wait)
>> 		CX18_DEBUG_FILE("Encoder poll started capture\n");
>> 	}
>>
>> -	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
>> -		return videobuf_poll_stream(filp, &id->vbuf_q, wait);
>> +		int videobuf_poll = videobuf_poll_stream(filp, &s->vbuf_q, wait);
>> +                if (eof && videobuf_poll == POLLERR)
>> +                        return POLLHUP;
>> +                else
>> +                        return videobuf_poll;
>> 	}
>>
>> 	/* add stream's waitq to the poll list */
>> @@ -652,7 +656,7 @@ int cx18_v4l2_mmap(struct file *file, struct
>> vm_area_struct *vma)
>> 	struct cx18_stream *s = &cx->streams[id->type];
>> 	int eof = test_bit(CX18_F_S_STREAMOFF, &s->s_flags);
>>
>> -	if ((id->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +	if ((s->vb_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> 		(id->type == CX18_ENC_STREAM_TYPE_YUV)) {
>>
>> 		/* Start a capture if there is none */
>> @@ -668,10 +672,10 @@ int cx18_v4l2_mmap(struct file *file, struct
>> vm_area_struct *vma)
>> 					s->name, rc);
>> 				return -EINVAL;
>> 			}
>> -			CX18_DEBUG_FILE("Encoder poll started capture\n");
>> +			CX18_DEBUG_FILE("Encoder mmap started capture\n");
>> 		}
>>
>> -		return videobuf_mmap_mapper(&id->vbuf_q, vma);
>> +		return videobuf_mmap_mapper(&s->vbuf_q, vma);
>> 	}
>>
>> 	return -EINVAL;
>> @@ -788,142 +792,6 @@ int cx18_v4l2_close(struct file *filp)
>> 	return 0;
>> }
>>
>> -void cx18_dma_free(struct videobuf_queue *q,
>> -	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
>> -{
>> -	videobuf_waiton(q, &buf->vb, 0, 0);
>> -	videobuf_vmalloc_free(&buf->vb);
>> -	buf->vb.state = VIDEOBUF_NEEDS_INIT;
>> -}
>> -
>> -static int cx18_prepare_buffer(struct videobuf_queue *q,
>> -	struct cx18_stream *s,
>> -	struct cx18_videobuf_buffer *buf,
>> -	u32 pixelformat,
>> -	unsigned int width, unsigned int height,
>> -	enum v4l2_field field)
>> -{
>> -	int rc = 0;
>> -
>> -	/* check settings */
>> -	buf->bytes_used = 0;
>> -
>> -	if ((width  < 48) || (height < 32))
>> -		return -EINVAL;
>> -
>> -	buf->vb.size = (width * height * 16 /*fmt->depth*/) >> 3;
>> -	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>> -		return -EINVAL;
>> -
>> -	/* alloc + fill struct (if changed) */
>> -	if (buf->vb.width != width || buf->vb.height != height ||
>> -	    buf->vb.field != field || s->pixelformat != pixelformat ||
>> -	    buf->tvnorm != s->tvnorm) {
>> -
>> -		buf->vb.width  = width;
>> -		buf->vb.height = height;
>> -		buf->vb.field  = field;
>> -		buf->tvnorm    = s->tvnorm;
>> -		s->pixelformat = pixelformat;
>> -
>> -		cx18_dma_free(q, s, buf);
>> -	}
>> -
>> -	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>> -		return -EINVAL;
>> -
>> -	if (buf->vb.field == 0)
>> -		buf->vb.field = V4L2_FIELD_INTERLACED;
>> -
>> -	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
>> -		buf->vb.width  = width;
>> -		buf->vb.height = height;
>> -		buf->vb.field  = field;
>> -		buf->tvnorm    = s->tvnorm;
>> -		s->pixelformat = pixelformat;
>> -
>> -		rc = videobuf_iolock(q, &buf->vb, &s->fbuf);
>> -		if (rc != 0)
>> -			goto fail;
>> -	}
>> -	buf->vb.state = VIDEOBUF_PREPARED;
>> -	return 0;
>> -
>> -fail:
>> -	cx18_dma_free(q, s, buf);
>> -	return rc;
>> -
>> -}
>> -
>> -#define VB_MIN_BUFFERS 32
>> -#define VB_MIN_BUFSIZE 0x208000
>> -
>> -static int buffer_setup(struct videobuf_queue *q,
>> -	unsigned int *count, unsigned int *size)
>> -{
>> -	struct cx18_open_id *id = q->priv_data;
>> -	struct cx18 *cx = id->cx;
>> -	struct cx18_stream *s = &cx->streams[id->type];
>> -
>> -	*size = 2 * s->vbwidth * s->vbheight;
>> -	if (*count == 0)
>> -		*count = VB_MIN_BUFFERS;
>> -
>> -	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
>> -		(*count)--;
>> -
>> -	q->field = V4L2_FIELD_INTERLACED;
>> -	q->last = V4L2_FIELD_INTERLACED;
>> -
>> -	return 0;
>> -}
>> -
>> -static int buffer_prepare(struct videobuf_queue *q,
>> -	struct videobuf_buffer *vb,
>> -	enum v4l2_field field)
>> -{
>> -	struct cx18_videobuf_buffer *buf =
>> -		container_of(vb, struct cx18_videobuf_buffer, vb);
>> -	struct cx18_open_id *id = q->priv_data;
>> -	struct cx18 *cx = id->cx;
>> -	struct cx18_stream *s = &cx->streams[id->type];
>> -
>> -	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
>> -		s->vbwidth, s->vbheight, field);
>> -}
>> -
>> -static void buffer_release(struct videobuf_queue *q,
>> -	struct videobuf_buffer *vb)
>> -{
>> -	struct cx18_videobuf_buffer *buf =
>> -		container_of(vb, struct cx18_videobuf_buffer, vb);
>> -	struct cx18_open_id *id = q->priv_data;
>> -	struct cx18 *cx = id->cx;
>> -	struct cx18_stream *s = &cx->streams[id->type];
>> -
>> -	cx18_dma_free(q, s, buf);
>> -}
>> -
>> -static void buffer_queue(struct videobuf_queue *q, struct
>> videobuf_buffer *vb)
>> -{
>> -	struct cx18_videobuf_buffer *buf =
>> -		container_of(vb, struct cx18_videobuf_buffer, vb);
>> -	struct cx18_open_id *id = q->priv_data;
>> -	struct cx18 *cx = id->cx;
>> -	struct cx18_stream *s = &cx->streams[id->type];
>> -
>> -	buf->vb.state = VIDEOBUF_QUEUED;
>> -
>> -	list_add_tail(&buf->vb.queue, &s->vb_capture);
>> -}
>> -
>> -static struct videobuf_queue_ops cx18_videobuf_qops = {
>> -	.buf_setup    = buffer_setup,
>> -	.buf_prepare  = buffer_prepare,
>> -	.buf_queue    = buffer_queue,
>> -	.buf_release  = buffer_release,
>> -};
>> -
>> static int cx18_serialized_open(struct cx18_stream *s, struct file
>> *filp)
>> {
>> 	struct cx18 *cx = s->cx;
>> @@ -942,8 +810,8 @@ static int cx18_serialized_open(struct cx18_stream
>> *s, struct file *filp)
>> 	item->cx = cx;
>> 	item->type = s->type;
>>
>> -	spin_lock_init(&item->s_lock);
>> -	item->vb_type = 0;
>> +	spin_lock_init(&s->vbuf_q_lock);
>> +	s->vb_type = 0;
>>
>> 	item->open_id = cx->open_id++;
>> 	filp->private_data = &item->fh;
>> @@ -979,15 +847,6 @@ static int cx18_serialized_open(struct cx18_stream
>> *s, struct file *filp)
>> 		/* Done! Unmute and continue. */
>> 		cx18_unmute(cx);
>> 	}
>> -	if (item->type == CX18_ENC_STREAM_TYPE_YUV) {
>> -		item->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> -		videobuf_queue_vmalloc_init(&item->vbuf_q, &cx18_videobuf_qops,
>> -			&cx->pci_dev->dev, &item->s_lock,
>> -			V4L2_BUF_TYPE_VIDEO_CAPTURE,
>> -			V4L2_FIELD_INTERLACED,
>> -			sizeof(struct cx18_videobuf_buffer),
>> -			item, &cx->serialize_lock);
>> -	}
>> 	v4l2_fh_add(&item->fh);
>> 	return 0;
>> }
>> diff --git a/drivers/media/video/cx18/cx18-ioctl.c
>> b/drivers/media/video/cx18/cx18-ioctl.c
>> index 777d726..1933d4d 100644
>> --- a/drivers/media/video/cx18/cx18-ioctl.c
>> +++ b/drivers/media/video/cx18/cx18-ioctl.c
>> @@ -41,18 +41,6 @@
>> #include <media/tveeprom.h>
>> #include <media/v4l2-chip-ident.h>
>>
>> -static struct v4l2_fmtdesc formats[] = {
>> -	{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>> -	  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
>> -	},
>> -	{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
>> -	  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
>> -	},
>> -	{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>> -	  "YUYV 4:2:2", V4L2_PIX_FMT_YUYV, { 0, 0, 0, 0 }
>> -	},
>> -};
>> -
>> u16 cx18_service2vbi(int type)
>> {
>> 	switch (type) {
>> @@ -172,8 +160,12 @@ static int cx18_g_fmt_vid_cap(struct file *file,
>> void *fh,
>> 	pixfmt->priv = 0;
>> 	if (id->type == CX18_ENC_STREAM_TYPE_YUV) {
>> 		pixfmt->pixelformat = s->pixelformat;
>> -		/* YUV size is (Y=(h*720) + UV=(h*(720/2))) */
>> -		pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
>> +		/* HM12 YUV size is (Y=(h*720) + UV=(h*(720/2)))
>> +		   UYUV YUV size is (Y=(h*720) + UV=(h*(720))) */
>> +		if (s->pixelformat == V4L2_PIX_FMT_HM12)
>> +			pixfmt->sizeimage = pixfmt->height * 720 * 3 / 2;
>> +		else
>> +			pixfmt->sizeimage = pixfmt->height * 720 * 2;
>> 		pixfmt->bytesperline = 720;
>> 	} else {
>> 		pixfmt->pixelformat = V4L2_PIX_FMT_MPEG;
>> @@ -296,16 +288,15 @@ static int cx18_s_fmt_vid_cap(struct file *file,
>> void *fh,
>> 	w = fmt->fmt.pix.width;
>> 	h = fmt->fmt.pix.height;
>>
>> -	s->pixelformat = fmt->fmt.pix.pixelformat;
>> -	s->vbheight = h;
>> -	s->vbwidth = w;
>> -
>> -	if (cx->cxhdl.width == w && cx->cxhdl.height == h)
>> +	if (cx->cxhdl.width == w && cx->cxhdl.height == h &&
>> +	    s->pixelformat == fmt->fmt.pix.pixelformat)
>> 		return 0;
>>
>> 	if (atomic_read(&cx->ana_capturing) > 0)
>> 		return -EBUSY;
>>
>> +	s->pixelformat = fmt->fmt.pix.pixelformat;
>> +
>> 	mbus_fmt.width = cx->cxhdl.width = w;
>> 	mbus_fmt.height = cx->cxhdl.height = h;
>> 	mbus_fmt.code = V4L2_MBUS_FMT_FIXED;
>> @@ -557,6 +548,18 @@ static int cx18_g_crop(struct file *file, void
>> *fh, struct v4l2_crop *crop)
>> static int cx18_enum_fmt_vid_cap(struct file *file, void *fh,
>> 					struct v4l2_fmtdesc *fmt)
>> {
>> +	static const struct v4l2_fmtdesc formats[] = {
>> +		{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>> +		  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
>> +		},
>> +		{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
>> +		  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
>> +		},
>> +		{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
>> +		  "UYVY 4:2:2", V4L2_PIX_FMT_UYVY, { 0, 0, 0, 0 }
>> +		},
>> +	};
>> +
>> 	if (fmt->index > ARRAY_SIZE(formats) - 1)
>> 		return -EINVAL;
>> 	*fmt = formats[fmt->index];
>> @@ -874,10 +877,12 @@ static int cx18_g_enc_index(struct file *file,
>> void *fh,
>> static struct videobuf_queue *cx18_vb_queue(struct cx18_open_id *id)
>> {
>> 	struct videobuf_queue *q = NULL;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>>
>> -	switch (id->vb_type) {
>> +	switch (s->vb_type) {
>> 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>> -		q = &id->vbuf_q;
>> +		q = &s->vbuf_q;
>> 		break;
>> 	case V4L2_BUF_TYPE_VBI_CAPTURE:
>> 		break;
>> @@ -895,15 +900,15 @@ static int cx18_streamon(struct file *file, void
>> *priv,
>> 	struct cx18_stream *s = &cx->streams[id->type];
>>
>> 	/* Start the hardware only if we're the video device */
>> -	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> -		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> 		return -EINVAL;
>>
>> 	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
>> 		return -EINVAL;
>>
>> 	/* Establish a buffer timeout */
>> -	mod_timer(&s->vb_timeout, jiffies + (HZ * 2));
>> +	mod_timer(&s->vb_timeout, msecs_to_jiffies(2000) + jiffies);
>>
>> 	return videobuf_streamon(cx18_vb_queue(id));
>> }
>> @@ -912,10 +917,12 @@ static int cx18_streamoff(struct file *file, void
>> *priv,
>> 	enum v4l2_buf_type type)
>> {
>> 	struct cx18_open_id *id = file->private_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>>
>> 	/* Start the hardware only if we're the video device */
>> -	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> -		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> 		return -EINVAL;
>>
>> 	if (id->type != CX18_ENC_STREAM_TYPE_YUV)
>> @@ -928,9 +935,11 @@ static int cx18_reqbufs(struct file *file, void
>> *priv,
>> 	struct v4l2_requestbuffers *rb)
>> {
>> 	struct cx18_open_id *id = file->private_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>>
>> -	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> -		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> 		return -EINVAL;
>>
>> 	return videobuf_reqbufs(cx18_vb_queue(id), rb);
>> @@ -940,9 +949,11 @@ static int cx18_querybuf(struct file *file, void
>> *priv,
>> 	struct v4l2_buffer *b)
>> {
>> 	struct cx18_open_id *id = file->private_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>>
>> -	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> -		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> 		return -EINVAL;
>>
>> 	return videobuf_querybuf(cx18_vb_queue(id), b);
>> @@ -951,9 +962,11 @@ static int cx18_querybuf(struct file *file, void
>> *priv,
>> static int cx18_qbuf(struct file *file, void *priv, struct v4l2_buffer
>> *b)
>> {
>> 	struct cx18_open_id *id = file->private_data;
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>>
>> -	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> -		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> 		return -EINVAL;
>>
>> 	return videobuf_qbuf(cx18_vb_queue(id), b);
>> @@ -962,8 +975,11 @@ static int cx18_qbuf(struct file *file, void
>> *priv, struct v4l2_buffer *b)
>> static int cx18_dqbuf(struct file *file, void *priv, struct v4l2_buffer
>> *b)
>> {
>> 	struct cx18_open_id *id = file->private_data;
>> -	if ((id->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> -		(id->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> +	struct cx18 *cx = id->cx;
>> +	struct cx18_stream *s = &cx->streams[id->type];
>> +
>> +	if ((s->vb_type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> +		(s->vb_type != V4L2_BUF_TYPE_VBI_CAPTURE))
>> 		return -EINVAL;
>>
>> 	return videobuf_dqbuf(cx18_vb_queue(id), b, file->f_flags &
>> O_NONBLOCK);
>> diff --git a/drivers/media/video/cx18/cx18-mailbox.c
>> b/drivers/media/video/cx18/cx18-mailbox.c
>> index d4d8873..5ecae93 100644
>> --- a/drivers/media/video/cx18/cx18-mailbox.c
>> +++ b/drivers/media/video/cx18/cx18-mailbox.c
>> @@ -177,7 +177,7 @@ static void cx18_mdl_send_to_videobuf(struct
>> cx18_stream *s,
>> 	if (list_empty(&s->vb_capture))
>> 		goto out;
>>
>> -	vb_buf = list_entry(s->vb_capture.next, struct cx18_videobuf_buffer,
>> +	vb_buf = list_first_entry(&s->vb_capture, struct
>> cx18_videobuf_buffer,
>> 		vb.queue);
>>
>> 	p = videobuf_to_vmalloc(&vb_buf->vb);
>> @@ -202,25 +202,14 @@ static void cx18_mdl_send_to_videobuf(struct
>> cx18_stream *s,
>> 		vb_buf->bytes_used = 0;
>> 	}
>>
>> -	/* */
>> 	if (dispatch) {
>> -
>> -		if (s->pixelformat == V4L2_PIX_FMT_YUYV) {
>> -			/* UYVY to YUYV */
>> -			for (i = 0; i < (720 * 480 * 2); i += 2) {
>> -				u = *(p + i);
>> -				*(p + i) = *(p + i + 1);
>> -				*(p + i + 1) = u;
>> -			}
>> -		}
>> -
>> -		do_gettimeofday(&vb_buf->vb.ts);
>> +		ktime_get_ts(&vb_buf->vb.ts);
>> 		list_del(&vb_buf->vb.queue);
>> 		vb_buf->vb.state = VIDEOBUF_DONE;
>> 		wake_up(&vb_buf->vb.done);
>> 	}
>>
>> -	mod_timer(&s->vb_timeout, jiffies + (HZ / 10));
>> +	mod_timer(&s->vb_timeout, msecs_to_jiffies(2000) + jiffies);
>>
>> out:
>> 	spin_unlock(&s->vb_lock);
>> diff --git a/drivers/media/video/cx18/cx18-streams.c
>> b/drivers/media/video/cx18/cx18-streams.c
>> index 53f5e4f..24c9688 100644
>> --- a/drivers/media/video/cx18/cx18-streams.c
>> +++ b/drivers/media/video/cx18/cx18-streams.c
>> @@ -98,6 +98,141 @@ static struct {
>> 	},
>> };
>>
>> +
>> +void cx18_dma_free(struct videobuf_queue *q,
>> +	struct cx18_stream *s, struct cx18_videobuf_buffer *buf)
>> +{
>> +	videobuf_waiton(q, &buf->vb, 0, 0);
>> +	videobuf_vmalloc_free(&buf->vb);
>> +	buf->vb.state = VIDEOBUF_NEEDS_INIT;
>> +}
>> +
>> +static int cx18_prepare_buffer(struct videobuf_queue *q,
>> +	struct cx18_stream *s,
>> +	struct cx18_videobuf_buffer *buf,
>> +	u32 pixelformat,
>> +	unsigned int width, unsigned int height,
>> +	enum v4l2_field field)
>> +{
>> +        struct cx18 *cx = s->cx;
>> +	int rc = 0;
>> +
>> +	/* check settings */
>> +	buf->bytes_used = 0;
>> +
>> +	if ((width  < 48) || (height < 32))
>> +		return -EINVAL;
>> +
>> +	buf->vb.size = (width * height * 2);
>> +	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>> +		return -EINVAL;
>> +
>> +	/* alloc + fill struct (if changed) */
>> +	if (buf->vb.width != width || buf->vb.height != height ||
>> +	    buf->vb.field != field || s->pixelformat != pixelformat ||
>> +	    buf->tvnorm != cx->std) {
>> +
>> +		buf->vb.width  = width;
>> +		buf->vb.height = height;
>> +		buf->vb.field  = field;
>> +		buf->tvnorm    = cx->std;
>> +		s->pixelformat = pixelformat;
>> +
>> +		cx18_dma_free(q, s, buf);
>> +	}
>> +
>> +	if ((buf->vb.baddr != 0) && (buf->vb.bsize < buf->vb.size))
>> +		return -EINVAL;
>> +
>> +	if (buf->vb.field == 0)
>> +		buf->vb.field = V4L2_FIELD_INTERLACED;
>> +
>> +	if (VIDEOBUF_NEEDS_INIT == buf->vb.state) {
>> +		buf->vb.width  = width;
>> +		buf->vb.height = height;
>> +		buf->vb.field  = field;
>> +		buf->tvnorm    = cx->std;
>> +		s->pixelformat = pixelformat;
>> +
>> +		rc = videobuf_iolock(q, &buf->vb, NULL);
>> +		if (rc != 0)
>> +			goto fail;
>> +	}
>> +	buf->vb.state = VIDEOBUF_PREPARED;
>> +	return 0;
>> +
>> +fail:
>> +	cx18_dma_free(q, s, buf);
>> +	return rc;
>> +
>> +}
>> +
>> +/* VB_MIN_BUFSIZE is lcm(1440 * 480, 1440 * 576)
>> +   1440 is a single line of 4:2:2 YUV at 720 luma samples wide
>> +*/
>> +#define VB_MIN_BUFFERS 32
>> +#define VB_MIN_BUFSIZE 4147200
>> +
>> +static int buffer_setup(struct videobuf_queue *q,
>> +	unsigned int *count, unsigned int *size)
>> +{
>> +	struct cx18_stream *s = q->priv_data;
>> +	struct cx18 *cx = s->cx;
>> +
>> +	*size = 2 * cx->cxhdl.width * cx->cxhdl.height;
>> +	if (*count == 0)
>> +		*count = VB_MIN_BUFFERS;
>> +
>> +	while (*size * *count > VB_MIN_BUFFERS * VB_MIN_BUFSIZE)
>> +		(*count)--;
>> +
>> +	q->field = V4L2_FIELD_INTERLACED;
>> +	q->last = V4L2_FIELD_INTERLACED;
>> +
>> +	return 0;
>> +}
>> +
>> +static int buffer_prepare(struct videobuf_queue *q,
>> +	struct videobuf_buffer *vb,
>> +	enum v4l2_field field)
>> +{
>> +	struct cx18_videobuf_buffer *buf =
>> +		container_of(vb, struct cx18_videobuf_buffer, vb);
>> +	struct cx18_stream *s = q->priv_data;
>> +	struct cx18 *cx = s->cx;
>> +
>> +	return cx18_prepare_buffer(q, s, buf, s->pixelformat,
>> +		cx->cxhdl.width, cx->cxhdl.height, field);
>> +}
>> +
>> +static void buffer_release(struct videobuf_queue *q,
>> +	struct videobuf_buffer *vb)
>> +{
>> +	struct cx18_videobuf_buffer *buf =
>> +		container_of(vb, struct cx18_videobuf_buffer, vb);
>> +	struct cx18_stream *s = q->priv_data;
>> +
>> +	cx18_dma_free(q, s, buf);
>> +}
>> +
>> +static void buffer_queue(struct videobuf_queue *q, struct
>> videobuf_buffer *vb)
>> +{
>> +	struct cx18_videobuf_buffer *buf =
>> +		container_of(vb, struct cx18_videobuf_buffer, vb);
>> +	struct cx18_stream *s = q->priv_data;
>> +
>> +	buf->vb.state = VIDEOBUF_QUEUED;
>> +
>> +	list_add_tail(&buf->vb.queue, &s->vb_capture);
>> +}
>> +
>> +static struct videobuf_queue_ops cx18_videobuf_qops = {
>> +	.buf_setup    = buffer_setup,
>> +	.buf_prepare  = buffer_prepare,
>> +	.buf_queue    = buffer_queue,
>> +	.buf_release  = buffer_release,
>> +};
>> +
>> static void cx18_stream_init(struct cx18 *cx, int type)
>> {
>> 	struct cx18_stream *s = &cx->streams[type];
>> @@ -139,9 +274,18 @@ static void cx18_stream_init(struct cx18 *cx, int
>> type)
>> 	s->vb_timeout.data = (unsigned long)s;
>> 	init_timer(&s->vb_timeout);
>> 	spin_lock_init(&s->vb_lock);
>> -
>> -	/* Assume the previous pixel default */
>> -	s->pixelformat = V4L2_PIX_FMT_HM12;
>> +	if (type == CX18_ENC_STREAM_TYPE_YUV) {
>> +		s->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +		videobuf_queue_vmalloc_init(&s->vbuf_q, &cx18_videobuf_qops,
>> +			&cx->pci_dev->dev, &s->vbuf_q_lock,
>> +			V4L2_BUF_TYPE_VIDEO_CAPTURE,
>> +			V4L2_FIELD_INTERLACED,
>> +			sizeof(struct cx18_videobuf_buffer),
>> +			s, &cx->serialize_lock);
>> +
>> +		/* Assume the previous pixel default */
>> +		s->pixelformat = V4L2_PIX_FMT_HM12;
>> +	}
>> }
>>
>> static int cx18_prep_dev(struct cx18 *cx, int type)
>> @@ -374,6 +518,9 @@ void cx18_streams_cleanup(struct cx18 *cx, int
>> unregister)
>> 		if (vdev == NULL)
>> 			continue;
>>
>> +		if (type == CX18_ENC_STREAM_TYPE_YUV)
>> +			videobuf_mmap_free(&cx->streams[type].vbuf_q);
>> +
>> 		cx18_stream_free(&cx->streams[type]);
>>
>> 		/* Unregister or release device */
>> @@ -583,7 +730,10 @@ static void cx18_stream_configure_mdls(struct
>> cx18_stream *s)
>> 		 * Set the MDL size to the exact size needed for one frame.
>> 		 * Use enough buffers per MDL to cover the MDL size
>> 		 */
>> -		s->mdl_size = 720 * s->cx->cxhdl.height * 3 / 2;
>> +		if (s->pixelformat == V4L2_PIX_FMT_HM12)
>> +			s->mdl_size = 720 * s->cx->cxhdl.height * 3 / 2;
>> +		else
>> +			s->mdl_size = 720 * s->cx->cxhdl.height * 2;
>> 		s->bufs_per_mdl = s->mdl_size / s->buf_size;
>> 		if (s->mdl_size % s->buf_size)
>> 			s->bufs_per_mdl++;
>> @@ -736,7 +886,7 @@ int cx18_start_v4l2_encode_stream(struct
>> cx18_stream *s)
>> 		 * rather than the default HM12 Macroblovk 4:2:0 support.
>> 		 */
>> 		if (captype == CAPTURE_CHANNEL_TYPE_YUV) {
>> -			if (s->pixelformat == V4L2_PIX_FMT_YUYV)
>> +			if (s->pixelformat == V4L2_PIX_FMT_UYVY)
>> 				cx18_vapi(cx, CX18_CPU_SET_VFC_PARAM, 2,
>> 					s->handle, 1);
>> 			else
>> -- 
>> 1.7.4
> 
> Simon,
> 
> If these two changes are going in, please also bump the driver version to 1.5.0 in cx18-version.c.  These changes are significant enough perturbation.
> 
> End users are going to look to driver version 1.4.1 as the first version for proper analog tuner support of the HVR1600 model 74351.
> 
> Mauro,
> 
> Is cx18 v1.4.1 with HVR1600 model 74351 analog tuner support, without these mmap() changes going to be visible in kernel version .39 ?

Hmm... This is what I have at my for_upstream tree:

$ git grep -i 74351 drivers/media/video/cx18/
drivers/media/video/cx18/cx18-driver.c:   case 74351: /* OEM models */

drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_MAJOR 1
drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_MINOR 4
drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_PATCHLEVEL 0




> 
> Regards,
> Andy

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Andy Walls]

On Tue, 2011-05-03 at 20:01 -0300, Mauro Carvalho Chehab wrote:
> Em 03-05-2011 19:51, Andy Walls escreveu:
> > Simon Farnsworth <simon.farnsworth@onelan.co.uk> wrote:
 

> > Simon,
> > 
> > If these two changes are going in, please also bump the driver
> version to 1.5.0 in cx18-version.c.  These changes are significant
> enough perturbation.
> > 
> > End users are going to look to driver version 1.4.1 as the first
> version for proper analog tuner support of the HVR1600 model 74351.
> > 
> > Mauro,
> > 
> > Is cx18 v1.4.1 with HVR1600 model 74351 analog tuner support,
> without these mmap() changes going to be visible in kernel
> version .39 ?
> 
> Hmm... This is what I have at my for_upstream tree:
> 
> $ git grep -i 74351 drivers/media/video/cx18/
> drivers/media/video/cx18/cx18-driver.c:   case 74351: /* OEM models */
> 
> drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_MAJOR 1
> drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_MINOR 4
> drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_PATCHLEVEL 0
> 

I was refering to these patches (which look like they are destined for
2.6.40 according to the subject lines)

http://thread.gmane.org/gmane.comp.video.linuxtv.scm/9918
http://thread.gmane.org/gmane.comp.video.linuxtv.scm/9917

Regards,
Andy

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Mauro Carvalho Chehab]

Em 03-05-2011 20:38, Andy Walls escreveu:
> On Tue, 2011-05-03 at 20:01 -0300, Mauro Carvalho Chehab wrote:
>> Em 03-05-2011 19:51, Andy Walls escreveu:
>>> Simon Farnsworth <simon.farnsworth@onelan.co.uk> wrote:
>  
> 
>>> Simon,
>>>
>>> If these two changes are going in, please also bump the driver
>> version to 1.5.0 in cx18-version.c.  These changes are significant
>> enough perturbation.
>>>
>>> End users are going to look to driver version 1.4.1 as the first
>> version for proper analog tuner support of the HVR1600 model 74351.
>>>
>>> Mauro,
>>>
>>> Is cx18 v1.4.1 with HVR1600 model 74351 analog tuner support,
>> without these mmap() changes going to be visible in kernel
>> version .39 ?
>>
>> Hmm... This is what I have at my for_upstream tree:
>>
>> $ git grep -i 74351 drivers/media/video/cx18/
>> drivers/media/video/cx18/cx18-driver.c:   case 74351: /* OEM models */
>>
>> drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_MAJOR 1
>> drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_MINOR 4
>> drivers/media/video/cx18/cx18-version.h:#define CX18_DRIVER_VERSION_PATCHLEVEL 0
>>
> 
> I was refering to these patches (which look like they are destined for
> 2.6.40 according to the subject lines)
> 
> http://thread.gmane.org/gmane.comp.video.linuxtv.scm/9918
> http://thread.gmane.org/gmane.comp.video.linuxtv.scm/9917

Yes they are at the for v2.6.40 series of patches.

Mauro.

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Simon Farnsworth]

On Tuesday 3 May 2011, Andy Walls <awalls@md.metrocast.net> wrote:
> Simon,
> 
> If these two changes are going in, please also bump the driver version to
> 1.5.0 in cx18-version.c.  These changes are significant enough
> perturbation.
> 
> End users are going to look to driver version 1.4.1 as the first version
> for proper analog tuner support of the HVR1600 model 74351.
> 
> Mauro,
> 
> Is cx18 v1.4.1 with HVR1600 model 74351 analog tuner support, without these
> mmap() changes going to be visible in kernel version .39 ?
> 

Mauro,

If you're going to accept these two patches, would you mind bumping the 
version in cx18-version.c for me as you apply them, or would you prefer me to 
provide either an incremental patch or a new patch with the bump added?
-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Mauro Carvalho Chehab]

Em 04-05-2011 06:32, Simon Farnsworth escreveu:
> On Tuesday 3 May 2011, Andy Walls <awalls@md.metrocast.net> wrote:
>> Simon,
>>
>> If these two changes are going in, please also bump the driver version to
>> 1.5.0 in cx18-version.c.  These changes are significant enough
>> perturbation.
>>
>> End users are going to look to driver version 1.4.1 as the first version
>> for proper analog tuner support of the HVR1600 model 74351.
>>
>> Mauro,
>>
>> Is cx18 v1.4.1 with HVR1600 model 74351 analog tuner support, without these
>> mmap() changes going to be visible in kernel version .39 ?
>>
> 
> Mauro,
> 
> If you're going to accept these two patches, would you mind bumping the 
> version in cx18-version.c for me as you apply them, or would you prefer me to 
> provide either an incremental patch or a new patch with the bump added?

Please, provide me a patch for it. I'm still stuck handling pending patches,
as patchwork lost some stuff.

Thanks!
Mauro

[PATCH] cx18: Bump driver version to 1.5.0

[Simon Farnsworth]

To simplify maintainer support of this driver, bump the version to
1.5.0 - this will be the first version that is expected to support
mmap() for raw video frames.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
Mauro,

This is an incremental patch to apply on top of my cleanup patch - if
you would prefer a complete new patch with this squashed into the
cleanup patch, just ask and it will be done.

 drivers/media/video/cx18/cx18-version.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/cx18/cx18-version.h b/drivers/media/video/cx18/cx18-version.h
index 62c6ca2..cd189b6 100644
--- a/drivers/media/video/cx18/cx18-version.h
+++ b/drivers/media/video/cx18/cx18-version.h
@@ -24,8 +24,8 @@
 
 #define CX18_DRIVER_NAME "cx18"
 #define CX18_DRIVER_VERSION_MAJOR 1
-#define CX18_DRIVER_VERSION_MINOR 4
-#define CX18_DRIVER_VERSION_PATCHLEVEL 1
+#define CX18_DRIVER_VERSION_MINOR 5
+#define CX18_DRIVER_VERSION_PATCHLEVEL 0
 
 #define CX18_VERSION __stringify(CX18_DRIVER_VERSION_MAJOR) "." __stringify(CX18_DRIVER_VERSION_MINOR) "." __stringify(CX18_DRIVER_VERSION_PATCHLEVEL)
 #define CX18_DRIVER_VERSION KERNEL_VERSION(CX18_DRIVER_VERSION_MAJOR, \
-- 
1.7.4

Re: [PATCH] cx18: Bump driver version to 1.5.0

[Andy Walls]

Simon Farnsworth <simon.farnsworth@onelan.co.uk> wrote:

>To simplify maintainer support of this driver, bump the version to
>1.5.0 - this will be the first version that is expected to support
>mmap() for raw video frames.
>
>Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
>---
>Mauro,
>
>This is an incremental patch to apply on top of my cleanup patch - if
>you would prefer a complete new patch with this squashed into the
>cleanup patch, just ask and it will be done.
>
> drivers/media/video/cx18/cx18-version.h |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/media/video/cx18/cx18-version.h
>b/drivers/media/video/cx18/cx18-version.h
>index 62c6ca2..cd189b6 100644
>--- a/drivers/media/video/cx18/cx18-version.h
>+++ b/drivers/media/video/cx18/cx18-version.h
>@@ -24,8 +24,8 @@
> 
> #define CX18_DRIVER_NAME "cx18"
> #define CX18_DRIVER_VERSION_MAJOR 1
>-#define CX18_DRIVER_VERSION_MINOR 4
>-#define CX18_DRIVER_VERSION_PATCHLEVEL 1
>+#define CX18_DRIVER_VERSION_MINOR 5
>+#define CX18_DRIVER_VERSION_PATCHLEVEL 0
> 
>#define CX18_VERSION __stringify(CX18_DRIVER_VERSION_MAJOR) "."
>__stringify(CX18_DRIVER_VERSION_MINOR) "."
>__stringify(CX18_DRIVER_VERSION_PATCHLEVEL)
>#define CX18_DRIVER_VERSION KERNEL_VERSION(CX18_DRIVER_VERSION_MAJOR, \
>-- 
>1.7.4
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media"
>in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks Simon.

Acked-by: Andy Walls <awalls@md.metrocast.net>

Regards,
Andy

[PATCH] cx18: Fix warnings introduced during cleanup

[Simon Farnsworth]

I misused the ktime API, and failed to remove some traces of the
in-kernel format conversion. Fix these, so the the driver builds
without warnings.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
Mauro,

You may want to squash this in with the cleanup patch itself - it's
plain and simple oversight on my part (I should have seen the compiler
warnings), and I should not have sent the cleanup patch to you without
fixing these errors.

Sorry,

Simon

drivers/media/video/cx18/cx18-mailbox.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/cx18/cx18-mailbox.c b/drivers/media/video/cx18/cx18-mailbox.c
index 5ecae93..c07191e 100644
--- a/drivers/media/video/cx18/cx18-mailbox.c
+++ b/drivers/media/video/cx18/cx18-mailbox.c
@@ -164,10 +164,9 @@ static void cx18_mdl_send_to_videobuf(struct cx18_stream *s,
 {
 	struct cx18_videobuf_buffer *vb_buf;
 	struct cx18_buffer *buf;
-	u8 *p, u;
+	u8 *p;
 	u32 offset = 0;
 	int dispatch = 0;
-	int i;
 
 	if (mdl->bytesused == 0)
 		return;
@@ -203,7 +202,7 @@ static void cx18_mdl_send_to_videobuf(struct cx18_stream *s,
 	}
 
 	if (dispatch) {
-		ktime_get_ts(&vb_buf->vb.ts);
+		vb_buf->vb.ts = ktime_to_timeval(ktime_get());
 		list_del(&vb_buf->vb.queue);
 		vb_buf->vb.state = VIDEOBUF_DONE;
 		wake_up(&vb_buf->vb.done);
-- 
1.7.4

Re: [PATCH] cx18: Fix warnings introduced during cleanup

[Mauro Carvalho Chehab]

Em 05-05-2011 09:42, Simon Farnsworth escreveu:
> I misused the ktime API, and failed to remove some traces of the
> in-kernel format conversion. Fix these, so the the driver builds
> without warnings.
> 
> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> ---
> Mauro,
> 
> You may want to squash this in with the cleanup patch itself - it's
> plain and simple oversight on my part (I should have seen the compiler
> warnings), and I should not have sent the cleanup patch to you without
> fixing these errors.
> 
It will all depend on how much time I'll have during the next merge window.
I can't do it at the already-applied patches, as other people use it as basis,
and a rebase there would break its clones.

Mauro.

Re: [PATCH] cx18: Fix warnings introduced during cleanup

[Simon Farnsworth]

On Thursday 5 May 2011, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> Em 05-05-2011 09:42, Simon Farnsworth escreveu:
> > I misused the ktime API, and failed to remove some traces of the
> > in-kernel format conversion. Fix these, so the the driver builds
> > without warnings.
> > 
> > Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> > ---
> > Mauro,
> > 
> > You may want to squash this in with the cleanup patch itself - it's
> > plain and simple oversight on my part (I should have seen the compiler
> > warnings), and I should not have sent the cleanup patch to you without
> > fixing these errors.
> 
> It will all depend on how much time I'll have during the next merge window.
> I can't do it at the already-applied patches, as other people use it as
> basis, and a rebase there would break its clones.
> 
> Mauro.

No problem; if it ends up as a separate patch in the tree, it's not going to 
hurt, as the version with warnings functions adequately enough to not break 
bisection.
-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/

[PATCH] cx18: Move spinlock and vb_type initialisation into stream_init

[Simon Farnsworth]

The initialisation of vb_type in serialized_open was preventing
REQBUFS from working reliably. Remove it, and move the spinlock into
stream_init for good measure - it's only used when we have a stream
that supports videobuf anyway.

Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
---
Mauro,

This fixes a bug I introduced, and noticed while trying to work out
how videobuf works and interacts with the rest of the driver, in
preparation for working out how to port this code to videobuf2.

Briefly, if you open a device node at the wrong time, you lose
videobuf support forever.

Please consider this for 2.6.40,

Simon

 drivers/media/video/cx18/cx18-fileops.c |    3 ---
 drivers/media/video/cx18/cx18-streams.c |    2 ++
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/cx18/cx18-fileops.c b/drivers/media/video/cx18/cx18-fileops.c
index 6609222..07411f3 100644
--- a/drivers/media/video/cx18/cx18-fileops.c
+++ b/drivers/media/video/cx18/cx18-fileops.c
@@ -810,9 +810,6 @@ static int cx18_serialized_open(struct cx18_stream *s, struct file *filp)
 	item->cx = cx;
 	item->type = s->type;
 
-	spin_lock_init(&s->vbuf_q_lock);
-	s->vb_type = 0;
-
 	item->open_id = cx->open_id++;
 	filp->private_data = &item->fh;
 
diff --git a/drivers/media/video/cx18/cx18-streams.c b/drivers/media/video/cx18/cx18-streams.c
index 24c9688..4282ff5 100644
--- a/drivers/media/video/cx18/cx18-streams.c
+++ b/drivers/media/video/cx18/cx18-streams.c
@@ -275,6 +275,8 @@ static void cx18_stream_init(struct cx18 *cx, int type)
 	init_timer(&s->vb_timeout);
 	spin_lock_init(&s->vb_lock);
 	if (type == CX18_ENC_STREAM_TYPE_YUV) {
+		spin_lock_init(&s->vbuf_q_lock);
+
 		s->vb_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 		videobuf_queue_vmalloc_init(&s->vbuf_q, &cx18_videobuf_qops,
 			&cx->pci_dev->dev, &s->vbuf_q_lock,
-- 
1.7.4

Re: [PATCH] cx18: Move spinlock and vb_type initialisation into stream_init

[Mauro Carvalho Chehab]

Em 10-05-2011 10:49, Simon Farnsworth escreveu:
> The initialisation of vb_type in serialized_open was preventing
> REQBUFS from working reliably. Remove it, and move the spinlock into
> stream_init for good measure - it's only used when we have a stream
> that supports videobuf anyway.
> 
> Signed-off-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
> ---
> Mauro,
> 
> This fixes a bug I introduced, and noticed while trying to work out
> how videobuf works and interacts with the rest of the driver, in
> preparation for working out how to port this code to videobuf2.
> 
> Briefly, if you open a device node at the wrong time, you lose
> videobuf support forever.
> 
> Please consider this for 2.6.40,

/me is assuming that Andy is ok with it.

Ok, I'm adding this to my series, as it is part of the code you added.


Thanks,
Mauro.

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Mauro Carvalho Chehab]

Hi Simon,

Em 04-05-2011 06:32, Simon Farnsworth escreveu:
> On Tuesday 3 May 2011, Andy Walls <awalls@md.metrocast.net> wrote:
>> Simon,
>>
>> If these two changes are going in, please also bump the driver version to
>> 1.5.0 in cx18-version.c.  These changes are significant enough
>> perturbation.
>>
>> End users are going to look to driver version 1.4.1 as the first version
>> for proper analog tuner support of the HVR1600 model 74351.
>>
>> Mauro,
>>
>> Is cx18 v1.4.1 with HVR1600 model 74351 analog tuner support, without these
>> mmap() changes going to be visible in kernel version .39 ?
>>
> 
> Mauro,
> 
> If you're going to accept these two patches, would you mind bumping the 
> version in cx18-version.c for me as you apply them, or would you prefer me to 
> provide either an incremental patch or a new patch with the bump added?

There are a few new warnings with your code:

drivers/media/video/cx18/cx18-mailbox.c: In function ‘cx18_mdl_send_to_videobuf’:
drivers/media/video/cx18/cx18-mailbox.c:206: warning: passing argument 1 of ‘ktime_get_ts’ from incompatible pointer type
include/linux/ktime.h:331: note: expected ‘struct timespec *’ but argument is of type ‘struct timeval *’
drivers/media/video/cx18/cx18-mailbox.c:170: warning: unused variable ‘i’
drivers/media/video/cx18/cx18-mailbox.c:167: warning: unused variable ‘u’

Could you please fix them?

Thanks,
Mauro

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Simon Farnsworth]

On Thursday 5 May 2011, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> There are a few new warnings with your code:
> 
> drivers/media/video/cx18/cx18-mailbox.c: In function
> ‘cx18_mdl_send_to_videobuf’: drivers/media/video/cx18/cx18-mailbox.c:206:
> warning: passing argument 1 of ‘ktime_get_ts’ from incompatible pointer
> type include/linux/ktime.h:331: note: expected ‘struct timespec *’ but
> argument is of type ‘struct timeval *’
> drivers/media/video/cx18/cx18-mailbox.c:170: warning: unused variable ‘i’
> drivers/media/video/cx18/cx18-mailbox.c:167: warning: unused variable ‘u’
> 
> Could you please fix them?
> 
I'm not doing well on the driving git front today, and I've managed to send 
the fix patch with a wrong "In-reply-to"; it's message ID is 
<1304599356-21951-1-git-send-email-simon.farnsworth@onelan.co.uk>, and it's 
elsewhere in this thread (in reply to <4DC138F7.5050400@infradead.org>)
-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/

Re: [PATCH] cx18: Clean up mmap() support for raw YUV

[Mauro Carvalho Chehab]

Em 05-05-2011 09:44, Simon Farnsworth escreveu:
> On Thursday 5 May 2011, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>> There are a few new warnings with your code:
>>
>> drivers/media/video/cx18/cx18-mailbox.c: In function
>> ‘cx18_mdl_send_to_videobuf’: drivers/media/video/cx18/cx18-mailbox.c:206:
>> warning: passing argument 1 of ‘ktime_get_ts’ from incompatible pointer
>> type include/linux/ktime.h:331: note: expected ‘struct timespec *’ but
>> argument is of type ‘struct timeval *’
>> drivers/media/video/cx18/cx18-mailbox.c:170: warning: unused variable ‘i’
>> drivers/media/video/cx18/cx18-mailbox.c:167: warning: unused variable ‘u’
>>
>> Could you please fix them?
>>
> I'm not doing well on the driving git front today, and I've managed to send 
> the fix patch with a wrong "In-reply-to"; it's message ID is 
> <1304599356-21951-1-git-send-email-simon.farnsworth@onelan.co.uk>, and it's 
> elsewhere in this thread (in reply to <4DC138F7.5050400@infradead.org>)

No problem. I don't rely very much on in-reply-to, as patchwork doesn't care
about it (unfortunately, as it would help to detect patches superseded/grouped).

Mauro.