[REVIEW PATCH 0/2] data_offset for single plane buffers, packed raw10

[REVIEW PATCH 0/2] data_offset for single plane buffers, packed raw10

[Sakari Ailus]

Hi folks,

These two patches add data_offset support for single-plane buffers and
definitions and documentation for 10-bit packed raw bayer formats.

-- 
Kind regards,
Sakari

[REVIEW PATCH 1/2] v4l: Add data_offset to struct v4l2_buffer

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The data_offset field tells the start of the image data from the beginning
of the buffer. The bsize field in struct v4l2_buffer includes this, but the
sizeimage field in struct v4l2_pix_format does not.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/DocBook/media/v4l/compat.xml      | 11 +++++++++++
 Documentation/DocBook/media/v4l/io.xml          | 18 +++++++++++++++---
 Documentation/DocBook/media/v4l/vidioc-qbuf.xml |  3 +--
 drivers/media/usb/cpia2/cpia2_v4l.c             |  2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c   |  4 ++--
 drivers/media/v4l2-core/videobuf2-core.c        | 17 ++++++++++++-----
 include/uapi/linux/videodev2.h                  |  4 +++-
 7 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
index 0a2debf..ad54e72 100644
--- a/Documentation/DocBook/media/v4l/compat.xml
+++ b/Documentation/DocBook/media/v4l/compat.xml
@@ -2579,6 +2579,17 @@ fields changed from _s32 to _u32.
       </orderedlist>
     </section>
 
+    <section>
+      <title>V4L2 in Linux 3.20</title>
+      <orderedlist>
+	<listitem>
+	  <para>Replaced <structfield>reserved2</structfield> by
+	  <strucfield>data_offset<structfield> in struct
+	  <structname>v4l2_buffer</structname>.</para>
+	</listitem>
+      </orderedlist>
+    </section>
+
     <section id="other">
       <title>Relation of V4L2 to other Linux multimedia APIs</title>
 
diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 1c17f80..13baeac 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -839,10 +839,22 @@ is the file descriptor associated with a DMABUF buffer.</entry>
 	  </row>
 	  <row>
 	    <entry>__u32</entry>
-	    <entry><structfield>reserved2</structfield></entry>
+	    <entry><structfield>data_offset</structfield></entry>
 	    <entry></entry>
-	    <entry>A place holder for future extensions. Applications
-should set this to 0.</entry>
+	    <entry>
+	      Start of the image data from the beginning of the buffer in
+	      bytes. Applications must set this for both
+	      <constant>V4L2_BUF_TYPE_VIDEO_OUTPUT</constant> buffers
+	      whereas driver must set this for
+	      <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant> buffers
+	      before &VIDIOC-PREPARE-BUF; and &VIDIOC-QBUF; IOCTLs. Note
+	      that data_offset is included in
+	      <structfield>bytesused</structfield>. So the size of the image
+	      in the plane is <structfield>bytesused</structfield>-
+	      <structfield>data_offset</structfield> at offset
+	      <structfield>data_offset</structfield> from the start of the
+	      plane.
+	    </entry>
 	  </row>
 	  <row>
 	    <entry>__u32</entry>
diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
index 3504a7f..f529e4d 100644
--- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
@@ -72,8 +72,7 @@ initialize the <structfield>bytesused</structfield>,
 <structfield>timestamp</structfield> fields, see <xref
 linkend="buffer" /> for details.
 Applications must also set <structfield>flags</structfield> to 0.
-The <structfield>reserved2</structfield> and
-<structfield>reserved</structfield> fields must be set to 0. When using
+The <structfield>reserved</structfield> field must be set to 0. When using
 the <link linkend="planar-apis">multi-planar API</link>, the
 <structfield>m.planes</structfield> field must contain a userspace pointer
 to a filled-in array of &v4l2-plane; and the <structfield>length</structfield>
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 9caea83..a94e83a 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -952,7 +952,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	buf->sequence = cam->buffers[buf->index].seq;
 	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
 	buf->length = cam->frame_size;
-	buf->reserved2 = 0;
+	buf->data_offset = 0;
 	buf->reserved = 0;
 	memset(&buf->timecode, 0, sizeof(buf->timecode));
 
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index af63543..e238066 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -326,7 +326,7 @@ struct v4l2_buffer32 {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__u32			data_offset;
 	__u32			reserved;
 };
 
@@ -491,7 +491,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
 		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
 		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
 		put_user(kp->sequence, &up->sequence) ||
-		put_user(kp->reserved2, &up->reserved2) ||
+		put_user(kp->data_offset, &up->data_offset) ||
 		put_user(kp->reserved, &up->reserved))
 			return -EFAULT;
 
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 7aed8f2..3162de8 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -607,6 +607,9 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 
 		if (b->bytesused > length)
 			return -EINVAL;
+
+		if (b->data_offset > 0 && b->data_offset >= bytesused)
+			return -EINVAL;
 	}
 
 	return 0;
@@ -657,7 +660,6 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 
 	/* Copy back data such as timestamp, flags, etc. */
 	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
-	b->reserved2 = vb->v4l2_buf.reserved2;
 	b->reserved = vb->v4l2_buf.reserved;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
@@ -666,14 +668,17 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 		 * for it. The caller has already verified memory and size.
 		 */
 		b->length = vb->num_planes;
+		b->data_offset = vb->v4l2_buf.data_offset;
 		memcpy(b->m.planes, vb->v4l2_planes,
 			b->length * sizeof(struct v4l2_plane));
 	} else {
 		/*
-		 * We use length and offset in v4l2_planes array even for
-		 * single-planar buffers, but userspace does not.
+		 * We use length, data_offset and bytesused in
+		 * v4l2_planes array even for single-planar buffers,
+		 * but userspace does not.
 		 */
 		b->length = vb->v4l2_planes[0].length;
+		b->data_offset = vb->v4l2_planes[0].data_offset;
 		b->bytesused = vb->v4l2_planes[0].bytesused;
 		if (q->memory == V4L2_MEMORY_MMAP)
 			b->m.offset = vb->v4l2_planes[0].m.mem_offset;
@@ -1306,11 +1311,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 			v4l2_planes[0].length = b->length;
 		}
 
-		if (V4L2_TYPE_IS_OUTPUT(b->type))
+		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
 			v4l2_planes[0].bytesused = b->bytesused ?
 				b->bytesused : v4l2_planes[0].length;
