On 09/11/2015 09:47 PM, Hans Verkuil wrote:
Hi Junghak,
Patch 7/8 helped a lot in reducing the size of this one. But it is still difficult
to review so I would like to request one final (honest!) split for this patch:
Move all the code that does not depend on the new buf_ops into a separate patch.
So the new q->is_output and q->multiplanar field can be moved to that patch.
But also the changes to functions like vb2_expbuf() or streamon/off where some
checks are moved from core.c to v4l2.c can be done separately.
All such pretty easy to review modifications should be put in a separate patch,
leaving me with one remaining patch that I really need to study.
I recommend that you wait until 4.3-rc1 is released and merged back in our tree
since that will contain a number of vb2 changes (Jan Kara's work). So it makes
sense to rebase on top of that first before doing more work on this.
I'll prepare to rebase on 4.3-rc1 and make it much easier to review.
I did find a few things in this patch as well, see my comments below:
On 09/09/2015 01:19 PM, Junghak Sung wrote:
Move v4l2-stuffs from videobuf2-core to videobuf2-v4l2. And make
wrappers that use the vb2_core_* functions.
Signed-off-by: Junghak Sung <jh1009.sung@xxxxxxxxxxx>
Signed-off-by: Geunyoung Kim <nenggun.kim@xxxxxxxxxxx>
Acked-by: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
Acked-by: Inki Dae <inki.dae@xxxxxxxxxxx>
---
drivers/media/v4l2-core/videobuf2-core.c | 517 ++++++++++++++++----------
drivers/media/v4l2-core/videobuf2-internal.h | 51 +--
drivers/media/v4l2-core/videobuf2-v4l2.c | 312 ++++++++++++----
include/media/videobuf2-core.h | 20 +-
include/media/videobuf2-v4l2.h | 3 +-
5 files changed, 601 insertions(+), 302 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 3e6ee0e..56d34f2 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
<snip>
@@ -454,13 +426,34 @@ static bool __buffers_in_use(struct vb2_queue *q)
{
unsigned int buffer;
for (buffer = 0; buffer < q->num_buffers; ++buffer) {
- if (__buffer_in_use(q, q->bufs[buffer]))
+ if (vb2_buffer_in_use(q, q->bufs[buffer]))
return true;
}
return false;
}
/**
+ * vb2_core_querybuf() - query video buffer information
+ * @q: videobuf queue
+ * @index: id number of the buffer
+ * @pb: buffer struct passed from userspace
+ *
+ * Should be called from vidioc_querybuf ioctl handler in driver.
+ * The passed buffer should have been verified.
+ * This function fills the relevant information for the userspace.
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_querybuf handler in driver.
+ */
+int vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
+{
+ call_bufop(q, fill_user_buffer, q->bufs[index], pb);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_core_querybuf);
This should be a void function since it never returns an error.
But do we need it at all? It really doesn't do anything that is core-specific.
The querybuf ioctl is really pure V4L2 and has nothing to do with the vb2 core.
I think it would be better to keep this code, like other vb2_core
functions. I do not want to call buf_ops callback function - i.e.
full_user_buffer() - from the v4l2-side.
And return value is also needed because fill_user_buffer() can return
error code.
But, this code should be modified like:
int vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
{
return call_bufop(q, fill_user_buffer, q->bufs[index], pb);
}
+
+/**
* __verify_userptr_ops() - verify that all memory operations required for
* USERPTR queue type have been provided
*/
@@ -1182,52 +1174,27 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
call_void_vb_qop(vb, buf_queue, vb);
}
-int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
+static int __buf_prepare(struct vb2_buffer *vb, void *pb)
{
- struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
struct vb2_queue *q = vb->vb2_queue;
int ret;
- ret = __verify_length(vb, b);
- if (ret < 0) {
- dprintk(1, "plane parameters verification failed: %d\n", ret);
- return ret;
- }
- if (b->field == V4L2_FIELD_ALTERNATE && V4L2_TYPE_IS_OUTPUT(q->type)) {
- /*
- * If the format's field is ALTERNATE, then the buffer's field
- * should be either TOP or BOTTOM, not ALTERNATE since that
- * makes no sense. The driver has to know whether the
- * buffer represents a top or a bottom field in order to
- * program any DMA correctly. Using ALTERNATE is wrong, since
- * that just says that it is either a top or a bottom field,
- * but not which of the two it is.
- */
- dprintk(1, "the field is incorrectly set to ALTERNATE for an output buffer\n");
- return -EINVAL;
- }
-
if (q->error) {
dprintk(1, "fatal error occurred on queue\n");
return -EIO;
}
- vb->state = VB2_BUF_STATE_PREPARING;
Hmmm, this moved to v4l2.c. Why? This still belongs here as far as I can tell.
I suspect that was a copy-and-paste mistake.
Correct! I'll fix it.
- vbuf->timestamp.tv_sec = 0;
- vbuf->timestamp.tv_usec = 0;
- vbuf->sequence = 0;
-
switch (q->memory) {
case VB2_MEMORY_MMAP:
- ret = __qbuf_mmap(vb, b);
+ ret = __qbuf_mmap(vb, pb);
break;
case VB2_MEMORY_USERPTR:
down_read(¤t->mm->mmap_sem);
- ret = __qbuf_userptr(vb, b);
+ ret = __qbuf_userptr(vb, pb);
up_read(¤t->mm->mmap_sem);
break;
case VB2_MEMORY_DMABUF:
- ret = __qbuf_dmabuf(vb, b);
+ ret = __qbuf_dmabuf(vb, pb);
break;
default:
WARN(1, "Invalid queue type\n");
@@ -1241,32 +1208,94 @@ int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
return ret;
}
-int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
- const char *opname)
+/**
+ * vb2_verify_buffer() - verify the buffer information passed from userspace
+ */
+int vb2_verify_buffer(struct vb2_queue *q,
+ enum vb2_memory memory, unsigned int type,
+ unsigned int index, unsigned int nplanes,
+ void *pplane, const char *opname)
{
- if (b->type != q->type) {
+ if (type != q->type) {
dprintk(1, "%s: invalid buffer type\n", opname);
return -EINVAL;
}
- if (b->index >= q->num_buffers) {
+ if (index >= q->num_buffers) {
dprintk(1, "%s: buffer index out of range\n", opname);
return -EINVAL;
}
- if (q->bufs[b->index] == NULL) {
+ if (q->bufs[index] == NULL) {
/* Should never happen */
dprintk(1, "%s: buffer is NULL\n", opname);
return -EINVAL;
}
- if (b->memory != q->memory) {
+ if (memory != VB2_MEMORY_UNKNOWN && memory != q->memory) {
dprintk(1, "%s: invalid memory type\n", opname);
return -EINVAL;
}
- return __verify_planes_array(q->bufs[b->index], b);
+ if (q->is_multiplanar) {
+ struct vb2_buffer *vb = q->bufs[index];
+
+ /* Is memory for copying plane information present? */
+ if (NULL == pplane) {
+ dprintk(1, "%s: multi-planar buffer passed but "
+ "planes array not provided\n", opname);
+ return -EINVAL;
+ }
This doesn't belong here. pplane is very much v4l2 specific and should be
tested there.
Apparently, it should be checked on v4l2-side. I'll fix it.
Thank you.
Best regards,
Junghak
+
+ if (nplanes < vb->num_planes || nplanes > VB2_MAX_PLANES) {
+ dprintk(1, "%s: incorrect planes array length, "
+ "expected %d, got %d\n",
+ opname, vb->num_planes, nplanes);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_verify_buffer);
+
+/**
+ * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace to the kernel
+ * @q: videobuf2 queue
+ * @index: id number of the buffer
+ * @pb: buffer structure passed from userspace to vidioc_prepare_buf
+ * handler in driver
+ *
+ * Should be called from vidioc_prepare_buf ioctl handler of a driver.
+ * The passed buffer should have been verified.
+ * This function calls buf_prepare callback in the driver (if provided),
+ * in which driver-specific buffer initialization can be performed,
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_prepare_buf handler in driver.
+ */
+int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
+{
+ struct vb2_buffer *vb;
+ int ret;
+
+ vb = q->bufs[index];
+ if (vb->state != VB2_BUF_STATE_DEQUEUED) {
+ dprintk(1, "invalid buffer state %d\n",
+ vb->state);
+ return -EINVAL;
+ }
+
+ ret = __buf_prepare(vb, pb);
+ if (!ret) {
+ /* Fill buffer information for the userspace */
+ call_bufop(q, fill_user_buffer, vb, pb);
+
+ dprintk(1, "prepare of buffer %d succeeded\n", vb->index);
+ }
+ return ret;
}
+EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
/**
* vb2_start_streaming() - Attempt to start streaming.
Regards,
Hans
--
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
