Hi Hans,
On Fri, Dec 05, 2014 at 04:10:05PM +0100, Hans Verkuil wrote:
> On 12/03/2014 12:14 PM, Sakari Ailus wrote:
> > From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >
> > The data_offset field tells the start of the image data from the beginning
> > of the buffer. The bsize field
>
> bsize field? There is no bsize field in v4l2_buffer, so I'm confused.
This is a brain'o. It should have been "bytesused". I'll fix that to the
next version.
> > in struct v4l2_buffer includes this, but the
> > sizeimage field in struct v4l2_pix_format does not.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > Documentation/DocBook/media/v4l/compat.xml | 11 +++++++++++
> > Documentation/DocBook/media/v4l/io.xml | 18 +++++++++++++++---
> > Documentation/DocBook/media/v4l/vidioc-qbuf.xml | 3 +--
> > drivers/media/usb/cpia2/cpia2_v4l.c | 2 +-
> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
> > drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++++-----
> > include/uapi/linux/videodev2.h | 4 +++-
> > 7 files changed, 45 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
> > index 0a2debf..ad54e72 100644
> > --- a/Documentation/DocBook/media/v4l/compat.xml
> > +++ b/Documentation/DocBook/media/v4l/compat.xml
> > @@ -2579,6 +2579,17 @@ fields changed from _s32 to _u32.
> > </orderedlist>
> > </section>
> >
> > + <section>
> > + <title>V4L2 in Linux 3.20</title>
> > + <orderedlist>
> > + <listitem>
> > + <para>Replaced <structfield>reserved2</structfield> by
> > + <strucfield>data_offset<structfield> in struct
> > + <structname>v4l2_buffer</structname>.</para>
> > + </listitem>
> > + </orderedlist>
> > + </section>
> > +
> > <section id="other">
> > <title>Relation of V4L2 to other Linux multimedia APIs</title>
> >
> > diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> > index 1c17f80..13baeac 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -839,10 +839,22 @@ is the file descriptor associated with a DMABUF buffer.</entry>
> > </row>
> > <row>
> > <entry>__u32</entry>
> > - <entry><structfield>reserved2</structfield></entry>
> > + <entry><structfield>data_offset</structfield></entry>
> > <entry></entry>
> > - <entry>A place holder for future extensions. Applications
> > -should set this to 0.</entry>
> > + <entry>
> > + Start of the image data from the beginning of the buffer in
> > + bytes. Applications must set this for both
>
> Drop 'both'.
Fixed.
> > + <constant>V4L2_BUF_TYPE_VIDEO_OUTPUT</constant> buffers
> > + whereas driver must set this for
> > + <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant> buffers
> > + before &VIDIOC-PREPARE-BUF; and &VIDIOC-QBUF; IOCTLs. Note
>
> s/IOCTLs/ioctls/
Fixed.
> > + that data_offset is included in
> > + <structfield>bytesused</structfield>. So the size of the image
> > + in the plane is <structfield>bytesused</structfield>-
> > + <structfield>data_offset</structfield> at offset
> > + <structfield>data_offset</structfield> from the start of the
> > + plane.
> > + </entry>
> > </row>
> > <row>
> > <entry>__u32</entry>
> > diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > index 3504a7f..f529e4d 100644
> > --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> > @@ -72,8 +72,7 @@ initialize the <structfield>bytesused</structfield>,
> > <structfield>timestamp</structfield> fields, see <xref
> > linkend="buffer" /> for details.
> > Applications must also set <structfield>flags</structfield> to 0.
> > -The <structfield>reserved2</structfield> and
> > -<structfield>reserved</structfield> fields must be set to 0. When using
> > +The <structfield>reserved</structfield> field must be set to 0. When using
> > the <link linkend="planar-apis">multi-planar API</link>, the
> > <structfield>m.planes</structfield> field must contain a userspace pointer
> > to a filled-in array of &v4l2-plane; and the <structfield>length</structfield>
> > diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
> > index 9caea83..a94e83a 100644
> > --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> > +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> > @@ -952,7 +952,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
> > buf->sequence = cam->buffers[buf->index].seq;
> > buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
> > buf->length = cam->frame_size;
> > - buf->reserved2 = 0;
> > + buf->data_offset = 0;
> > buf->reserved = 0;
> > memset(&buf->timecode, 0, sizeof(buf->timecode));
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > index af63543..e238066 100644
> > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > @@ -326,7 +326,7 @@ struct v4l2_buffer32 {
> > __s32 fd;
> > } m;
> > __u32 length;
> > - __u32 reserved2;
> > + __u32 data_offset;
> > __u32 reserved;
> > };
> >
> > @@ -491,7 +491,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
> > put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
> > copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
> > put_user(kp->sequence, &up->sequence) ||
> > - put_user(kp->reserved2, &up->reserved2) ||
> > + put_user(kp->data_offset, &up->data_offset) ||
> > put_user(kp->reserved, &up->reserved))
> > return -EFAULT;
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 7aed8f2..3162de8 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -607,6 +607,9 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> >
> > if (b->bytesused > length)
> > return -EINVAL;
> > +
> > + if (b->data_offset > 0 && b->data_offset >= bytesused)
> > + return -EINVAL;
> > }
> >
> > return 0;
> > @@ -657,7 +660,6 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >
> > /* Copy back data such as timestamp, flags, etc. */
> > memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
> > - b->reserved2 = vb->v4l2_buf.reserved2;
> > b->reserved = vb->v4l2_buf.reserved;
> >
> > if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
> > @@ -666,14 +668,17 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> > * for it. The caller has already verified memory and size.
> > */
> > b->length = vb->num_planes;
> > + b->data_offset = vb->v4l2_buf.data_offset;
> > memcpy(b->m.planes, vb->v4l2_planes,
> > b->length * sizeof(struct v4l2_plane));
> > } else {
> > /*
> > - * We use length and offset in v4l2_planes array even for
> > - * single-planar buffers, but userspace does not.
> > + * We use length, data_offset and bytesused in
> > + * v4l2_planes array even for single-planar buffers,
> > + * but userspace does not.
> > */
> > b->length = vb->v4l2_planes[0].length;
> > + b->data_offset = vb->v4l2_planes[0].data_offset;
> > b->bytesused = vb->v4l2_planes[0].bytesused;
> > if (q->memory == V4L2_MEMORY_MMAP)
> > b->m.offset = vb->v4l2_planes[0].m.mem_offset;
> > @@ -1306,11 +1311,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
> > v4l2_planes[0].length = b->length;
> > }
> >
> > - if (V4L2_TYPE_IS_OUTPUT(b->type))
> > + if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> > v4l2_planes[0].bytesused = b->bytesused ?
> > b->bytesused : v4l2_planes[0].length;
> > - else
> > + v4l2_planes[0].data_offset = b->data_offset;
> > + } else {
> > v4l2_planes[0].bytesused = 0;
> > + }
> >
> > }
> >
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 1c2f84f..e9806c6 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -675,6 +675,8 @@ struct v4l2_plane {
> > * @length: size in bytes of the buffer (NOT its payload) for single-plane
> > * buffers (when type != *_MPLANE); number of elements in the
> > * planes array for multi-plane buffers
> > + * @data_offset: Offset of the start of data from the beginning of the
> > + * buffer. Typically zero.
> > *
> > * Contains data exchanged by application and driver using one of the Streaming
> > * I/O methods.
> > @@ -698,7 +700,7 @@ struct v4l2_buffer {
> > __s32 fd;
> > } m;
> > __u32 length;
> > - __u32 reserved2;
> > + __u32 data_offset;
> > __u32 reserved;
> > };
> >
> >
>
> I think we need to add new helper functions that give back the real plane size
> (i.e. bytesused - data_offset) and the actual plane start position (plane start
> + data_offset). It will be a bit tricky though to check existing drivers.
I think this mostly applies to OUTPUT buffers.
I find the definition for multi-plane buffers a little bit odd --- why not
allow setting this for CAPTURE buffers as well, on hardware that supports
it? This makes sense, in order to use the buffers on other interfaces
without memory copies this may be even mandatory.
I wonder if we should change the spec regarding this, even if no driver
support was added yet.
> AFAICT vivid is one driver that uses vb2_plane_size() to check if enough space
> is available for the image, but that doesn't take the data_offset into account.
>
> I suspect that similar problems occur for output drivers. And what isn't properly
> defined at the moment is what should happen if an output driver doesn't support
> a particular data_offset value.
>
> I think the only thing you can do in that case is to return an error when QBUF
> is called.
I'd think so. Same for PREPARE_BUF.
I suppose very few drivers support this at the moment, and the ones that
don't would return -EINVAL on QBUF. This could reveal broken user space
applications. An alternative would be to silently assign a valid value to
the field, but I'm not sure if that's any better.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
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