-		else
+			v4l2_planes[0].data_offset = b->data_offset;
+		} else {
 			v4l2_planes[0].bytesused = 0;
+		}
 
 	}
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 1c2f84f..e9806c6 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -675,6 +675,8 @@ struct v4l2_plane {
  * @length:	size in bytes of the buffer (NOT its payload) for single-plane
  *		buffers (when type != *_MPLANE); number of elements in the
  *		planes array for multi-plane buffers
+ * @data_offset: Offset of the start of data from the beginning of the
+ *		buffer. Typically zero.
  *
  * Contains data exchanged by application and driver using one of the Streaming
  * I/O methods.
@@ -698,7 +700,7 @@ struct v4l2_buffer {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__u32			data_offset;
 	__u32			reserved;
 };
 
-- 
1.9.1

Re: [REVIEW PATCH 1/2] v4l: Add data_offset to struct v4l2_buffer

[Hans Verkuil]

On 12/03/2014 12:14 PM, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> The data_offset field tells the start of the image data from the beginning
> of the buffer. The bsize field 

bsize field? There is no bsize field in v4l2_buffer, so I'm confused.

> in struct v4l2_buffer includes this, but the
> sizeimage field in struct v4l2_pix_format does not.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/DocBook/media/v4l/compat.xml      | 11 +++++++++++
>  Documentation/DocBook/media/v4l/io.xml          | 18 +++++++++++++++---
>  Documentation/DocBook/media/v4l/vidioc-qbuf.xml |  3 +--
>  drivers/media/usb/cpia2/cpia2_v4l.c             |  2 +-
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c   |  4 ++--
>  drivers/media/v4l2-core/videobuf2-core.c        | 17 ++++++++++++-----
>  include/uapi/linux/videodev2.h                  |  4 +++-
>  7 files changed, 45 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
> index 0a2debf..ad54e72 100644
> --- a/Documentation/DocBook/media/v4l/compat.xml
> +++ b/Documentation/DocBook/media/v4l/compat.xml
> @@ -2579,6 +2579,17 @@ fields changed from _s32 to _u32.
>        </orderedlist>
>      </section>
>  
> +    <section>
> +      <title>V4L2 in Linux 3.20</title>
> +      <orderedlist>
> +	<listitem>
> +	  <para>Replaced <structfield>reserved2</structfield> by
> +	  <strucfield>data_offset<structfield> in struct
> +	  <structname>v4l2_buffer</structname>.</para>
> +	</listitem>
> +      </orderedlist>
> +    </section>
> +
>      <section id="other">
>        <title>Relation of V4L2 to other Linux multimedia APIs</title>
>  
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 1c17f80..13baeac 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -839,10 +839,22 @@ is the file descriptor associated with a DMABUF buffer.</entry>
>  	  </row>
>  	  <row>
>  	    <entry>__u32</entry>
> -	    <entry><structfield>reserved2</structfield></entry>
> +	    <entry><structfield>data_offset</structfield></entry>
>  	    <entry></entry>
> -	    <entry>A place holder for future extensions. Applications
> -should set this to 0.</entry>
> +	    <entry>
> +	      Start of the image data from the beginning of the buffer in
> +	      bytes. Applications must set this for both

Drop 'both'.

> +	      <constant>V4L2_BUF_TYPE_VIDEO_OUTPUT</constant> buffers
> +	      whereas driver must set this for
> +	      <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant> buffers
> +	      before &VIDIOC-PREPARE-BUF; and &VIDIOC-QBUF; IOCTLs. Note

s/IOCTLs/ioctls/

> +	      that data_offset is included in
> +	      <structfield>bytesused</structfield>. So the size of the image
> +	      in the plane is <structfield>bytesused</structfield>-
> +	      <structfield>data_offset</structfield> at offset
> +	      <structfield>data_offset</structfield> from the start of the
> +	      plane.
> +	    </entry>
>  	  </row>
>  	  <row>
>  	    <entry>__u32</entry>
> diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> index 3504a7f..f529e4d 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> @@ -72,8 +72,7 @@ initialize the <structfield>bytesused</structfield>,
>  <structfield>timestamp</structfield> fields, see <xref
>  linkend="buffer" /> for details.
>  Applications must also set <structfield>flags</structfield> to 0.
> -The <structfield>reserved2</structfield> and
> -<structfield>reserved</structfield> fields must be set to 0. When using
> +The <structfield>reserved</structfield> field must be set to 0. When using
>  the <link linkend="planar-apis">multi-planar API</link>, the
>  <structfield>m.planes</structfield> field must contain a userspace pointer
>  to a filled-in array of &v4l2-plane; and the <structfield>length</structfield>
> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
> index 9caea83..a94e83a 100644
> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> @@ -952,7 +952,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  	buf->sequence = cam->buffers[buf->index].seq;
>  	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>  	buf->length = cam->frame_size;
> -	buf->reserved2 = 0;
> +	buf->data_offset = 0;
>  	buf->reserved = 0;
>  	memset(&buf->timecode, 0, sizeof(buf->timecode));
>  
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index af63543..e238066 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -326,7 +326,7 @@ struct v4l2_buffer32 {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__u32			data_offset;
>  	__u32			reserved;
>  };
>  
> @@ -491,7 +491,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>  		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>  		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
>  		put_user(kp->sequence, &up->sequence) ||
> -		put_user(kp->reserved2, &up->reserved2) ||
> +		put_user(kp->data_offset, &up->data_offset) ||
>  		put_user(kp->reserved, &up->reserved))
>  			return -EFAULT;
>  
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 7aed8f2..3162de8 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -607,6 +607,9 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>  
>  		if (b->bytesused > length)
>  			return -EINVAL;
> +
> +		if (b->data_offset > 0 && b->data_offset >= bytesused)
> +			return -EINVAL;
>  	}
>  
>  	return 0;
> @@ -657,7 +660,6 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  
>  	/* Copy back data such as timestamp, flags, etc. */
>  	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
> -	b->reserved2 = vb->v4l2_buf.reserved2;
>  	b->reserved = vb->v4l2_buf.reserved;
>  
>  	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
> @@ -666,14 +668,17 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  		 * for it. The caller has already verified memory and size.
>  		 */
>  		b->length = vb->num_planes;
> +		b->data_offset = vb->v4l2_buf.data_offset;
>  		memcpy(b->m.planes, vb->v4l2_planes,
>  			b->length * sizeof(struct v4l2_plane));
>  	} else {
>  		/*
> -		 * We use length and offset in v4l2_planes array even for
> -		 * single-planar buffers, but userspace does not.
> +		 * We use length, data_offset and bytesused in
> +		 * v4l2_planes array even for single-planar buffers,
> +		 * but userspace does not.
>  		 */
>  		b->length = vb->v4l2_planes[0].length;
> +		b->data_offset = vb->v4l2_planes[0].data_offset;
>  		b->bytesused = vb->v4l2_planes[0].bytesused;
>  		if (q->memory == V4L2_MEMORY_MMAP)
>  			b->m.offset = vb->v4l2_planes[0].m.mem_offset;
> @@ -1306,11 +1311,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>  			v4l2_planes[0].length = b->length;
>  		}
>  
> -		if (V4L2_TYPE_IS_OUTPUT(b->type))
> +		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>  			v4l2_planes[0].bytesused = b->bytesused ?
>  				b->bytesused : v4l2_planes[0].length;
> -		else
> +			v4l2_planes[0].data_offset = b->data_offset;
> +		} else {
>  			v4l2_planes[0].bytesused = 0;
> +		}
>  
>  	}
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 1c2f84f..e9806c6 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -675,6 +675,8 @@ struct v4l2_plane {
>   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>   *		buffers (when type != *_MPLANE); number of elements in the
>   *		planes array for multi-plane buffers
> + * @data_offset: Offset of the start of data from the beginning of the
> + *		buffer. Typically zero.
>   *
>   * Contains data exchanged by application and driver using one of the Streaming
>   * I/O methods.
> @@ -698,7 +700,7 @@ struct v4l2_buffer {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__u32			data_offset;
>  	__u32			reserved;
>  };
>  
> 

I think we need to add new helper functions that give back the real plane size
(i.e. bytesused - data_offset) and the actual plane start position (plane start
+ data_offset). It will be a bit tricky though to check existing drivers.

AFAICT vivid is one driver that uses vb2_plane_size() to check if enough space
is available for the image, but that doesn't take the data_offset into account.

I suspect that similar problems occur for output drivers. And what isn't properly
defined at the moment is what should happen if an output driver doesn't support
a particular data_offset value.

I think the only thing you can do in that case is to return an error when QBUF
is called.

Regards,

	Hans

Re: [REVIEW PATCH 1/2] v4l: Add data_offset to struct v4l2_buffer

[Sakari Ailus]

Hi Hans,

On Fri, Dec 05, 2014 at 04:10:05PM +0100, Hans Verkuil wrote:
> On 12/03/2014 12:14 PM, Sakari Ailus wrote:
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > The data_offset field tells the start of the image data from the beginning
> > of the buffer. The bsize field 
> 
> bsize field? There is no bsize field in v4l2_buffer, so I'm confused.

This is a brain'o. It should have been "bytesused". I'll fix that to the
next version.

> > in struct v4l2_buffer includes this, but the
> > sizeimage field in struct v4l2_pix_format does not.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  Documentation/DocBook/media/v4l/compat.xml      | 11 +++++++++++
> >  Documentation/DocBook/media/v4l/io.xml          | 18 +++++++++++++++---
> >  Documentation/DocBook/media/v4l/vidioc-qbuf.xml |  3 +--
> >  drivers/media/usb/cpia2/cpia2_v4l.c             |  2 +-
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c   |  4 ++--
> >  drivers/media/v4l2-core/videobuf2-core.c        | 17 ++++++++++++-----
> >  include/uapi/linux/videodev2.h                  |  4 +++-
> >  7 files changed, 45 insertions(+), 14 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
> > index 0a2debf..ad54e72 100644
> > --- a/Documentation/DocBook/media/v4l/compat.xml
> > +++ b/Documentation/DocBook/media/v4l/compat.xml
> > @@ -2579,6 +2579,17 @@ fields changed from _s32 to _u32.
> >        </orderedlist>
> >      </section>
> >  
> > +    <section>
> > +      <title>V4L2 in Linux 3.20</title>
> > +      <orderedlist>
> > +	<listitem>
> > +	  <para>Replaced <structfield>reserved2</structfield> by
> > +	  <strucfield>data_offset<structfield> in struct
> > +	  <structname>v4l2_buffer</structname>.</para>
> > +	</listitem>
> > +      </orderedlist>
> > +    </section>
> > +
> >      <section id="other">
> >        <title>Relation of V4L2 to other Linux multimedia APIs</title>
> >  
> > diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> > index 1c17f80..13baeac 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -839,10 +839,22 @@ is the file descriptor associated with a DMABUF buffer.</entry>
> >  	  </row>
> >  	  <row>
> >  	    <entry>__u32</entry>
> > -	    <entry><structfield>reserved2</structfield></entry>
> > +	    <entry><structfield>data_offset</structfield></entry>
> >  	    <entry></entry>
> > -	    <entry>A place holder for future extensions. Applications
> > -should set this to 0.</entry>
> > +	    <entry>
> > +	      Start of the image data from the beginning of the buffer in
> > +	      bytes. Applications must set this for both
> 
> Drop 'both'.

Fixed.

> > +	      <constant>V4L2_BUF_TYPE_VIDEO_OUTPUT</constant> buffers
> > +	      whereas driver must set this for
> > +	      <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant> buffers
> > +	      before &VIDIOC-PREPARE-BUF; and &VIDIOC-QBUF; IOCTLs. Note
> 
> s/IOCTLs/ioctls/

Fixed.

> > +	      that data_offset is included in
> > +	      <structfield>bytesused</structfield>. So the size of the image
> > +	      in the plane is <structfield>bytesused</structfield>-
> > +	      <structfield>data_offset</structfield> at offset
> > +	      <structfield>data_offset</structfield> from the start of the
> > +	      plane.
> > +	    </entry>
> >  	  </row>
> >  	  <row>
> >  	    <entry>__u32</entry>
> > diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > index 3504a7f..f529e4d 100644
> > --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > @@ -72,8 +72,7 @@ initialize the <structfield>bytesused</structfield>,
> >  <structfield>timestamp</structfield> fields, see <xref
> >  linkend="buffer" /> for details.
> >  Applications must also set <structfield>flags</structfield> to 0.
> > -The <structfield>reserved2</structfield> and
> > -<structfield>reserved</structfield> fields must be set to 0. When using
> > +The <structfield>reserved</structfield> field must be set to 0. When using
> >  the <link linkend="planar-apis">multi-planar API</link>, the
> >  <structfield>m.planes</structfield> field must contain a userspace pointer
> >  to a filled-in array of &v4l2-plane; and the <structfield>length</structfield>
> > diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
> > index 9caea83..a94e83a 100644
> > --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> > +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> > @@ -952,7 +952,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
> >  	buf->sequence = cam->buffers[buf->index].seq;
> >  	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
> >  	buf->length = cam->frame_size;
> > -	buf->reserved2 = 0;
> > +	buf->data_offset = 0;
> >  	buf->reserved = 0;
> >  	memset(&buf->timecode, 0, sizeof(buf->timecode));
> >  
> > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > index af63543..e238066 100644
> > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > @@ -326,7 +326,7 @@ struct v4l2_buffer32 {
> >  		__s32		fd;
> >  	} m;
> >  	__u32			length;
> > -	__u32			reserved2;
> > +	__u32			data_offset;
> >  	__u32			reserved;
> >  };
> >  
> > @@ -491,7 +491,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
> >  		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
> >  		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
> >  		put_user(kp->sequence, &up->sequence) ||
> > -		put_user(kp->reserved2, &up->reserved2) ||
> > +		put_user(kp->data_offset, &up->data_offset) ||
> >  		put_user(kp->reserved, &up->reserved))
> >  			return -EFAULT;
> >  
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 7aed8f2..3162de8 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -607,6 +607,9 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> >  
> >  		if (b->bytesused > length)
> >  			return -EINVAL;
> > +
> > +		if (b->data_offset > 0 && b->data_offset >= bytesused)
> > +			return -EINVAL;
> >  	}
> >  
> >  	return 0;
> > @@ -657,7 +660,6 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  
> >  	/* Copy back data such as timestamp, flags, etc. */
> >  	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
> > -	b->reserved2 = vb->v4l2_buf.reserved2;
> >  	b->reserved = vb->v4l2_buf.reserved;
> >  
> >  	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
> > @@ -666,14 +668,17 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  		 * for it. The caller has already verified memory and size.
> >  		 */
> >  		b->length = vb->num_planes;
> > +		b->data_offset = vb->v4l2_buf.data_offset;
> >  		memcpy(b->m.planes, vb->v4l2_planes,
> >  			b->length * sizeof(struct v4l2_plane));
> >  	} else {
> >  		/*
> > -		 * We use length and offset in v4l2_planes array even for
> > -		 * single-planar buffers, but userspace does not.
> > +		 * We use length, data_offset and bytesused in
> > +		 * v4l2_planes array even for single-planar buffers,
> > +		 * but userspace does not.
> >  		 */
> >  		b->length = vb->v4l2_planes[0].length;
> > +		b->data_offset = vb->v4l2_planes[0].data_offset;
> >  		b->bytesused = vb->v4l2_planes[0].bytesused;
> >  		if (q->memory == V4L2_MEMORY_MMAP)
> >  			b->m.offset = vb->v4l2_planes[0].m.mem_offset;
> > @@ -1306,11 +1311,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
> >  			v4l2_planes[0].length = b->length;
> >  		}
> >  
> > -		if (V4L2_TYPE_IS_OUTPUT(b->type))
> > +		if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> >  			v4l2_planes[0].bytesused = b->bytesused ?
> >  				b->bytesused : v4l2_planes[0].length;
> > -		else
> > +			v4l2_planes[0].data_offset = b->data_offset;
> > +		} else {
> >  			v4l2_planes[0].bytesused = 0;
> > +		}
> >  
> >  	}
> >  
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 1c2f84f..e9806c6 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -675,6 +675,8 @@ struct v4l2_plane {
> >   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
> >   *		buffers (when type != *_MPLANE); number of elements in the
> >   *		planes array for multi-plane buffers
> > + * @data_offset: Offset of the start of data from the beginning of the
> > + *		buffer. Typically zero.
> >   *
> >   * Contains data exchanged by application and driver using one of the Streaming
> >   * I/O methods.
> > @@ -698,7 +700,7 @@ struct v4l2_buffer {
> >  		__s32		fd;
> >  	} m;
> >  	__u32			length;
> > -	__u32			reserved2;
> > +	__u32			data_offset;
> >  	__u32			reserved;
> >  };
> >  
> > 
> 
> I think we need to add new helper functions that give back the real plane size
> (i.e. bytesused - data_offset) and the actual plane start position (plane start
> + data_offset). It will be a bit tricky though to check existing drivers.

