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@xxxxxxxxxxxxxxxx>
> > ---
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
