[PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Laurent Pinchart]

Hello,

The v4l2_plane data_offset field has been introduced at the same time as the
the multiplane API to convey header size information between kernelspace and
userspace.

The API then became slightly controversial, both because different developers
understood the purpose of the field differently (resulting for instance in an
out-of-tree driver abusing the field for a different purpose), and because of
competing proposals (see for instance "[RFC] Multi format stream support" at
http://www.spinics.net/lists/linux-media/msg69130.html).

Furthermore, the data_offset field isn't used by any mainline driver except
vivid (for testing purpose).

I need a different data offset in planes to allow data capture to or data
output from a userspace-selected offset within a buffer (mainly for the
DMABUF and MMAP memory types). As the data_offset field already has the
right name, is unused, and ill-defined, I propose repurposing it. This is what
this RFC is about.

If the proposal is accepted I'll add another patch to update data_offset usage
in the vivid driver.

Laurent Pinchart (2):
  v4l: Repurpose the v4l2_plane data_offset field
  videobuf2: Repurpose the v4l2_plane data_offset field

 Documentation/DocBook/media/v4l/io.xml   | 19 +++++++------
 drivers/media/v4l2-core/videobuf2-core.c | 46 +++++++++++++++++++++++---------
 include/media/videobuf2-core.h           |  4 +++
 include/media/videobuf2-dma-contig.h     |  2 +-
 include/uapi/linux/videodev2.h           |  6 +++--
 5 files changed, 54 insertions(+), 23 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH/RFC 1/2] v4l: Repurpose the v4l2_plane data_offset field

[Laurent Pinchart]

The data_offset field has been introduced along with the multiplane API
to convey header size information between kernelspace and userspace.
It's not used by any mainline driver except vivid (for testing purpose).

A different data offset is needed to allow data capture to or data
output from a userspace-selected offset within a buffer (mainly for the
DMABUF and MMAP memory types). As the data_offset field already has the
right name and is unused, repurpose it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/DocBook/media/v4l/io.xml | 19 +++++++++++--------
 include/uapi/linux/videodev2.h         |  6 ++++--
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 1c17f80..416c05a 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -870,7 +870,7 @@ should set this to 0.</entry>
 	      If the application sets this to 0 for an output stream, then
 	      <structfield>bytesused</structfield> will be set to the size of the
 	      plane (see the <structfield>length</structfield> field of this struct)
-	      by the driver. Note that the actual image data starts at
+	      by the driver. Note that the actual plane data content starts at
 	      <structfield>data_offset</structfield> which may not be 0.</entry>
 	  </row>
 	  <row>
@@ -917,13 +917,16 @@ should set this to 0.</entry>
 	    <entry>__u32</entry>
 	    <entry><structfield>data_offset</structfield></entry>
 	    <entry></entry>