I think this mostly applies to OUTPUT buffers.

I find the definition for multi-plane buffers a little bit odd --- why not
allow setting this for CAPTURE buffers as well, on hardware that supports
it? This makes sense, in order to use the buffers on other interfaces
without memory copies this may be even mandatory.

I wonder if we should change the spec regarding this, even if no driver
support was added yet.

> AFAICT vivid is one driver that uses vb2_plane_size() to check if enough space
> is available for the image, but that doesn't take the data_offset into account.
> 
> I suspect that similar problems occur for output drivers. And what isn't properly
> defined at the moment is what should happen if an output driver doesn't support
> a particular data_offset value.
> 
> I think the only thing you can do in that case is to return an error when QBUF
> is called.

I'd think so. Same for PREPARE_BUF.

I suppose very few drivers support this at the moment, and the ones that
don't would return -EINVAL on QBUF. This could reveal broken user space
applications. An alternative would be to silently assign a valid value to
the field, but I'm not sure if that's any better.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [REVIEW PATCH 1/2] v4l: Add data_offset to struct v4l2_buffer

[Hans Verkuil]

On 12/06/2014 12:48 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Dec 05, 2014 at 04:10:05PM +0100, Hans Verkuil wrote:
>> On 12/03/2014 12:14 PM, Sakari Ailus wrote:
>>> From: Sakari Ailus <sakari.ailus@linux.intel.com>

