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

  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(-)
  

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
  

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@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</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