-	    <entry>Offset in bytes to video data in the plane.
-	      Drivers must set this field when <structfield>type</structfield>
-	      refers to an input stream, applications when it refers to an output stream.
-	      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>Offset in bytes from the start of the plane buffer to the start of
+	      captured or output data. Applications set this field for all stream types
+	      when calling the <link linkend="vidioc-qbuf">VIDIOC_PREPARE_BUF</link> or
+	      <link linkend="vidioc-qbuf">VIDIOC_QBUF</link> ioctls to instruct the driver
+	      to capture or output data starting at an offset in the plane buffer. If the
+	      requested data offset doesn't match device or driver constraints, device
+	      drivers must return the &EINVAL; and either leave the field value untouched
+	      if they support data offsets, or set it to 0 if they don't support data
+	      offsets at all. Note that <structfield>data_offset</structfield> is not
+	      included in <structfield>bytesused</structfield>.
 	    </entry>
 	  </row>
 	  <row>
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index fa376f7..261fb66 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -706,8 +706,10 @@ struct v4l2_requestbuffers {
  *			pointing to this plane
  * @fd:			when memory is V4L2_MEMORY_DMABUF, a userspace file
  *			descriptor associated with this plane
- * @data_offset:	offset in the plane to the start of data; usually 0,
- *			unless there is a header in front of the data
+ * @data_offset:	offset in bytes from the start of the plane buffer to
+ *			the start of data; usually 0 unless applications need to
+ *			capture data to or output data from elsewhere than the
+ *			start of the buffer
  *
  * Multi-planar buffers consist of one or more planes, e.g. an YCbCr buffer
  * with two planes can have one plane for Y, and another for interleaved CbCr
-- 
2.0.5

Re: [PATCH/RFC 1/2] v4l: Repurpose the v4l2_plane data_offset field

[Sakari Ailus]

Hi Laurent,

Thank you for the patchset.

On Tue, Apr 14, 2015 at 10:44:48PM +0300, Laurent Pinchart wrote:
> The data_offset field has been introduced along with the multiplane API
> to convey header size information between kernelspace and userspace.
> It's not used by any mainline driver except vivid (for testing purpose).
> 
> A different data offset is needed to allow data capture to or data
> output from a userspace-selected offset within a buffer (mainly for the
> DMABUF and MMAP memory types). As the data_offset field already has the
> right name and is unused, repurpose it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/DocBook/media/v4l/io.xml | 19 +++++++++++--------
>  include/uapi/linux/videodev2.h         |  6 ++++--
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 1c17f80..416c05a 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -870,7 +870,7 @@ should set this to 0.</entry>
>  	      If the application sets this to 0 for an output stream, then
>  	      <structfield>bytesused</structfield> will be set to the size of the
>  	      plane (see the <structfield>length</structfield> field of this struct)
> -	      by the driver. Note that the actual image data starts at
> +	      by the driver. Note that the actual plane data content starts at
>  	      <structfield>data_offset</structfield> which may not be 0.</entry>
>  	  </row>
>  	  <row>
> @@ -917,13 +917,16 @@ should set this to 0.</entry>
>  	    <entry>__u32</entry>
>  	    <entry><structfield>data_offset</structfield></entry>
>  	    <entry></entry>
> -	    <entry>Offset in bytes to video data in the plane.
> -	      Drivers must set this field when <structfield>type</structfield>
> -	      refers to an input stream, applications when it refers to an output stream.
> -	      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>Offset in bytes from the start of the plane buffer to the start of
> +	      captured or output data. Applications set this field for all stream types
> +	      when calling the <link linkend="vidioc-qbuf">VIDIOC_PREPARE_BUF</link> or
> +	      <link linkend="vidioc-qbuf">VIDIOC_QBUF</link> ioctls to instruct the driver
> +	      to capture or output data starting at an offset in the plane buffer. If the
> +	      requested data offset doesn't match device or driver constraints, device
> +	      drivers must return the &EINVAL; and either leave the field value untouched
> +	      if they support data offsets, or set it to 0 if they don't support data
> +	      offsets at all. Note that <structfield>data_offset</structfield> is not
> +	      included in <structfield>bytesused</structfield>.

At most 80 characters per line would be nice.

How does the user discover what data_offsets are possible if the driver
returns an error if the data_offset does not match hardware capabilities?

I'd rather have the driver to adjust data_offset to match what it can do. If
the user needs to know that the data_offset was not modified, it should
check the field value after QBUF/PREPARE_BUF.

>  	    </entry>
>  	  </row>
>  	  <row>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index fa376f7..261fb66 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -706,8 +706,10 @@ struct v4l2_requestbuffers {
>   *			pointing to this plane
>   * @fd:			when memory is V4L2_MEMORY_DMABUF, a userspace file
>   *			descriptor associated with this plane
> - * @data_offset:	offset in the plane to the start of data; usually 0,
> - *			unless there is a header in front of the data
> + * @data_offset:	offset in bytes from the start of the plane buffer to
> + *			the start of data; usually 0 unless applications need to
> + *			capture data to or output data from elsewhere than the
> + *			start of the buffer
>   *
>   * Multi-planar buffers consist of one or more planes, e.g. an YCbCr buffer
>   * with two planes can have one plane for Y, and another for interleaved CbCr

-- 
Kind regards,

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

Re: [PATCH/RFC 1/2] v4l: Repurpose the v4l2_plane data_offset field

[Laurent Pinchart]

Hi Sakari,

Thank you for the review.

On Tuesday 14 April 2015 23:10:05 Sakari Ailus wrote:
> On Tue, Apr 14, 2015 at 10:44:48PM +0300, Laurent Pinchart wrote:
> > The data_offset field has been introduced along with the multiplane API
> > to convey header size information between kernelspace and userspace.
> > It's not used by any mainline driver except vivid (for testing purpose).
> > 
> > A different data offset is needed to allow data capture to or data
> > output from a userspace-selected offset within a buffer (mainly for the
> > DMABUF and MMAP memory types). As the data_offset field already has the
> > right name and is unused, repurpose it.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > ---
> > 
> >  Documentation/DocBook/media/v4l/io.xml | 19 +++++++++++--------
> >  include/uapi/linux/videodev2.h         |  6 ++++--
> >  2 files changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/io.xml
> > b/Documentation/DocBook/media/v4l/io.xml index 1c17f80..416c05a 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -870,7 +870,7 @@ should set this to 0.</entry>
> > 
> >  	      If the application sets this to 0 for an output stream, then
> >  	      <structfield>bytesused</structfield> will be set to the size of
> >  	      the
> >  	      plane (see the <structfield>length</structfield> field of this
> >  	      struct)
> > -	      by the driver. Note that the actual image data starts at
> > +	      by the driver. Note that the actual plane data content starts 
at
> >  	      <structfield>data_offset</structfield> which may not be 
0.</entry>
> >  	  </row>
> >  	  <row>
> > @@ -917,13 +917,16 @@ should set this to 0.</entry>
> >  	    <entry>__u32</entry>
> >  	    <entry><structfield>data_offset</structfield></entry>
> >  	    <entry></entry>
> > -	    <entry>Offset in bytes to video data in the plane.
> > -	      Drivers must set this field when 
<structfield>type</structfield>
> > -	      refers to an input stream, applications when it refers to an
> > output stream. -	      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</structfiel
> > d> at -	      offset <structfield>data_offset</structfield> from the 
start
> > of the plane. +	    <entry>Offset in bytes from the start of the plane
> > buffer to the start of +	      captured or output data. Applications 
set
> > this field for all stream types +	      when calling the <link
> > linkend="vidioc-qbuf">VIDIOC_PREPARE_BUF</link> or +	      <link
> > linkend="vidioc-qbuf">VIDIOC_QBUF</link> ioctls to instruct the driver +	
> >      to capture or output data starting at an offset in the plane buffer.
> > If the +	      requested data offset doesn't match device or driver
> > constraints, device +	      drivers must return the &EINVAL; and either
> > leave the field value untouched +	      if they support data offsets, 
or
> > set it to 0 if they don't support data +	      offsets at all. Note that
> > <structfield>data_offset</structfield> is not +	      included in
> > <structfield>bytesused</structfield>.
> 
> At most 80 characters per line would be nice.

I've followed the coding style of the file, but I can certainly change that, 
no issue.

> How does the user discover what data_offsets are possible if the driver
> returns an error if the data_offset does not match hardware capabilities?
> 
> I'd rather have the driver to adjust data_offset to match what it can do. If
> the user needs to know that the data_offset was not modified, it should
> check the field value after QBUF/PREPARE_BUF.

I've discussed this with Hans before, and we thought negotiating data_offset 
wasn't very useful. data_offset values used by applications are pretty much 
mandatory, if you need to write the UV plane of an NV12 image at a given 
offset in a Y+UV contiguous buffer, using a different negotiated offset is 
pointless. Feel free to point us to use cases though.

> >  	    </entry>
> >  	  </row>
> >  	  <row>
> > diff --git a/include/uapi/linux/videodev2.h
> > b/include/uapi/linux/videodev2.h index fa376f7..261fb66 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -706,8 +706,10 @@ struct v4l2_requestbuffers {
> >   *			pointing to this plane
> >   * @fd:			when memory is V4L2_MEMORY_DMABUF, a userspace file
> >   *			descriptor associated with this plane
> > - * @data_offset:	offset in the plane to the start of data; usually 0,
> > - *			unless there is a header in front of the data
> > + * @data_offset:	offset in bytes from the start of the plane buffer to
> > + *			the start of data; usually 0 unless applications need to
> > + *			capture data to or output data from elsewhere than the
> > + *			start of the buffer
> >   *
> >   * Multi-planar buffers consist of one or more planes, e.g. an YCbCr
> >   buffer
> >   * with two planes can have one plane for Y, and another for interleaved
> >   CbCr

-- 
Regards,

Laurent Pinchart

[PATCH/RFC 2/2] videobuf2: Repurpose the v4l2_plane data_offset field

[Laurent Pinchart]

The v4l2_plane data_offset field has been repurposed in the V4L2 API.
Update videobuf2 accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/media/v4l2-core/videobuf2-core.c | 46 +++++++++++++++++++++++---------
 include/media/videobuf2-core.h           |  4 +++
 include/media/videobuf2-dma-contig.h     |  2 +-
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 1329dcc..43f8fc5 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -572,15 +572,29 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer
 }
 
 /**
- * __verify_length() - Verify that the bytesused value for each plane fits in
- * the plane length and that the data offset doesn't exceed the bytesused value.
+ * __verify_length() - Verify for each plane that the data_offset matches driver
+ * constraints and that the bytesused plus data_offset value fits in the plane
+ * length.
  */
-static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
+static int __verify_length(struct vb2_buffer *vb, struct v4l2_buffer *b)
 {
 	unsigned int length;
 	unsigned int bytesused;
+	unsigned int size;
 	unsigned int plane;
 
+	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
+		unsigned int mask = vb->vb2_queue->data_offset_mask;
+
+		for (plane = 0; plane < vb->num_planes; ++plane) {
+			if (b->m.planes[plane].data_offset & ~mask) {
+				if (!mask)
+					b->m.planes[plane].data_offset = 0;
+				return -EINVAL;
+			}
+		}
+	}
+
 	if (!V4L2_TYPE_IS_OUTPUT(b->type))
 		return 0;
 
@@ -590,14 +604,16 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 				  b->memory == V4L2_MEMORY_DMABUF)
 			       ? b->m.planes[plane].length
 			       : vb->v4l2_planes[plane].length;
-			bytesused = b->m.planes[plane].bytesused
-				  ? b->m.planes[plane].bytesused : length;
 
-			if (b->m.planes[plane].bytesused > length)
+			if (b->m.planes[plane].data_offset >= length)
 				return -EINVAL;
 
-			if (b->m.planes[plane].data_offset > 0 &&
-			    b->m.planes[plane].data_offset >= bytesused)
+			size = b->m.planes[plane].bytesused
+			     + b->m.planes[plane].data_offset;
+
+			/* Protect against integer overflows. */
+			if (size < b->m.planes[plane].bytesused ||
+			    size > length)
 				return -EINVAL;
 		}
 	} else {
@@ -1122,11 +1138,13 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
  */
 void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
 {
+	void *addr;
+
 	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
 		return NULL;
 
-	return call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
-
+	addr = call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
+	return addr + vb->v4l2_planes[plane_no].data_offset;
 }
 EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
 
@@ -1258,6 +1276,11 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 	}
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
+		for (plane = 0; plane < vb->num_planes; ++plane) {
+			v4l2_planes[plane].data_offset =
+				b->m.planes[plane].data_offset;
+		}
+
 		if (b->memory == V4L2_MEMORY_USERPTR) {
 			for (plane = 0; plane < vb->num_planes; ++plane) {
 				v4l2_planes[plane].m.userptr =
@@ -1302,7 +1325,6 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 				else
 					pdst->bytesused = psrc->bytesused ?
 						psrc->bytesused : pdst->length;
-				pdst->data_offset = psrc->data_offset;
 			}
 		}
 	} else {
@@ -1618,7 +1640,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 	call_void_vb_qop(vb, buf_queue, vb);
 }
 