<snip>

>> I think we need to add new helper functions that give back the real plane size
>> (i.e. bytesused - data_offset) and the actual plane start position (plane start
>> + data_offset). It will be a bit tricky though to check existing drivers.
> 
> I think this mostly applies to OUTPUT buffers.
> 
> I find the definition for multi-plane buffers a little bit odd --- why not
> allow setting this for CAPTURE buffers as well, on hardware that supports
> it? This makes sense, in order to use the buffers on other interfaces
> without memory copies this may be even mandatory.

It's meant for drivers that have a header before the actual image (e.g. sensor
metadata passed on before the image). Userspace has no control over that, so
that's why it is set by the driver at capture time.

> 
> I wonder if we should change the spec regarding this, even if no driver
> support was added yet.

I don't think so. There is a good and clear reason for this.

> 
>> AFAICT vivid is one driver that uses vb2_plane_size() to check if enough space
>> is available for the image, but that doesn't take the data_offset into account.
>>
>> I suspect that similar problems occur for output drivers. And what isn't properly
>> defined at the moment is what should happen if an output driver doesn't support
>> a particular data_offset value.
>>
>> I think the only thing you can do in that case is to return an error when QBUF
>> is called.
> 
> I'd think so. Same for PREPARE_BUF.
> 
> I suppose very few drivers support this at the moment, and the ones that
> don't would return -EINVAL on QBUF. This could reveal broken user space
> applications. An alternative would be to silently assign a valid value to
> the field, but I'm not sure if that's any better.

