Re: [PATCH v3 1/2] v4l: Add memory-to-memory device helper framework for videobuf.
Here is my review...
On Monday 29 March 2010 09:36:46 Pawel Osciak wrote:
A mem-to-mem device is a device that uses memory buffers passed by
userspace applications for both their source and destination data. This
is different from existing drivers, which utilize memory buffers for either
input or output, but not both.
In terms of V4L2 such a device would be both of OUTPUT and CAPTURE type.
Examples of such devices would be: image 'resizers', 'rotators',
'colorspace converters', etc.
This patch adds a separate Kconfig sub-menu for mem-to-mem devices as well.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Reviewed-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/media/video/Kconfig| 14 +
drivers/media/video/Makefile |2 +
drivers/media/video/v4l2-mem2mem.c | 685
include/media/v4l2-mem2mem.h | 154
4 files changed, 855 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/v4l2-mem2mem.c
create mode 100644 include/media/v4l2-mem2mem.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index f8fc865..5fd041e 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -45,6 +45,10 @@ config VIDEO_TUNER
tristate
depends on MEDIA_TUNER
+config V4L2_MEM2MEM_DEV
+ tristate
+ depends on VIDEOBUF_GEN
+
#
# Multimedia Video device configuration
#
@@ -1107,3 +,13 @@ config USB_S2255
endif # V4L_USB_DRIVERS
endif # VIDEO_CAPTURE_DRIVERS
+
+menuconfig V4L_MEM2MEM_DRIVERS
+ bool Memory-to-memory multimedia devices
+ depends on VIDEO_V4L2
+ default n
+ ---help---
+ Say Y here to enable selecting drivers for V4L devices that
+ use system memory for both source and destination buffers, as opposed
+ to capture and output drivers, which use memory buffers for just
+ one of those.
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index b88b617..e974680 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -117,6 +117,8 @@ obj-$(CONFIG_VIDEOBUF_VMALLOC) += videobuf-vmalloc.o
obj-$(CONFIG_VIDEOBUF_DVB) += videobuf-dvb.o
obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
+obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
+
obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
obj-$(CONFIG_VIDEO_CX2341X) += cx2341x.o
diff --git a/drivers/media/video/v4l2-mem2mem.c
b/drivers/media/video/v4l2-mem2mem.c
new file mode 100644
index 000..a78157f
--- /dev/null
+++ b/drivers/media/video/v4l2-mem2mem.c
@@ -0,0 +1,685 @@
+/*
+ * Memory-to-memory device framework for Video for Linux 2 and videobuf.
+ *
+ * Helper functions for devices that use videobuf buffers for both their
+ * source and destination.
+ *
+ * Copyright (c) 2009-2010 Samsung Electronics Co., Ltd.
+ * Pawel Osciak, p.osc...@samsung.com
+ * Marek Szyprowski, m.szyprow...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include linux/module.h
+#include linux/sched.h
+#include media/videobuf-core.h
+#include media/v4l2-mem2mem.h
+
+MODULE_DESCRIPTION(Mem to mem device framework for videobuf);
+MODULE_AUTHOR(Pawel Osciak, p.osc...@samsung.com);
+MODULE_LICENSE(GPL);
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define dprintk(fmt, arg...) \
+ do {\
+ if (debug = 1) \
+ printk(KERN_DEBUG %s: fmt, __func__, ## arg);\
+ } while (0)
+
+
+/* Instance is already queued on the jobqueue */
+#define TRANS_QUEUED (1 0)
+/* Instance is currently running in hardware */
+#define TRANS_RUNNING(1 1)
+
+
+/* Offset base for buffers on the destination queue - used to distinguish
+ * between source and destination buffers when mmapping - they receive the
same
+ * offsets but for different queues */
+#define DST_QUEUE_OFF_BASE (1 30)
+
+
+/**
+ * struct v4l2_m2m_dev - per-device context
+ * @curr_ctx:currently running instance
+ * @jobqueue:instances queued to run
+ * @job_spinlock:protects jobqueue
+ * @m2m_ops: driver callbacks
+ */
+struct v4l2_m2m_dev {
+ struct v4l2_m2m_ctx *curr_ctx;
+
+ struct list_headjobqueue;
Using job_queue is a bit more consistent as you also use an '_' in job_spinlock.
+ spinlock_t job_spinlock;
+
+ struct v4l2_m2m_ops *m2m_ops;
+};
+
+static struct v4l2_m2m_queue_ctx
RE: [PATCH v3 1/2] v4l: Add memory-to-memory device helper framework for videobuf.
-Original Message-
From: Hans Verkuil [mailto:hverk...@xs4all.nl]
Sent: Thursday, April 01, 2010 2:37 PM
To: Pawel Osciak
Cc: linux-media@vger.kernel.org; m.szyprow...@samsung.com;
kyungmin.p...@samsung.com; Hiremath, Vaibhav
Subject: Re: [PATCH v3 1/2] v4l: Add memory-to-memory device helper
framework for videobuf.
Here is my review...
[Hiremath, Vaibhav] Pawel,
Some of the Hans's comments have already been addressed in my cleanup patch, so
please make note of it.
Thanks,
Vaibhav
On Monday 29 March 2010 09:36:46 Pawel Osciak wrote:
A mem-to-mem device is a device that uses memory buffers passed by
userspace applications for both their source and destination data. This
is different from existing drivers, which utilize memory buffers for
either
input or output, but not both.
In terms of V4L2 such a device would be both of OUTPUT and CAPTURE type.
Examples of such devices would be: image 'resizers', 'rotators',
'colorspace converters', etc.
This patch adds a separate Kconfig sub-menu for mem-to-mem devices as
well.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Reviewed-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/media/video/Kconfig| 14 +
drivers/media/video/Makefile |2 +
drivers/media/video/v4l2-mem2mem.c | 685
include/media/v4l2-mem2mem.h | 154
4 files changed, 855 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/v4l2-mem2mem.c
create mode 100644 include/media/v4l2-mem2mem.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index f8fc865..5fd041e 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -45,6 +45,10 @@ config VIDEO_TUNER
tristate
depends on MEDIA_TUNER
+config V4L2_MEM2MEM_DEV
+ tristate
+ depends on VIDEOBUF_GEN
+
#
# Multimedia Video device configuration
#
@@ -1107,3 +,13 @@ config USB_S2255
endif # V4L_USB_DRIVERS
endif # VIDEO_CAPTURE_DRIVERS
+
+menuconfig V4L_MEM2MEM_DRIVERS
+ bool Memory-to-memory multimedia devices
+ depends on VIDEO_V4L2
+ default n
+ ---help---
+ Say Y here to enable selecting drivers for V4L devices that
+ use system memory for both source and destination buffers, as
opposed
+ to capture and output drivers, which use memory buffers for just
+ one of those.
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index b88b617..e974680 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -117,6 +117,8 @@ obj-$(CONFIG_VIDEOBUF_VMALLOC) += videobuf-vmalloc.o
obj-$(CONFIG_VIDEOBUF_DVB) += videobuf-dvb.o
obj-$(CONFIG_VIDEO_BTCX) += btcx-risc.o
+obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
+
obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
obj-$(CONFIG_VIDEO_CX2341X) += cx2341x.o
diff --git a/drivers/media/video/v4l2-mem2mem.c
b/drivers/media/video/v4l2-mem2mem.c
new file mode 100644
index 000..a78157f
--- /dev/null
+++ b/drivers/media/video/v4l2-mem2mem.c
@@ -0,0 +1,685 @@
+/*
+ * Memory-to-memory device framework for Video for Linux 2 and videobuf.
+ *
+ * Helper functions for devices that use videobuf buffers for both their
+ * source and destination.
+ *
+ * Copyright (c) 2009-2010 Samsung Electronics Co., Ltd.
+ * Pawel Osciak, p.osc...@samsung.com
+ * Marek Szyprowski, m.szyprow...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include linux/module.h
+#include linux/sched.h
+#include media/videobuf-core.h
+#include media/v4l2-mem2mem.h
+
+MODULE_DESCRIPTION(Mem to mem device framework for videobuf);
+MODULE_AUTHOR(Pawel Osciak, p.osc...@samsung.com);
+MODULE_LICENSE(GPL);
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define dprintk(fmt, arg...)
\
+ do {\
+ if (debug = 1) \
+ printk(KERN_DEBUG %s: fmt, __func__, ## arg);\
+ } while (0)
+
+
+/* Instance is already queued on the jobqueue */
+#define TRANS_QUEUED (1 0)
+/* Instance is currently running in hardware */
+#define TRANS_RUNNING (1 1)
+
+
+/* Offset base for buffers on the destination queue - used to distinguish
+ * between source and destination buffers when mmapping - they receive
the same
+ * offsets but for different queues */
+#define DST_QUEUE_OFF_BASE (1 30)
+
+
+/**
+ * struct
RE: [PATCH v3 1/2] v4l: Add memory-to-memory device helper framework for videobuf.
Hi Hans,
thank you for the review. My comments below.
Hans Verkuil [mailto:hverk...@xs4all.nl] wrote:
Here is my review...
[...]
+/**
+ * v4l2_m2m_next_src_buf() - return next source buffer from the list of
ready
+ * buffers
+ */
+inline void *v4l2_m2m_next_src_buf(struct v4l2_m2m_ctx *m2m_ctx)
inline makes no sense for an exported function.
You can also move this to the header and make it static inline. That would be
a valid option for a lot of these small one-liner functions.
I must have missed that, thanks.
[...]
+static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+{
+struct v4l2_m2m_dev *m2m_dev;
+unsigned long flags_job, flags;
+
+m2m_dev = m2m_ctx-m2m_dev;
+dprintk(Trying to schedule a job for m2m_ctx: %p\n, m2m_ctx);
+
+if (!m2m_ctx-out_q_ctx.q.streaming
+|| !m2m_ctx-cap_q_ctx.q.streaming) {
+dprintk(Streaming needs to be on for both queues\n);
+return;
+}
+
+spin_lock_irqsave(m2m_dev-job_spinlock, flags_job);
+if (m2m_ctx-job_flags TRANS_QUEUED) {
+spin_unlock_irqrestore(m2m_dev-job_spinlock, flags_job);
+dprintk(On job queue already\n);
+return;
+}
+
+spin_lock_irqsave(m2m_ctx-out_q_ctx.q.irqlock, flags);
Two spin_lock_irqsave's? I think it will work, but it is very unusual.
Does job_spinlock really have to be held with interrupts turned off?
Yeah, it is unusual. But a driver may need to access current context from
its interrupt handler. That requires locking the job queue.
The insertion of a new instance to the job queue requires verification of all
those conditions. Hence the spinlocks.
[...]
+unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
+ struct poll_table_struct *wait)
+{
+struct videobuf_queue *dst_q;
+struct videobuf_buffer *vb = NULL;
+unsigned int rc = 0;
+
+dst_q = v4l2_m2m_get_dst_vq(m2m_ctx);
+
+mutex_lock(dst_q-vb_lock);
+
+if (dst_q-streaming) {
+if (!list_empty(dst_q-stream))
+vb = list_entry(dst_q-stream.next,
+struct videobuf_buffer, stream);
+}
+
+if (!vb)
+rc = POLLERR;
+
+if (0 == rc) {
+poll_wait(file, vb-done, wait);
+if (vb-state == VIDEOBUF_DONE || vb-state == VIDEOBUF_ERROR)
+rc = POLLOUT | POLLRDNORM;
+}
+
+mutex_unlock(dst_q-vb_lock);
Would there be any need for polling for writing?
Something has to be chosen, we could be polling for both of course, but in
my opinion in case of m2m devices we are usually interested in the result
of the operation. And in a great majority of cases (1:1 src:dst buffers) this
will also mean src buffer is ready as well. Besides, the app can always simply
sleep on dqbuf in one thread.
+struct v4l2_m2m_dev *v4l2_m2m_init(struct v4l2_m2m_ops *m2m_ops)
+{
+struct v4l2_m2m_dev *m2m_dev;
+
+if (!m2m_ops)
+return ERR_PTR(-EINVAL);
+
+BUG_ON(!m2m_ops-device_run);
+BUG_ON(!m2m_ops-job_abort);
No need to crash. Use WARN_ON and return an error.
Learned something today: I didn't know ERR_PTR existed! Nice.
Those BUG_ONs were actually inspired by similar code in videobuf-core ;)
+m2m_dev = kzalloc(sizeof *m2m_dev, GFP_KERNEL);
+if (!m2m_dev)
+return ERR_PTR(-ENOMEM);
+
+m2m_dev-curr_ctx = NULL;
+m2m_dev-m2m_ops = m2m_ops;
+INIT_LIST_HEAD(m2m_dev-jobqueue);
+spin_lock_init(m2m_dev-job_spinlock);
+
+return m2m_dev;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_init);
+
+/**
+ * v4l2_m2m_release() - cleans up and frees a m2m_dev structure
+ *
+ * Usually called from driver's remove() function.
+ */
+void v4l2_m2m_release(struct v4l2_m2m_dev *m2m_dev)
+{
+kfree(m2m_dev);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_release);
Wouldn't it make sense to embed this struct in a filehandle structure?
Then there is no need to allocate anything, you just need an init function.
I like embedding structures: it's quite clean.
This was actually my initial design, but I thought that moving as much as
possible
out of drivers will simplify them. That was my main target when writing this
framework... But if people prefer it embedded, we can try to move it back.
Although - as I said - I wanted to simplify drivers as much as possible.
[...]
Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 1/2] v4l: Add memory-to-memory device helper framework for videobuf.
Pawel Osciak [mailto:p.osc...@samsung.com] wrote:
+unsigned int v4l2_m2m_poll(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
+ struct poll_table_struct *wait)
+{
+ struct videobuf_queue *dst_q;
+ struct videobuf_buffer *vb = NULL;
+ unsigned int rc = 0;
+
+ dst_q = v4l2_m2m_get_dst_vq(m2m_ctx);
+
+ mutex_lock(dst_q-vb_lock);
+
+ if (dst_q-streaming) {
+ if (!list_empty(dst_q-stream))
+ vb = list_entry(dst_q-stream.next,
+ struct videobuf_buffer, stream);
+ }
+
+ if (!vb)
+ rc = POLLERR;
+
+ if (0 == rc) {
+ poll_wait(file, vb-done, wait);
+ if (vb-state == VIDEOBUF_DONE || vb-state == VIDEOBUF_ERROR)
+ rc = POLLOUT | POLLRDNORM;
+ }
+
+ mutex_unlock(dst_q-vb_lock);
Would there be any need for polling for writing?
Something has to be chosen, we could be polling for both of course, but in
my opinion in case of m2m devices we are usually interested in the result
of the operation. And in a great majority of cases (1:1 src:dst buffers) this
will also mean src buffer is ready as well. Besides, the app can always simply
sleep on dqbuf in one thread.
On second thought, we could set both and let the application use it as it sees
fit...
Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 1/2] v4l: Add memory-to-memory device helper framework for videobuf.
Andy Walls [mailto:awa...@md.metrocast.net] wrote:
I didn't do a full review (I have no time lately), but I noticed this:
+static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+{
+struct v4l2_m2m_dev *m2m_dev;
[...]
+v4l2_m2m_try_run(m2m_dev);
+}
[...]
+void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
+ struct v4l2_m2m_ctx *m2m_ctx)
+{
[...]
+ v4l2_m2m_try_schedule(m2m_ctx);
+v4l2_m2m_try_run(m2m_dev);
+}
I assume it is not bad, but was it your intention to have an addtitonal
call to v4l2_m2m_try_run() ?
Thanks for noticing that, but yes, this is intentional. Simplifies the code
a bit and will work properly.
Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] v4l: Add memory-to-memory device helper framework for videobuf.
On Mon, 2010-03-29 at 09:36 +0200, Pawel Osciak wrote:
A mem-to-mem device is a device that uses memory buffers passed by
userspace applications for both their source and destination data. This
is different from existing drivers, which utilize memory buffers for either
input or output, but not both.
In terms of V4L2 such a device would be both of OUTPUT and CAPTURE type.
Examples of such devices would be: image 'resizers', 'rotators',
'colorspace converters', etc.
This patch adds a separate Kconfig sub-menu for mem-to-mem devices as well.
Signed-off-by: Pawel Osciak p.osc...@samsung.com
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Reviewed-by: Kyungmin Park kyungmin.p...@samsung.com
---
Pawel,
I didn't do a full review (I have no time lately), but I noticed this:
+static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
+{
+ struct v4l2_m2m_dev *m2m_dev;
[...]
+ v4l2_m2m_try_run(m2m_dev);
+}
[...]
+void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
+ struct v4l2_m2m_ctx *m2m_ctx)
+{
[...]
+ v4l2_m2m_try_schedule(m2m_ctx);
+ v4l2_m2m_try_run(m2m_dev);
+}
I assume it is not bad, but was it your intention to have an addtitonal
call to v4l2_m2m_try_run() ?
Regards,
Andy
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