-static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
+static int __buf_prepare(struct vb2_buffer *vb, struct v4l2_buffer *b)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 	int ret;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index a5790fd..c51c481 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -362,6 +362,9 @@ struct v4l2_fh;
  *		start_streaming() can be called. Used when a DMA engine
  *		cannot be started unless at least this number of buffers
  *		have been queued into the driver.
+ * @data_offset_mask: the data_offset alignment constraints expressed as a
+ *		mask of bits that can be set in the data_offset value; "0"
+ *		indicates the driver doesn't support data_offset
  *
  * @mmap_lock:	private mutex used when buffers are allocated/freed/mmapped
  * @memory:	current memory type used
@@ -401,6 +404,7 @@ struct vb2_queue {
 	u32				timestamp_flags;
 	gfp_t				gfp_flags;
 	u32				min_buffers_needed;
+	u32				data_offset_mask;
 
 /* private: internal use only */
 	struct mutex			mmap_lock;
diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h
index 8197f87..ff60fac 100644
--- a/include/media/videobuf2-dma-contig.h
+++ b/include/media/videobuf2-dma-contig.h
@@ -21,7 +21,7 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no)
 {
 	dma_addr_t *addr = vb2_plane_cookie(vb, plane_no);
 
-	return *addr;
+	return *addr + vb->v4l2_planes[plane_no].data_offset;
 }
 
 void *vb2_dma_contig_init_ctx(struct device *dev);
-- 
2.0.5

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Hans Verkuil]

Hi Laurent,

On 04/14/2015 09:44 PM, Laurent Pinchart wrote:
> Hello,
> 
> The v4l2_plane data_offset field has been introduced at the same time as the
> the multiplane API to convey header size information between kernelspace and
> userspace.
> 
> The API then became slightly controversial, both because different developers
> understood the purpose of the field differently (resulting for instance in an
> out-of-tree driver abusing the field for a different purpose), and because of
> competing proposals (see for instance "[RFC] Multi format stream support" at
> http://www.spinics.net/lists/linux-media/msg69130.html).
> 
> Furthermore, the data_offset field isn't used by any mainline driver except
> vivid (for testing purpose).
> 
> I need a different data offset in planes to allow data capture to or data
> output from a userspace-selected offset within a buffer (mainly for the
> DMABUF and MMAP memory types). As the data_offset field already has the
> right name, is unused, and ill-defined, I propose repurposing it. This is what
> this RFC is about.
> 
> If the proposal is accepted I'll add another patch to update data_offset usage
> in the vivid driver.

I am skeptical about all this for a variety of reasons:

1) The data_offset field is well-defined in the spec. There really is no doubt
about the meaning of the field.

2) We really don't know who else might be using it, or which applications might
be using it (a lot of work was done in gstreamer recently, I wonder if data_offset
support was implemented there).

3) You offer no alternative to this feature. Basically this is my main objection.
It is not at all unusual to have headers in front of the frame data. We (Cisco)
use it in one of our product series for example. And I suspect it is something that
happens especially in systems with an FPGA that does custom processing, and those
systems are exactly the ones that are generally not upstreamed and so are not
visible to us.

IMHO the functionality it provides is very much relevant, and I would like to see
an alternative in place before it is repurposed.

But frankly, I really don't see why you would want to repurpose it. Adding a new
field (buf_offset) would do exactly what you want it to do without causing an ABI
change.

Should we ever implement a better alternative for data_offset, then that field can
be renamed to 'reserved2' or whatever at some point.

Frankly, I don't think data_offset is all that bad. What is missing is info about
the format (so add a 'data_format' field) and possible similar info about a footer
(footer_size, footer_format). Yes, the name could have been better (header_size),
but nobody is perfect...

Regards,

	Hans

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Laurent Pinchart]

Hi Hans,

On Friday 17 April 2015 12:27:41 Hans Verkuil wrote:
> On 04/14/2015 09:44 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > The v4l2_plane data_offset field has been introduced at the same time as
> > the the multiplane API to convey header size information between
> > kernelspace and userspace.
> > 
> > The API then became slightly controversial, both because different
> > developers understood the purpose of the field differently (resulting for
> > instance in an out-of-tree driver abusing the field for a different
> > purpose), and because of competing proposals (see for instance "[RFC]
> > Multi format stream support" at
> > http://www.spinics.net/lists/linux-media/msg69130.html).
> > 
> > Furthermore, the data_offset field isn't used by any mainline driver
> > except vivid (for testing purpose).
> > 
> > I need a different data offset in planes to allow data capture to or data
> > output from a userspace-selected offset within a buffer (mainly for the
> > DMABUF and MMAP memory types). As the data_offset field already has the
> > right name, is unused, and ill-defined, I propose repurposing it. This is
> > what this RFC is about.
> > 
> > If the proposal is accepted I'll add another patch to update data_offset
> > usage in the vivid driver.
> 
> I am skeptical about all this for a variety of reasons:

That's all good, it's an RFC :-)

> 1) The data_offset field is well-defined in the spec. There really is no
> doubt about the meaning of the field.

I only partly agree. I believe the purpose of the data_offset field to be 
clear among the core V4L2 developers, but the documentation isn't precise 
enough. I've seen out-of-tree drivers using the data_offset field for other 
purposes than specifying the header size. The situation is a bit better now 
that videobuf2 handles the field properly (and avoids copying it from user to 
kernel for capture devices for instance), but there are still many users of 
older kernels.

This being said, the problem wouldn't be difficult to fix, it just requires a 
documentation patch.

> 2) We really don't know who else might be using it, or which applications
> might be using it (a lot of work was done in gstreamer recently, I wonder
> if data_offset support was implemented there).

It's funny you mention that. I cloned the gstreamer repositories and tried to 
investigate. The gstreamer v4l2 elements started using data_offset a year ago 
in

commit 92bdd596f2b07dbf4ccc9b8bf3d17620d44f131a
Author: Nicolas Dufresne <nicolas.dufresne-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
Date:   Fri Apr 11 17:10:11 2014 -0400

    v4l2: Add DMABUF and USERPTR importation

(I've CC'ed Nicolas to this e-mail)

I'm not too familiar with the latest gstreamer code, but after a first 
investigation it seems that gstreamer uses the data_offset field for the 
purpose introduced by this patch, not to convey the header size. One more 
argument in favour of repurposing the field ;-)

> 3) You offer no alternative to this feature. Basically this is my main
> objection. It is not at all unusual to have headers in front of the frame
> data. We (Cisco) use it in one of our product series for example. And I
> suspect it is something that happens especially in systems with an FPGA
> that does custom processing, and those systems are exactly the ones that
> are generally not upstreamed and so are not visible to us.
> 
> IMHO the functionality it provides is very much relevant, and I would like
> to see an alternative in place before it is repurposed.
>
> But frankly, I really don't see why you would want to repurpose it. Adding a
> new field (buf_offset) would do exactly what you want it to do without
> causing an ABI change.
> 
> Should we ever implement a better alternative for data_offset, then that
> field can be renamed to 'reserved2' or whatever at some point.
> 
> Frankly, I don't think data_offset is all that bad. What is missing is info
> about the format (so add a 'data_format' field) and possible similar info
> about a footer (footer_size, footer_format). Yes, the name could have been
> better (header_size), but nobody is perfect...

I totally agree that the functionality is relevant, and we certainly need an 
API for that.

My point, however, was twofold : I believe we need a better (as in more 
powerful) API than data_offset to specify plane content, and the current usage 
of data_offset in out-of-tree drivers, and it seems in gstreamer too, is 
different than what we had intended the field to be used for.

For those two reasons, I believe it would make sense to repurpose the field 
and introduce a new API to specify information about the plane content. Let's 
kickstart the discussion :-)

The following information comes to my mind as being useful to specify:

- format
- header size
- footer size

There is, however, another point I'd like to raise. I'm working on an H.264 
encoder that produces slices without headers. Userspace is thus responsible 
for filling the headers, based on information produced by the encoder.

When a capture buffer at the output of the encoder contains a single slice, 
that's pretty easy to handle. Userspace can use data_offset (in its new 
purpose, or buf_offset if we decide to introduce a new field instead) to 
reserve space for a header if the header size is known in advance by the 
application, or the driver (or possibly the device) can reserve space for the 
header and report the header size.