I wouldn't do that. In my opinion it is a clear error.

Regards,

	Hans

Re: [REVIEW PATCH 1/2] v4l: Add data_offset to struct v4l2_buffer

[Sakari Ailus]

Hi Hans,

On Sat, Dec 06, 2014 at 01:05:16PM +0100, Hans Verkuil wrote:
> On 12/06/2014 12:48 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Fri, Dec 05, 2014 at 04:10:05PM +0100, Hans Verkuil wrote:
> >> On 12/03/2014 12:14 PM, Sakari Ailus wrote:
> >>> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> <snip>
> 
> >> I think we need to add new helper functions that give back the real plane size
> >> (i.e. bytesused - data_offset) and the actual plane start position (plane start
> >> + data_offset). It will be a bit tricky though to check existing drivers.
> > 
> > I think this mostly applies to OUTPUT buffers.
> > 
> > I find the definition for multi-plane buffers a little bit odd --- why not
> > allow setting this for CAPTURE buffers as well, on hardware that supports
> > it? This makes sense, in order to use the buffers on other interfaces
> > without memory copies this may be even mandatory.
> 
> It's meant for drivers that have a header before the actual image (e.g. sensor
> metadata passed on before the image). Userspace has no control over that, so
> that's why it is set by the driver at capture time.

This depends on hardware actually. Some devices can choose the offset the
data is written to in a buffer, meaning the beginning of the buffer would
not be written to by the hardware.

The "header" is very probably metadata that should be passed to the user
space, but this is out of scope of this discussion.

> > 
> > I wonder if we should change the spec regarding this, even if no driver
> > support was added yet.
> 
> I don't think so. There is a good and clear reason for this.
> 
> > 
> >> AFAICT vivid is one driver that uses vb2_plane_size() to check if enough space
> >> is available for the image, but that doesn't take the data_offset into account.
> >>
> >> I suspect that similar problems occur for output drivers. And what isn't properly
> >> defined at the moment is what should happen if an output driver doesn't support
> >> a particular data_offset value.
> >>
> >> I think the only thing you can do in that case is to return an error when QBUF
> >> is called.
> > 
> > I'd think so. Same for PREPARE_BUF.
> > 
> > I suppose very few drivers support this at the moment, and the ones that
> > don't would return -EINVAL on QBUF. This could reveal broken user space
> > applications. An alternative would be to silently assign a valid value to
> > the field, but I'm not sure if that's any better.
> 
> I wouldn't do that. In my opinion it is a clear error.

I agree. Anyway some people are quite pedantic about it, however broken this
user space application would be.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[REVIEW PATCH 2/2] v4l: Add packed Bayer raw10 pixel formats

[Sakari Ailus]

From: Aviv Greenberg <aviv.d.greenberg@intel.com>

These formats are just like 10-bit raw bayer formats that exist already, but
the pixels are not padded to byte boundaries. Instead, the eight high order
bits of four consecutive pixels are stored in four bytes, followed by a byte
of two low order bits of each of the four pixels.

Signed-off-by: Aviv Greenberg <aviv.d.greenberg@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 .../DocBook/media/v4l/pixfmt-srggb10p.xml          | 83 ++++++++++++++++++++++
 Documentation/DocBook/media/v4l/pixfmt.xml         |  1 +
 include/uapi/linux/videodev2.h                     |  5 ++
 3 files changed, 89 insertions(+)
 create mode 100644 Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml

diff --git a/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml b/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml
new file mode 100644
index 0000000..3e88d8d
--- /dev/null
+++ b/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml
@@ -0,0 +1,83 @@
+    <refentry id="pixfmt-srggb10p">
+      <refmeta>
+	<refentrytitle>V4L2_PIX_FMT_SRGGB10P ('pRAA'),
+	 V4L2_PIX_FMT_SGRBG10P ('pgAA'),
+	 V4L2_PIX_FMT_SGBRG10P ('pGAA'),
+	 V4L2_PIX_FMT_SBGGR10P ('pBAA'),
+	 </refentrytitle>
+	&manvol;
+      </refmeta>
+      <refnamediv>
+	<refname id="V4L2-PIX-FMT-SRGGB10P"><constant>V4L2_PIX_FMT_SRGGB10P</constant></refname>
+	<refname id="V4L2-PIX-FMT-SGRBG10P"><constant>V4L2_PIX_FMT_SGRBG10P</constant></refname>
+	<refname id="V4L2-PIX-FMT-SGBRG10P"><constant>V4L2_PIX_FMT_SGBRG10P</constant></refname>
+	<refname id="V4L2-PIX-FMT-SBGGR10P"><constant>V4L2_PIX_FMT_SBGGR10P</constant></refname>
+	<refpurpose>10-bit packed Bayer formats</refpurpose>
+      </refnamediv>
+      <refsect1>
+	<title>Description</title>
+
+	<para>The following four pixel formats are packed raw sRGB /
+	Bayer formats with 10 bits per colour. Every four consequtive
+	colour components are packed into 5 bytes such that each of
+	the first 4 bytes contain their 8 high bits, and the fifth
+	byte contains 4 groups of 2 their low bits. Bytes are stored
+	in memory in little endian order.</para>
+
+	<para>Each n-pixel row contains n/2 green samples and n/2 blue
+	or red samples, with alternating green-red and green-blue
+	rows. They are conventionally described as GRGR... BGBG...,
+	RGRG... GBGB..., etc. Below is an example of one of these
+	formats</para>
+
+    <example>
+      <title><constant>V4L2_PIX_FMT_SBGGR10P</constant> 4 &times; 4
+      pixel image</title>
+
+      <formalpara>
+	<title>Byte Order.</title>
+	<para>Each cell is one byte.
+	  <informaltable frame="none">
+	    <tgroup cols="5" align="center">
+	      <colspec align="left" colwidth="2*" />
+	      <tbody valign="top">
+		<row>
+		  <entry>start&nbsp;+&nbsp;0:</entry>
+		  <entry>B<subscript>00high</subscript></entry>
+		  <entry>G<subscript>01high</subscript></entry>
+		  <entry>B<subscript>02high</subscript></entry>
+		  <entry>G<subscript>03high</subscript></entry>
+		  <entry>B+G<subscript>0-3low</subscript></entry>
+		</row>
+		<row>
+		  <entry>start&nbsp;+&nbsp;5:</entry>
+		  <entry>G<subscript>04high</subscript></entry>
+		  <entry>R<subscript>05high</subscript></entry>
+		  <entry>G<subscript>06high</subscript></entry>
+		  <entry>R<subscript>07high</subscript></entry>
+		  <entry>G+R<subscript>4-7low</subscript></entry>
+		</row>
+		<row>
+		  <entry>start&nbsp;+&nbsp;10:</entry>
+		  <entry>B<subscript>08high</subscript></entry>
+		  <entry>G<subscript>09high</subscript></entry>
+		  <entry>B<subscript>10high</subscript></entry>
+		  <entry>G<subscript>11high</subscript></entry>
+		  <entry>B+G<subscript>8-11low</subscript></entry>
+		</row>
+		<row>
+          <entry>start&nbsp;+&nbsp;15:</entry>
+		  <entry>G<subscript>12high</subscript></entry>
+		  <entry>R<subscript>13high</subscript></entry>
+		  <entry>G<subscript>14high</subscript></entry>
+		  <entry>R<subscript>15high</subscript></entry>
+		  <entry>G+R<subscript>12-15low</subscript></entry>
+		</row>
+	      </tbody>
+	    </tgroup>
+	  </informaltable>
+	</para>
+      </formalpara>
+    </example>
+  </refsect1>
+</refentry>
diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml b/Documentation/DocBook/media/v4l/pixfmt.xml
index df5b23d..5a83d9c 100644
--- a/Documentation/DocBook/media/v4l/pixfmt.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt.xml
@@ -716,6 +716,7 @@ access the palette, this must be done with ioctls of the Linux framebuffer API.<
     &sub-srggb10alaw8;
     &sub-srggb10dpcm8;
     &sub-srggb12;
+    &sub-srggb10p;
   </section>
 
   <section id="yuv-formats">
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index e9806c6..faba23a 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -402,6 +402,11 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_SGBRG10DPCM8 v4l2_fourcc('b', 'G', 'A', '8')
 #define V4L2_PIX_FMT_SGRBG10DPCM8 v4l2_fourcc('B', 'D', '1', '0')
 #define V4L2_PIX_FMT_SRGGB10DPCM8 v4l2_fourcc('b', 'R', 'A', '8')
