On 09/22/2015 11:48 PM, Hans Verkuil wrote:
Hi Junghak,
This looks pretty good!
I have a few small comments below, but overall it is much improved.
On 22-09-15 15:30, Junghak Sung wrote:
Prepare to divide videobuf2
- Separate vb2 trace events from v4l2 trace event.
- Make wrapper functions that will move to v4l2-side
- Make vb2_core_* functions that remain in vb2 core side
- Rename internal functions as vb2_*
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/Makefile | 2 +-
drivers/media/v4l2-core/v4l2-trace.c | 8 +-
drivers/media/v4l2-core/vb2-trace.c | 9 +
drivers/media/v4l2-core/videobuf2-core.c | 556 ++++++++++++++++++++----------
include/trace/events/v4l2.h | 28 +-
include/trace/events/vb2.h | 65 ++++
6 files changed, 457 insertions(+), 211 deletions(-)
create mode 100644 drivers/media/v4l2-core/vb2-trace.c
create mode 100644 include/trace/events/vb2.h
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index ad07401..1dc8bba 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -14,7 +14,7 @@ ifeq ($(CONFIG_OF),y)
videodev-objs += v4l2-of.o
endif
ifeq ($(CONFIG_TRACEPOINTS),y)
- videodev-objs += v4l2-trace.o
+ videodev-objs += vb2-trace.o v4l2-trace.o
endif
obj-$(CONFIG_VIDEO_V4L2) += videodev.o
diff --git a/drivers/media/v4l2-core/v4l2-trace.c b/drivers/media/v4l2-core/v4l2-trace.c
index 4004814..7416010 100644
--- a/drivers/media/v4l2-core/v4l2-trace.c
+++ b/drivers/media/v4l2-core/v4l2-trace.c
@@ -5,7 +5,7 @@
#define CREATE_TRACE_POINTS
#include <trace/events/v4l2.h>
-EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_buf_done);
-EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_buf_queue);
-EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_dqbuf);
-EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_qbuf);
+EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_v4l2_buf_done);
+EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_v4l2_buf_queue);
+EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_v4l2_dqbuf);
+EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_v4l2_qbuf);
diff --git a/drivers/media/v4l2-core/vb2-trace.c b/drivers/media/v4l2-core/vb2-trace.c
new file mode 100644
index 0000000..61e74f5
--- /dev/null
+++ b/drivers/media/v4l2-core/vb2-trace.c
@@ -0,0 +1,9 @@
+#include <media/videobuf2-core.h>
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/vb2.h>
+
+EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_buf_done);
+EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_buf_queue);
+EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_dqbuf);
+EXPORT_TRACEPOINT_SYMBOL_GPL(vb2_qbuf);
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 32fa425..380536d 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -30,7 +30,7 @@
#include <media/v4l2-common.h>
#include <media/videobuf2-v4l2.h>
-#include <trace/events/v4l2.h>
+#include <trace/events/vb2.h>
static int debug;
module_param(debug, int, 0644);
@@ -612,10 +612,10 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
}
/**
- * __buffer_in_use() - return true if the buffer is in use and
+ * vb2_buffer_in_use() - return true if the buffer is in use and
* the queue cannot be freed (by the means of REQBUFS(0)) call
*/
-static bool __buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
+static bool vb2_buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
{
unsigned int plane;
for (plane = 0; plane < vb->num_planes; ++plane) {
@@ -640,7 +640,7 @@ 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;
@@ -650,8 +650,9 @@ static bool __buffers_in_use(struct vb2_queue *q)
* __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
* returned to userspace
*/
-static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
+static int __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
Why use a void * here? Wouldn't a struct vb2_buffer pointer be better? That way it all
remains type-safe. Ditto elsewhere in this patch, of course.
I disagree with this idea, IMHO.
This function is for filling struct v4l2_buffer with information to be
returned to userspace. So, if the void pointer are replaced with
struct vb2_buffer pointer, a additional function will be needed in order
to translate the vb2_buffer to v4l2_buffer and the function should be
duplicated with __fill_v4l2_buffer() by meaning of functionality.
{
+ struct v4l2_buffer *b = pb;
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
struct vb2_queue *q = vb->vb2_queue;
unsigned int plane;
@@ -742,8 +743,28 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
break;
}
- if (__buffer_in_use(q, vb))
+ if (vb2_buffer_in_use(q, vb))
b->flags |= V4L2_BUF_FLAG_MAPPED;
+
+ return 0;
+}
+
+/**
+ * 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.
+ */
+static int vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
+{
+ return __fill_v4l2_buffer(q->bufs[index], pb);
}
/**
@@ -775,11 +796,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
}
vb = q->bufs[b->index];
ret = __verify_planes_array(vb, b);
- if (!ret)
- __fill_v4l2_buffer(vb, b);
- return ret;
+
+ return ret ? ret : vb2_core_querybuf(q, b->index, b);
}
-EXPORT_SYMBOL(vb2_querybuf);
+EXPORT_SYMBOL_GPL(vb2_querybuf);
I prefer that such license changes are done in a separate patch. Now it is hidden
in a large patch, and others might want to jump in whether or not this is a
good thing.
OK. I will revert this change.
/**
* __verify_userptr_ops() - verify that all memory operations required for
<snip>
@@ -2121,7 +2211,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
* Will sleep if required for nonblocking == false.
*/
static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb,
- struct v4l2_buffer *b, int nonblocking)
+ int nonblocking)
{
unsigned long flags;
int ret;
@@ -2142,10 +2232,10 @@ static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb,
/*
* Only remove the buffer from done_list if v4l2_buffer can handle all
* the planes.
+ * Verifing planes is NOT necessary since it aleady has been checked
s/Verifing/Verifying/
s/aleady/already/
+ * before the buffer is queued/prepared. So it can never fails
s/fails/fail./
Oh, ridiculous mistakes. I will correct those typos.
Regards,
Junghak
*/
- ret = __verify_planes_array(*vb, b);
- if (!ret)
- list_del(&(*vb)->done_entry);
+ list_del(&(*vb)->done_entry);
spin_unlock_irqrestore(&q->done_lock, flags);
return ret;
<snip>
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