However, a capture buffer can contain multiple slices, with gaps between the 
slices for the headers. The position and size of the gaps need to be known by 
the application. I'm not sure yet if userspace can compute them, or if they're 
dynamic and need to be passed from the driver to the application on a per-
frame basis. In the latter case I would need more than a header size and 
footer size per plane.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Hans Verkuil]

On 04/17/2015 02:53 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 17 April 2015 12:27:41 Hans Verkuil wrote:
>> On 04/14/2015 09:44 PM, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> The v4l2_plane data_offset field has been introduced at the same time as
>>> the the multiplane API to convey header size information between
>>> kernelspace and userspace.
>>>
>>> The API then became slightly controversial, both because different
>>> developers understood the purpose of the field differently (resulting for
>>> instance in an out-of-tree driver abusing the field for a different
>>> purpose), and because of competing proposals (see for instance "[RFC]
>>> Multi format stream support" at
>>> http://www.spinics.net/lists/linux-media/msg69130.html).
>>>
>>> Furthermore, the data_offset field isn't used by any mainline driver
>>> except vivid (for testing purpose).
>>>
>>> I need a different data offset in planes to allow data capture to or data
>>> output from a userspace-selected offset within a buffer (mainly for the
>>> DMABUF and MMAP memory types). As the data_offset field already has the
>>> right name, is unused, and ill-defined, I propose repurposing it. This is
>>> what this RFC is about.
>>>
>>> If the proposal is accepted I'll add another patch to update data_offset
>>> usage in the vivid driver.
>>
>> I am skeptical about all this for a variety of reasons:
> 
> That's all good, it's an RFC :-)
> 
>> 1) The data_offset field is well-defined in the spec. There really is no
>> doubt about the meaning of the field.
> 
> I only partly agree. I believe the purpose of the data_offset field to be 
> clear among the core V4L2 developers, but the documentation isn't precise 
> enough. I've seen out-of-tree drivers using the data_offset field for other 
> purposes than specifying the header size. The situation is a bit better now 
> that videobuf2 handles the field properly (and avoids copying it from user to 
> kernel for capture devices for instance), but there are still many users of 
> older kernels.
> 
> This being said, the problem wouldn't be difficult to fix, it just requires a 
> documentation patch.
> 
>> 2) We really don't know who else might be using it, or which applications
>> might be using it (a lot of work was done in gstreamer recently, I wonder
>> if data_offset support was implemented there).
> 
> It's funny you mention that. I cloned the gstreamer repositories and tried to 
> investigate. The gstreamer v4l2 elements started using data_offset a year ago 
> in
> 
> commit 92bdd596f2b07dbf4ccc9b8bf3d17620d44f131a
> Author: Nicolas Dufresne <nicolas.dufresne-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Date:   Fri Apr 11 17:10:11 2014 -0400
> 
>     v4l2: Add DMABUF and USERPTR importation
> 
> (I've CC'ed Nicolas to this e-mail)
> 
> I'm not too familiar with the latest gstreamer code, but after a first 
> investigation it seems that gstreamer uses the data_offset field for the 
> purpose introduced by this patch, not to convey the header size. One more 
> argument in favour of repurposing the field ;-)
> 
>> 3) You offer no alternative to this feature. Basically this is my main
>> objection. It is not at all unusual to have headers in front of the frame
>> data. We (Cisco) use it in one of our product series for example. And I
>> suspect it is something that happens especially in systems with an FPGA
>> that does custom processing, and those systems are exactly the ones that
>> are generally not upstreamed and so are not visible to us.
>>
>> IMHO the functionality it provides is very much relevant, and I would like
>> to see an alternative in place before it is repurposed.
>>
>> But frankly, I really don't see why you would want to repurpose it. Adding a
>> new field (buf_offset) would do exactly what you want it to do without
>> causing an ABI change.
>>
>> Should we ever implement a better alternative for data_offset, then that
>> field can be renamed to 'reserved2' or whatever at some point.
>>
>> Frankly, I don't think data_offset is all that bad. What is missing is info
>> about the format (so add a 'data_format' field) and possible similar info
>> about a footer (footer_size, footer_format). Yes, the name could have been
>> better (header_size), but nobody is perfect...
> 
> I totally agree that the functionality is relevant, and we certainly need an 
> API for that.
> 
> My point, however, was twofold : I believe we need a better (as in more 
> powerful) API than data_offset to specify plane content, and the current usage 
> of data_offset in out-of-tree drivers, and it seems in gstreamer too, is 
> different than what we had intended the field to be used for.
> 
> For those two reasons, I believe it would make sense to repurpose the field 
> and introduce a new API to specify information about the plane content. Let's 
> kickstart the discussion :-)
> 
> The following information comes to my mind as being useful to specify:
> 
> - format
> - header size
> - footer size
> 
> There is, however, another point I'd like to raise. I'm working on an H.264 
> encoder that produces slices without headers. Userspace is thus responsible 
> for filling the headers, based on information produced by the encoder.
> 
> When a capture buffer at the output of the encoder contains a single slice, 
> that's pretty easy to handle. Userspace can use data_offset (in its new 
> purpose, or buf_offset if we decide to introduce a new field instead) to 
> reserve space for a header if the header size is known in advance by the 
> application, or the driver (or possibly the device) can reserve space for the 
> header and report the header size.
> 
> However, a capture buffer can contain multiple slices, with gaps between the 
> slices for the headers. The position and size of the gaps need to be known by 
> the application. I'm not sure yet if userspace can compute them, or if they're 
> dynamic and need to be passed from the driver to the application on a per-
> frame basis. In the latter case I would need more than a header size and 
> footer size per plane.

I wonder if the current V4L2_PIX_FMT_H264* fourcc formats support multiple slices
in one buffer. Kamil might know. But I suspect you'll have to make a new fourcc
for that. Just for reference you might want to look at VIDIOC_G_ENC_INDEX (used
by ivtv) for a somewhat similar purpose. It's an old API, and I would probably not
recommend reusing this, but it may be interesting.

Is the size of the gaps programmable in the H.264 encoder hardware?

In any case, I believe your particular use-case has absolutely nothing to do with
headers/footers in a plane (the original topic). Your headers are intrinsic to the
format, i.e. without them applications cannot handle the stream. Filling those in
is the responsibility of the whole stack (driver + any libv4l plugin) leading to a
valid data buffer.

The headers/footers in the original use-case are just due to metadata that hardware
decides to throw in for whoever is interested (or in some cases it's just garbage)
and that are not part of the actual image data.

Regards,

	Hans

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Laurent Pinchart]

Hi Hans,