+	/* 10bit raw bayer packed, 5 bytes for every 4 pixels */
+#define V4L2_PIX_FMT_SBGGR10P v4l2_fourcc('p', 'B', 'A', 'A')
+#define V4L2_PIX_FMT_SGBRG10P v4l2_fourcc('p', 'G', 'A', 'A')
+#define V4L2_PIX_FMT_SGRBG10P v4l2_fourcc('p', 'g', 'A', 'A')
+#define V4L2_PIX_FMT_SRGGB10P v4l2_fourcc('p', 'R', 'A', 'A')
 	/*
 	 * 10bit raw bayer, expanded to 16 bits
 	 * xxxxrrrrrrrrrrxxxxgggggggggg xxxxggggggggggxxxxbbbbbbbbbb...
-- 
1.9.1

Re: [REVIEW PATCH 2/2] v4l: Add packed Bayer raw10 pixel formats

[Hans Verkuil]

On 12/03/2014 12:14 PM, Sakari Ailus wrote:
> From: Aviv Greenberg <aviv.d.greenberg@intel.com>
> 
> These formats are just like 10-bit raw bayer formats that exist already, but
> the pixels are not padded to byte boundaries. Instead, the eight high order
> bits of four consecutive pixels are stored in four bytes, followed by a byte
> of two low order bits of each of the four pixels.
> 
> Signed-off-by: Aviv Greenberg <aviv.d.greenberg@intel.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  .../DocBook/media/v4l/pixfmt-srggb10p.xml          | 83 ++++++++++++++++++++++
>  Documentation/DocBook/media/v4l/pixfmt.xml         |  1 +
>  include/uapi/linux/videodev2.h                     |  5 ++
>  3 files changed, 89 insertions(+)
>  create mode 100644 Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml
> 
> diff --git a/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml b/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml
> new file mode 100644
> index 0000000..3e88d8d
> --- /dev/null
> +++ b/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml
> @@ -0,0 +1,83 @@
> +    <refentry id="pixfmt-srggb10p">
> +      <refmeta>
> +	<refentrytitle>V4L2_PIX_FMT_SRGGB10P ('pRAA'),
> +	 V4L2_PIX_FMT_SGRBG10P ('pgAA'),
> +	 V4L2_PIX_FMT_SGBRG10P ('pGAA'),
> +	 V4L2_PIX_FMT_SBGGR10P ('pBAA'),
> +	 </refentrytitle>
> +	&manvol;
> +      </refmeta>
> +      <refnamediv>
> +	<refname id="V4L2-PIX-FMT-SRGGB10P"><constant>V4L2_PIX_FMT_SRGGB10P</constant></refname>
> +	<refname id="V4L2-PIX-FMT-SGRBG10P"><constant>V4L2_PIX_FMT_SGRBG10P</constant></refname>
> +	<refname id="V4L2-PIX-FMT-SGBRG10P"><constant>V4L2_PIX_FMT_SGBRG10P</constant></refname>
> +	<refname id="V4L2-PIX-FMT-SBGGR10P"><constant>V4L2_PIX_FMT_SBGGR10P</constant></refname>
> +	<refpurpose>10-bit packed Bayer formats</refpurpose>
> +      </refnamediv>
> +      <refsect1>
> +	<title>Description</title>
> +
> +	<para>The following four pixel formats are packed raw sRGB /
> +	Bayer formats with 10 bits per colour. Every four consequtive

Typo: consecutive

> +	colour components are packed into 5 bytes such that each of
> +	the first 4 bytes contain their 8 high bits, and the fifth
> +	byte contains 4 groups of 2 their low bits. Bytes are stored
> +	in memory in little endian order.</para>
> +
> +	<para>Each n-pixel row contains n/2 green samples and n/2 blue
> +	or red samples, with alternating green-red and green-blue
> +	rows. They are conventionally described as GRGR... BGBG...,
> +	RGRG... GBGB..., etc. Below is an example of one of these
> +	formats</para>

s/formats/formats:/

> +
> +    <example>
> +      <title><constant>V4L2_PIX_FMT_SBGGR10P</constant> 4 &times; 4
> +      pixel image</title>
> +
> +      <formalpara>
> +	<title>Byte Order.</title>
> +	<para>Each cell is one byte.
> +	  <informaltable frame="none">
> +	    <tgroup cols="5" align="center">
> +	      <colspec align="left" colwidth="2*" />
> +	      <tbody valign="top">
> +		<row>
> +		  <entry>start&nbsp;+&nbsp;0:</entry>
> +		  <entry>B<subscript>00high</subscript></entry>
> +		  <entry>G<subscript>01high</subscript></entry>
> +		  <entry>B<subscript>02high</subscript></entry>
> +		  <entry>G<subscript>03high</subscript></entry>
> +		  <entry>B+G<subscript>0-3low</subscript></entry>
> +		</row>
> +		<row>
> +		  <entry>start&nbsp;+&nbsp;5:</entry>
> +		  <entry>G<subscript>04high</subscript></entry>
> +		  <entry>R<subscript>05high</subscript></entry>
> +		  <entry>G<subscript>06high</subscript></entry>
> +		  <entry>R<subscript>07high</subscript></entry>
> +		  <entry>G+R<subscript>4-7low</subscript></entry>
> +		</row>
> +		<row>
> +		  <entry>start&nbsp;+&nbsp;10:</entry>
> +		  <entry>B<subscript>08high</subscript></entry>
> +		  <entry>G<subscript>09high</subscript></entry>
> +		  <entry>B<subscript>10high</subscript></entry>
> +		  <entry>G<subscript>11high</subscript></entry>
> +		  <entry>B+G<subscript>8-11low</subscript></entry>
> +		</row>
> +		<row>
> +          <entry>start&nbsp;+&nbsp;15:</entry>
> +		  <entry>G<subscript>12high</subscript></entry>
> +		  <entry>R<subscript>13high</subscript></entry>
> +		  <entry>G<subscript>14high</subscript></entry>
> +		  <entry>R<subscript>15high</subscript></entry>
> +		  <entry>G+R<subscript>12-15low</subscript></entry>
> +		</row>
> +	      </tbody>
> +	    </tgroup>
> +	  </informaltable>
> +	</para>
> +      </formalpara>
> +    </example>
> +  </refsect1>
> +</refentry>
> diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml b/Documentation/DocBook/media/v4l/pixfmt.xml
> index df5b23d..5a83d9c 100644
> --- a/Documentation/DocBook/media/v4l/pixfmt.xml
> +++ b/Documentation/DocBook/media/v4l/pixfmt.xml
> @@ -716,6 +716,7 @@ access the palette, this must be done with ioctls of the Linux framebuffer API.<
>      &sub-srggb10alaw8;
>      &sub-srggb10dpcm8;
>      &sub-srggb12;
> +    &sub-srggb10p;
>    </section>
>  
>    <section id="yuv-formats">
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index e9806c6..faba23a 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -402,6 +402,11 @@ struct v4l2_pix_format {
>  #define V4L2_PIX_FMT_SGBRG10DPCM8 v4l2_fourcc('b', 'G', 'A', '8')
>  #define V4L2_PIX_FMT_SGRBG10DPCM8 v4l2_fourcc('B', 'D', '1', '0')
>  #define V4L2_PIX_FMT_SRGGB10DPCM8 v4l2_fourcc('b', 'R', 'A', '8')
> +	/* 10bit raw bayer packed, 5 bytes for every 4 pixels */
> +#define V4L2_PIX_FMT_SBGGR10P v4l2_fourcc('p', 'B', 'A', 'A')
> +#define V4L2_PIX_FMT_SGBRG10P v4l2_fourcc('p', 'G', 'A', 'A')
> +#define V4L2_PIX_FMT_SGRBG10P v4l2_fourcc('p', 'g', 'A', 'A')
> +#define V4L2_PIX_FMT_SRGGB10P v4l2_fourcc('p', 'R', 'A', 'A')
>  	/*
>  	 * 10bit raw bayer, expanded to 16 bits
>  	 * xxxxrrrrrrrrrrxxxxgggggggggg xxxxggggggggggxxxxbbbbbbbbbb...
> 

After fixing those two typos you can add my:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

Re: [REVIEW PATCH 2/2] v4l: Add packed Bayer raw10 pixel formats

[Sakari Ailus]

Hi Hans,

Thank you for the review.

On Fri, Dec 05, 2014 at 04:12:31PM +0100, Hans Verkuil wrote:
> On 12/03/2014 12:14 PM, Sakari Ailus wrote:
> > From: Aviv Greenberg <aviv.d.greenberg@intel.com>
> > 
> > These formats are just like 10-bit raw bayer formats that exist already, but
> > the pixels are not padded to byte boundaries. Instead, the eight high order
> > bits of four consecutive pixels are stored in four bytes, followed by a byte
> > of two low order bits of each of the four pixels.
> > 
> > Signed-off-by: Aviv Greenberg <aviv.d.greenberg@intel.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  .../DocBook/media/v4l/pixfmt-srggb10p.xml          | 83 ++++++++++++++++++++++
> >  Documentation/DocBook/media/v4l/pixfmt.xml         |  1 +
> >  include/uapi/linux/videodev2.h                     |  5 ++
> >  3 files changed, 89 insertions(+)
> >  create mode 100644 Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml
> > 
> > diff --git a/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml b/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml
> > new file mode 100644
> > index 0000000..3e88d8d
> > --- /dev/null
> > +++ b/Documentation/DocBook/media/v4l/pixfmt-srggb10p.xml
> > @@ -0,0 +1,83 @@
> > +    <refentry id="pixfmt-srggb10p">
> > +      <refmeta>
> > +	<refentrytitle>V4L2_PIX_FMT_SRGGB10P ('pRAA'),
> > +	 V4L2_PIX_FMT_SGRBG10P ('pgAA'),
> > +	 V4L2_PIX_FMT_SGBRG10P ('pGAA'),
> > +	 V4L2_PIX_FMT_SBGGR10P ('pBAA'),
> > +	 </refentrytitle>
> > +	&manvol;
> > +      </refmeta>
> > +      <refnamediv>
> > +	<refname id="V4L2-PIX-FMT-SRGGB10P"><constant>V4L2_PIX_FMT_SRGGB10P</constant></refname>
> > +	<refname id="V4L2-PIX-FMT-SGRBG10P"><constant>V4L2_PIX_FMT_SGRBG10P</constant></refname>
> > +	<refname id="V4L2-PIX-FMT-SGBRG10P"><constant>V4L2_PIX_FMT_SGBRG10P</constant></refname>
> > +	<refname id="V4L2-PIX-FMT-SBGGR10P"><constant>V4L2_PIX_FMT_SBGGR10P</constant></refname>
> > +	<refpurpose>10-bit packed Bayer formats</refpurpose>
> > +      </refnamediv>
> > +      <refsect1>
> > +	<title>Description</title>
> > +
> > +	<para>The following four pixel formats are packed raw sRGB /
> > +	Bayer formats with 10 bits per colour. Every four consequtive
> 
> Typo: consecutive
> 
> > +	colour components are packed into 5 bytes such that each of
> > +	the first 4 bytes contain their 8 high bits, and the fifth
> > +	byte contains 4 groups of 2 their low bits. Bytes are stored
> > +	in memory in little endian order.</para>
> > +
> > +	<para>Each n-pixel row contains n/2 green samples and n/2 blue
> > +	or red samples, with alternating green-red and green-blue
> > +	rows. They are conventionally described as GRGR... BGBG...,
> > +	RGRG... GBGB..., etc. Below is an example of one of these
> > +	formats</para>
> 
> s/formats/formats:/

Will fix the two.

> > +
> > +    <example>
> > +      <title><constant>V4L2_PIX_FMT_SBGGR10P</constant> 4 &times; 4
> > +      pixel image</title>
> > +
> > +      <formalpara>
> > +	<title>Byte Order.</title>
> > +	<para>Each cell is one byte.
> > +	  <informaltable frame="none">
> > +	    <tgroup cols="5" align="center">
> > +	      <colspec align="left" colwidth="2*" />
> > +	      <tbody valign="top">
> > +		<row>
> > +		  <entry>start&nbsp;+&nbsp;0:</entry>
> > +		  <entry>B<subscript>00high</subscript></entry>
> > +		  <entry>G<subscript>01high</subscript></entry>
> > +		  <entry>B<subscript>02high</subscript></entry>
> > +		  <entry>G<subscript>03high</subscript></entry>
> > +		  <entry>B+G<subscript>0-3low</subscript></entry>
> > +		</row>
> > +		<row>
> > +		  <entry>start&nbsp;+&nbsp;5:</entry>
> > +		  <entry>G<subscript>04high</subscript></entry>
> > +		  <entry>R<subscript>05high</subscript></entry>
> > +		  <entry>G<subscript>06high</subscript></entry>
> > +		  <entry>R<subscript>07high</subscript></entry>
> > +		  <entry>G+R<subscript>4-7low</subscript></entry>
> > +		</row>
> > +		<row>
> > +		  <entry>start&nbsp;+&nbsp;10:</entry>
> > +		  <entry>B<subscript>08high</subscript></entry>
> > +		  <entry>G<subscript>09high</subscript></entry>
> > +		  <entry>B<subscript>10high</subscript></entry>
> > +		  <entry>G<subscript>11high</subscript></entry>
> > +		  <entry>B+G<subscript>8-11low</subscript></entry>
> > +		</row>
> > +		<row>
> > +          <entry>start&nbsp;+&nbsp;15:</entry>
> > +		  <entry>G<subscript>12high</subscript></entry>
> > +		  <entry>R<subscript>13high</subscript></entry>
> > +		  <entry>G<subscript>14high</subscript></entry>
> > +		  <entry>R<subscript>15high</subscript></entry>
> > +		  <entry>G+R<subscript>12-15low</subscript></entry>
> > +		</row>
> > +	      </tbody>
> > +	    </tgroup>
> > +	  </informaltable>
> > +	</para>
> > +      </formalpara>
> > +    </example>
> > +  </refsect1>
> > +</refentry>
> > diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml b/Documentation/DocBook/media/v4l/pixfmt.xml
> > index df5b23d..5a83d9c 100644
> > --- a/Documentation/DocBook/media/v4l/pixfmt.xml
> > +++ b/Documentation/DocBook/media/v4l/pixfmt.xml
> > @@ -716,6 +716,7 @@ access the palette, this must be done with ioctls of the Linux framebuffer API.<
> >      &sub-srggb10alaw8;
> >      &sub-srggb10dpcm8;
> >      &sub-srggb12;
> > +    &sub-srggb10p;

I'll bump this up as well.

> >    </section>
> >  
> >    <section id="yuv-formats">
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index e9806c6..faba23a 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -402,6 +402,11 @@ struct v4l2_pix_format {
> >  #define V4L2_PIX_FMT_SGBRG10DPCM8 v4l2_fourcc('b', 'G', 'A', '8')
> >  #define V4L2_PIX_FMT_SGRBG10DPCM8 v4l2_fourcc('B', 'D', '1', '0')
> >  #define V4L2_PIX_FMT_SRGGB10DPCM8 v4l2_fourcc('b', 'R', 'A', '8')
> > +	/* 10bit raw bayer packed, 5 bytes for every 4 pixels */
> > +#define V4L2_PIX_FMT_SBGGR10P v4l2_fourcc('p', 'B', 'A', 'A')
> > +#define V4L2_PIX_FMT_SGBRG10P v4l2_fourcc('p', 'G', 'A', 'A')
> > +#define V4L2_PIX_FMT_SGRBG10P v4l2_fourcc('p', 'g', 'A', 'A')
> > +#define V4L2_PIX_FMT_SRGGB10P v4l2_fourcc('p', 'R', 'A', 'A')
> >  	/*
> >  	 * 10bit raw bayer, expanded to 16 bits
> >  	 * xxxxrrrrrrrrrrxxxxgggggggggg xxxxggggggggggxxxxbbbbbbbbbb...
> > 
> 
> After fixing those two typos you can add my:
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

I think I may separate this from the data_offset addition since there seems
to be more work and time needed on that side.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk