Em Tue, 14 Aug 2018 16:20:31 +0200
Hans Verkuil <hverk...@xs4all.nl> escreveu:
> From: Hans Verkuil <hans.verk...@cisco.com>
>
> The userspace-provided plane data needs to be stored in
> vb2_v4l2_buffer. Currently this information is applied by
> __fill_vb2_buffer() which is called by the core prepare_buf
> and qbuf functions, but when using requests these functions
> aren't called yet since the buffer won't be prepared until
> the media request is actually queued.
>
> In the meantime this information has to be stored somewhere
> and vb2_v4l2_buffer is a good place for it.
>
> The __fill_vb2_buffer callback now just copies the relevant
> information from vb2_v4l2_buffer into the planes array.
>
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 43 ++++++-------
> .../media/common/videobuf2/videobuf2-v4l2.c | 60 +++++++++++++++----
> drivers/media/dvb-core/dvb_vb2.c | 3 +-
> include/media/videobuf2-core.h | 3 +-
> include/media/videobuf2-v4l2.h | 2 +
> 5 files changed, 72 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 5653e8eebe2b..7401a17c80ca 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -967,20 +967,19 @@ EXPORT_SYMBOL_GPL(vb2_discard_done);
> /*
> * __prepare_mmap() - prepare an MMAP buffer
> */
> -static int __prepare_mmap(struct vb2_buffer *vb, const void *pb)
> +static int __prepare_mmap(struct vb2_buffer *vb)
> {
> int ret = 0;
>
> - if (pb)
> - ret = call_bufop(vb->vb2_queue, fill_vb2_buffer,
> - vb, pb, vb->planes);
> + ret = call_bufop(vb->vb2_queue, fill_vb2_buffer,
> + vb, vb->planes);
> return ret ? ret : call_vb_qop(vb, buf_prepare, vb);
> }
>
> /*
> * __prepare_userptr() - prepare a USERPTR buffer
> */
> -static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> +static int __prepare_userptr(struct vb2_buffer *vb)
> {
> struct vb2_plane planes[VB2_MAX_PLANES];
> struct vb2_queue *q = vb->vb2_queue;
> @@ -991,12 +990,10 @@ static int __prepare_userptr(struct vb2_buffer *vb,
> const void *pb)
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> /* Copy relevant information provided by the userspace */
> - if (pb) {
> - ret = call_bufop(vb->vb2_queue, fill_vb2_buffer,
> - vb, pb, planes);
> - if (ret)
> - return ret;
> - }
> + ret = call_bufop(vb->vb2_queue, fill_vb2_buffer,
> + vb, planes);
> + if (ret)
> + return ret;
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> /* Skip the plane if already verified */
> @@ -1096,7 +1093,7 @@ static int __prepare_userptr(struct vb2_buffer *vb,
> const void *pb)
> /*
> * __prepare_dmabuf() - prepare a DMABUF buffer
> */
> -static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> +static int __prepare_dmabuf(struct vb2_buffer *vb)
> {
> struct vb2_plane planes[VB2_MAX_PLANES];
> struct vb2_queue *q = vb->vb2_queue;
> @@ -1107,12 +1104,10 @@ static int __prepare_dmabuf(struct vb2_buffer *vb,
> const void *pb)
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> /* Copy relevant information provided by the userspace */
> - if (pb) {
> - ret = call_bufop(vb->vb2_queue, fill_vb2_buffer,
> - vb, pb, planes);
> - if (ret)
> - return ret;
> - }
> + ret = call_bufop(vb->vb2_queue, fill_vb2_buffer,
> + vb, planes);
> + if (ret)
> + return ret;
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
> @@ -1241,7 +1236,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 void *pb)
> +static int __buf_prepare(struct vb2_buffer *vb)
> {
> struct vb2_queue *q = vb->vb2_queue;
> unsigned int plane;
> @@ -1256,13 +1251,13 @@ static int __buf_prepare(struct vb2_buffer *vb, const
> void *pb)
>
> switch (q->memory) {
> case VB2_MEMORY_MMAP:
> - ret = __prepare_mmap(vb, pb);
> + ret = __prepare_mmap(vb);
> break;
> case VB2_MEMORY_USERPTR:
> - ret = __prepare_userptr(vb, pb);
> + ret = __prepare_userptr(vb);
> break;
> case VB2_MEMORY_DMABUF:
> - ret = __prepare_dmabuf(vb, pb);
> + ret = __prepare_dmabuf(vb);
> break;
> default:
> WARN(1, "Invalid queue type\n");
> @@ -1296,7 +1291,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned
> int index, void *pb)
> return -EINVAL;
> }
>
> - ret = __buf_prepare(vb, pb);
> + ret = __buf_prepare(vb);
> if (ret)
> return ret;
>
> @@ -1386,7 +1381,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
> index, void *pb)
>
> switch (vb->state) {
> case VB2_BUF_STATE_DEQUEUED:
> - ret = __buf_prepare(vb, pb);
> + ret = __buf_prepare(vb);
> if (ret)
> return ret;
> break;
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 57848ddc584f..360dc4e7d413 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -154,17 +154,11 @@ static void vb2_warn_zero_bytesused(struct vb2_buffer
> *vb)
> pr_warn("use the actual size instead.\n");
> }
>
> -/*
> - * __fill_vb2_buffer() - fill a vb2_buffer with information provided in a
> - * v4l2_buffer by the userspace. It also verifies that struct
> - * v4l2_buffer has a valid number of planes.
> - */
> -static int __fill_vb2_buffer(struct vb2_buffer *vb,
> - const void *pb, struct vb2_plane *planes)
> +static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct
> v4l2_buffer *b)
> {
> struct vb2_queue *q = vb->vb2_queue;
> - const struct v4l2_buffer *b = pb;
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct vb2_plane *planes = vbuf->planes;
> unsigned int plane;
> int ret;
>
> @@ -186,7 +180,6 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
> dprintk(1, "the field is incorrectly set to ALTERNATE for an
> output buffer\n");
> return -EINVAL;
> }
> - vb->timestamp = 0;
See my note below about this removal. On a quick look, I guess we may have
a regression here for output buffers (non-m2m).
> vbuf->sequence = 0;
>
> if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
> @@ -208,6 +201,12 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
> }
> break;
> default:
> + for (plane = 0; plane < vb->num_planes; ++plane) {
> + planes[plane].m.offset =
> + vb->planes[plane].m.offset;
> + planes[plane].length =
> + vb->planes[plane].length;
> + }
> break;
> }
>
> @@ -269,9 +268,12 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
> planes[0].length = b->length;
> break;
> default:
> + planes[0].m.offset = vb->planes[0].m.offset;
> + planes[0].length = vb->planes[0].length;
> break;
> }
>
> + planes[0].data_offset = 0;
> if (V4L2_TYPE_IS_OUTPUT(b->type)) {
> if (b->bytesused == 0)
> vb2_warn_zero_bytesused(vb);
> @@ -286,7 +288,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
>
> }
>
> - /* Zero flags that the vb2 core handles */
> + /* Zero flags that we handle */
> vbuf->flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS;
> if (!vb->vb2_queue->copy_timestamp || !V4L2_TYPE_IS_OUTPUT(b->type)) {
> /*
> @@ -319,6 +321,10 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
> static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer
> *b,
> const char *opname)
> {
> + struct vb2_v4l2_buffer *vbuf;
> + struct vb2_buffer *vb;
> + int ret;
> +
> if (b->type != q->type) {
> dprintk(1, "%s: invalid buffer type\n", opname);
> return -EINVAL;
> @@ -340,7 +346,15 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q,
> struct v4l2_buffer *b,
> return -EINVAL;
> }
>
> - return __verify_planes_array(q->bufs[b->index], b);
> + vb = q->bufs[b->index];
> + vbuf = to_vb2_v4l2_buffer(vb);
> + ret = __verify_planes_array(vb, b);
> + if (ret)
> + return ret;
> +
> + /* Copy relevant information provided by the userspace */
> + memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
> + return vb2_fill_vb2_v4l2_buffer(vb, b);
> }
>
> /*
> @@ -448,6 +462,30 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb,
> void *pb)
> q->last_buffer_dequeued = true;
> }
>
> +/*
> + * __fill_vb2_buffer() - fill a vb2_buffer with information provided in a
> + * v4l2_buffer by the userspace. It also verifies that struct
> + * v4l2_buffer has a valid number of planes.
> + */
> +static int __fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + unsigned int plane;
> +
> + if (!vb->vb2_queue->is_output || !vb->vb2_queue->copy_timestamp)
> + vb->timestamp = 0;
When vb->vb2_queue->copy_timestamp is not NULL, timestamp will be copied,
but how VB2 will fill it if is_output?
I suspect that the right logic here would be just:
if (!vb->vb2_queue->copy_timestamp)
vb->timestamp = 0;
> +
> + for (plane = 0; plane < vb->num_planes; ++plane) {
> + if (vb->vb2_queue->memory != VB2_MEMORY_MMAP) {
> + planes[plane].m = vbuf->planes[plane].m;
> + planes[plane].length = vbuf->planes[plane].length;
> + }
> + planes[plane].bytesused = vbuf->planes[plane].bytesused;
> + planes[plane].data_offset = vbuf->planes[plane].data_offset;
> + }
> + return 0;
> +}
> +
> static const struct vb2_buf_ops v4l2_buf_ops = {
> .verify_planes_array = __verify_planes_array_core,
> .fill_user_buffer = __fill_v4l2_buffer,
> diff --git a/drivers/media/dvb-core/dvb_vb2.c
> b/drivers/media/dvb-core/dvb_vb2.c
> index b811adf88afa..da6a8cec7d42 100644
> --- a/drivers/media/dvb-core/dvb_vb2.c
> +++ b/drivers/media/dvb-core/dvb_vb2.c
> @@ -146,8 +146,7 @@ static void _fill_dmx_buffer(struct vb2_buffer *vb, void
> *pb)
> dprintk(3, "[%s]\n", ctx->name);
> }
>
> -static int _fill_vb2_buffer(struct vb2_buffer *vb,
> - const void *pb, struct vb2_plane *planes)
> +static int _fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
> {
> struct dvb_vb2_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
>
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index f6818f732f34..224c4820a044 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -417,8 +417,7 @@ struct vb2_ops {
> struct vb2_buf_ops {
> int (*verify_planes_array)(struct vb2_buffer *vb, const void *pb);
> void (*fill_user_buffer)(struct vb2_buffer *vb, void *pb);
> - int (*fill_vb2_buffer)(struct vb2_buffer *vb, const void *pb,
> - struct vb2_plane *planes);
> + int (*fill_vb2_buffer)(struct vb2_buffer *vb, struct vb2_plane *planes);
> void (*copy_timestamp)(struct vb2_buffer *vb, const void *pb);
> };
>
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index 3d5e2d739f05..097bf3e6951d 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -32,6 +32,7 @@
> * &enum v4l2_field.
> * @timecode: frame timecode.
> * @sequence: sequence count of this frame.
> + * @planes: plane information (userptr/fd, length, bytesused, data_offset).
> *
> * Should contain enough information to be able to cover all the fields
> * of &struct v4l2_buffer at ``videodev2.h``.
> @@ -43,6 +44,7 @@ struct vb2_v4l2_buffer {
> __u32 field;
> struct v4l2_timecode timecode;
> __u32 sequence;
> + struct vb2_plane planes[VB2_MAX_PLANES];
> };
>
> /*
Thanks,
Mauro