On Monday 20 April 2015 11:34:44 Hans Verkuil wrote:
> On 04/17/2015 02:53 PM, Laurent Pinchart wrote:
> > On Friday 17 April 2015 12:27:41 Hans Verkuil wrote:
> >> On 04/14/2015 09:44 PM, Laurent Pinchart wrote:
> >>> Hello,
> >>> 
> >>> The v4l2_plane data_offset field has been introduced at the same time as
> >>> the the multiplane API to convey header size information between
> >>> kernelspace and userspace.
> >>> 
> >>> The API then became slightly controversial, both because different
> >>> developers understood the purpose of the field differently (resulting
> >>> for instance in an out-of-tree driver abusing the field for a different
> >>> purpose), and because of competing proposals (see for instance "[RFC]
> >>> Multi format stream support" at
> >>> http://www.spinics.net/lists/linux-media/msg69130.html).
> >>> 
> >>> Furthermore, the data_offset field isn't used by any mainline driver
> >>> except vivid (for testing purpose).
> >>> 
> >>> I need a different data offset in planes to allow data capture to or
> >>> data output from a userspace-selected offset within a buffer (mainly for
> >>> the DMABUF and MMAP memory types). As the data_offset field already has
> >>> the right name, is unused, and ill-defined, I propose repurposing it.
> >>> This is what this RFC is about.
> >>> 
> >>> If the proposal is accepted I'll add another patch to update data_offset
> >>> usage in the vivid driver.
> >> 
> >> I am skeptical about all this for a variety of reasons:
> > That's all good, it's an RFC :-)
> > 
> >> 1) The data_offset field is well-defined in the spec. There really is no
> >> doubt about the meaning of the field.
> > 
> > I only partly agree. I believe the purpose of the data_offset field to be
> > clear among the core V4L2 developers, but the documentation isn't precise
> > enough. I've seen out-of-tree drivers using the data_offset field for
> > other purposes than specifying the header size. The situation is a bit
> > better now that videobuf2 handles the field properly (and avoids copying
> > it from user to kernel for capture devices for instance), but there are
> > still many users of older kernels.
> > 
> > This being said, the problem wouldn't be difficult to fix, it just
> > requires a documentation patch.
> > 
> >> 2) We really don't know who else might be using it, or which applications
> >> might be using it (a lot of work was done in gstreamer recently, I wonder
> >> if data_offset support was implemented there).
> > 
> > It's funny you mention that. I cloned the gstreamer repositories and tried
> > to investigate. The gstreamer v4l2 elements started using data_offset a
> > year ago in
> > 
> > commit 92bdd596f2b07dbf4ccc9b8bf3d17620d44f131a
> > Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Date:   Fri Apr 11 17:10:11 2014 -0400
> > 
> >     v4l2: Add DMABUF and USERPTR importation
> > 
> > (I've CC'ed Nicolas to this e-mail)
> > 
> > I'm not too familiar with the latest gstreamer code, but after a first
> > investigation it seems that gstreamer uses the data_offset field for the
> > purpose introduced by this patch, not to convey the header size. One more
> > argument in favour of repurposing the field ;-)
> > 
> >> 3) You offer no alternative to this feature. Basically this is my main
> >> objection. It is not at all unusual to have headers in front of the frame
> >> data. We (Cisco) use it in one of our product series for example. And I
> >> suspect it is something that happens especially in systems with an FPGA
> >> that does custom processing, and those systems are exactly the ones that
> >> are generally not upstreamed and so are not visible to us.
> >> 
> >> IMHO the functionality it provides is very much relevant, and I would
> >> like
> >> to see an alternative in place before it is repurposed.
> >> 
> >> But frankly, I really don't see why you would want to repurpose it.
> >> Adding a new field (buf_offset) would do exactly what you want it to do
> >> without causing an ABI change.
> >> 
> >> Should we ever implement a better alternative for data_offset, then that
> >> field can be renamed to 'reserved2' or whatever at some point.
> >> 
> >> Frankly, I don't think data_offset is all that bad. What is missing is
> >> info
> >> about the format (so add a 'data_format' field) and possible similar info
> >> about a footer (footer_size, footer_format). Yes, the name could have
> >> been
> >> better (header_size), but nobody is perfect...
> > 
> > I totally agree that the functionality is relevant, and we certainly need
> > an API for that.
> > 
> > My point, however, was twofold : I believe we need a better (as in more
> > powerful) API than data_offset to specify plane content, and the current
> > usage of data_offset in out-of-tree drivers, and it seems in gstreamer
> > too, is different than what we had intended the field to be used for.
> > 
> > For those two reasons, I believe it would make sense to repurpose the
> > field
> > and introduce a new API to specify information about the plane content.
> > Let's kickstart the discussion :-)
> > 
> > The following information comes to my mind as being useful to specify:
> > 
> > - format
> > - header size
> > - footer size
> > 
> > There is, however, another point I'd like to raise. I'm working on an
> > H.264
> > encoder that produces slices without headers. Userspace is thus
> > responsible
> > for filling the headers, based on information produced by the encoder.
> > 
> > When a capture buffer at the output of the encoder contains a single
> > slice,
> > that's pretty easy to handle. Userspace can use data_offset (in its new
> > purpose, or buf_offset if we decide to introduce a new field instead) to
> > reserve space for a header if the header size is known in advance by the
> > application, or the driver (or possibly the device) can reserve space for
> > the header and report the header size.
> > 
> > However, a capture buffer can contain multiple slices, with gaps between
> > the slices for the headers. The position and size of the gaps need to be
> > known by the application. I'm not sure yet if userspace can compute them,
> > or if they're dynamic and need to be passed from the driver to the
> > application on a per- frame basis. In the latter case I would need more
> > than a header size and footer size per plane.
> 
> I wonder if the current V4L2_PIX_FMT_H264* fourcc formats support multiple
> slices in one buffer. Kamil might know. But I suspect you'll have to make a
> new fourcc for that. Just for reference you might want to look at
> VIDIOC_G_ENC_INDEX (used by ivtv) for a somewhat similar purpose. It's an
> old API, and I would probably not recommend reusing this, but it may be
> interesting.
> 
> Is the size of the gaps programmable in the H.264 encoder hardware?

I don't know yet, I'm waiting for more information.

> In any case, I believe your particular use-case has absolutely nothing to do
> with headers/footers in a plane (the original topic). Your headers are
> intrinsic to the format, i.e. without them applications cannot handle the
> stream. Filling those in is the responsibility of the whole stack (driver +
> any libv4l plugin) leading to a valid data buffer.

I could agree with that. I'll wait until I get more information about the 
hardware before discussing this topic further.

This patch set remains valid though, it's unrelated to my H.264 encoder.

> The headers/footers in the original use-case are just due to metadata that
> hardware decides to throw in for whoever is interested (or in some cases
> it's just garbage) and that are not part of the actual image data.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Nicolas Dufresne]

Le vendredi 17 avril 2015 à 15:53 +0300, Laurent Pinchart a écrit :
> It's funny you mention that. I cloned the gstreamer repositories and
> tried to 
> investigate. The gstreamer v4l2 elements started using data_offset a
> year ago 
> in
> 
> commit 92bdd596f2b07dbf4ccc9b8bf3d17620d44f131a
> Author: Nicolas Dufresne <nicolas.dufresne-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Date:   Fri Apr 11 17:10:11 2014 -0400
> 
>     v4l2: Add DMABUF and USERPTR importation
> 
> (I've CC'ed Nicolas to this e-mail)
> 
> I'm not too familiar with the latest gstreamer code, but after a
> first 
> investigation it seems that gstreamer uses the data_offset field for
> the 
> purpose introduced by this patch, not to convey the header size. One
> more 
> argument in favour of repurposing the field ;-)

My impression was that the data before the offset was non-generic and
had to be skipped by applications that aren't aware. An example usage
would be to a camera with custom sensor producing data serialized with
the frames. The sensor data could be set in a header using custom but
documented format, generic application would simply skip that and work
as usual. Be aware that the implementation in GStreamer is incomplete
and untested as all tested drivers where setting this offset to 0.

cheers,
Nicolas

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Sakari Ailus]

Hi Hans,

On Fri, Apr 17, 2015 at 12:27:41PM +0200, Hans Verkuil wrote:
> Hi Laurent,
> 
> On 04/14/2015 09:44 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > The v4l2_plane data_offset field has been introduced at the same time as the
> > the multiplane API to convey header size information between kernelspace and
> > userspace.
> > 
> > The API then became slightly controversial, both because different developers
> > understood the purpose of the field differently (resulting for instance in an
> > out-of-tree driver abusing the field for a different purpose), and because of
> > competing proposals (see for instance "[RFC] Multi format stream support" at
> > http://www.spinics.net/lists/linux-media/msg69130.html).
> > 
> > Furthermore, the data_offset field isn't used by any mainline driver except
> > vivid (for testing purpose).
> > 
> > I need a different data offset in planes to allow data capture to or data
> > output from a userspace-selected offset within a buffer (mainly for the
> > DMABUF and MMAP memory types). As the data_offset field already has the
> > right name, is unused, and ill-defined, I propose repurposing it. This is what
> > this RFC is about.
> > 
> > If the proposal is accepted I'll add another patch to update data_offset usage
> > in the vivid driver.
> 
> I am skeptical about all this for a variety of reasons:
> 
> 1) The data_offset field is well-defined in the spec. There really is no doubt
> about the meaning of the field.

I think that's debatable. :-) The specification doesn't say much what the
data_offset is really about. For instance, it does not specify what may be
in the buffer before data_offset.

The kerneldoc documentation next to struct v4l2_plane suggests there might
be a header, but that's primarily for driver developers rather than users.

I, for instance, understood data_offset to mean essentially how this set
"re-purposes" it. I wonder if there are others who have originally
understood it as such.

> 
> 2) We really don't know who else might be using it, or which applications might
> be using it (a lot of work was done in gstreamer recently, I wonder if data_offset
> support was implemented there).
> 
> 3) You offer no alternative to this feature. Basically this is my main objection.
> It is not at all unusual to have headers in front of the frame data. We (Cisco)
> use it in one of our product series for example. And I suspect it is something that
> happens especially in systems with an FPGA that does custom processing, and those
> systems are exactly the ones that are generally not upstreamed and so are not
> visible to us.

If you have a header before the image, the header probably has a format as
well. Some headers are device specific whereas some are more generic. The
SMIA standard, for example, does specify a metadata (header or footer!)
format.

It'd be useful to be able to tell the user what kind of header there is. For
that, the header could be located on a different plane, with a specific
format.

There's room for format information in struct v4l2_plane_pix_format but
hardly much else. It still would cover a number of potential use cases.

I might still consider making the planes independent of each other;
conveniently there's 8 bytes of free space in struct v4l2_pix_format_mplane
for alternative plane related information. It'd be nice to be able to do
this without an additional buffer type since that's visible in a large
number of other places: there's plenty of room in struct v4l2_plane for
any video buffer related information.

Frame descriptors are not needed for this --- you're quite right in that.
But the frame descriptors, when implemented, will very probably need plane
specific formats in the end as not many receivers are able to separate
different parts of the image to different buffers.

> 
> IMHO the functionality it provides is very much relevant, and I would like to see
> an alternative in place before it is repurposed.
> 
> But frankly, I really don't see why you would want to repurpose it. Adding a new
> field (buf_offset) would do exactly what you want it to do without causing an ABI
> change.

I said I ok with adding buf_offset field, but it might not be the best
choice we can make: it's a temporary solution for a very specific problem,
leaves the API with similar field names with different meanings (data_offset
vs. buf_offset, where the other is about the header and the other about the
data) and is not extensible. In addition, the size of the header is not
specified; it might be smaller than what's between buf_offset and
data_offset. Some devices produce footers as well; currently we have no way
to specify how they are dealt with.

I'd like to at least investigate if we could have something more
future-proof for this purpose.

-- 
Kind regards,

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

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Hans Verkuil]

Hi Sakari,

On 04/18/2015 03:04 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Apr 17, 2015 at 12:27:41PM +0200, Hans Verkuil wrote:
>> Hi Laurent,
>>
>> On 04/14/2015 09:44 PM, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> The v4l2_plane data_offset field has been introduced at the same time as the
>>> the multiplane API to convey header size information between kernelspace and
>>> userspace.
>>>
>>> The API then became slightly controversial, both because different developers
>>> understood the purpose of the field differently (resulting for instance in an
>>> out-of-tree driver abusing the field for a different purpose), and because of
>>> competing proposals (see for instance "[RFC] Multi format stream support" at
>>> http://www.spinics.net/lists/linux-media/msg69130.html).
>>>
>>> Furthermore, the data_offset field isn't used by any mainline driver except
>>> vivid (for testing purpose).
>>>
>>> I need a different data offset in planes to allow data capture to or data
>>> output from a userspace-selected offset within a buffer (mainly for the
>>> DMABUF and MMAP memory types). As the data_offset field already has the
>>> right name, is unused, and ill-defined, I propose repurposing it. This is what
>>> this RFC is about.
>>>
>>> If the proposal is accepted I'll add another patch to update data_offset usage
>>> in the vivid driver.
>>
>> I am skeptical about all this for a variety of reasons:
>>
>> 1) The data_offset field is well-defined in the spec. There really is no doubt
>> about the meaning of the field.
> 
> I think that's debatable. :-) The specification doesn't say much what the
> data_offset is really about. For instance, it does not specify what may be
> in the buffer before data_offset.

That's correct. Now, it is my view that, while it would be nice if a fourcc like
value would be available to tell the format of that header, in practice that format
is so tied to a specific type of hardware that you either know it (i.e. it is a custom
app for that hardware), or you ignore it altogether. There may be some exceptions for
somewhat standardized types of metadata (SMIA), but those never materialized as actual
code.

> The kerneldoc documentation next to struct v4l2_plane suggests there might
> be a header, but that's primarily for driver developers rather than users.
> 
> I, for instance, understood data_offset to mean essentially how this set
> "re-purposes" it. I wonder if there are others who have originally
> understood it as such.

I know it was mis-understood, the spec was fairly vague in the past, and while more
specific you are right in that it does not actually tell the reason for the field
(i.e. skip headers).

In no way can you re-purpose the field, though.

1) It is in use.
2) If you thought it was confusing today, then that's nothing compared to the confusion
   once you change the meaning from one kernel to another.

Either keep the current meaning and improve the specification, or deprecate it: warn
when it is non-zero and just mark it as 'don't use' in the spec.

> 
>>
>> 2) We really don't know who else might be using it, or which applications might
>> be using it (a lot of work was done in gstreamer recently, I wonder if data_offset
>> support was implemented there).
>>
>> 3) You offer no alternative to this feature. Basically this is my main objection.
>> It is not at all unusual to have headers in front of the frame data. We (Cisco)
>> use it in one of our product series for example. And I suspect it is something that
>> happens especially in systems with an FPGA that does custom processing, and those
>> systems are exactly the ones that are generally not upstreamed and so are not
>> visible to us.
> 
> If you have a header before the image, the header probably has a format as
> well. Some headers are device specific whereas some are more generic. The
> SMIA standard, for example, does specify a metadata (header or footer!)
> format.
> 
> It'd be useful to be able to tell the user what kind of header there is. For
> that, the header could be located on a different plane, with a specific
> format.
> 
> There's room for format information in struct v4l2_plane_pix_format but
> hardly much else. It still would cover a number of potential use cases.
> 
> I might still consider making the planes independent of each other;
> conveniently there's 8 bytes of free space in struct v4l2_pix_format_mplane
> for alternative plane related information. It'd be nice to be able to do
> this without an additional buffer type since that's visible in a large
> number of other places: there's plenty of room in struct v4l2_plane for
> any video buffer related information.

Please don't confuse things: each struct v4l2_plane_pix_format relates to a
single buffer that contains the data for that plane. If one buffer contains
both metadata and actual image data, then that's all part of the same plane
since it was all transferred to the buffer with the same DMA transfer.

You cannot put the header/footer into separate planes since the only way to
achieve that would be a memcpy and the header/footer would still be part of
the actual plane data.

If the metadata arrives through its own DMA channel, then I would have no
objection to seeing that as a separate plane. But I think in general that
might be a bad idea because such metadata may come at an earlier/later time
compared to the image data, and usually apps want to receive things asap.

I still see it as a simple problem: I have a buffer, it contains a picture
of a given pixel format, but there may be a header and (currently not implemented)
a footer. Header and/or footer may have a format (also not implemented yet).

Applications can use the offsets to ignore all those headers/footers and just
go straight to the image data. Or they use it to interpret the data in the
headers/footers.

Perhaps it is a total lack of imagination, but I really cannot see what else
it is you would need. Of course, you can think of really crazy schemes, but
then you likely need to just use a new pixelformat since it is so crazy that
it doesn't fit into anything existing.

The whole point of data_offset was that it is nuts to have to come up with a
new pixelformat for otherwise standard pixelformats that just happen to have
a header in front of them. You can't duplicate all pixel formats just for that.

> Frame descriptors are not needed for this --- you're quite right in that.
> But the frame descriptors, when implemented, will very probably need plane
> specific formats in the end as not many receivers are able to separate
> different parts of the image to different buffers.
> 
>>
>> IMHO the functionality it provides is very much relevant, and I would like to see
>> an alternative in place before it is repurposed.
>>
>> But frankly, I really don't see why you would want to repurpose it. Adding a new
>> field (buf_offset) would do exactly what you want it to do without causing an ABI
>> change.
> 
> I said I ok with adding buf_offset field, but it might not be the best
> choice we can make: it's a temporary solution for a very specific problem,
> leaves the API with similar field names with different meanings (data_offset
> vs. buf_offset, where the other is about the header and the other about the
> data) and is not extensible. In addition, the size of the header is not
> specified; it might be smaller than what's between buf_offset and
> data_offset. Some devices produce footers as well; currently we have no way
> to specify how they are dealt with.
> 
> I'd like to at least investigate if we could have something more
> future-proof for this purpose.
> 

Proposals welcome!

Regards,

	Hans

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Laurent Pinchart]

Hi Hans,

On Monday 20 April 2015 11:16:53 Hans Verkuil wrote:
> On 04/18/2015 03:04 PM, Sakari Ailus wrote:
> > On Fri, Apr 17, 2015 at 12:27:41PM +0200, Hans Verkuil wrote:
> >> On 04/14/2015 09:44 PM, Laurent Pinchart wrote:
> >>> Hello,
> >>> 
> >>> The v4l2_plane data_offset field has been introduced at the same time as
> >>> the the multiplane API to convey header size information between
> >>> kernelspace and userspace.
> >>> 
> >>> The API then became slightly controversial, both because different
> >>> developers understood the purpose of the field differently (resulting
> >>> for instance in an out-of-tree driver abusing the field for a different
> >>> purpose), and because of competing proposals (see for instance "[RFC]
> >>> Multi format stream support" at
> >>> http://www.spinics.net/lists/linux-media/msg69130.html).
> >>> 
> >>> Furthermore, the data_offset field isn't used by any mainline driver
> >>> except vivid (for testing purpose).
> >>> 
> >>> I need a different data offset in planes to allow data capture to or
> >>> data output from a userspace-selected offset within a buffer (mainly for
> >>> the DMABUF and MMAP memory types). As the data_offset field already has
> >>> the right name, is unused, and ill-defined, I propose repurposing it.
> >>> This is what this RFC is about.
> >>> 
> >>> If the proposal is accepted I'll add another patch to update data_offset
> >>> usage in the vivid driver.
> >> 
> >> I am skeptical about all this for a variety of reasons:
> >> 
> >> 1) The data_offset field is well-defined in the spec. There really is no
> >> doubt about the meaning of the field.
> > 
> > I think that's debatable. :-) The specification doesn't say much what the
> > data_offset is really about. For instance, it does not specify what may be
> > in the buffer before data_offset.
> 
> That's correct. Now, it is my view that, while it would be nice if a fourcc
> like value would be available to tell the format of that header, in
> practice that format is so tied to a specific type of hardware that you
> either know it (i.e. it is a custom app for that hardware), or you ignore
> it altogether. There may be some exceptions for somewhat standardized types
> of metadata (SMIA), but those never materialized as actual code.
> 
> > The kerneldoc documentation next to struct v4l2_plane suggests there might
> > be a header, but that's primarily for driver developers rather than users.
> > 
> > I, for instance, understood data_offset to mean essentially how this set
> > "re-purposes" it. I wonder if there are others who have originally
> > understood it as such.
> 
> I know it was mis-understood, the spec was fairly vague in the past, and
> while more specific you are right in that it does not actually tell the
> reason for the field (i.e. skip headers).
> 
> In no way can you re-purpose the field, though.
> 
> 1) It is in use.

It's of course hard to get an overall view of all users, but the more I look 
at the problem the more it seems like both out-of-tree kernel drivers (in 
particular a Marvell CSI-2 receiver driver) and userspace (in particular 
gstreamer) have implemented support for the data_offset field as proposed in 
this patch series. We could of course argue that this is incorrect, and that 
there are out-of-tree drivers and userspace code that use data_offset for the 
purpose it was initially envisioned for (I'm thinking about Cisco code 
possibly, at least the one you've had the opportunity to review :-)). However, 
if the majority of users use data_offset "incorrectly", maybe we should 
consider that usage as being the de-facto standard and consider this series as 
a documentation fix.

The question is thus, what does the majority of the users do ?

> 2) If you thought it was confusing today, then that's nothing compared to
> the confusion once you change the meaning from one kernel to another.
> 
> Either keep the current meaning and improve the specification, or deprecate
> it: warn when it is non-zero and just mark it as 'don't use' in the spec.
> 
> >> 2) We really don't know who else might be using it, or which applications
> >> might be using it (a lot of work was done in gstreamer recently, I
> >> wonder if data_offset support was implemented there).
> >> 
> >> 3) You offer no alternative to this feature. Basically this is my main
> >> objection. It is not at all unusual to have headers in front of the
> >> frame data. We (Cisco) use it in one of our product series for example.
> >> And I suspect it is something that happens especially in systems with an
> >> FPGA that does custom processing, and those systems are exactly the ones
> >> that are generally not upstreamed and so are not visible to us.
> > 
> > If you have a header before the image, the header probably has a format as
> > well. Some headers are device specific whereas some are more generic. The
> > SMIA standard, for example, does specify a metadata (header or footer!)
> > format.
> > 
> > It'd be useful to be able to tell the user what kind of header there is.
> > For that, the header could be located on a different plane, with a
> > specific format.
> > 
> > There's room for format information in struct v4l2_plane_pix_format but
> > hardly much else. It still would cover a number of potential use cases.
> > 
> > I might still consider making the planes independent of each other;
> > conveniently there's 8 bytes of free space in struct
> > v4l2_pix_format_mplane for alternative plane related information. It'd be
> > nice to be able to do this without an additional buffer type since that's
> > visible in a large number of other places: there's plenty of room in
> > struct v4l2_plane for any video buffer related information.
> 
> Please don't confuse things: each struct v4l2_plane_pix_format relates to a
> single buffer that contains the data for that plane. If one buffer contains
> both metadata and actual image data, then that's all part of the same plane
> since it was all transferred to the buffer with the same DMA transfer.
> 
> You cannot put the header/footer into separate planes since the only way to
> achieve that would be a memcpy and the header/footer would still be part of
> the actual plane data.
> 
> If the metadata arrives through its own DMA channel, then I would have no
> objection to seeing that as a separate plane. But I think in general that
> might be a bad idea because such metadata may come at an earlier/later time
> compared to the image data, and usually apps want to receive things asap.
> 
> I still see it as a simple problem: I have a buffer, it contains a picture
> of a given pixel format, but there may be a header and (currently not
> implemented) a footer. Header and/or footer may have a format (also not
> implemented yet).
> 
> Applications can use the offsets to ignore all those headers/footers and
> just go straight to the image data. Or they use it to interpret the data in
> the headers/footers.
> 
> Perhaps it is a total lack of imagination, but I really cannot see what else
> it is you would need. Of course, you can think of really crazy schemes, but
> then you likely need to just use a new pixelformat since it is so crazy
> that it doesn't fit into anything existing.
> 
> The whole point of data_offset was that it is nuts to have to come up with a
> new pixelformat for otherwise standard pixelformats that just happen to
> have a header in front of them. You can't duplicate all pixel formats just
> for that.
>
> > Frame descriptors are not needed for this --- you're quite right in that.
> > But the frame descriptors, when implemented, will very probably need plane
> > specific formats in the end as not many receivers are able to separate
> > different parts of the image to different buffers.
> > 
> >> IMHO the functionality it provides is very much relevant, and I would
> >> like to see an alternative in place before it is repurposed.
> >> 
> >> But frankly, I really don't see why you would want to repurpose it.
> >> Adding a new field (buf_offset) would do exactly what you want it to do
> >> without causing an ABI change.
> > 
> > I said I ok with adding buf_offset field, but it might not be the best
> > choice we can make: it's a temporary solution for a very specific problem,
> > leaves the API with similar field names with different meanings
> > (data_offset vs. buf_offset, where the other is about the header and the
> > other about the data) and is not extensible. In addition, the size of the
> > header is not specified; it might be smaller than what's between
> > buf_offset and data_offset. Some devices produce footers as well;
> > currently we have no way to specify how they are dealt with.
> > 
> > I'd like to at least investigate if we could have something more
> > future-proof for this purpose.
> 
> Proposals welcome!

I certainly second that :-) Sakari, do you have something in mind ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/2] Repurpose the v4l2_plane data_offset field

[Sakari Ailus]

Hi Hans,

On Mon, Apr 20, 2015 at 11:16:53AM +0200, Hans Verkuil wrote:
> Hi Sakari,
> 
> On 04/18/2015 03:04 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Fri, Apr 17, 2015 at 12:27:41PM +0200, Hans Verkuil wrote:
> >> Hi Laurent,
> >>
> >> On 04/14/2015 09:44 PM, Laurent Pinchart wrote:
> >>> Hello,
> >>>
> >>> The v4l2_plane data_offset field has been introduced at the same time as the
> >>> the multiplane API to convey header size information between kernelspace and
> >>> userspace.
> >>>
> >>> The API then became slightly controversial, both because different developers
> >>> understood the purpose of the field differently (resulting for instance in an
> >>> out-of-tree driver abusing the field for a different purpose), and because of
> >>> competing proposals (see for instance "[RFC] Multi format stream support" at
> >>> http://www.spinics.net/lists/linux-media/msg69130.html).
> >>>
> >>> Furthermore, the data_offset field isn't used by any mainline driver except
> >>> vivid (for testing purpose).
> >>>
> >>> I need a different data offset in planes to allow data capture to or data
> >>> output from a userspace-selected offset within a buffer (mainly for the
> >>> DMABUF and MMAP memory types). As the data_offset field already has the
> >>> right name, is unused, and ill-defined, I propose repurposing it. This is what
> >>> this RFC is about.
> >>>
> >>> If the proposal is accepted I'll add another patch to update data_offset usage
> >>> in the vivid driver.
> >>
> >> I am skeptical about all this for a variety of reasons:
> >>
> >> 1) The data_offset field is well-defined in the spec. There really is no doubt
> >> about the meaning of the field.
> > 
> > I think that's debatable. :-) The specification doesn't say much what the
> > data_offset is really about. For instance, it does not specify what may be
> > in the buffer before data_offset.
> 
> That's correct. Now, it is my view that, while it would be nice if a fourcc like
> value would be available to tell the format of that header, in practice that format
> is so tied to a specific type of hardware that you either know it (i.e. it is a custom
> app for that hardware), or you ignore it altogether. There may be some exceptions for
> somewhat standardized types of metadata (SMIA), but those never materialized as actual
> code.

At least not yet, and part of the reason is that we have no generic means to
pass that to tell which format if actually is and pass that to the user
space.

Also not every SMIA compliant sensor produces metadata at all.

When they do, however, there's often also a footer, of the same format than
the header. Some sensors produce statistics after that footer as well.

> > The kerneldoc documentation next to struct v4l2_plane suggests there might
> > be a header, but that's primarily for driver developers rather than users.
> > 
> > I, for instance, understood data_offset to mean essentially how this set
> > "re-purposes" it. I wonder if there are others who have originally
> > understood it as such.
> 
> I know it was mis-understood, the spec was fairly vague in the past, and while more
> specific you are right in that it does not actually tell the reason for the field
> (i.e. skip headers).
> 
> In no way can you re-purpose the field, though.
> 
> 1) It is in use.

How is it being used? It'd be nice to have a guesstimate for different
usages (the original intent vs. how apparently quite a few have understood
it).

> 2) If you thought it was confusing today, then that's nothing compared to the confusion
>    once you change the meaning from one kernel to another.
> 
> Either keep the current meaning and improve the specification, or deprecate it: warn
> when it is non-zero and just mark it as 'don't use' in the spec.
> 
> > 
> >>
> >> 2) We really don't know who else might be using it, or which applications might
> >> be using it (a lot of work was done in gstreamer recently, I wonder if data_offset
> >> support was implemented there).
> >>
> >> 3) You offer no alternative to this feature. Basically this is my main objection.
> >> It is not at all unusual to have headers in front of the frame data. We (Cisco)
> >> use it in one of our product series for example. And I suspect it is something that
> >> happens especially in systems with an FPGA that does custom processing, and those
> >> systems are exactly the ones that are generally not upstreamed and so are not
> >> visible to us.
> > 
> > If you have a header before the image, the header probably has a format as
> > well. Some headers are device specific whereas some are more generic. The
> > SMIA standard, for example, does specify a metadata (header or footer!)
> > format.
> > 
> > It'd be useful to be able to tell the user what kind of header there is. For
> > that, the header could be located on a different plane, with a specific
> > format.
> > 
> > There's room for format information in struct v4l2_plane_pix_format but
> > hardly much else. It still would cover a number of potential use cases.
> > 
> > I might still consider making the planes independent of each other;
> > conveniently there's 8 bytes of free space in struct v4l2_pix_format_mplane
> > for alternative plane related information. It'd be nice to be able to do
> > this without an additional buffer type since that's visible in a large
> > number of other places: there's plenty of room in struct v4l2_plane for
> > any video buffer related information.
> 
> Please don't confuse things: each struct v4l2_plane_pix_format relates to a
> single buffer that contains the data for that plane. If one buffer contains
> both metadata and actual image data, then that's all part of the same plane
> since it was all transferred to the buffer with the same DMA transfer.

The API does not prevent using the same DMA-BUF buffer (nor USERPTR for that
matter) on multiple planes. Then the question would be how would the user
know when to do that and when not. We could add a V4L2_PIX_FMT_FLAG telling
the plane uses the same memory buffer than the previous one, for instance.

Alternatively, one more layer of abstraction could be used: multi-format
planes. That would mean an array of one or more formatted sections inside a
plane. 

> 
> You cannot put the header/footer into separate planes since the only way to
> achieve that would be a memcpy and the header/footer would still be part of
> the actual plane data.

Assuming it's a separate memory buffer, yes.

For most of the time the hardware can do either one, but there could be
cases where the user would benefit from being able to choose. This can be
made easily by making no difference between a memory buffer with sections of
different formats and several memory buffers with a single format each.

> If the metadata arrives through its own DMA channel, then I would have no
> objection to seeing that as a separate plane. But I think in general that
> might be a bad idea because such metadata may come at an earlier/later time
> compared to the image data, and usually apps want to receive things asap.

Indeed. That's another interesting matter. Sometimes a part of e.g. a
statistics buffer may be interesting (and available from hardware) before
the entire buffer is done.

That was why the different parts of the frame were split into different
video buffer queues:

<URL:http://www.retiisi.org.uk/v4l2/foil/v4l2-multi-format.pdf>

But it won't help in all cases, like the one described above.

> 
> I still see it as a simple problem: I have a buffer, it contains a picture
> of a given pixel format, but there may be a header and (currently not implemented)
> a footer. Header and/or footer may have a format (also not implemented yet).
> 
> Applications can use the offsets to ignore all those headers/footers and just
> go straight to the image data. Or they use it to interpret the data in the
> headers/footers.
> 
> Perhaps it is a total lack of imagination, but I really cannot see what else
> it is you would need. Of course, you can think of really crazy schemes, but
> then you likely need to just use a new pixelformat since it is so crazy that
> it doesn't fit into anything existing.

The statistics I mentioned above; they are not related to the header or the
footer. It'd be very good to be able to describe them in a generic way;
adding a header and a footer support now in a way that recognises they're
the header and the footer is unlikely to be meaningfully extendable when
something else comes up.

These are still just examples, we all know hardware engineers have a
virtually unlimited imagination. :-)

> 
> The whole point of data_offset was that it is nuts to have to come up with a
> new pixelformat for otherwise standard pixelformats that just happen to have
> a header in front of them. You can't duplicate all pixel formats just for that.

I fully agree with the problem statement and the undesirable solution. :-)

> Proposals welcome!

I can send an RFC this or the next week, with more detailed description of
the use cases.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org