Re: [PATCH] s2255drv: removal of big kernel lock
On Wednesday 31 March 2010 16:41:51 Dean A. wrote: # HG changeset patch # User Dean Anderson d...@sensoray.com # Date 1270046291 25200 # Node ID c72bdc8732abc0cf7bc376babfd06b2d999bdcf4 # Parent 2ab296deae938864b06b29cc224eb4b670ae3aa9 s2255drv: removal of BKL From: Dean Anderson d...@sensoray.com big kernel lock removed from open function. v4l2 code does not require locking the open function except to check asynchronous firmware load state, which is protected by a mutex Can you do the same for the go7007 driver? And for both drivers you can also switch to using unlocked_ioctl. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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 0/2] Mem-to-mem device framework
-Original Message- From: Pawel Osciak [mailto:p.osc...@samsung.com] Sent: Monday, March 29, 2010 1:07 PM To: linux-media@vger.kernel.org Cc: p.osc...@samsung.com; m.szyprow...@samsung.com; kyungmin.p...@samsung.com; Hiremath, Vaibhav Subject: [PATCH v3 0/2] Mem-to-mem device framework Hello, this is the third version of the mem-to-mem memory device framework. It addresses previous comments and issues raised in Norway as well. It is rather independent from videobuf so I believe it can be merged separately. Changes in v3: - streamon, streamoff now have to be called for both queues separately - added automatic rescheduling of an instance after finish (if ready) - tweaked up locking - addressed Andy Walls' comments We have been using v2 for three different devices on an embedded system. I did some additional testing of v3 on a 4-core SMP as well. The series contains: [PATCH v3 1/2] v4l: Add memory-to-memory device helper framework for videobuf. [PATCH v3 2/2] v4l: Add a mem-to-mem videobuf framework test device. [Hiremath, Vaibhav] I have reviewed the changes and also tested it here at my end, even I have tested it with real hardware module (OMAP3 Resizer driver) so I think we can merge these patches now. I have cleanup patch (Submitting shortly), I just changed while reviewing/testing the code. So you can directly merge the patch into your next version. Also it would be really great if we could add documentation for this. You can also add, Reviewed-by: Hiremath Vaibhav hvaib...@ti.com Tested-by: Hiremath Vaibhav hvaib...@ti.com Thanks, Vaibhav 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
[PATCH 1/2] v4l2-mem2mem: Code cleanup
From: Vaibhav Hiremath hvaib...@ti.com
Signed-off-by: Vaibhav Hiremath hvaib...@ti.com
---
drivers/media/video/v4l2-mem2mem.c | 40 ++-
1 files changed, 16 insertions(+), 24 deletions(-)
diff --git a/drivers/media/video/v4l2-mem2mem.c
b/drivers/media/video/v4l2-mem2mem.c
index a78157f..4cd79ba 100644
--- a/drivers/media/video/v4l2-mem2mem.c
+++ b/drivers/media/video/v4l2-mem2mem.c
@@ -23,12 +23,12 @@ 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);
+static bool debug;
+module_param(debug, bool, 0644);
#define dprintk(fmt, arg...) \
do {\
- if (debug = 1) \
+ if (debug) \
printk(KERN_DEBUG %s: fmt, __func__, ## arg);\
} while (0)
@@ -215,12 +215,10 @@ EXPORT_SYMBOL(v4l2_m2m_dst_buf_remove);
void *v4l2_m2m_get_curr_priv(struct v4l2_m2m_dev *m2m_dev)
{
unsigned long flags;
- void *ret;
+ void *ret = NULL;
spin_lock_irqsave(m2m_dev-job_spinlock, flags);
- if (!m2m_dev-curr_ctx)
- ret = NULL;
- else
+ if (m2m_dev-curr_ctx)
ret = m2m_dev-curr_ctx-priv;
spin_unlock_irqrestore(m2m_dev-job_spinlock, flags);
@@ -319,10 +317,9 @@ static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx
*m2m_ctx)
return;
}
- if (!(m2m_ctx-job_flags TRANS_QUEUED)) {
- list_add_tail(m2m_ctx-queue, m2m_dev-jobqueue);
- m2m_ctx-job_flags |= TRANS_QUEUED;
- }
+ list_add_tail(m2m_ctx-queue, m2m_dev-jobqueue);
+ m2m_ctx-job_flags |= TRANS_QUEUED;
+
spin_unlock_irqrestore(m2m_dev-job_spinlock, flags_job);
v4l2_m2m_try_run(m2m_dev);
@@ -414,12 +411,10 @@ int v4l2_m2m_qbuf(struct file *file, struct v4l2_m2m_ctx
*m2m_ctx,
vq = v4l2_m2m_get_vq(m2m_ctx, buf-type);
ret = videobuf_qbuf(vq, buf);
- if (ret)
- return ret;
-
- v4l2_m2m_try_schedule(m2m_ctx);
+ if (!ret)
+ v4l2_m2m_try_schedule(m2m_ctx);
- return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(v4l2_m2m_qbuf);
@@ -448,12 +443,10 @@ int v4l2_m2m_streamon(struct file *file, struct
v4l2_m2m_ctx *m2m_ctx,
vq = v4l2_m2m_get_vq(m2m_ctx, type);
ret = videobuf_streamon(vq);
- if (ret)
- return ret;
-
- v4l2_m2m_try_schedule(m2m_ctx);
+ if (!ret)
+ v4l2_m2m_try_schedule(m2m_ctx);
- return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(v4l2_m2m_streamon);
@@ -587,8 +580,7 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(void *priv, struct
v4l2_m2m_dev *m2m_dev,
enum v4l2_buf_type))
{
struct v4l2_m2m_ctx *m2m_ctx;
- struct v4l2_m2m_queue_ctx *out_q_ctx;
- struct v4l2_m2m_queue_ctx *cap_q_ctx;
+ struct v4l2_m2m_queue_ctx *out_q_ctx, *cap_q_ctx;
if (!vq_init)
return ERR_PTR(-EINVAL);
@@ -662,7 +654,7 @@ EXPORT_SYMBOL_GPL(v4l2_m2m_ctx_release);
/**
* v4l2_m2m_buf_queue() - add a buffer to the proper ready buffers list.
*
- * Call from withing buf_queue() videobuf_queue_ops callback.
+ * Call from buf_queue(), videobuf_queue_ops callback.
*
* Locking: Caller holds q-irqlock (taken by videobuf before calling buf_queue
* callback in the driver).
--
1.6.2.4
--
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
[PATCH 2/2] mem2mem_testdev: Code cleanup
From: Vaibhav Hiremath hvaib...@ti.com
Signed-off-by: Vaibhav Hiremath hvaib...@ti.com
---
drivers/media/video/mem2mem_testdev.c | 58 ++--
1 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/drivers/media/video/mem2mem_testdev.c
b/drivers/media/video/mem2mem_testdev.c
index 05630e3..1f35b7e 100644
--- a/drivers/media/video/mem2mem_testdev.c
+++ b/drivers/media/video/mem2mem_testdev.c
@@ -98,11 +98,10 @@ static struct m2mtest_fmt formats[] = {
};
/* Per-queue, driver-specific private data */
-struct m2mtest_q_data
-{
- unsigned intwidth;
- unsigned intheight;
- unsigned intsizeimage;
+struct m2mtest_q_data {
+ u32 width;
+ u32 height;
+ u32 sizeimage;
struct m2mtest_fmt *fmt;
};
@@ -123,11 +122,10 @@ static struct m2mtest_q_data *get_q_data(enum
v4l2_buf_type type)
return q_data[V4L2_M2M_DST];
default:
BUG();
- return NULL;
}
+ return NULL;
}
-
#define V4L2_CID_TRANS_TIME_MSEC V4L2_CID_PRIVATE_BASE
#define V4L2_CID_TRANS_NUM_BUFS(V4L2_CID_PRIVATE_BASE + 1)
@@ -158,7 +156,7 @@ static struct v4l2_queryctrl m2mtest_ctrls[] = {
static struct m2mtest_fmt *find_format(struct v4l2_format *f)
{
struct m2mtest_fmt *fmt;
- unsigned int k;
+ u32 k;
for (k = 0; k NUM_FORMATS; k++) {
fmt = formats[k];
@@ -237,12 +235,12 @@ static int device_process(struct m2mtest_ctx *ctx,
if (!p_in || !p_out) {
v4l2_err(dev-v4l2_dev,
Acquiring kernel pointers to buffers failed\n);
- return 1;
+ return -EFAULT;
}
if (in_buf-vb.size out_buf-vb.size) {
v4l2_err(dev-v4l2_dev, Output buffer is too small\n);
- return 1;
+ return -EINVAL;
}
tile_w = (in_buf-vb.width * (q_data[V4L2_M2M_DST].fmt-depth 3))
@@ -361,8 +359,6 @@ static void device_isr(unsigned long priv)
spin_unlock_irqrestore(m2mtest_dev-irqlock, flags);
device_run(curr_ctx);
}
-
- return;
}
@@ -384,10 +380,7 @@ static int vidioc_querycap(struct file *file, void *priv,
static int enum_fmt(struct v4l2_fmtdesc *f, u32 type)
{
- int i, num;
- struct m2mtest_fmt *fmt;
-
- num = 0;
+ int i, num = 0;
for (i = 0; i NUM_FORMATS; ++i) {
if (formats[i].types type) {
@@ -402,9 +395,9 @@ static int enum_fmt(struct v4l2_fmtdesc *f, u32 type)
if (i NUM_FORMATS) {
/* Format found */
- fmt = formats[i];
- strncpy(f-description, fmt-name, sizeof(f-description) - 1);
- f-pixelformat = fmt-fourcc;
+ strncpy(f-description, formats[i].name,
+ sizeof(f-description) - 1);
+ f-pixelformat = formats[i].fourcc;
return 0;
}
@@ -430,6 +423,9 @@ static int vidioc_g_fmt(struct m2mtest_ctx *ctx, struct
v4l2_format *f)
struct m2mtest_q_data *q_data;
vq = v4l2_m2m_get_vq(ctx-m2m_ctx, f-type);
+ if (!vq)
+ return -EINVAL;
+
q_data = get_q_data(f-type);
f-fmt.pix.width= q_data-width;
@@ -524,9 +520,10 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct
v4l2_format *f)
{
struct m2mtest_q_data *q_data;
struct videobuf_queue *vq;
- int ret = 0;
vq = v4l2_m2m_get_vq(ctx-m2m_ctx, f-type);
+ if (!vq)
+ return -EINVAL;
q_data = get_q_data(f-type);
if (!q_data)
return -EINVAL;
@@ -535,8 +532,8 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct
v4l2_format *f)
if (videobuf_queue_is_busy(vq)) {
v4l2_err(ctx-dev-v4l2_dev, %s queue busy\n, __func__);
- ret = -EBUSY;
- goto out;
+ mutex_unlock(vq-vb_lock);
+ return -EBUSY;
}
q_data-fmt = find_format(f);
@@ -550,9 +547,7 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct
v4l2_format *f)
Setting format for type %d, wxh: %dx%d, fmt: %d\n,
f-type, q_data-width, q_data-height, q_data-fmt-fourcc);
-out:
- mutex_unlock(vq-vb_lock);
- return ret;
+ return 0;
}
static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
@@ -857,13 +852,9 @@ static int m2mtest_open(struct file *file)
struct m2mtest_dev *dev = video_drvdata(file);
struct m2mtest_ctx *ctx = NULL;
- atomic_inc(dev-num_inst);
-
ctx = kzalloc(sizeof *ctx, GFP_KERNEL);
- if (!ctx) {
- atomic_dec(dev-num_inst);
+ if (!ctx)
return -ENOMEM;
- }
Re: [PATCH 12/24] media/video: fix dangling pointers
On Tuesday 30 March 2010 14:39:12 Wolfram Sang wrote: Hans, But this just feels like an i2c core thing to me. After remove() is called the core should just set the client data to NULL. If there are drivers that rely on the current behavior, then those drivers should be reviewed first as to the reason why they need it. It will be done this way now. As you have taken part in the discussion, I guess the media-subsystem never really considered picking those patches up ;) I remember that there was one patch in your patch series where the client data was set after it was freed. That should still be fixed (by just removing the i2c_set_clientdata). Can you post that one again? Regards, Hans Regards, Wolfram -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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.
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: V4L-DVB drivers and BKL
Hi Hans,
On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:
Hi all,
I just read on LWN that the core kernel guys are putting more effort into
removing the BKL. We are still using it in our own drivers, mostly V4L.
I added a BKL column to my driver list:
http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers
If you 'own' one of these drivers that still use BKL, then it would be nice
if you can try and remove the use of the BKL from those drivers.
The other part that needs to be done is to move from using the .ioctl file
op to using .unlocked_ioctl. Very few drivers do that, but I suspect
almost no driver actually needs to use .ioctl.
What about something like this patch as a first step ?
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 7090699..14e1b1c 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -25,6 +25,7 @@
#include linux/init.h
#include linux/kmod.h
#include linux/slab.h
+#include linux/smp_lock.h
#include asm/uaccess.h
#include asm/system.h
@@ -215,28 +216,22 @@ static unsigned int v4l2_poll(struct file *filp, struct
poll_table_struct *poll)
return vdev-fops-poll(filp, poll);
}
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg)
-{
- struct video_device *vdev = video_devdata(filp);
-
- if (!vdev-fops-ioctl)
- return -ENOTTY;
- /* Allow ioctl to continue even if the device was unregistered.
- Things like dequeueing buffers might still be useful. */
- return vdev-fops-ioctl(filp, cmd, arg);
-}
-
static long v4l2_unlocked_ioctl(struct file *filp,
unsigned int cmd, unsigned long arg)
{
struct video_device *vdev = video_devdata(filp);
+ int ret = -ENOTTY;
- if (!vdev-fops-unlocked_ioctl)
- return -ENOTTY;
/* Allow ioctl to continue even if the device was unregistered.
Things like dequeueing buffers might still be useful. */
- return vdev-fops-unlocked_ioctl(filp, cmd, arg);
+ if (vdev-fops-ioctl) {
+ lock_kernel();
+ ret = vdev-fops-ioctl(filp, cmd, arg);
+ unlock_kernel();
+ } else if (vdev-fops-unlocked_ioctl)
+ ret = vdev-fops-unlocked_ioctl(filp, cmd, arg);
+
+ return ret;
}
#ifdef CONFIG_MMU
@@ -323,22 +318,6 @@ static const struct file_operations v4l2_unlocked_fops = {
.llseek = no_llseek,
};
-static const struct file_operations v4l2_fops = {
- .owner = THIS_MODULE,
- .read = v4l2_read,
- .write = v4l2_write,
- .open = v4l2_open,
- .get_unmapped_area = v4l2_get_unmapped_area,
- .mmap = v4l2_mmap,
- .ioctl = v4l2_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = v4l2_compat_ioctl32,
-#endif
- .release = v4l2_release,
- .poll = v4l2_poll,
- .llseek = no_llseek,
-};
-
/**
* get_index - assign stream index number based on parent device
* @vdev: video_device to assign index number to, vdev-parent should be
assigned
@@ -517,10 +496,7 @@ static int __video_register_device(struct video_device
*vdev, int type, int nr,
ret = -ENOMEM;
goto cleanup;
}
- if (vdev-fops-unlocked_ioctl)
- vdev-cdev-ops = v4l2_unlocked_fops;
- else
- vdev-cdev-ops = v4l2_fops;
+ vdev-cdev-ops = v4l2_unlocked_fops;
vdev-cdev-owner = vdev-fops-owner;
ret = cdev_add(vdev-cdev, MKDEV(VIDEO_MAJOR, vdev-minor), 1);
if (ret 0) {
A second step would be to replace lock_kernel/unlock_kernel with a
V4L-specific lock, and the third step to push the lock into drivers.
--
Regards,
Laurent Pinchart
--
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
[GIT PATCHES FOR 2.6.35] Updates for the uvcvideo driver
The following changes since commit 975b06b6c01ba2da4d26a7ba6ea783d5f670aa7d: Jonathan Corbet (1): V4L/DVB: ov7670: silence some compiler warnings are available in the git repository at: git://linuxtv.org/pinchartl/uvcvideo.git uvcvideo Laurent Pinchart (5): uvcvideo: Add support for unbranded Arkmicro 18ec:3290 webcams uvcvideo: Add support for Packard Bell EasyNote MX52 integrated webcam v4l: Add V4L2_CID_IRIS_ABSOLUTE and V4L2_CID_IRIS_RELATIVE controls uvcvideo: Support iris absolute and relative controls uvcvideo: Use POLLOUT and POLLWRNORM for output devices Documentation/DocBook/v4l/compat.xml | 11 +++ Documentation/DocBook/v4l/controls.xml| 19 +++ Documentation/DocBook/v4l/videodev2.h.xml |3 +++ drivers/media/video/uvc/uvc_ctrl.c| 20 drivers/media/video/uvc/uvc_driver.c | 18 ++ drivers/media/video/uvc/uvc_queue.c |8 ++-- drivers/media/video/v4l2-common.c |3 +++ include/linux/videodev2.h |3 +++ 8 files changed, 83 insertions(+), 2 deletions(-) -- Regards, Laurent Pinchart -- 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 1/2] v4l2-mem2mem: Code cleanup
Hi,
thanks for the patch, one comment below:
Vaibhav Hiremath (hvaib...@ti.com) wrote:
Signed-off-by: Vaibhav Hiremath hvaib...@ti.com
---
drivers/media/video/v4l2-mem2mem.c | 40 ++-
1 files changed, 16 insertions(+), 24 deletions(-)
[...]
@@ -319,10 +317,9 @@ static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx
*m2m_ctx)
return;
}
- if (!(m2m_ctx-job_flags TRANS_QUEUED)) {
- list_add_tail(m2m_ctx-queue, m2m_dev-jobqueue);
- m2m_ctx-job_flags |= TRANS_QUEUED;
- }
+ list_add_tail(m2m_ctx-queue, m2m_dev-jobqueue);
+ m2m_ctx-job_flags |= TRANS_QUEUED;
+
spin_unlock_irqrestore(m2m_dev-job_spinlock, flags_job);
v4l2_m2m_try_run(m2m_dev);
Nice catch! This wasn't the case before, but as v3 is now holding the
job_spinlock
for the whole time, the check can be safely removed.
[...]
Acked-by: Pawel Osciak p.osc...@samsung.com
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
[RESEND: PATCH-V6 0/2] OMAP2/3: Add V4L2 display driver support
From: Vaibhav Hiremath hvaib...@ti.com The previous patch submission had a dependancy on ti-media directory patch, and since we were not having any conclusion on that patch, I have created omap directory (device specific name) and moved V4L2 driver (as of now applicable to OMAP2/3 devices) to this new directory so that atleast v4l2 display driver can be unblocked and get merged to main-line. Vaibhav Hiremath (2): OMAP2/3 V4L2: Add support for OMAP2/3 V4L2 driver on top of DSS2 OMAP2/3: Add V4L2 DSS driver support in device.c arch/arm/mach-omap2/devices.c | 28 + drivers/media/video/Kconfig |2 + drivers/media/video/Makefile|2 + drivers/media/video/omap/Kconfig| 11 + drivers/media/video/omap/Makefile |7 + drivers/media/video/omap/omap_vout.c| 2655 +++ drivers/media/video/omap/omap_voutdef.h | 148 ++ drivers/media/video/omap/omap_voutlib.c | 258 +++ drivers/media/video/omap/omap_voutlib.h | 34 + 9 files changed, 3145 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/omap/Kconfig create mode 100644 drivers/media/video/omap/Makefile create mode 100644 drivers/media/video/omap/omap_vout.c create mode 100644 drivers/media/video/omap/omap_voutdef.h create mode 100644 drivers/media/video/omap/omap_voutlib.c create mode 100644 drivers/media/video/omap/omap_voutlib.h -- 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
[PATCH 2/2] OMAP2/3: Add V4L2 DSS driver support in device.c
From: Vaibhav Hiremath hvaib...@ti.com
Signed-off-by: Vaibhav Hiremath hvaib...@ti.com
---
arch/arm/mach-omap2/devices.c | 28
1 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 18ad931..7aaffe7 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -763,6 +763,33 @@ static inline void omap_hdq_init(void)
static inline void omap_hdq_init(void) {}
#endif
+/*---*/
+
+#if defined(CONFIG_VIDEO_OMAP2_VOUT) || \
+ defined(CONFIG_VIDEO_OMAP2_VOUT_MODULE)
+#if defined(CONFIG_FB_OMAP2) || defined(CONFIG_FB_OMAP2_MODULE)
+static struct resource omap_vout_resource[3 - CONFIG_FB_OMAP2_NUM_FBS] = {
+};
+#else
+static struct resource omap_vout_resource[2] = {
+};
+#endif
+
+static struct platform_device omap_vout_device = {
+ .name = omap_vout,
+ .num_resources = ARRAY_SIZE(omap_vout_resource),
+ .resource = omap_vout_resource[0],
+ .id = -1,
+};
+static void omap_init_vout(void)
+{
+ if (platform_device_register(omap_vout_device) 0)
+ printk(KERN_ERR Unable to register OMAP-VOUT device\n);
+}
+#else
+static inline void omap_init_vout(void) {}
+#endif
+
/*-*/
static int __init omap2_init_devices(void)
@@ -777,6 +804,7 @@ static int __init omap2_init_devices(void)
omap_hdq_init();
omap_init_sti();
omap_init_sha1_md5();
+ omap_init_vout();
return 0;
}
--
1.6.2.4
--
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 2/2] mem2mem_testdev: Code cleanup
Hi again,
Vaibhav Hiremath hvaib...@ti.com wrote:
From: Vaibhav Hiremath hvaib...@ti.com
Signed-off-by: Vaibhav Hiremath hvaib...@ti.com
---
drivers/media/video/mem2mem_testdev.c | 58 ++--
1 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/drivers/media/video/mem2mem_testdev.c
b/drivers/media/video/mem2mem_testdev.c
index 05630e3..1f35b7e 100644
--- a/drivers/media/video/mem2mem_testdev.c
+++ b/drivers/media/video/mem2mem_testdev.c
@@ -98,11 +98,10 @@ static struct m2mtest_fmt formats[] = {
};
/* Per-queue, driver-specific private data */
-struct m2mtest_q_data
-{
- unsigned intwidth;
- unsigned intheight;
- unsigned intsizeimage;
+struct m2mtest_q_data {
+ u32 width;
+ u32 height;
+ u32 sizeimage;
struct m2mtest_fmt *fmt;
};
Could you explain this change?
[...]
@@ -158,7 +156,7 @@ static struct v4l2_queryctrl m2mtest_ctrls[] = {
static struct m2mtest_fmt *find_format(struct v4l2_format *f)
{
struct m2mtest_fmt *fmt;
- unsigned int k;
+ u32 k;
This is a loop index... Is there any reason for using u32?
[...]
@@ -535,8 +532,8 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct
v4l2_format *f)
if (videobuf_queue_is_busy(vq)) {
v4l2_err(ctx-dev-v4l2_dev, %s queue busy\n, __func__);
- ret = -EBUSY;
- goto out;
+ mutex_unlock(vq-vb_lock);
+ return -EBUSY;
}
q_data-fmt = find_format(f);
@@ -550,9 +547,7 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct
v4l2_format *f)
Setting format for type %d, wxh: %dx%d, fmt: %d\n,
f-type, q_data-width, q_data-height, q_data-fmt-fourcc);
-out:
- mutex_unlock(vq-vb_lock);
- return ret;
+ return 0;
}
Unless I'm somehow misreading patch output, aren't you removing mutex_unlock
for the path
that reaches the end of the function?
[...]
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 2/2] mem2mem_testdev: Code cleanup
-Original Message-
From: Pawel Osciak [mailto:p.osc...@samsung.com]
Sent: Thursday, April 01, 2010 3:39 PM
To: Hiremath, Vaibhav
Cc: linux-media@vger.kernel.org; Marek Szyprowski; kyungmin.p...@samsung.com
Subject: RE: [PATCH 2/2] mem2mem_testdev: Code cleanup
Hi again,
Vaibhav Hiremath hvaib...@ti.com wrote:
From: Vaibhav Hiremath hvaib...@ti.com
Signed-off-by: Vaibhav Hiremath hvaib...@ti.com
---
drivers/media/video/mem2mem_testdev.c | 58 ++---
---
1 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/drivers/media/video/mem2mem_testdev.c
b/drivers/media/video/mem2mem_testdev.c
index 05630e3..1f35b7e 100644
--- a/drivers/media/video/mem2mem_testdev.c
+++ b/drivers/media/video/mem2mem_testdev.c
@@ -98,11 +98,10 @@ static struct m2mtest_fmt formats[] = {
};
/* Per-queue, driver-specific private data */
-struct m2mtest_q_data
-{
-unsigned intwidth;
-unsigned intheight;
-unsigned intsizeimage;
+struct m2mtest_q_data {
+u32 width;
+u32 height;
+u32 sizeimage;
struct m2mtest_fmt *fmt;
};
Could you explain this change?
[Hiremath, Vaibhav] No specific reason for this change, this is normal practice
I learned from very first patch.
[...]
@@ -158,7 +156,7 @@ static struct v4l2_queryctrl m2mtest_ctrls[] = {
static struct m2mtest_fmt *find_format(struct v4l2_format *f)
{
struct m2mtest_fmt *fmt;
-unsigned int k;
+u32 k;
This is a loop index... Is there any reason for using u32?
[Hiremath, Vaibhav] Same as above.
[...]
@@ -535,8 +532,8 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct
v4l2_format *f)
if (videobuf_queue_is_busy(vq)) {
v4l2_err(ctx-dev-v4l2_dev, %s queue busy\n, __func__);
-ret = -EBUSY;
-goto out;
+mutex_unlock(vq-vb_lock);
+return -EBUSY;
}
q_data-fmt = find_format(f);
@@ -550,9 +547,7 @@ static int vidioc_s_fmt(struct m2mtest_ctx *ctx, struct
v4l2_format *f)
Setting format for type %d, wxh: %dx%d, fmt: %d\n,
f-type, q_data-width, q_data-height, q_data-fmt-fourcc);
-out:
-mutex_unlock(vq-vb_lock);
-return ret;
+return 0;
}
Unless I'm somehow misreading patch output, aren't you removing mutex_unlock
for the path
that reaches the end of the function?
[Hiremath, Vaibhav] Oops, how did this got merged to patch? I had removed it.
Just remove/ignore this change while merging.
Thanks,
Vaibhav
[...]
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: V4L-DVB drivers and BKL
On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:
Hi Hans,
On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:
Hi all,
I just read on LWN that the core kernel guys are putting more effort into
removing the BKL. We are still using it in our own drivers, mostly V4L.
I added a BKL column to my driver list:
http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers
If you 'own' one of these drivers that still use BKL, then it would be nice
if you can try and remove the use of the BKL from those drivers.
The other part that needs to be done is to move from using the .ioctl file
op to using .unlocked_ioctl. Very few drivers do that, but I suspect
almost no driver actually needs to use .ioctl.
What about something like this patch as a first step ?
That doesn't fix anything. You just move the BKL from one place to another.
I don't see any benefit from that.
Regards,
Hans
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 7090699..14e1b1c 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -25,6 +25,7 @@
#include linux/init.h
#include linux/kmod.h
#include linux/slab.h
+#include linux/smp_lock.h
#include asm/uaccess.h
#include asm/system.h
@@ -215,28 +216,22 @@ static unsigned int v4l2_poll(struct file *filp, struct
poll_table_struct *poll)
return vdev-fops-poll(filp, poll);
}
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg)
-{
- struct video_device *vdev = video_devdata(filp);
-
- if (!vdev-fops-ioctl)
- return -ENOTTY;
- /* Allow ioctl to continue even if the device was unregistered.
-Things like dequeueing buffers might still be useful. */
- return vdev-fops-ioctl(filp, cmd, arg);
-}
-
static long v4l2_unlocked_ioctl(struct file *filp,
unsigned int cmd, unsigned long arg)
{
struct video_device *vdev = video_devdata(filp);
+ int ret = -ENOTTY;
- if (!vdev-fops-unlocked_ioctl)
- return -ENOTTY;
/* Allow ioctl to continue even if the device was unregistered.
Things like dequeueing buffers might still be useful. */
- return vdev-fops-unlocked_ioctl(filp, cmd, arg);
+ if (vdev-fops-ioctl) {
+ lock_kernel();
+ ret = vdev-fops-ioctl(filp, cmd, arg);
+ unlock_kernel();
+ } else if (vdev-fops-unlocked_ioctl)
+ ret = vdev-fops-unlocked_ioctl(filp, cmd, arg);
+
+ return ret;
}
#ifdef CONFIG_MMU
@@ -323,22 +318,6 @@ static const struct file_operations v4l2_unlocked_fops =
{
.llseek = no_llseek,
};
-static const struct file_operations v4l2_fops = {
- .owner = THIS_MODULE,
- .read = v4l2_read,
- .write = v4l2_write,
- .open = v4l2_open,
- .get_unmapped_area = v4l2_get_unmapped_area,
- .mmap = v4l2_mmap,
- .ioctl = v4l2_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = v4l2_compat_ioctl32,
-#endif
- .release = v4l2_release,
- .poll = v4l2_poll,
- .llseek = no_llseek,
-};
-
/**
* get_index - assign stream index number based on parent device
* @vdev: video_device to assign index number to, vdev-parent should be
assigned
@@ -517,10 +496,7 @@ static int __video_register_device(struct video_device
*vdev, int type, int nr,
ret = -ENOMEM;
goto cleanup;
}
- if (vdev-fops-unlocked_ioctl)
- vdev-cdev-ops = v4l2_unlocked_fops;
- else
- vdev-cdev-ops = v4l2_fops;
+ vdev-cdev-ops = v4l2_unlocked_fops;
vdev-cdev-owner = vdev-fops-owner;
ret = cdev_add(vdev-cdev, MKDEV(VIDEO_MAJOR, vdev-minor), 1);
if (ret 0) {
A second step would be to replace lock_kernel/unlock_kernel with a
V4L-specific lock, and the third step to push the lock into drivers.
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
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: adv7180 as SoC camera device
On Tue, Mar 30, 2010 at 04:06:11PM +0200, Rodolfo Giometti wrote:
On Tue, Feb 23, 2010 at 12:19:13AM +0100, Richard Röjfors wrote:
We use it as a subdev to a driver not yet committed from us. So I think
you should extend it, not move it.
Finally I got something functional... but I'm puzzled to know how I
can add platform data configuration struct by using the I2C's
platform_data pointer if it is already used to hold struct
soc_camera_device... O_o
Here my solution:
static __devinit int adv7180_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct adv7180_state *state;
#if defined(CONFIG_SOC_CAMERA)
struct soc_camera_device *icd = client-dev.platform_data;
struct soc_camera_link *icl;
struct adv7180_platform_data *pdata = NULL;
#else
struct adv7180_platform_data *pdata =
client-dev.platform_data;
#endif
struct v4l2_subdev *sd;
int i, ret;
/* Check if the adapter supports the needed features */
if (!i2c_check_functionality(client-adapter,
I2C_FUNC_SMBUS_BYTE_DATA))
return -EIO;
v4l_info(client, chip found @ 0x%02x (%s)\n,
client-addr 1, client-adapter-name);
#if defined(CONFIG_SOC_CAMERA)
if (icd) {
icl = to_soc_camera_link(icd);
if (!icl || !icl-priv) {
v4l_err(client, missing platform data!\n);
return -EINVAL;
}
pdata = icl-priv;
icd-ops = adv7180_soc_ops;
v4l_info(client, soc-camera support enabled\n);
}
#endif
state = kzalloc(sizeof(struct adv7180_state), GFP_KERNEL);
if (state == NULL) {
ret = -ENOMEM;
goto err;
}
state-irq = client-irq;
INIT_WORK(state-work, adv7180_work);
mutex_init(state-mutex);
state-autodetect = true;
sd = state-sd;
v4l2_i2c_subdev_init(sd, client, adv7180_ops);
if (pdata)
for (i = 0; pdata[i].reg = 0; i++) {
printk( %x %x\n, pdata[i].reg,
pdata[i].val);
ret = i2c_smbus_write_byte_data(client,
pdata[i].reg, pdata[i].val);
if (ret 0)
goto err_unreg_subdev;
}
Rodolfo
--
GNU/Linux Solutions e-mail: giome...@enneenne.com
Linux Device Driver giome...@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
--
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
[PATCH] drivers/media/IR - improve keyup/keydown logic
The attached patch rewrites the keyup/keydown logic in
drivers/media/IR/ir-keytable.c.
(applies to http://git.linuxtv.org/mchehab/ir.git)
All knowledge of keystates etc is now internal to ir-keytable.c
and not scattered around ir-raw-event.c and ir-nec-decoder.c (where
it doesn't belong).
In addition, I've changed the API slightly so that ir_input_dev is
passed as the first argument rather than input_dev. If we're ever
going to support multiple keytables we need to move towards making
ir_input_dev the main interface from a driver POV and obscure away
the input_dev as an implementational detail in ir-core.
Mauro, once this patch is merged I'll start working on patches to
migrate drivers which use ir_input_* in ir-functions.c over to
the ir-keytable.c code instead.
Signed-off-by: David Härdeman da...@hardeman.nu
---
drivers/media/IR/ir-keytable.c| 127 +
drivers/media/IR/ir-nec-decoder.c |7 +--
drivers/media/IR/ir-raw-event.c | 14
include/media/ir-core.h | 15 +++--
4 files changed, 112 insertions(+), 51 deletions(-)
diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
index 6b0d13e..e1fd20b 100644
--- a/drivers/media/IR/ir-keytable.c
+++ b/drivers/media/IR/ir-keytable.c
@@ -19,6 +19,9 @@
#define IR_TAB_MIN_SIZE32
#define IR_TAB_MAX_SIZE1024
+/* FIXME: IR_KEYPRESS_TIMEOUT should be protocol specific */
+#define IR_KEYPRESS_TIMEOUT 250
+
/**
* ir_seek_table() - returns the element order on the table
* @rc_tab:the ir_scancode_table with the keymap to be used
@@ -392,56 +395,122 @@ EXPORT_SYMBOL_GPL(ir_g_keycode_from_table);
/**
* ir_keyup() - generates input event to cleanup a key press
- * @input_dev: the struct input_dev descriptor of the device
+ * @ir:the struct ir_input_dev descriptor of the device
*
- * This routine is used by the input routines when a key is pressed at the
- * IR. It reports a keyup input event via input_report_key().
+ * This routine is used to signal that a key has been released on the
+ * remote control. It reports a keyup input event via input_report_key().
*/
-void ir_keyup(struct input_dev *dev)
+static void ir_keyup(struct ir_input_dev *ir)
{
- struct ir_input_dev *ir = input_get_drvdata(dev);
-
if (!ir-keypressed)
return;
- IR_dprintk(1, keyup key 0x%04x\n, ir-keycode);
- input_report_key(dev, ir-keycode, 0);
- input_sync(dev);
- ir-keypressed = 0;
+ IR_dprintk(1, keyup key 0x%04x\n, ir-last_keycode);
+ input_report_key(ir-input_dev, ir-last_keycode, 0);
+ input_sync(ir-input_dev);
+ ir-keypressed = false;
}
-EXPORT_SYMBOL_GPL(ir_keyup);
+
+/**
+ * ir_timer_keyup() - generates a keyup event after a timeout
+ * @cookie:a pointer to struct ir_input_dev passed to setup_timer()
+ *
+ * This routine will generate a keyup event some time after a keydown event
+ * is generated when no further activity has been detected.
+ */
+static void ir_timer_keyup(unsigned long cookie)
+{
+ struct ir_input_dev *ir = (struct ir_input_dev *)cookie;
+ unsigned long flags;
+
+ /*
+* ir-keyup_jiffies is used to prevent a race condition if a
+* hardware interrupt occurs at this point and the keyup timer
+* event is moved further into the future as a result.
+*
+* The timer will then be reactivated and this function called
+* again in the future. We need to exit gracefully in that case
+* to allow the input subsystem to do its auto-repeat magic or
+* a keyup event might follow immediately after the keydown.
+*/
+ spin_lock_irqsave(ir-keylock, flags);
+ if (time_is_after_eq_jiffies(ir-keyup_jiffies))
+ ir_keyup(ir);
+ spin_unlock_irqrestore(ir-keylock, flags);
+}
+
+/**
+ * ir_repeat() - notifies the IR core that a key is still pressed
+ * @ir:the struct ir_input_dev descriptor of the device
+ *
+ * This routine is used by IR decoders when a repeat message which does
+ * not include the necessary bits to reproduce the scancode has been
+ * received.
+ */
+void ir_repeat(struct ir_input_dev *ir)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(ir-keylock, flags);
+
+ if (!ir-keypressed)
+ goto out;
+
+ ir-keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT);
+ mod_timer(ir-timer_keyup, ir-keyup_jiffies);
+
+out:
+ spin_unlock_irqrestore(ir-keylock, flags);
+}
+EXPORT_SYMBOL_GPL(ir_repeat);
/**
* ir_keydown() - generates input event for a key press
- * @input_dev: the struct input_dev descriptor of the device
+ * @ir:the struct ir_input_dev descriptor of the device
* @scancode: the scancode that we're seeking
+ * @toggle:the toggle value (protocol dependent, if the protocol doesn't
+ * support toggle values, this should be set to
Re: V4L-DVB drivers and BKL
Hans Verkuil wrote: On the DVB side there seem to be only two sources that use the BKL: linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel(); This is from an incomplete conversion from .ioctl to .unlocked_ioctl (no conversion really, only a BKL push-down). linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); This is from when the BKL was pushed down into drivers' open() methods. To remove BKL from open(), check for possible races with module insertion. (A driver's module_init has to have set up everything that's going to be used by open() before the char device is being registered.) Apart from those two BKL uses in media/dvb/, there are also - remaining .ioctl that need to be checked for possible concurrency issues, then converted to .unlocked_ioctl, - remaining .llseek uses (all implicit) which need to be checked whether they should be no_llseek() (accompanied by nonseekable_open) or generic_file_llseek() or default_llseek(). -- Stefan Richter -=-==-=- -=-- = http://arcgraph.de/sr/ -- 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: V4L-DVB drivers and BKL
Hi Hans,
On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:
On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:
On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:
Hi all,
I just read on LWN that the core kernel guys are putting more effort
into removing the BKL. We are still using it in our own drivers,
mostly V4L.
I added a BKL column to my driver list:
http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Dri
vers
If you 'own' one of these drivers that still use BKL, then it would be
nice if you can try and remove the use of the BKL from those drivers.
The other part that needs to be done is to move from using the .ioctl
file op to using .unlocked_ioctl. Very few drivers do that, but I
suspect almost no driver actually needs to use .ioctl.
What about something like this patch as a first step ?
That doesn't fix anything. You just move the BKL from one place to another.
I don't see any benefit from that.
Removing the BKL is a long-term project that basically pushes the BKL from
core code to subsystems and drivers, and then replace it on a case by case
basis. This patch (along with a replacement of lock_kernel/unlock_kernel by a
V4L-specific lock) goes into that direction and removes the BKL usage from V4L
ioctls. The V4L lock would then need to be pushed into individual drivers.
diff --git a/drivers/media/video/v4l2-dev.c
b/drivers/media/video/v4l2-dev.c index 7090699..14e1b1c 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -25,6 +25,7 @@
#include linux/init.h
#include linux/kmod.h
#include linux/slab.h
+#include linux/smp_lock.h
#include asm/uaccess.h
#include asm/system.h
@@ -215,28 +216,22 @@ static unsigned int v4l2_poll(struct file *filp,
struct poll_table_struct *poll)
return vdev-fops-poll(filp, poll);
}
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg)
-{
- struct video_device *vdev = video_devdata(filp);
-
- if (!vdev-fops-ioctl)
- return -ENOTTY;
- /* Allow ioctl to continue even if the device was unregistered.
- Things like dequeueing buffers might still be useful. */
- return vdev-fops-ioctl(filp, cmd, arg);
-}
-
static long v4l2_unlocked_ioctl(struct file *filp,
unsigned int cmd, unsigned long arg)
{
struct video_device *vdev = video_devdata(filp);
+ int ret = -ENOTTY;
- if (!vdev-fops-unlocked_ioctl)
- return -ENOTTY;
/* Allow ioctl to continue even if the device was unregistered.
Things like dequeueing buffers might still be useful. */
- return vdev-fops-unlocked_ioctl(filp, cmd, arg);
+ if (vdev-fops-ioctl) {
+ lock_kernel();
+ ret = vdev-fops-ioctl(filp, cmd, arg);
+ unlock_kernel();
+ } else if (vdev-fops-unlocked_ioctl)
+ ret = vdev-fops-unlocked_ioctl(filp, cmd, arg);
+
+ return ret;
}
#ifdef CONFIG_MMU
@@ -323,22 +318,6 @@ static const struct file_operations
v4l2_unlocked_fops = {
.llseek = no_llseek,
};
-static const struct file_operations v4l2_fops = {
- .owner = THIS_MODULE,
- .read = v4l2_read,
- .write = v4l2_write,
- .open = v4l2_open,
- .get_unmapped_area = v4l2_get_unmapped_area,
- .mmap = v4l2_mmap,
- .ioctl = v4l2_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = v4l2_compat_ioctl32,
-#endif
- .release = v4l2_release,
- .poll = v4l2_poll,
- .llseek = no_llseek,
-};
-
/**
* get_index - assign stream index number based on parent device
* @vdev: video_device to assign index number to, vdev-parent should be
assigned
@@ -517,10 +496,7 @@ static int __video_register_device(struct
video_device *vdev, int type, int nr,
ret = -ENOMEM;
goto cleanup;
}
- if (vdev-fops-unlocked_ioctl)
- vdev-cdev-ops = v4l2_unlocked_fops;
- else
- vdev-cdev-ops = v4l2_fops;
+ vdev-cdev-ops = v4l2_unlocked_fops;
vdev-cdev-owner = vdev-fops-owner;
ret = cdev_add(vdev-cdev, MKDEV(VIDEO_MAJOR, vdev-minor), 1);
if (ret 0) {
A second step would be to replace lock_kernel/unlock_kernel with a
V4L-specific lock, and the third step to push the lock into drivers.
--
Regards,
Laurent Pinchart
--
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: V4L-DVB drivers and BKL
On Thursday 01 April 2010 13:57:13 Stefan Richter wrote: Hans Verkuil wrote: I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L. I added a BKL column to my driver list: http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers. The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl. Also note that struct file_operations.llseek() grabs the BKL if .llseek = default_llseek, or if .llseek == NULL (struct file.f_mode FMODE_LSEEK) != 0. I guess V4L/DVB character device file ABIs do not involve lseek() and friends, do they? If so, are the files flagged as non-seekable? They are. The file op .llseek is always set to no_llseek for v4l. DVB seems to leave it at NULL but I don't believe it is ever implemented. On the DVB side there seem to be only two sources that use the BKL: linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); At first glance it doesn't seem too difficult to remove them, but I leave that to the DVB experts. As a dvb/firewire/firedtv user, I started to mess around with dvbdev and firedtv: https://patchwork.kernel.org/patch/88778/ Great! Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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: V4L-DVB drivers and BKL
Stefan Richter wrote: linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); This is from when the BKL was pushed down into drivers' open() methods. To remove BKL from open(), check for possible races with module insertion. (A driver's module_init has to have set up everything that's going to be used by open() before the char device is being registered.) Last sentence was supposed to mean: Before the char device is being registered, a driver's module_init has to have set up everything that's going to be used by openers of the file. (Traditionally, the BKL serialized open() with module initialization, which was not obvious to driver writers because it happened deep in the core kernel.) -- Stefan Richter -=-==-=- -=-- = http://arcgraph.de/sr/ -- 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: V4L-DVB drivers and BKL
Hans Verkuil wrote: I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L. I added a BKL column to my driver list: http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers. The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl. Also note that struct file_operations.llseek() grabs the BKL if .llseek = default_llseek, or if .llseek == NULL (struct file.f_mode FMODE_LSEEK) != 0. I guess V4L/DVB character device file ABIs do not involve lseek() and friends, do they? If so, are the files flagged as non-seekable? On the DVB side there seem to be only two sources that use the BKL: linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); At first glance it doesn't seem too difficult to remove them, but I leave that to the DVB experts. As a dvb/firewire/firedtv user, I started to mess around with dvbdev and firedtv: https://patchwork.kernel.org/patch/88778/ -- Stefan Richter -=-==-=- -=-- = http://arcgraph.de/sr/ -- 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.
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: V4L-DVB drivers and BKL
On Thursday 01 April 2010 16:12:50 Mauro Carvalho Chehab wrote: Laurent Pinchart wrote: Hi Hans, On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote: On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote: On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote: Hi all, I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L. I added a BKL column to my driver list: http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Dri vers If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers. The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl. What about something like this patch as a first step ? That doesn't fix anything. You just move the BKL from one place to another. I don't see any benefit from that. Removing the BKL is a long-term project that basically pushes the BKL from core code to subsystems and drivers, and then replace it on a case by case basis. This patch (along with a replacement of lock_kernel/unlock_kernel by a V4L-specific lock) goes into that direction and removes the BKL usage from V4L ioctls. The V4L lock would then need to be pushed into individual drivers. True, but, as almost all V4L drivers share a common ancestor, fixing the problems for one will also fix for the others. One typical problem that I see is that some drivers register too soon: they first register, then initialize some things. I've seen (and fixed) some race conditions due to that. Just moving the register to the end solves this issue. Correct. What to do if we have multiple device nodes? E.g. video0 and vbi0? Should we allow access to video0 when vbi0 is not yet registered? Or should we block access until all video nodes are registered? One (far from perfect) solution, would be to add a mutex protecting the entire ioctl loop inside the drivers, and the open/close methods. This can later be optimized by a mutex that will just protect the operations that can actually cause problems if happening in parallel. I have thought about this in the past. What I think would be needed to make locking much more reliable is the following: 1) Currently when a device is unregistered all read()s, write()s, poll()s, etc. are blocked. Except for ioctl(). The comment in v4l2-dev.c says this: /* Allow ioctl to continue even if the device was unregistered. Things like dequeueing buffers might still be useful. */ I disagree with this. Once the device is gone (USB disconnect and similar hotplug scenarios), then the only thing an application can do is to close. Allowing ioctl to still work makes it hard for drivers since every ioctl op that might do something with the device has to call video_is_registered() to check whether the device is still alive. I know, this is not directly related to the BKL, but it is an additional complication. 2) Add a new video_device flag that turns on serialization. Basically all calls are serialized with a mutex in v4l2_device. To handle blocking calls like read() or VIDIOC_DQBUF we can either not take the serialization mutex in the core, or instead the driver needs to unlock the mutex before it waits for an event and lock it afterwards. In the first case the core has to know all the exceptions. Perhaps we should just add a second flag: whether the core should do full serialization (and the driver will have to unlock/lock around blocking waits) or smart serialization where know blocking operations are allowed unserialized. I think it is fairly simple to add this serialization mechanism. And for many drivers this will actually be more than enough. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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
[RFC] Non-FIFO waiting on buffers in videobuf
Hello, we would like to propose a change to how videobuf handles the process of waking up buffers, to allow returning them to userspace in a different order than FIFO, if a driver (device) so requires. Currently, when poll() or dqbuf() is called, videobuf takes the first (i.e. first qbuf()-ed) buffer from the stream queue and waits for it to change its state (if it has not changed already). In other words, it is assumed that buffers are consumed in FIFO order and that they should be dequeued in that order as well. It would be essential for some of our devices though, particularly video codecs, to allow dequeuing buffers not only in FIFO order, but just any buffer that has been consumed. Video codecs may need to hold some buffers (usually keyframes) for longer periods of time than others. Moreover, there is no way in V4L2 API to wait for a particular buffer. According to it, buffer index in v4l2_buffer has and can only be filled by a driver anyway. So switching to waiting for any buffer - instead of for the first one - is not a breach of V4L2 API. And unless I am missing something, since videobuf always uses the first buffer, why have separate queues in each? Drivers could use any other way to indicate that they are finished with the particular buffer instead of waking up their queues (actually, they are already - by marking them with VIDEOBUF_ERROR or VIDEOBUF_DONE). So the current method seems superfluous. I am not really fond of the way in which drivers have to finish operations on buffers at the moment anyway. Normally, it is done in a way similar to: /* buf points to the buffer to be returned */ spin_lock_irqsave(irqlock, ...); /* take buf off of driver's queue */ list_del(buf-vb.queue); buf-vb.state = VIDEOBUF_DONE; /* or VIDEOBUF_ERROR */ wake_up(buf-vb.done); spin_unlock_irqrestore(irqlock, ...); Wouldn't it be more clear if drivers called some kind of a finish() function in videobuf instead, which would mark the buffers as DONE/ERROR and wake them up? I am not convinced that drivers have to be aware of the videobuf's internal buffer waking mechanism... I believe they normally just want to signal that they are finished with this particular buffer while optionally indicating an error. Wouldn't it be better to move the actual waking logic to videobuf? Not only it would simplify the drivers, but would allow to introduce changes/improvements to the whole process of waking up more easily, without having to modify all the drivers. Our first idea and a preliminary proposal is as follows: 1. Add one, additional, general buffer wait queue to a videobuf_queue, on which poll and dqbuf would actually wait. 2. When a driver is to release a buffer, it could call a videobuf function (instead of waking buffers manually), which could handle whatever would be required to return this buffer to the userspace. That would include waking up process(es) waiting on the main queue in dqbuf() or poll(). All the abovementioned arguments aside, I kind of feel that there may have been a good reason for having separate queues in each buffer, instead of just one, common queue. If so, is anybody able to point it out? 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: V4L-DVB drivers and BKL
On Thursday 01 April 2010 17:02:01 Mauro Carvalho Chehab wrote:
Hans Verkuil wrote:
What to do if we have multiple device nodes? E.g. video0 and vbi0? Should we
allow access to video0 when vbi0 is not yet registered? Or should we block
access until all video nodes are registered?
It will depend on the driver implementation, but, as new udev implementations
try to open v4l devices asap,
Yes, that is very annoying when you also have to do firmware uploads. The ivtv
driver does that on the first open, but that doesn't help anymore when hal opens
it automatically.
the better is to lock the register operation
to avoid an open while not finished.
I remember I had to do it on em28xx:
This is the init code for it:
...
mutex_init(dev-lock);
mutex_lock(dev-lock);
em28xx_init_dev(dev, udev, interface, nr);
...
request_modules(dev);
/* Should be the last thing to do, to avoid newer udev's to
open the device before fully initializing it
*/
mutex_unlock(dev-lock);
...
And this is the open code:
static int em28xx_v4l2_open(struct file *filp)
{
...
mutex_lock(dev-lock);
...
mutex_unlock(dev-lock);
The same lock is also used at the ioctl handlers that need to be protected,
like:
static int radio_g_tuner(struct file *file, void *priv,
struct v4l2_tuner *t)
{
...
mutex_lock(dev-lock);
v4l2_device_call_all(dev-v4l2_dev, 0, tuner, g_tuner, t);
mutex_unlock(dev-lock);
...
}
There are some obvious cases where no lock is needed, like for example
vidioc_querycap.
One (far from perfect) solution, would be to add a mutex protecting the
entire
ioctl loop inside the drivers, and the open/close methods. This can later
be
optimized by a mutex that will just protect the operations that can
actually
cause problems if happening in parallel.
I have thought about this in the past.
What I think would be needed to make locking much more reliable is the
following:
1) Currently when a device is unregistered all read()s, write()s, poll()s,
etc.
are blocked. Except for ioctl().
The comment in v4l2-dev.c says this:
/* Allow ioctl to continue even if the device was unregistered.
Things like dequeueing buffers might still be useful. */
I disagree with this. Once the device is gone (USB disconnect and similar
hotplug scenarios), then the only thing an application can do is to close.
Allowing ioctl to still work makes it hard for drivers since every ioctl
op that might do something with the device has to call video_is_registered()
to check whether the device is still alive.
I know, this is not directly related to the BKL, but it is an additional
complication.
Depending on how the video buffers are implemented, you may need to run
dequeue,
in order to allow freeing the mmaped memories. That's said, maybe we could use
a kref implementation for those kind or resources.
What should be the correct sequence in an application when the device it is
capturing from is suddenly unplugged?
I guess it is a STREAMOFF followed by a REQBUFS with a count of 0. At least
according to the spec (videobuf doesn't accept a count of 0 at the moment).
So those two ioctls would need to be allowed through.
2) Add a new video_device flag that turns on serialization. Basically all
calls are serialized with a mutex in v4l2_device. To handle blocking calls
like read() or VIDIOC_DQBUF we can either not take the serialization mutex
in the core, or instead the driver needs to unlock the mutex before it
waits for an event and lock it afterwards.
In the first case the core has to know all the exceptions.
Perhaps we should just add a second flag: whether the core should do full
serialization (and the driver will have to unlock/lock around blocking
waits)
or smart serialization where know blocking operations are allowed
unserialized.
I think it is fairly simple to add this serialization mechanism. And for
many
drivers this will actually be more than enough.
I remember I proposed a solution to implement the mutex at V4L core level,
when we had this discussion with Alan Cox BKL patches.
The conclusion I had from the discussion is that, while this is a simple way,
it may end that a poorly implemented lock would stay there forever.
Also, core has no way to foresee what the driver is doing on their side, and
may
miss some cases where the lock needs to be used.
I don't think that adding flags would help to improve it.
Why not? Seriously, most drivers only need a simple serialization flag.
Adding this feature in the core by just setting a V4L2_FL_SERIALIZE flag
is easy to do and it is very simple to implement and review. Given the fact
that a lot of drivers (esp. older ones) have very
[patch] ir-keytable: avoid double lock
It's possible that we wanted to resize to a smaller size but we didn't
have enough memory to create the new table. We need to test for that
here so we don't try to lock twice and dead lock. Also we free the
oldkeymap on that path and that would be bad.
Signed-off-by: Dan Carpenter erro...@gmail.com
---
I don't know this code very well. Maybe we should just take the lock
earlier in the function for the resize case and the non resize case.
Can we add new keys while the resize is taking place?
diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
index 0a3b4ed..51cd0f3 100644
--- a/drivers/media/IR/ir-keytable.c
+++ b/drivers/media/IR/ir-keytable.c
@@ -216,7 +216,7 @@ static void ir_delete_key(struct ir_scancode_table *rc_tab,
int elem)
memcpy(newkeymap[elem], oldkeymap[elem + 1],
(newsize - elem) * sizeof(*newkeymap));
- if (resize) {
+ if (resize newkeymap != oldkeymap) {
/*
* As the copy happened to a temporary table, only here
* it needs to lock while replacing the table pointers
--
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: [GIT FIX for 2.6.34] V4L - vpfe capture - fix for kernel crash on DM365
Muralidharan Karicheri wrote: Mauro, When I had last replied to your email, I didn't check if the patch is actually applied to your v4l_for_linux branch of fixes.git tree. But Today I checked and I can't find the patch merged to this tree as you had mentioned.. So if you haven't merged it for some reason, please merge my updated patch available at https://patchwork.kernel.org/patch/86731/ I had merged, but, as I haven't pushed on linuxtv, I decided to rebase my local tree. Patch applied. -- Cheers, Mauro -- 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: [GIT FIX for 2.6.34] V4L - vpfe capture - fix for kernel crash on DM365
Mauro, Thanks a lot for your support. Do you plan to merge my pull request for 2.6.35 anytime soon? Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com -Original Message- From: Mauro Carvalho Chehab [mailto:mche...@redhat.com] Sent: Thursday, April 01, 2010 12:21 PM To: Muralidharan Karicheri Cc: Karicheri, Muralidharan; linux-media@vger.kernel.org; hverk...@xs4all.nl; davinci-linux-open-sou...@linux.davincidsp.com Subject: Re: [GIT FIX for 2.6.34] V4L - vpfe capture - fix for kernel crash on DM365 Muralidharan Karicheri wrote: Mauro, When I had last replied to your email, I didn't check if the patch is actually applied to your v4l_for_linux branch of fixes.git tree. But Today I checked and I can't find the patch merged to this tree as you had mentioned.. So if you haven't merged it for some reason, please merge my updated patch available at https://patchwork.kernel.org/patch/86731/ I had merged, but, as I haven't pushed on linuxtv, I decided to rebase my local tree. Patch applied. -- Cheers, Mauro -- 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: [GIT FIX for 2.6.34] V4L - vpfe capture - fix for kernel crash on DM365
Karicheri, Muralidharan wrote: Mauro, Thanks a lot for your support. Do you plan to merge my pull request for 2.6.35 anytime soon? It were already merged locally. I'm running a compilation of the tree with the merged patches. If all compile ok, your pull request (and others) will be at linuxtv server soon. -- Cheers, Mauro -- 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: V4L-DVB drivers and BKL
Hans Verkuil wrote: Hi all, I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L. I added a BKL column to my driver list: http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Drivers If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers. The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl. The removal of BKL is generally as simple as review the locks inside the driver, being sure that an ioctl won't interfere on another ioctl, or on open/close ops. On the DVB side there seem to be only two sources that use the BKL: linux/drivers/media/dvb/bt8xx/dst_ca.c: lock_kernel(); linux/drivers/media/dvb/bt8xx/dst_ca.c: unlock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: lock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); linux/drivers/media/dvb/dvb-core/dvbdev.c: unlock_kernel(); At first glance it doesn't seem too difficult to remove them, but I leave that to the DVB experts. The main issue is at dvbdev, since it is used by all devices. We need to get rid of it. That's said, Stefan Richter sent a patch meant to reduce the issues with DVB. Unfortunately, I haven't seen any comments on it. It would be really important to test his approach. It will probably come a time where the drivers that still uses BKL will stop working, as they will remove BKL. I remember that, during KS/2009, it was proposed by someone to just mark all drivers that use BKL as BROKEN. This didn't happen (yet), but I don't doubt it will happen on the next few kernel versions. -- Cheers, Mauro -- 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: V4L-DVB drivers and BKL
Laurent Pinchart wrote: One typical problem that I see is that some drivers register too soon: they first register, then initialize some things. I've seen (and fixed) some race conditions due to that. Just moving the register to the end solves this issue. That's right, devices should not be registered until they are ready to be opened by userspace. However, I don't see how that's related to the BKL. Before the BKL changes, open were allowed only after the full module loading. One (far from perfect) solution, would be to add a mutex protecting the entire ioctl loop inside the drivers, and the open/close methods. This can later be optimized by a mutex that will just protect the operations that can actually cause problems if happening in parallel. The BKL protects both open() and ioctl(), but the ioctl operation can't be called before open succeeds anyway, so I'm not sure we have a problem there. You may have, as one file handler may be doing an ioctl, while another application opens or closes another file handler. Depending on what the driver have on the open handler, it might interfere on the ioctl. The real problem is that most drivers rely on ioctls being serialized by the BKL. The drivers need to be fixed on a case by case basis, but we could already drop the BKL there by using a V4L-specific lock to serialize ioctl calls. Yes, that's my point. It is not hard to write such patch, moving from BKL to an ioctl/open/close mutex, and it should be safe, provided that it doesn't introduce any dead lock with some existing mutexes. -- Cheers, Mauro -- 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: V4L-DVB drivers and BKL
Hans Verkuil wrote:
What to do if we have multiple device nodes? E.g. video0 and vbi0? Should we
allow access to video0 when vbi0 is not yet registered? Or should we block
access until all video nodes are registered?
It will depend on the driver implementation, but, as new udev implementations
try to open v4l devices asap, the better is to lock the register operation
to avoid an open while not finished.
I remember I had to do it on em28xx:
This is the init code for it:
...
mutex_init(dev-lock);
mutex_lock(dev-lock);
em28xx_init_dev(dev, udev, interface, nr);
...
request_modules(dev);
/* Should be the last thing to do, to avoid newer udev's to
open the device before fully initializing it
*/
mutex_unlock(dev-lock);
...
And this is the open code:
static int em28xx_v4l2_open(struct file *filp)
{
...
mutex_lock(dev-lock);
...
mutex_unlock(dev-lock);
The same lock is also used at the ioctl handlers that need to be protected,
like:
static int radio_g_tuner(struct file *file, void *priv,
struct v4l2_tuner *t)
{
...
mutex_lock(dev-lock);
v4l2_device_call_all(dev-v4l2_dev, 0, tuner, g_tuner, t);
mutex_unlock(dev-lock);
...
}
There are some obvious cases where no lock is needed, like for example
vidioc_querycap.
One (far from perfect) solution, would be to add a mutex protecting the
entire
ioctl loop inside the drivers, and the open/close methods. This can later be
optimized by a mutex that will just protect the operations that can actually
cause problems if happening in parallel.
I have thought about this in the past.
What I think would be needed to make locking much more reliable is the
following:
1) Currently when a device is unregistered all read()s, write()s, poll()s,
etc.
are blocked. Except for ioctl().
The comment in v4l2-dev.c says this:
/* Allow ioctl to continue even if the device was unregistered.
Things like dequeueing buffers might still be useful. */
I disagree with this. Once the device is gone (USB disconnect and similar
hotplug scenarios), then the only thing an application can do is to close.
Allowing ioctl to still work makes it hard for drivers since every ioctl
op that might do something with the device has to call video_is_registered()
to check whether the device is still alive.
I know, this is not directly related to the BKL, but it is an additional
complication.
Depending on how the video buffers are implemented, you may need to run dequeue,
in order to allow freeing the mmaped memories. That's said, maybe we could use
a kref implementation for those kind or resources.
2) Add a new video_device flag that turns on serialization. Basically all
calls are serialized with a mutex in v4l2_device. To handle blocking calls
like read() or VIDIOC_DQBUF we can either not take the serialization mutex
in the core, or instead the driver needs to unlock the mutex before it
waits for an event and lock it afterwards.
In the first case the core has to know all the exceptions.
Perhaps we should just add a second flag: whether the core should do full
serialization (and the driver will have to unlock/lock around blocking waits)
or smart serialization where know blocking operations are allowed
unserialized.
I think it is fairly simple to add this serialization mechanism. And for many
drivers this will actually be more than enough.
I remember I proposed a solution to implement the mutex at V4L core level,
when we had this discussion with Alan Cox BKL patches.
The conclusion I had from the discussion is that, while this is a simple way,
it may end that a poorly implemented lock would stay there forever.
Also, core has no way to foresee what the driver is doing on their side, and may
miss some cases where the lock needs to be used.
I don't think that adding flags would help to improve it.
--
Cheers,
Mauro
--
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: V4L-DVB drivers and BKL
Laurent Pinchart wrote: Hi Hans, On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote: On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote: On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote: Hi all, I just read on LWN that the core kernel guys are putting more effort into removing the BKL. We are still using it in our own drivers, mostly V4L. I added a BKL column to my driver list: http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Dri vers If you 'own' one of these drivers that still use BKL, then it would be nice if you can try and remove the use of the BKL from those drivers. The other part that needs to be done is to move from using the .ioctl file op to using .unlocked_ioctl. Very few drivers do that, but I suspect almost no driver actually needs to use .ioctl. What about something like this patch as a first step ? That doesn't fix anything. You just move the BKL from one place to another. I don't see any benefit from that. Removing the BKL is a long-term project that basically pushes the BKL from core code to subsystems and drivers, and then replace it on a case by case basis. This patch (along with a replacement of lock_kernel/unlock_kernel by a V4L-specific lock) goes into that direction and removes the BKL usage from V4L ioctls. The V4L lock would then need to be pushed into individual drivers. True, but, as almost all V4L drivers share a common ancestor, fixing the problems for one will also fix for the others. One typical problem that I see is that some drivers register too soon: they first register, then initialize some things. I've seen (and fixed) some race conditions due to that. Just moving the register to the end solves this issue. One (far from perfect) solution, would be to add a mutex protecting the entire ioctl loop inside the drivers, and the open/close methods. This can later be optimized by a mutex that will just protect the operations that can actually cause problems if happening in parallel. -- Cheers, Mauro -- 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: webcam problem after suspend/hibernate
Hi! Do you mean the dmesg output ? A full dmesg is included in this address : http://pastebin.com/8XU619Uk Not in all suspend/hibernate the problem comes, only in some of them and this included dmesg output is just after a non working case of webcam fault. I also have found this in `/var/log/messages | grep uvcvideo` Mar 31 00:31:16 linux-l365 kernel: [399905.714743] usbcore: deregistering interface driver uvcvideo Mar 31 00:31:24 linux-l365 kernel: [399914.121386] uvcvideo: Found UVC 1.00 device LG Webcam (0c45:62c0) Mar 31 00:31:24 linux-l365 kernel: [399914.135661] usbcore: registered new interface driver uvcvideo Also try unloading uvcvideo before suspend and reloading it after resume... -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- 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: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
I remember I had to do it on em28xx:
This is the init code for it:
...
mutex_init(dev-lock);
mutex_lock(dev-lock);
em28xx_init_dev(dev, udev, interface, nr);
...
request_modules(dev);
/* Should be the last thing to do, to avoid newer udev's to
open the device before fully initializing it
*/
mutex_unlock(dev-lock);
...
And this is the open code:
static int em28xx_v4l2_open(struct file *filp)
{
...
mutex_lock(dev-lock);
...
mutex_unlock(dev-lock);
It's probably worth noting that this change is actually pretty badly
broken. Because the modules are loading asynchronously, there is a
high probability that the em28xx-dvb driver will still be loading when
hald connects in to the v4l device. That's the big reason people
often see things like tvp5150 i2c errors when the driver is first
loaded up.
It's a good idea in theory, but pretty fatally flawed due to the async
loading (as to make it work properly you would have to do something
like locking the mutex in em28xx and clearing it in em28xx-dvb at the
end of its initialization).
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: webcam problem after suspend/hibernate
Pavel Machek pa...@ucw.cz writes: Do you mean the dmesg output ? A full dmesg is included in this address : http://pastebin.com/8XU619Uk Not in all suspend/hibernate the problem comes, only in some of them and this included dmesg output is just after a non working case of webcam fault. I also have found this in `/var/log/messages | grep uvcvideo` Mar 31 00:31:16 linux-l365 kernel: [399905.714743] usbcore: deregistering interface driver uvcvideo Mar 31 00:31:24 linux-l365 kernel: [399914.121386] uvcvideo: Found UVC 1.00 device LG Webcam (0c45:62c0) Mar 31 00:31:24 linux-l365 kernel: [399914.135661] usbcore: registered new interface driver uvcvideo Also try unloading uvcvideo before suspend and reloading it after resume... I have a similar problem with a Creative Optia webcam. I have found that removing the ehci_hcd module and reinserting it fixes the problem. If your kernel ships with ehci_hcd built-in (F11 and later), the script included also fixes the problem (it rebind the device). Of course, I'd love to see this issue fixed. Phil. Script: /etc/pm/sleep.d/50kickuvc #!/bin/sh case $1 in resume|thaw) cd /sys/bus/usb/drivers/uvcvideo || exit 1 devices='' for i in [0-9]*-[0-9]*:* do [ -L $i ] || break saved_IFS=$IFS IFS=: set -- $i IFS=$saved_IFS found=no for j in $devices do if [ $j = $1 ] then found=yes fi done if [ $found = no ] then devices=$devices $1 fi done if [ $devices != ] then cd /sys/bus/usb/drivers/usb || exit 1 for i in $devices do echo $i unbind sleep 1 echo $i bind done fi ;; esac -- 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: V4L-DVB drivers and BKL
Devin Heitmueller wrote:
On Thu, Apr 1, 2010 at 11:02 AM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
I remember I had to do it on em28xx:
This is the init code for it:
...
mutex_init(dev-lock);
mutex_lock(dev-lock);
em28xx_init_dev(dev, udev, interface, nr);
...
request_modules(dev);
/* Should be the last thing to do, to avoid newer udev's to
open the device before fully initializing it
*/
mutex_unlock(dev-lock);
...
And this is the open code:
static int em28xx_v4l2_open(struct file *filp)
{
...
mutex_lock(dev-lock);
...
mutex_unlock(dev-lock);
It's probably worth noting that this change is actually pretty badly
broken. Because the modules are loading asynchronously, there is a
high probability that the em28xx-dvb driver will still be loading when
hald connects in to the v4l device. That's the big reason people
often see things like tvp5150 i2c errors when the driver is first
loaded up.
It's a good idea in theory, but pretty fatally flawed due to the async
loading (as to make it work properly you would have to do something
like locking the mutex in em28xx and clearing it in em28xx-dvb at the
end of its initialization).
If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due
to the async load, we'll need to add the same locking at *alsa and *dvb
parts of em28xx.
Yet, in this specific case, as the errors are due to the reception of
wrong data from tvp5150, maybe the problem is due to the lack of a
proper lock at the i2c access.
Devin
--
Cheers,
Mauro
--
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
[RFC] Serialization flag example
I made a quick implementation which is available here: http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize It's pretty easy to use and it also gives you a very simple way to block access to the video device nodes until all have been allocated by simply taking the serialization lock and holding it until we are done with the initialization. I converted radio-mr800.c and ivtv. That said, almost all drivers that register multiple device nodes probably suffer from a race condition when one of the device node registrations returns an error and all devices have to be unregistered and the driver needs to release all resources. Currently most if not all drivers just release resources and free the memory. But if an application managed to open one device before the driver removes it again, then we have almost certainly a crash. It is possible to do this correctly in the driver, but it really needs core support where a release callback can be installed in v4l2_device that is called when the last video_device is closed by the application. We already can cleanup correctly after the last close of a video_device, but there is no top-level release yet. Anyway, I tried to use the serialization flag in bttv as well, but I ran into problems with videobuf. Basically when you need to wait for some event you should release the serialization lock and grab it after the event arrives. Unfortunately videobuf has no access to v4l2_device at the moment. If we would have that, then videobuf can just release the serialization lock while waiting for something to happen. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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
[PATCH 11/15] V4L/DVB: saa7134: clear warning noise
drivers/media/video/saa7134/saa7134-input.c: In function
‘saa7134_raw_decode_irq’:
drivers/media/video/saa7134/saa7134-input.c:957: warning: unused variable
‘oldpulse’
drivers/media/video/saa7134/saa7134-input.c:957: warning: unused variable
‘count’
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/video/saa7134/saa7134-input.c
b/drivers/media/video/saa7134/saa7134-input.c
index 8dd74ae..a42c953 100644
--- a/drivers/media/video/saa7134/saa7134-input.c
+++ b/drivers/media/video/saa7134/saa7134-input.c
@@ -954,7 +954,7 @@ static int saa7134_raw_decode_irq(struct saa7134_dev *dev)
{
struct card_ir *ir = dev-remote;
unsigned long timeout;
- int count, pulse, oldpulse;
+ int pulse;
/* Generate initial event */
saa_clearb(SAA7134_GPIO_GPMODE3, SAA7134_GPIO_GPRESCAN);
--
1.6.6.1
--
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
[PATCH 00/15] ir-core: Several improvements to allow adding LIRC and decoder plugins
This series of 15 patches improves support for IR, as discussed at the What are the goals for the architecture of an in-kernel IR system? thread. It basically adds a raw decoder layer at ir-core, allowing decoders to plug into IR core, and preparing for the addition of a lirc_dev driver that will allow raw IR codes to be sent to userspace. There's no lirc patch in this series. I have also a few other patches from David Härdeman that I'm about to test/review probably later today, but as I prefer to first merge what I have at V4L/DVB tree, before applying them. There are two patches on this series that deserve a better analysis, IMO: - V4L/DVB: ir-core: rename sysfs remote controller class from ir to rc As discussed, IR is not a good name, as this infrastructure could later be used by other types of Remote Controllers, as it has nothing that is specific to IR inside the code, except for the name. So, I'm proposing to replace the sysfs notes do rc, instead of ir. The sooner we do such changes, the better, as userspace apps using it are still under development. So, an API change is still possible, without causing much hurt. Also, as some RC devices allow RC code transmission, we probably need to add a TX node somewhere, associated with the same RX part (as some devices don't allow simultaneous usage of TX and RX). So, we have a few alternatives for the RC device sysfs node: a) /sys/class/rc/rc0 |-- rx --- tx b) /sys/class/rc/rcrcv0 /sys/class/rc/rctx0 c) /sys/class/rc/rc0 and have there the RX and TX nodes/attributes mixed. IMO, (b) is a bad idea, so, I am between (a) and (c). - V4L/DVB: input: Add support for EVIO[CS]GKEYCODEBIG Adds two new ioctls in order to handle with big keycode tables. As already said, we'll need another ioctl, in order to get the maximum keycode supported by a given device. I didn't wrote the patch for the new ioctl yet. This patch will probably have a small conflict with upstream input, but I prefer to keep it on my tree and fix the upstream conflicts when submiting it, as the rest of the new IR code is also on my tree, and this patch is needed to procced with the IR code development. Mauro Carvalho Chehab (15): V4L/DVB: ir-core: be less pedantic with RC protocol name V4L/DVB: saa7134: use a full scancode table for M135A V4L/DVB: saa7134: add code to allow changing IR protocol V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core V4L/DVB: ir-core: add two functions to report keyup/keydown events V4L/DVB: ir-core/saa7134: Move ir keyup/keydown code to the ir-core V4L/DVB: saa7134: don't wait too much to generate an IR event on raw_decode V4L/DVB: ir-core: dynamically load the compiled IR protocols V4L/DVB: ir-core: prepare to add more operations for ir decoders V4L/DVB: ir-nec-decoder: Add sysfs node to enable/disable per irrcv V4L/DVB: saa7134: clear warning noise V4L/DVB: ir-core: rename sysfs remote controller class from ir to rc V4L/DVB: ir-core: Add callbacks for input/evdev open/close on IR core V4L/DVB: cx88: Only start IR if the input device is opened V4L/DVB: input: Add support for EVIO[CS]GKEYCODEBIG drivers/input/evdev.c | 39 +++ drivers/input/input.c | 260 ++-- drivers/media/IR/Kconfig|9 + drivers/media/IR/Makefile |3 +- drivers/media/IR/ir-keymaps.c | 98 drivers/media/IR/ir-keytable.c | 75 ++- drivers/media/IR/ir-nec-decoder.c | 351 +++ drivers/media/IR/ir-raw-event.c | 231 ++ drivers/media/IR/ir-sysfs.c | 29 ++- drivers/media/video/cx88/cx88-input.c | 69 +- drivers/media/video/cx88/cx88-video.c |6 +- drivers/media/video/cx88/cx88.h |6 +- drivers/media/video/saa7134/saa7134-core.c |2 +- drivers/media/video/saa7134/saa7134-input.c | 207 +++-- drivers/media/video/saa7134/saa7134.h |4 +- include/linux/input.h | 40 +++- include/media/ir-common.h |9 +- include/media/ir-core.h | 59 +- 18 files changed, 1368 insertions(+), 129 deletions(-) create mode 100644 drivers/media/IR/ir-nec-decoder.c create mode 100644 drivers/media/IR/ir-raw-event.c -- 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
[PATCH 10/15] V4L/DVB: ir-nec-decoder: Add sysfs node to enable/disable per irrcv
With the help of raw_register/raw_unregister, adds a sysfs group
associated with the decoder, inside the corresponding irrcv node.
Writing 1 to nec_decoder/enabled enables the decoder, while
writing 0 disables it.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/ir-nec-decoder.c
b/drivers/media/IR/ir-nec-decoder.c
index cb57cc2..83a9912 100644
--- a/drivers/media/IR/ir-nec-decoder.c
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -39,6 +39,85 @@
#define REPEAT_TIME240 /* ms */
+/* Used to register nec_decoder clients */
+static LIST_HEAD(decoder_list);
+static spinlock_t decoder_lock;
+
+struct decoder_data {
+ struct list_headlist;
+ struct ir_input_dev *ir_dev;
+ int enabled:1;
+};
+
+
+/**
+ * get_decoder_data() - gets decoder data
+ * @input_dev: input device
+ *
+ * Returns the struct decoder_data that corresponds to a device
+ */
+
+static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev)
+{
+ struct decoder_data *data = NULL;
+
+ spin_lock(decoder_lock);
+ list_for_each_entry(data, decoder_list, list) {
+ if (data-ir_dev == ir_dev)
+ break;
+ }
+ spin_unlock(decoder_lock);
+ return data;
+}
+
+static ssize_t store_enabled(struct device *d,
+struct device_attribute *mattr,
+const char *buf,
+size_t len)
+{
+ unsigned long value;
+ struct ir_input_dev *ir_dev = dev_get_drvdata(d);
+ struct decoder_data *data = get_decoder_data(ir_dev);
+
+ if (!data)
+ return -EINVAL;
+
+ if (strict_strtoul(buf, 10, value) || value 1)
+ return -EINVAL;
+
+ data-enabled = value;
+
+ return len;
+}
+
+static ssize_t show_enabled(struct device *d,
+struct device_attribute *mattr, char *buf)
+{
+ struct ir_input_dev *ir_dev = dev_get_drvdata(d);
+ struct decoder_data *data = get_decoder_data(ir_dev);
+
+ if (!data)
+ return -EINVAL;
+
+ if (data-enabled)
+ return sprintf(buf, 1\n);
+ else
+ return sprintf(buf, 0\n);
+}
+
+static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled);
+
+static struct attribute *decoder_attributes[] = {
+ dev_attr_enabled.attr,
+ NULL
+};
+
+static struct attribute_group decoder_attribute_group = {
+ .name = nec_decoder,
+ .attrs = decoder_attributes,
+};
+
+
/** is_repeat - Check if it is a NEC repeat event
* @input_dev: the struct input_dev descriptor of the device
* @pos: the position of the first event
@@ -184,9 +263,15 @@ static int ir_nec_decode(struct input_dev *input_dev,
struct ir_raw_event *evs,
int len)
{
+ struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+ struct decoder_data *data;
int pos = 0;
int rc = 0;
+ data = get_decoder_data(ir_dev);
+ if (!data || !data-enabled)
+ return 0;
+
while (pos len) {
if (__ir_nec_decode(input_dev, evs, len, pos) 0)
rc++;
@@ -194,8 +279,54 @@ static int ir_nec_decode(struct input_dev *input_dev,
return rc;
}
+static int ir_nec_register(struct input_dev *input_dev)
+{
+ struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+ struct decoder_data *data;
+ int rc;
+
+ rc = sysfs_create_group(ir_dev-dev.kobj, decoder_attribute_group);
+ if (rc 0)
+ return rc;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ sysfs_remove_group(ir_dev-dev.kobj, decoder_attribute_group);
+ return -ENOMEM;
+ }
+
+ data-ir_dev = ir_dev;
+ data-enabled = 1;
+
+ spin_lock(decoder_lock);
+ list_add_tail(data-list, decoder_list);
+ spin_unlock(decoder_lock);
+
+ return 0;
+}
+
+static int ir_nec_unregister(struct input_dev *input_dev)
+{
+ struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+ static struct decoder_data *data;
+
+ data = get_decoder_data(ir_dev);
+ if (!data)
+ return 0;
+
+ sysfs_remove_group(ir_dev-dev.kobj, decoder_attribute_group);
+
+ spin_lock(decoder_lock);
+ list_del(data-list);
+ spin_unlock(decoder_lock);
+
+ return 0;
+}
+
static struct ir_raw_handler nec_handler = {
- .decode = ir_nec_decode,
+ .decode = ir_nec_decode,
+ .raw_register = ir_nec_register,
+ .raw_unregister = ir_nec_unregister,
};
static int __init ir_nec_decode_init(void)
diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
index 11f23f4..371d88e 100644
--- a/drivers/media/IR/ir-raw-event.c
+++ b/drivers/media/IR/ir-raw-event.c
@@ -223,7 +223,7
[PATCH 09/15] V4L/DVB: ir-core: prepare to add more operations for ir decoders
Some decoders and a lirc_dev interface may need some other operations to work.
For example: IR device register/unregister and ir_keydown events may need to
be tracked.
As some operations can occur in interrupt time, and a lock is needed to prevent
un-registering a decode while decoding a key, the lock needed to be convert
into a spin lock.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/ir-nec-decoder.c
b/drivers/media/IR/ir-nec-decoder.c
index c9a986d..cb57cc2 100644
--- a/drivers/media/IR/ir-nec-decoder.c
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -178,8 +178,7 @@ err:
* @input_dev: the struct input_dev descriptor of the device
* @evs: event array with type/duration of pulse/space
* @len: length of the array
- * This function returns the number of decoded pulses or -EINVAL if no
- * pulse got decoded
+ * This function returns the number of decoded pulses
*/
static int ir_nec_decode(struct input_dev *input_dev,
struct ir_raw_event *evs,
@@ -192,9 +191,6 @@ static int ir_nec_decode(struct input_dev *input_dev,
if (__ir_nec_decode(input_dev, evs, len, pos) 0)
rc++;
}
-
- if (!rc)
- return -EINVAL;
return rc;
}
diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
index 3eae128..11f23f4 100644
--- a/drivers/media/IR/ir-raw-event.c
+++ b/drivers/media/IR/ir-raw-event.c
@@ -14,13 +14,41 @@
#include media/ir-core.h
#include linux/workqueue.h
+#include linux/spinlock.h
/* Define the max number of bit transitions per IR keycode */
#define MAX_IR_EVENT_SIZE 256
/* Used to handle IR raw handler extensions */
static LIST_HEAD(ir_raw_handler_list);
-static DEFINE_MUTEX(ir_raw_handler_lock);
+static spinlock_t ir_raw_handler_lock;
+
+/**
+ * RUN_DECODER() - runs an operation on all IR decoders
+ * @ops: IR raw handler operation to be called
+ * @arg: arguments to be passed to the callback
+ *
+ * Calls ir_raw_handler::ops for all registered IR handlers. It prevents
+ * new decode addition/removal while running, by locking ir_raw_handler_lock
+ * mutex. If an error occurs, it stops the ops. Otherwise, it returns a sum
+ * of the return codes.
+ */
+#define RUN_DECODER(ops, ...) ({ \
+ struct ir_raw_handler *_ir_raw_handler; \
+ int _sumrc = 0, _rc;\
+ spin_lock(ir_raw_handler_lock);\
+ list_for_each_entry(_ir_raw_handler, ir_raw_handler_list, list) { \
+ if (_ir_raw_handler-ops) { \
+ _rc = _ir_raw_handler-ops(__VA_ARGS__);\
+ if (_rc 0)\
+ break; \
+ _sumrc += _rc; \
+ } \
+ } \
+ spin_unlock(ir_raw_handler_lock); \
+ _sumrc; \
+})
+
/* Used to load the decoders */
static struct work_struct wq_load;
@@ -38,6 +66,8 @@ int ir_raw_event_register(struct input_dev *input_dev)
int rc, size;
ir-raw = kzalloc(sizeof(*ir-raw), GFP_KERNEL);
+ if (!ir-raw)
+ return -ENOMEM;
size = sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE * 2;
size = roundup_pow_of_two(size);
@@ -48,6 +78,19 @@ int ir_raw_event_register(struct input_dev *input_dev)
set_bit(EV_REP, input_dev-evbit);
rc = kfifo_alloc(ir-raw-kfifo, size, GFP_KERNEL);
+ if (rc 0) {
+ kfree(ir-raw);
+ ir-raw = NULL;
+ return rc;
+ }
+
+ rc = RUN_DECODER(raw_register, input_dev);
+ if (rc 0) {
+ kfifo_free(ir-raw-kfifo);
+ kfree(ir-raw);
+ ir-raw = NULL;
+ return rc;
+ }
return rc;
}
@@ -62,6 +105,8 @@ void ir_raw_event_unregister(struct input_dev *input_dev)
del_timer_sync(ir-raw-timer_keyup);
+ RUN_DECODER(raw_unregister, input_dev);
+
kfifo_free(ir-raw-kfifo);
kfree(ir-raw);
ir-raw = NULL;
@@ -109,7 +154,6 @@ int ir_raw_event_handle(struct input_dev *input_dev)
int rc;
struct ir_raw_event *evs;
int len, i;
- struct ir_raw_handler *ir_raw_handler;
/*
* Store the events into a temporary buffer. This allows calling more
than
@@ -133,13 +177,10 @@ int ir_raw_event_handle(struct input_dev
[PATCH 08/15] V4L/DVB: ir-core: dynamically load the compiled IR protocols
Instead of hardcoding the protocols into ir-core, add a register interface
for the IR protocol decoders, and convert ir-nec-decoder into a client of
ir-core.
With this approach, it is possible to dynamically load the needed IR protocols,
and to add a RAW IR interface module, registered as one IR raw protocol decoder.
This patch opens a way to register a lirc_dev interface to work as an userspace
IR protocol decoder.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/Kconfig b/drivers/media/IR/Kconfig
index 4dde7d1..de410d4 100644
--- a/drivers/media/IR/Kconfig
+++ b/drivers/media/IR/Kconfig
@@ -7,3 +7,12 @@ config VIDEO_IR
tristate
depends on IR_CORE
default IR_CORE
+
+config IR_NEC_DECODER
+ tristate Enable IR raw decoder for NEC protocol
+ depends on IR_CORE
+ default y
+
+ ---help---
+ Enable this option if you have IR with NEC protocol, and
+ if the IR is decoded in software
diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
index 18794c7..6140b27 100644
--- a/drivers/media/IR/Makefile
+++ b/drivers/media/IR/Makefile
@@ -1,5 +1,6 @@
ir-common-objs := ir-functions.o ir-keymaps.o
-ir-core-objs := ir-keytable.o ir-sysfs.o ir-raw-event.o ir-nec-decoder.o
+ir-core-objs := ir-keytable.o ir-sysfs.o ir-raw-event.o
obj-$(CONFIG_IR_CORE) += ir-core.o
obj-$(CONFIG_VIDEO_IR) += ir-common.o
+obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
diff --git a/drivers/media/IR/ir-nec-decoder.c
b/drivers/media/IR/ir-nec-decoder.c
index 104482a..c9a986d 100644
--- a/drivers/media/IR/ir-nec-decoder.c
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -1,4 +1,4 @@
-/* ir-raw-event.c - handle IR Pulse/Space event
+/* ir-nec-decoder.c - handle NEC IR Pulse/Space protocol
*
* Copyright (C) 2010 by Mauro Carvalho Chehab mche...@redhat.com
*
@@ -147,7 +147,7 @@ static int __ir_nec_decode(struct input_dev *input_dev,
if (++count == 32)
break;
}
- *pos++;
+ (*pos)++;
/*
* Fixme: may need to accept Extended NEC protocol?
@@ -181,9 +181,9 @@ err:
* This function returns the number of decoded pulses or -EINVAL if no
* pulse got decoded
*/
-int ir_nec_decode(struct input_dev *input_dev,
- struct ir_raw_event *evs,
- int len)
+static int ir_nec_decode(struct input_dev *input_dev,
+struct ir_raw_event *evs,
+int len)
{
int pos = 0;
int rc = 0;
@@ -198,4 +198,27 @@ int ir_nec_decode(struct input_dev *input_dev,
return rc;
}
-EXPORT_SYMBOL_GPL(ir_nec_decode);
+static struct ir_raw_handler nec_handler = {
+ .decode = ir_nec_decode,
+};
+
+static int __init ir_nec_decode_init(void)
+{
+ ir_raw_handler_register(nec_handler);
+
+ printk(KERN_INFO IR NEC protocol handler initialized\n);
+ return 0;
+}
+
+static void __exit ir_nec_decode_exit(void)
+{
+ ir_raw_handler_unregister(nec_handler);
+}
+
+module_init(ir_nec_decode_init);
+module_exit(ir_nec_decode_exit);
+
+MODULE_LICENSE(GPL);
+MODULE_AUTHOR(Mauro Carvalho Chehab mche...@redhat.com);
+MODULE_AUTHOR(Red Hat Inc. (http://www.redhat.com));
+MODULE_DESCRIPTION(NEC IR protocol decoder);
diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
index 0ae5543..3eae128 100644
--- a/drivers/media/IR/ir-raw-event.c
+++ b/drivers/media/IR/ir-raw-event.c
@@ -13,10 +13,18 @@
*/
#include media/ir-core.h
+#include linux/workqueue.h
/* Define the max number of bit transitions per IR keycode */
#define MAX_IR_EVENT_SIZE 256
+/* Used to handle IR raw handler extensions */
+static LIST_HEAD(ir_raw_handler_list);
+static DEFINE_MUTEX(ir_raw_handler_lock);
+
+/* Used to load the decoders */
+static struct work_struct wq_load;
+
static void ir_keyup_timer(unsigned long data)
{
struct input_dev *input_dev = (struct input_dev *)data;
@@ -101,6 +109,7 @@ int ir_raw_event_handle(struct input_dev *input_dev)
int rc;
struct ir_raw_event *evs;
int len, i;
+ struct ir_raw_handler *ir_raw_handler;
/*
* Store the events into a temporary buffer. This allows calling more
than
@@ -122,10 +131,56 @@ int ir_raw_event_handle(struct input_dev *input_dev)
evs[i].type, (evs[i].delta.tv_nsec + 500) / 1000);
}
- rc = ir_nec_decode(input_dev, evs, len);
+ /*
+* Call all ir decoders. This allows decoding the same event with
+* more than one protocol handler.
+* FIXME: better handle the returned code: does it make sense to use
+* other decoders, if the first one already handled the IR?
+*/
+ list_for_each_entry(ir_raw_handler, ir_raw_handler_list, list) {
+ rc =
[PATCH 07/15] V4L/DVB: saa7134: don't wait too much to generate an IR event on raw_decode
At raw_decode mode, the key is processed after the end of a timer. The
previous code resets the timer every time something is received at the IR
port. While this works fine with IR's that don't implement repeat, like
Avermedia RM-JX IR, it keeps waiting until keydown, on IR's that implement
NEC repeat command, like the Terratec yellow.
The solution is to change the behaviour to do the timeout after the first
received data.
The timeout is currently set to 15 ms, as it works fine with NEC protcocol.
It may need some adjustments to support other protocols and to better handle
spurious detections that may happen with some IR sensors.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
index ab60730..f25193c 100644
--- a/drivers/media/IR/ir-keytable.c
+++ b/drivers/media/IR/ir-keytable.c
@@ -404,6 +404,7 @@ void ir_keyup(struct input_dev *dev)
if (!ir-keypressed)
return;
+ IR_dprintk(1, keyup key 0x%04x\n, ir-keycode);
input_report_key(dev, ir-keycode, 0);
input_sync(dev);
ir-keypressed = 0;
diff --git a/drivers/media/IR/ir-nec-decoder.c
b/drivers/media/IR/ir-nec-decoder.c
index a58c717..104482a 100644
--- a/drivers/media/IR/ir-nec-decoder.c
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -14,21 +14,51 @@
#include media/ir-core.h
-/* Start time: 4.5 ms */
-#define MIN_START_TIME 390
-#define MAX_START_TIME 510
+/* Start time: 4.5 ms + 560 us of the next pulse */
+#define MIN_START_TIME (390 + 56)
+#define MAX_START_TIME (510 + 56)
-/* Pulse time: 560 us */
-#define MIN_PULSE_TIME 46
-#define MAX_PULSE_TIME 66
+/* Bit 1 time: 2.25ms us */
+#define MIN_BIT1_TIME 205
+#define MAX_BIT1_TIME 245
-/* Bit 1 space time: 2.25ms-560 us */
-#define MIN_BIT1_TIME 149
-#define MAX_BIT1_TIME 189
+/* Bit 0 time: 1.12ms us */
+#define MIN_BIT0_TIME 92
+#define MAX_BIT0_TIME 132
-/* Bit 0 space time: 1.12ms-560 us */
-#define MIN_BIT0_TIME 36
-#define MAX_BIT0_TIME 76
+/* Total IR code is 110 ms, including the 9 ms for the start pulse */
+#define MAX_NEC_TIME 400
+
+/* Total IR code is 110 ms, including the 9 ms for the start pulse */
+#define MIN_REPEAT_TIME9900
+#define MAX_REPEAT_TIME11200
+
+/* Repeat time: 2.25ms us */
+#define MIN_REPEAT_START_TIME 205
+#define MAX_REPEAT_START_TIME 300
+
+#define REPEAT_TIME240 /* ms */
+
+/** is_repeat - Check if it is a NEC repeat event
+ * @input_dev: the struct input_dev descriptor of the device
+ * @pos: the position of the first event
+ * @len: the length of the buffer
+ */
+static int is_repeat(struct ir_raw_event *evs, int len, int pos)
+{
+ if ((evs[pos].delta.tv_nsec MIN_REPEAT_START_TIME) ||
+ (evs[pos].delta.tv_nsec MAX_REPEAT_START_TIME))
+ return 0;
+
+ if (++pos = len)
+ return 0;
+
+ if ((evs[pos].delta.tv_nsec MIN_REPEAT_TIME) ||
+ (evs[pos].delta.tv_nsec MAX_REPEAT_TIME))
+ return 0;
+
+ return 1;
+}
/**
* __ir_nec_decode() - Decode one NEC pulsecode
@@ -36,49 +66,59 @@
* @evs: event array with type/duration of pulse/space
* @len: length of the array
* @pos: position to start seeking for a code
- * This function returns the decoded ircode or -EINVAL if no pulse got decoded
+ * This function returns -EINVAL if no pulse got decoded,
+ * 0 if buffer is empty and 1 if one keycode were handled.
*/
static int __ir_nec_decode(struct input_dev *input_dev,
struct ir_raw_event *evs,
int len, int *pos)
{
+ struct ir_input_dev *ir = input_get_drvdata(input_dev);
int count = -1;
int ircode = 0, not_code = 0;
/* Be sure that the first event is an start one and is a pulse */
for (; *pos len; (*pos)++) {
- if (evs[*pos].type (IR_START_EVENT | IR_PULSE))
+ /* Very long delays are considered as start events */
+ if (evs[*pos].delta.tv_nsec MAX_NEC_TIME)
break;
- }
- (*pos)++; /* First event doesn't contain data */
+ if (evs[*pos].type IR_START_EVENT)
+ break;
+ IR_dprintk(1, %luus: Spurious NEC %s\n,
+ (evs[*pos].delta.tv_nsec + 500) / 1000,
+ (evs[*pos].type IR_SPACE) ? space : pulse);
+ }
if (*pos = len)
return 0;
+ (*pos)++; /* First event doesn't contain data */
+
+ if (evs[*pos].type != IR_PULSE)
+ goto err;
+
+ /* Check if it is a NEC repeat event */
+ if (is_repeat(evs, len, *pos)) {
+ *pos += 2;
+ if (ir-keypressed) {
+ mod_timer(ir-raw-timer_keyup,
+
[PATCH 13/15] V4L/DVB: ir-core: Add callbacks for input/evdev open/close on IR core
Especially when IR needs to do polling, it generates lots of wakeups per
second. This makes no sense, if the input event device is closed.
Adds a callback handler to the IR hardware driver, to allow registering
an open/close ops.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
index f25193c..db54562 100644
--- a/drivers/media/IR/ir-keytable.c
+++ b/drivers/media/IR/ir-keytable.c
@@ -445,6 +445,19 @@ void ir_keydown(struct input_dev *dev, int scancode)
}
EXPORT_SYMBOL_GPL(ir_keydown);
+static int ir_open(struct input_dev *input_dev)
+{
+ struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+
+ return ir_dev-props-open(ir_dev-props-priv);
+}
+
+static void ir_close(struct input_dev *input_dev)
+{
+ struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+
+ ir_dev-props-close(ir_dev-props-priv);
+}
/**
* ir_input_register() - sets the IR keycode table and add the handlers
@@ -494,6 +507,10 @@ int ir_input_register(struct input_dev *input_dev,
ir_copy_table(ir_dev-rc_tab, rc_tab);
ir_dev-props = props;
+ if (props props-open)
+ input_dev-open = ir_open;
+ if (props props-close)
+ input_dev-close = ir_close;
/* set the bits for the keys */
IR_dprintk(1, key map size: %d\n, rc_tab-size);
diff --git a/drivers/media/video/saa7134/saa7134-core.c
b/drivers/media/video/saa7134/saa7134-core.c
index a7ad781..68cda10 100644
--- a/drivers/media/video/saa7134/saa7134-core.c
+++ b/drivers/media/video/saa7134/saa7134-core.c
@@ -1227,7 +1227,7 @@ static int saa7134_resume(struct pci_dev *pci_dev)
if (card_has_mpeg(dev))
saa7134_ts_init_hw(dev);
if (dev-remote)
- saa7134_ir_start(dev, dev-remote);
+ saa7134_ir_start(dev);
saa7134_hw_enable1(dev);
msleep(100);
diff --git a/drivers/media/video/saa7134/saa7134-input.c
b/drivers/media/video/saa7134/saa7134-input.c
index a42c953..9e2b32c 100644
--- a/drivers/media/video/saa7134/saa7134-input.c
+++ b/drivers/media/video/saa7134/saa7134-input.c
@@ -399,7 +399,14 @@ static int get_key_pinnacle_color(struct IR_i2c *ir, u32
*ir_key, u32 *ir_raw)
void saa7134_input_irq(struct saa7134_dev *dev)
{
- struct card_ir *ir = dev-remote;
+ struct card_ir *ir;
+
+ if (!dev || !dev-remote)
+ return;
+
+ ir = dev-remote;
+ if (!ir-running)
+ return;
if (ir-nec_gpio) {
saa7134_nec_irq(dev);
@@ -431,10 +438,20 @@ void ir_raw_decode_timer_end(unsigned long data)
ir-active = 0;
}
-void saa7134_ir_start(struct saa7134_dev *dev, struct card_ir *ir)
+static int __saa7134_ir_start(void *priv)
{
+ struct saa7134_dev *dev = priv;
+ struct card_ir *ir;
+
+ if (!dev)
+ return -EINVAL;
+
+ ir = dev-remote;
+ if (!ir)
+ return -EINVAL;
+
if (ir-running)
- return;
+ return 0;
ir-running = 1;
if (ir-polling) {
@@ -466,11 +483,21 @@ void saa7134_ir_start(struct saa7134_dev *dev, struct
card_ir *ir)
ir-timer_end.data = (unsigned long)dev;
ir-active = 0;
}
+
+ return 0;
}
-void saa7134_ir_stop(struct saa7134_dev *dev)
+static void __saa7134_ir_stop(void *priv)
{
- struct card_ir *ir = dev-remote;
+ struct saa7134_dev *dev = priv;
+ struct card_ir *ir;
+
+ if (!dev)
+ return;
+
+ ir = dev-remote;
+ if (!ir)
+ return;
if (!ir-running)
return;
@@ -486,8 +513,42 @@ void saa7134_ir_stop(struct saa7134_dev *dev)
}
ir-running = 0;
+
+ return;
}
+int saa7134_ir_start(struct saa7134_dev *dev)
+{
+ if (dev-remote-users)
+ return __saa7134_ir_start(dev);
+
+ return 0;
+}
+
+void saa7134_ir_stop(struct saa7134_dev *dev)
+{
+ if (dev-remote-users)
+ __saa7134_ir_stop(dev);
+}
+
+static int saa7134_ir_open(void *priv)
+{
+ struct saa7134_dev *dev = priv;
+
+ dev-remote-users++;
+ return __saa7134_ir_start(dev);
+}
+
+static void saa7134_ir_close(void *priv)
+{
+ struct saa7134_dev *dev = priv;
+
+ dev-remote-users--;
+ if (!dev-remote-users)
+ __saa7134_ir_stop(dev);
+}
+
+
int saa7134_ir_change_protocol(void *priv, u64 ir_type)
{
struct saa7134_dev *dev = priv;
@@ -512,7 +573,7 @@ int saa7134_ir_change_protocol(void *priv, u64 ir_type)
saa7134_ir_stop(dev);
ir-nec_gpio = nec_gpio;
ir-rc5_gpio = rc5_gpio;
- saa7134_ir_start(dev, ir);
+ saa7134_ir_start(dev);
} else {
ir-nec_gpio = nec_gpio;
ir-rc5_gpio = rc5_gpio;
@@ -787,9 +848,13 @@ int
[PATCH 03/15] V4L/DVB: saa7134: add code to allow changing IR protocol
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/video/saa7134/saa7134-input.c
b/drivers/media/video/saa7134/saa7134-input.c
index d7e73de..8187928 100644
--- a/drivers/media/video/saa7134/saa7134-input.c
+++ b/drivers/media/video/saa7134/saa7134-input.c
@@ -420,6 +420,10 @@ static void saa7134_input_timer(unsigned long data)
void saa7134_ir_start(struct saa7134_dev *dev, struct card_ir *ir)
{
+ if (ir-running)
+ return;
+
+ ir-running = 1;
if (ir-polling) {
setup_timer(ir-timer, saa7134_input_timer,
(unsigned long)dev);
@@ -447,8 +451,50 @@ void saa7134_ir_start(struct saa7134_dev *dev, struct
card_ir *ir)
void saa7134_ir_stop(struct saa7134_dev *dev)
{
+ struct card_ir *ir = dev-remote;
+
+ if (!ir-running)
+ return;
if (dev-remote-polling)
del_timer_sync(dev-remote-timer);
+ else if (ir-rc5_gpio)
+ del_timer_sync(ir-timer_end);
+ else if (ir-nec_gpio)
+ tasklet_kill(ir-tlet);
+ ir-running = 0;
+}
+
+int saa7134_ir_change_protocol(void *priv, u64 ir_type)
+{
+ struct saa7134_dev *dev = priv;
+ struct card_ir *ir = dev-remote;
+ u32 nec_gpio, rc5_gpio;
+
+ if (ir_type == IR_TYPE_RC5) {
+ dprintk(Changing protocol to RC5\n);
+ nec_gpio = 0;
+ rc5_gpio = 1;
+ } else if (ir_type == IR_TYPE_NEC) {
+ dprintk(Changing protocol to NEC\n);
+ nec_gpio = 1;
+ rc5_gpio = 0;
+ } else {
+ dprintk(IR protocol type %ud is not supported\n,
+ (unsigned)ir_type);
+ return -EINVAL;
+ }
+
+ if (ir-running) {
+ saa7134_ir_stop(dev);
+ ir-nec_gpio = nec_gpio;
+ ir-rc5_gpio = rc5_gpio;
+ saa7134_ir_start(dev, ir);
+ } else {
+ ir-nec_gpio = nec_gpio;
+ ir-rc5_gpio = rc5_gpio;
+ }
+
+ return 0;
}
int saa7134_input_init1(struct saa7134_dev *dev)
@@ -697,6 +743,9 @@ int saa7134_input_init1(struct saa7134_dev *dev)
}
ir-dev = input_dev;
+ dev-remote = ir;
+
+ ir-running = 0;
/* init hardware-specific stuff */
ir-mask_keycode = mask_keycode;
@@ -712,6 +761,14 @@ int saa7134_input_init1(struct saa7134_dev *dev)
snprintf(ir-phys, sizeof(ir-phys), pci-%s/ir0,
pci_name(dev-pci));
+ if (ir_codes-ir_type != IR_TYPE_OTHER) {
+ ir-props.allowed_protos = IR_TYPE_RC5 | IR_TYPE_NEC;
+ ir-props.priv = dev;
+ ir-props.change_protocol = saa7134_ir_change_protocol;
+
+ /* Set IR protocol */
+ saa7134_ir_change_protocol(ir-props.priv, ir_codes-ir_type);
+ }
err = ir_input_init(input_dev, ir-ir, ir_type);
if (err 0)
goto err_out_free;
@@ -729,13 +786,12 @@ int saa7134_input_init1(struct saa7134_dev *dev)
}
input_dev-dev.parent = dev-pci-dev;
- dev-remote = ir;
- saa7134_ir_start(dev, ir);
-
- err = ir_input_register(ir-dev, ir_codes, NULL, MODULE_NAME);
+ err = ir_input_register(ir-dev, ir_codes, ir-props, MODULE_NAME);
if (err)
goto err_out_stop;
+ saa7134_ir_start(dev, ir);
+
/* the remote isn't as bouncy as a keyboard */
ir-dev-rep[REP_DELAY] = repeat_delay;
ir-dev-rep[REP_PERIOD] = repeat_period;
diff --git a/include/media/ir-common.h b/include/media/ir-common.h
index c30b283..41469b7 100644
--- a/include/media/ir-common.h
+++ b/include/media/ir-common.h
@@ -51,6 +51,9 @@ struct card_ir {
charname[32];
charphys[32];
+ u32 running:1;
+ struct ir_dev_props props;
+
/* Usual gpio signalling */
u32 mask_keycode;
--
1.6.6.1
--
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
[PATCH 15/15] V4L/DVB: input: Add support for EVIO[CS]GKEYCODEBIG
Several devices use a high number of bits for scancodes. One important
group is the Remote Controllers. Some new protocols like RC-6 define a
scancode space of 64 bits.
The current EVIO[CS]GKEYCODE ioctls allow replace the scancode/keycode
translation tables, but it is limited to up to 32 bits for scancode.
Also, if userspace wants to clean the existing table, replacing it by
a new one, it needs to run a loop calling the old ioctls, over the
entire sparsed scancode userspace.
To solve those problems, this patch introduces two new ioctls:
EVIOCGKEYCODEBIG - reads a scancode from the translation table;
EVIOSGKEYCODEBIG - writes a scancode into the translation table.
The EVIOSGKEYCODEBIG can also be used to cleanup the translation entries
by associating KEY_RESERVED to a scancode.
EVIOCGKEYCODEBIG uses kt_entry::index field in order to retrieve a keycode
from the table. This field is unused on EVIOSGKEYCODEBIG.
By default, kernel will implement a default handler that will work with
both EVIO[CS]GKEYCODEBIG and the legacy EVIO[CS]GKEYCODE ioctls.
Compatibility code were also added to allow drivers that implement
only the ops handler for EVIO[CS]GKEYCODE to keep working.
Userspace compatibility for EVIO[CS]GKEYCODE is also granted: the evdev/input
ioctl handler will automatically map those ioctls with the new
getkeycodebig()/setkeycodebig() operations to handle a request using the
legacy API.
So, new drivers should only implement the EVIO[CS]GKEYCODEBIG operation
handlers: getkeycodebig()/setkeycodebig().
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 258c639..1730b5b 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -513,6 +513,8 @@ static long evdev_do_ioctl(struct file *file, unsigned int
cmd,
struct input_absinfo abs;
struct ff_effect effect;
int __user *ip = (int __user *)p;
+ struct keycode_table_entry kt, *kt_p = p;
+ char scancode[16];
int i, t, u, v;
int error;
@@ -567,6 +569,43 @@ static long evdev_do_ioctl(struct file *file, unsigned int
cmd,
return input_set_keycode(dev, t, v);
+ case EVIOCGKEYCODEBIG:
+ if (copy_from_user(kt, kt_p, sizeof(kt)))
+ return -EFAULT;
+
+ if (kt.len sizeof(scancode))
+ return -EINVAL;
+
+ kt.scancode = scancode;
+
+ error = input_get_keycode_big(dev, kt);
+ if (error)
+ return error;
+
+ if (copy_to_user(kt_p, kt, sizeof(kt)))
+ return -EFAULT;
+
+ /* FIXME: probably need some compat32 code */
+ if (copy_to_user(kt_p-scancode, kt.scancode, kt.len))
+ return -EFAULT;
+
+ return 0;
+
+ case EVIOCSKEYCODEBIG:
+ if (copy_from_user(kt, kt_p, sizeof(kt)))
+ return -EFAULT;
+
+ if (kt.len sizeof(scancode))
+ return -EINVAL;
+
+ kt.scancode = scancode;
+
+ /* FIXME: probably need some compat32 code */
+ if (copy_from_user(kt.scancode, kt_p-scancode, kt.len))
+ return -EFAULT;
+
+ return input_set_keycode_big(dev, kt);
+
case EVIOCRMFF:
return input_ff_erase(dev, (int)(unsigned long) p, file);
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 86cb2d2..d2bb5b5 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -551,6 +551,11 @@ static void input_disconnect_device(struct input_dev *dev)
spin_unlock_irq(dev-event_lock);
}
+/*
+ * Those routines handle the default case where no [gs]etkeycode() is
+ * defined. In this case, an array indexed by the scancode is used.
+ */
+
static int input_fetch_keycode(struct input_dev *dev, int scancode)
{
switch (dev-keycodesize) {
@@ -565,25 +570,74 @@ static int input_fetch_keycode(struct input_dev *dev, int
scancode)
}
}
-static int input_default_getkeycode(struct input_dev *dev,
- int scancode, int *keycode)
+/*
+ * Supports only 8, 16 and 32 bit scancodes. It wouldn't be that
+ * hard to write some machine-endian logic to support 24 bit scancodes,
+ * but it seemed overkill. It should also be noticed that, since there
+ * are, in general, less than 256 scancodes sparsed into the scancode
+ * space, even with 16 bits, the codespace is sparsed, with leads into
+ * memory and code ineficiency, when retrieving the entire scancode
+ * space.
+ * So, it is highly recommended to implement getkeycodebig/setkeycodebig
+ * instead of using a normal table approach, when more than 8 bits is
+ * needed for the scancode.
+ */
+static int input_fetch_scancode(struct keycode_table_entry *kt_entry,
+ u32 *scancode)
{
+
[PATCH 02/15] V4L/DVB: saa7134: use a full scancode table for M135A
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/ir-keymaps.c b/drivers/media/IR/ir-keymaps.c
index dfc777b..55e7acd 100644
--- a/drivers/media/IR/ir-keymaps.c
+++ b/drivers/media/IR/ir-keymaps.c
@@ -122,55 +122,57 @@ static struct ir_scancode ir_codes_avermedia_dvbt[] = {
};
IR_TABLE(avermedia_dvbt, IR_TYPE_UNKNOWN, ir_codes_avermedia_dvbt);
-/* Mauro Carvalho Chehab mche...@infradead.org */
-static struct ir_scancode ir_codes_avermedia_m135a[] = {
- { 0x00, KEY_POWER2 },
- { 0x2e, KEY_DOT }, /* '.' */
- { 0x01, KEY_MODE }, /* TV/FM */
-
- { 0x05, KEY_1 },
- { 0x06, KEY_2 },
- { 0x07, KEY_3 },
- { 0x09, KEY_4 },
- { 0x0a, KEY_5 },
- { 0x0b, KEY_6 },
- { 0x0d, KEY_7 },
- { 0x0e, KEY_8 },
- { 0x0f, KEY_9 },
- { 0x11, KEY_0 },
-
- { 0x13, KEY_RIGHT },/* - */
- { 0x12, KEY_LEFT }, /* - */
-
- { 0x17, KEY_SLEEP },/* Capturar Imagem */
- { 0x10, KEY_SHUFFLE }, /* Amostra */
-
- /* FIXME: The keys bellow aren't ok */
-
- { 0x43, KEY_CHANNELUP },
- { 0x42, KEY_CHANNELDOWN },
- { 0x1f, KEY_VOLUMEUP },
- { 0x1e, KEY_VOLUMEDOWN },
- { 0x0c, KEY_ENTER },
-
- { 0x14, KEY_MUTE },
- { 0x08, KEY_AUDIO },
-
- { 0x03, KEY_TEXT },
- { 0x04, KEY_EPG },
- { 0x2b, KEY_TV2 }, /* TV2 */
-
- { 0x1d, KEY_RED },
- { 0x1c, KEY_YELLOW },
- { 0x41, KEY_GREEN },
- { 0x40, KEY_BLUE },
-
- { 0x1a, KEY_PLAYPAUSE },
- { 0x19, KEY_RECORD },
- { 0x18, KEY_PLAY },
- { 0x1b, KEY_STOP },
+/*
+ * Avermedia M135A with IR model RM-JX
+ * The same codes exist on both Positivo (BR) and original IR
+ * Mauro Carvalho Chehab mche...@infradead.org
+ */
+static struct ir_scancode ir_codes_avermedia_m135a_rm_jx[] = {
+ { 0x0200, KEY_POWER2 },
+ { 0x022e, KEY_DOT },/* '.' */
+ { 0x0201, KEY_MODE }, /* TV/FM or SOURCE */
+
+ { 0x0205, KEY_1 },
+ { 0x0206, KEY_2 },
+ { 0x0207, KEY_3 },
+ { 0x0209, KEY_4 },
+ { 0x020a, KEY_5 },
+ { 0x020b, KEY_6 },
+ { 0x020d, KEY_7 },
+ { 0x020e, KEY_8 },
+ { 0x020f, KEY_9 },
+ { 0x0211, KEY_0 },
+
+ { 0x0213, KEY_RIGHT }, /* - or L */
+ { 0x0212, KEY_LEFT }, /* - or R */
+
+ { 0x0217, KEY_SLEEP }, /* Capturar Imagem or Snapshot */
+ { 0x0210, KEY_SHUFFLE },/* Amostra or 16 chan prev */
+
+ { 0x0303, KEY_CHANNELUP },
+ { 0x0302, KEY_CHANNELDOWN },
+ { 0x021f, KEY_VOLUMEUP },
+ { 0x021e, KEY_VOLUMEDOWN },
+ { 0x020c, KEY_ENTER }, /* Full Screen */
+
+ { 0x0214, KEY_MUTE },
+ { 0x0208, KEY_AUDIO },
+
+ { 0x0203, KEY_TEXT }, /* Teletext */
+ { 0x0204, KEY_EPG },
+ { 0x022b, KEY_TV2 },/* TV2 or PIP */
+
+ { 0x021d, KEY_RED },
+ { 0x021c, KEY_YELLOW },
+ { 0x0301, KEY_GREEN },
+ { 0x0300, KEY_BLUE },
+
+ { 0x021a, KEY_PLAYPAUSE },
+ { 0x0219, KEY_RECORD },
+ { 0x0218, KEY_PLAY },
+ { 0x021b, KEY_STOP },
};
-IR_TABLE(avermedia_m135a, IR_TYPE_UNKNOWN, ir_codes_avermedia_m135a);
+IR_TABLE(aver-m135a-RM-JX, IR_TYPE_NEC, ir_codes_avermedia_m135a_rm_jx);
/* Oldrich Jedlicka oldium@seznam.cz */
static struct ir_scancode ir_codes_avermedia_cardbus[] = {
diff --git a/drivers/media/video/saa7134/saa7134-input.c
b/drivers/media/video/saa7134/saa7134-input.c
index 8c67904..d7e73de 100644
--- a/drivers/media/video/saa7134/saa7134-input.c
+++ b/drivers/media/video/saa7134/saa7134-input.c
@@ -524,9 +524,9 @@ int saa7134_input_init1(struct saa7134_dev *dev)
saa_setb(SAA7134_GPIO_GPSTATUS0, 0x4);
break;
case SAA7134_BOARD_AVERMEDIA_M135A:
- ir_codes = ir_codes_avermedia_m135a_table;
+ ir_codes = ir_codes_avermedia_m135a_rm_jx_table;
mask_keydown = 0x004;
- mask_keycode = 0x00013f;
+ mask_keycode = 0x;
nec_gpio = 1;
break;
case SAA7134_BOARD_AVERMEDIA_777:
diff --git a/include/media/ir-common.h b/include/media/ir-common.h
index c662980..c30b283 100644
--- a/include/media/ir-common.h
+++ b/include/media/ir-common.h
@@ -102,7 +102,7 @@ void ir_rc5_timer_keyup(unsigned long data);
extern struct ir_scancode_table ir_codes_empty_table;
extern struct ir_scancode_table ir_codes_avermedia_table;
extern struct ir_scancode_table ir_codes_avermedia_dvbt_table;
-extern struct ir_scancode_table ir_codes_avermedia_m135a_table;
+extern struct ir_scancode_table ir_codes_avermedia_m135a_rm_jx_table;
extern struct ir_scancode_table ir_codes_avermedia_cardbus_table;
extern struct ir_scancode_table ir_codes_apac_viewcomp_table;
extern struct
[PATCH 01/15] V4L/DVB: ir-core: be less pedantic with RC protocol name
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com diff --git a/drivers/media/IR/ir-sysfs.c b/drivers/media/IR/ir-sysfs.c index bbddf2f..ee6b36d 100644 --- a/drivers/media/IR/ir-sysfs.c +++ b/drivers/media/IR/ir-sysfs.c @@ -92,7 +92,7 @@ static ssize_t store_protocol(struct device *d, buf = strsep((char **) data, \n); - if (!strcasecmp(buf, rc-5)) + if (!strcasecmp(buf, rc-5) || !strcasecmp(buf, rc5)) ir_type = IR_TYPE_RC5; else if (!strcasecmp(buf, pd)) ir_type = IR_TYPE_PD; -- 1.6.6.1 -- 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
[PATCH 12/15] V4L/DVB: ir-core: rename sysfs remote controller class from ir to rc
IR is an alias for Infrared Remote, while RC is an alias for Remote
Controller.
While currently all implementations are with Infrared Remote Controller,
this subsystem is not meant to be used only by IR type of RC's. So,
as discussed on both linux-media and linux-input, the better is to
rename the subsystem as Remote Controller.
While, currently, the only application that uses the /sys/class/irrcv is
ir-keytable application, and its sysfs support works only with the
current linux-next code, it is still possible to change the userspace API
without the risk of breaking applications. So, better to rename this
sooner than later.
Later patches will be needed to rename the files and to move them away
from drivers/media, but this is not a critical issue. So, for now,
let's just change the name of the sysfs class/nodes.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/ir-sysfs.c b/drivers/media/IR/ir-sysfs.c
index 2dbce59..435d83e 100644
--- a/drivers/media/IR/ir-sysfs.c
+++ b/drivers/media/IR/ir-sysfs.c
@@ -21,14 +21,14 @@
/* bit array to represent IR sysfs device number */
static unsigned long ir_core_dev_number;
-/* class for /sys/class/irrcv */
+/* class for /sys/class/rc */
static char *ir_devnode(struct device *dev, mode_t *mode)
{
- return kasprintf(GFP_KERNEL, irrcv/%s, dev_name(dev));
+ return kasprintf(GFP_KERNEL, rc/%s, dev_name(dev));
}
static struct class ir_input_class = {
- .name = irrcv,
+ .name = rc,
.devnode= ir_devnode,
};
@@ -39,7 +39,7 @@ static struct class ir_input_class = {
* @buf: a pointer to the output buffer
*
* This routine is a callback routine for input read the IR protocol type.
- * it is trigged by reading /sys/class/irrcv/irrcv?/current_protocol.
+ * it is trigged by reading /sys/class/rc/rcrcv?/current_protocol.
* It returns the protocol name, as understood by the driver.
*/
static ssize_t show_protocol(struct device *d,
@@ -74,7 +74,7 @@ static ssize_t show_protocol(struct device *d,
* @len: length of the input buffer
*
* This routine is a callback routine for changing the IR protocol type.
- * it is trigged by reading /sys/class/irrcv/irrcv?/current_protocol.
+ * it is trigged by reading /sys/class/rc/rcrcv?/current_protocol.
* It changes the IR the protocol name, if the IR type is recognized
* by the driver.
* If an unknown protocol name is used, returns -EINVAL.
@@ -171,7 +171,7 @@ static struct device_type ir_dev_type = {
};
/**
- * ir_register_class() - creates the sysfs for /sys/class/irrcv/irrcv?
+ * ir_register_class() - creates the sysfs for /sys/class/rc/rcrcv?
* @input_dev: the struct input_dev descriptor of the device
*
* This routine is used to register the syfs code for IR class
@@ -191,7 +191,7 @@ int ir_register_class(struct input_dev *input_dev)
ir_dev-dev.type = ir_dev_type;
ir_dev-dev.class = ir_input_class;
ir_dev-dev.parent = input_dev-dev.parent;
- dev_set_name(ir_dev-dev, irrcv%d, devno);
+ dev_set_name(ir_dev-dev, rcrcv%d, devno);
dev_set_drvdata(ir_dev-dev, ir_dev);
rc = device_register(ir_dev-dev);
if (rc)
@@ -222,7 +222,7 @@ int ir_register_class(struct input_dev *input_dev)
/**
* ir_unregister_class() - removes the sysfs for sysfs for
- */sys/class/irrcv/irrcv?
+ */sys/class/rc/rcrcv?
* @input_dev: the struct input_dev descriptor of the device
*
* This routine is used to unregister the syfs code for IR class
@@ -239,14 +239,14 @@ void ir_unregister_class(struct input_dev *input_dev)
}
/*
- * Init/exit code for the module. Basically, creates/removes /sys/class/irrcv
+ * Init/exit code for the module. Basically, creates/removes /sys/class/rc
*/
static int __init ir_core_init(void)
{
int rc = class_register(ir_input_class);
if (rc) {
- printk(KERN_ERR ir_core: unable to register irrcv class\n);
+ printk(KERN_ERR ir_core: unable to register rc class\n);
return rc;
}
--
1.6.6.1
--
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
[PATCH 04/15] V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core
Adds a method to pass IR raw pulse/code events into ir-core. This is
needed in order to support LIRC. It also helps to move common code
from the drivers into the core.
In order to allow testing, it implements a simple NEC protocol decoder
at ir-nec-decoder.c file. The logic is about the same used at saa7134
driver that handles Avermedia M135A and Encore FM53 boards.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
create mode 100644 drivers/media/IR/ir-nec-decoder.c
create mode 100644 drivers/media/IR/ir-raw-event.c
diff --git a/drivers/media/IR/Makefile b/drivers/media/IR/Makefile
index 171890e..18794c7 100644
--- a/drivers/media/IR/Makefile
+++ b/drivers/media/IR/Makefile
@@ -1,5 +1,5 @@
ir-common-objs := ir-functions.o ir-keymaps.o
-ir-core-objs := ir-keytable.o ir-sysfs.o
+ir-core-objs := ir-keytable.o ir-sysfs.o ir-raw-event.o ir-nec-decoder.o
obj-$(CONFIG_IR_CORE) += ir-core.o
obj-$(CONFIG_VIDEO_IR) += ir-common.o
diff --git a/drivers/media/IR/ir-nec-decoder.c
b/drivers/media/IR/ir-nec-decoder.c
new file mode 100644
index 000..16360eb
--- /dev/null
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -0,0 +1,131 @@
+/* ir-raw-event.c - handle IR Pulse/Space event
+ *
+ * Copyright (C) 2010 by Mauro Carvalho Chehab mche...@redhat.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 version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include media/ir-core.h
+
+/* Start time: 4.5 ms */
+#define MIN_START_TIME 390
+#define MAX_START_TIME 510
+
+/* Pulse time: 560 us */
+#define MIN_PULSE_TIME 46
+#define MAX_PULSE_TIME 66
+
+/* Bit 1 space time: 2.25ms-560 us */
+#define MIN_BIT1_TIME 149
+#define MAX_BIT1_TIME 189
+
+/* Bit 0 space time: 1.12ms-560 us */
+#define MIN_BIT0_TIME 36
+#define MAX_BIT0_TIME 76
+
+
+/** Decode NEC pulsecode. This code can take up to 76.5 ms to run.
+ Unfortunately, using IRQ to decode pulse didn't work, since it uses
+ a pulse train of 38KHz. This means one pulse on each 52 us
+*/
+
+int ir_nec_decode(struct input_dev *input_dev,
+ struct ir_raw_event *evs,
+ int len)
+{
+ int i, count = -1;
+ int ircode = 0, not_code = 0;
+#if 0
+ /* Needed only after porting the event code to the decoder */
+ struct ir_input_dev *ir = input_get_drvdata(input_dev);
+#endif
+
+ /* Be sure that the first event is an start one and is a pulse */
+ for (i = 0; i len; i++) {
+ if (evs[i].type (IR_START_EVENT | IR_PULSE))
+ break;
+ }
+ i++;/* First event doesn't contain data */
+
+ if (i = len)
+ return 0;
+
+ /* First space should have 4.5 ms otherwise is not NEC protocol */
+ if ((evs[i].delta.tv_nsec MIN_START_TIME) |
+ (evs[i].delta.tv_nsec MAX_START_TIME) |
+ (evs[i].type != IR_SPACE))
+ goto err;
+
+ /*
+* FIXME: need to implement the repeat sequence
+*/
+
+ count = 0;
+ for (i++; i len; i++) {
+ int bit;
+
+ if ((evs[i].delta.tv_nsec MIN_PULSE_TIME) |
+ (evs[i].delta.tv_nsec MAX_PULSE_TIME) |
+ (evs[i].type != IR_PULSE))
+ goto err;
+
+ if (++i = len)
+ goto err;
+ if (evs[i].type != IR_SPACE)
+ goto err;
+
+ if ((evs[i].delta.tv_nsec MIN_BIT1_TIME)
+ (evs[i].delta.tv_nsec MAX_BIT1_TIME))
+ bit = 1;
+ else if ((evs[i].delta.tv_nsec MIN_BIT0_TIME)
+(evs[i].delta.tv_nsec MAX_BIT0_TIME))
+ bit = 0;
+ else
+ goto err;
+
+ if (bit) {
+ int shift = count;
+ /* Address first, then command */
+ if (shift 8) {
+ shift += 8;
+ ircode |= 1 shift;
+ } else if (shift 16) {
+ not_code |= 1 shift;
+ } else if (shift 24) {
+ shift -= 16;
+ ircode |= 1 shift;
+ } else {
+ shift -= 24;
+ not_code |= 1 shift;
+ }
+ }
+ if (++count == 32)
+ break;
+ }
+
+ /*
+* Fixme: may need to accept Extended
[PATCH 14/15] V4L/DVB: cx88: Only start IR if the input device is opened
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/video/cx88/cx88-input.c
b/drivers/media/video/cx88/cx88-input.c
index 8f1b846..a7214d0 100644
--- a/drivers/media/video/cx88/cx88-input.c
+++ b/drivers/media/video/cx88/cx88-input.c
@@ -39,6 +39,10 @@ struct cx88_IR {
struct cx88_core *core;
struct input_dev *input;
struct ir_input_state ir;
+ struct ir_dev_props props;
+
+ int users;
+
char name[32];
char phys[32];
@@ -160,8 +164,16 @@ static enum hrtimer_restart cx88_ir_work(struct hrtimer
*timer)
return HRTIMER_RESTART;
}
-void cx88_ir_start(struct cx88_core *core, struct cx88_IR *ir)
+static int __cx88_ir_start(void *priv)
{
+ struct cx88_core *core = priv;
+ struct cx88_IR *ir;
+
+ if (!core || !core-ir)
+ return -EINVAL;
+
+ ir = core-ir;
+
if (ir-polling) {
hrtimer_init(ir-timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
ir-timer.function = cx88_ir_work;
@@ -174,10 +186,18 @@ void cx88_ir_start(struct cx88_core *core, struct cx88_IR
*ir)
cx_write(MO_DDS_IO, 0xa80a80); /* 4 kHz sample rate */
cx_write(MO_DDSCFG_IO, 0x5);/* enable */
}
+ return 0;
}
-void cx88_ir_stop(struct cx88_core *core, struct cx88_IR *ir)
+static void __cx88_ir_stop(void *priv)
{
+ struct cx88_core *core = priv;
+ struct cx88_IR *ir;
+
+ if (!core || !core-ir)
+ return;
+
+ ir = core-ir;
if (ir-sampling) {
cx_write(MO_DDSCFG_IO, 0x0);
core-pci_irqmask = ~PCI_INT_IR_SMPINT;
@@ -187,6 +207,37 @@ void cx88_ir_stop(struct cx88_core *core, struct cx88_IR
*ir)
hrtimer_cancel(ir-timer);
}
+int cx88_ir_start(struct cx88_core *core)
+{
+ if (core-ir-users)
+ return __cx88_ir_start(core);
+
+ return 0;
+}
+
+void cx88_ir_stop(struct cx88_core *core)
+{
+ if (core-ir-users)
+ __cx88_ir_stop(core);
+}
+
+static int cx88_ir_open(void *priv)
+{
+ struct cx88_core *core = priv;
+
+ core-ir-users++;
+ return __cx88_ir_start(core);
+}
+
+static void cx88_ir_close(void *priv)
+{
+ struct cx88_core *core = priv;
+
+ core-ir-users--;
+ if (!core-ir-users)
+ __cx88_ir_stop(core);
+}
+
/* -- */
int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
@@ -382,19 +433,19 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev
*pci)
ir-core = core;
core-ir = ir;
- cx88_ir_start(core, ir);
+ ir-props.priv = core;
+ ir-props.open = cx88_ir_open;
+ ir-props.close = cx88_ir_close;
/* all done */
- err = ir_input_register(ir-input, ir_codes, NULL, MODULE_NAME);
+ err = ir_input_register(ir-input, ir_codes, ir-props, MODULE_NAME);
if (err)
- goto err_out_stop;
+ goto err_out_free;
return 0;
- err_out_stop:
- cx88_ir_stop(core, ir);
- core-ir = NULL;
err_out_free:
+ core-ir = NULL;
kfree(ir);
return err;
}
@@ -407,7 +458,7 @@ int cx88_ir_fini(struct cx88_core *core)
if (NULL == ir)
return 0;
- cx88_ir_stop(core, ir);
+ cx88_ir_stop(core);
ir_input_unregister(ir-input);
kfree(ir);
diff --git a/drivers/media/video/cx88/cx88-video.c
b/drivers/media/video/cx88/cx88-video.c
index 48c450f..8e414f7 100644
--- a/drivers/media/video/cx88/cx88-video.c
+++ b/drivers/media/video/cx88/cx88-video.c
@@ -1977,7 +1977,7 @@ static void __devexit cx8800_finidev(struct pci_dev
*pci_dev)
}
if (core-ir)
- cx88_ir_stop(core, core-ir);
+ cx88_ir_stop(core);
cx88_shutdown(core); /* FIXME */
pci_disable_device(pci_dev);
@@ -2015,7 +2015,7 @@ static int cx8800_suspend(struct pci_dev *pci_dev,
pm_message_t state)
spin_unlock(dev-slock);
if (core-ir)
- cx88_ir_stop(core, core-ir);
+ cx88_ir_stop(core);
/* FIXME -- shutdown device */
cx88_shutdown(core);
@@ -2056,7 +2056,7 @@ static int cx8800_resume(struct pci_dev *pci_dev)
/* FIXME: re-initialize hardware */
cx88_reset(core);
if (core-ir)
- cx88_ir_start(core, core-ir);
+ cx88_ir_start(core);
cx_set(MO_PCI_INTMSK, core-pci_irqmask);
diff --git a/drivers/media/video/cx88/cx88.h b/drivers/media/video/cx88/cx88.h
index b5f054d..bdb03d3 100644
--- a/drivers/media/video/cx88/cx88.h
+++ b/drivers/media/video/cx88/cx88.h
@@ -41,7 +41,7 @@
#include linux/version.h
#include linux/mutex.h
-#define CX88_VERSION_CODE KERNEL_VERSION(0,0,7)
+#define CX88_VERSION_CODE KERNEL_VERSION(0, 0, 8)
#define UNSET (-1U)
@@ -683,8 +683,8 @@ s32
[PATCH 05/15] V4L/DVB: ir-core: add two functions to report keyup/keydown events
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/ir-keytable.c b/drivers/media/IR/ir-keytable.c
index 2d9ba84..ab60730 100644
--- a/drivers/media/IR/ir-keytable.c
+++ b/drivers/media/IR/ir-keytable.c
@@ -364,7 +364,7 @@ static int ir_setkeycode(struct input_dev *dev,
*
* This routine is used by the input routines when a key is pressed at the
* IR. The scancode is received and needs to be converted into a keycode.
- * If the key is not found, it returns KEY_UNKNOWN. Otherwise, returns the
+ * If the key is not found, it returns KEY_RESERVED. Otherwise, returns the
* corresponding keycode from the table.
*/
u32 ir_g_keycode_from_table(struct input_dev *dev, u32 scancode)
@@ -391,6 +391,61 @@ u32 ir_g_keycode_from_table(struct input_dev *dev, u32
scancode)
EXPORT_SYMBOL_GPL(ir_g_keycode_from_table);
/**
+ * ir_keyup() - generates input event to cleanup a key press
+ * @input_dev: the struct input_dev descriptor of the device
+ *
+ * This routine is used by the input routines when a key is pressed at the
+ * IR. It reports a keyup input event via input_report_key().
+ */
+void ir_keyup(struct input_dev *dev)
+{
+ struct ir_input_dev *ir = input_get_drvdata(dev);
+
+ if (!ir-keypressed)
+ return;
+
+ input_report_key(dev, ir-keycode, 0);
+ input_sync(dev);
+ ir-keypressed = 0;
+}
+EXPORT_SYMBOL_GPL(ir_keyup);
+
+/**
+ * ir_keydown() - generates input event for a key press
+ * @input_dev: the struct input_dev descriptor of the device
+ * @scancode: the scancode that we're seeking
+ *
+ * This routine is used by the input routines when a key is pressed at the
+ * IR. It gets the keycode for a scancode and reports an input event via
+ * input_report_key().
+ */
+void ir_keydown(struct input_dev *dev, int scancode)
+{
+ struct ir_input_dev *ir = input_get_drvdata(dev);
+
+ u32 keycode = ir_g_keycode_from_table(dev, scancode);
+
+ /* If already sent a keydown, do a keyup */
+ if (ir-keypressed)
+ ir_keyup(dev);
+
+ if (KEY_RESERVED == keycode)
+ return;
+
+ ir-keycode = keycode;
+ ir-keypressed = 1;
+
+ IR_dprintk(1, %s: key down event, key 0x%04x, scancode 0x%04x\n,
+ dev-name, keycode, scancode);
+
+ input_report_key(dev, ir-keycode, 1);
+ input_sync(dev);
+
+}
+EXPORT_SYMBOL_GPL(ir_keydown);
+
+
+/**
* ir_input_register() - sets the IR keycode table and add the handlers
* for keymap table get/set
* @input_dev: the struct input_dev descriptor of the device
diff --git a/include/media/ir-core.h b/include/media/ir-core.h
index 369969d..198fd61 100644
--- a/include/media/ir-core.h
+++ b/include/media/ir-core.h
@@ -72,6 +72,10 @@ struct ir_input_dev {
unsigned long devno; /* device number */
const struct ir_dev_props *props; /* Device properties */
struct ir_raw_event_ctrl*raw; /* for raw pulse/space
events */
+
+ /* key info - needed by IR keycode handlers */
+ u32 keycode;/* linux key code */
+ int keypressed; /* current state */
};
#define to_ir_input_dev(_attr) container_of(_attr, struct ir_input_dev, attr)
--
1.6.6.1
--
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
[PATCH 06/15] V4L/DVB: ir-core/saa7134: Move ir keyup/keydown code to the ir-core
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
diff --git a/drivers/media/IR/ir-nec-decoder.c
b/drivers/media/IR/ir-nec-decoder.c
index 16360eb..a58c717 100644
--- a/drivers/media/IR/ir-nec-decoder.c
+++ b/drivers/media/IR/ir-nec-decoder.c
@@ -30,37 +30,35 @@
#define MIN_BIT0_TIME 36
#define MAX_BIT0_TIME 76
-
-/** Decode NEC pulsecode. This code can take up to 76.5 ms to run.
- Unfortunately, using IRQ to decode pulse didn't work, since it uses
- a pulse train of 38KHz. This means one pulse on each 52 us
-*/
-
-int ir_nec_decode(struct input_dev *input_dev,
- struct ir_raw_event *evs,
- int len)
+/**
+ * __ir_nec_decode() - Decode one NEC pulsecode
+ * @input_dev: the struct input_dev descriptor of the device
+ * @evs: event array with type/duration of pulse/space
+ * @len: length of the array
+ * @pos: position to start seeking for a code
+ * This function returns the decoded ircode or -EINVAL if no pulse got decoded
+ */
+static int __ir_nec_decode(struct input_dev *input_dev,
+ struct ir_raw_event *evs,
+ int len, int *pos)
{
- int i, count = -1;
+ int count = -1;
int ircode = 0, not_code = 0;
-#if 0
- /* Needed only after porting the event code to the decoder */
- struct ir_input_dev *ir = input_get_drvdata(input_dev);
-#endif
/* Be sure that the first event is an start one and is a pulse */
- for (i = 0; i len; i++) {
- if (evs[i].type (IR_START_EVENT | IR_PULSE))
+ for (; *pos len; (*pos)++) {
+ if (evs[*pos].type (IR_START_EVENT | IR_PULSE))
break;
}
- i++;/* First event doesn't contain data */
+ (*pos)++; /* First event doesn't contain data */
- if (i = len)
+ if (*pos = len)
return 0;
/* First space should have 4.5 ms otherwise is not NEC protocol */
- if ((evs[i].delta.tv_nsec MIN_START_TIME) |
- (evs[i].delta.tv_nsec MAX_START_TIME) |
- (evs[i].type != IR_SPACE))
+ if ((evs[*pos].delta.tv_nsec MIN_START_TIME) |
+ (evs[*pos].delta.tv_nsec MAX_START_TIME) |
+ (evs[*pos].type != IR_SPACE))
goto err;
/*
@@ -68,24 +66,24 @@ int ir_nec_decode(struct input_dev *input_dev,
*/
count = 0;
- for (i++; i len; i++) {
+ for ((*pos)++; *pos len; (*pos)++) {
int bit;
- if ((evs[i].delta.tv_nsec MIN_PULSE_TIME) |
- (evs[i].delta.tv_nsec MAX_PULSE_TIME) |
- (evs[i].type != IR_PULSE))
+ if ((evs[*pos].delta.tv_nsec MIN_PULSE_TIME) |
+ (evs[*pos].delta.tv_nsec MAX_PULSE_TIME) |
+ (evs[*pos].type != IR_PULSE))
goto err;
- if (++i = len)
+ if (++*pos = len)
goto err;
- if (evs[i].type != IR_SPACE)
+ if (evs[*pos].type != IR_SPACE)
goto err;
- if ((evs[i].delta.tv_nsec MIN_BIT1_TIME)
- (evs[i].delta.tv_nsec MAX_BIT1_TIME))
+ if ((evs[*pos].delta.tv_nsec MIN_BIT1_TIME)
+ (evs[*pos].delta.tv_nsec MAX_BIT1_TIME))
bit = 1;
- else if ((evs[i].delta.tv_nsec MIN_BIT0_TIME)
-(evs[i].delta.tv_nsec MAX_BIT0_TIME))
+ else if ((evs[*pos].delta.tv_nsec MIN_BIT0_TIME)
+(evs[*pos].delta.tv_nsec MAX_BIT0_TIME))
bit = 0;
else
goto err;
@@ -120,12 +118,40 @@ int ir_nec_decode(struct input_dev *input_dev,
}
IR_dprintk(1, NEC scancode 0x%04x\n, ircode);
+ ir_keydown(input_dev, ircode);
+ ir_keyup(input_dev);
return ircode;
err:
IR_dprintk(1, NEC decoded failed at bit %d while decoding %luus
time\n,
- count, (evs[i].delta.tv_nsec + 500) / 1000);
+ count, (evs[*pos].delta.tv_nsec + 500) / 1000);
return -EINVAL;
}
+
+/**
+ * __ir_nec_decode() - Decodes all NEC pulsecodes on a given array
+ * @input_dev: the struct input_dev descriptor of the device
+ * @evs: event array with type/duration of pulse/space
+ * @len: length of the array
+ * This function returns the number of decoded pulses or -EINVAL if no
+ * pulse got decoded
+ */
+int ir_nec_decode(struct input_dev *input_dev,
+ struct ir_raw_event *evs,
+ int len)
+{
+ int pos = 0;
+ int rc = 0;
+
+ while (pos len) {
+ if (__ir_nec_decode(input_dev, evs, len, pos) = 0)
+ rc++;
+ }
+
+ if (!rc)
+ return -EINVAL;
+ return rc;
+}
+
Re: [RFC] Serialization flag example
Hans, Hans Verkuil wrote: I made a quick implementation which is available here: http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize It's pretty easy to use and it also gives you a very simple way to block access to the video device nodes until all have been allocated by simply taking the serialization lock and holding it until we are done with the initialization. I converted radio-mr800.c and ivtv. That said, almost all drivers that register multiple device nodes probably suffer from a race condition when one of the device node registrations returns an error and all devices have to be unregistered and the driver needs to release all resources. Currently most if not all drivers just release resources and free the memory. But if an application managed to open one device before the driver removes it again, then we have almost certainly a crash. It is possible to do this correctly in the driver, but it really needs core support where a release callback can be installed in v4l2_device that is called when the last video_device is closed by the application. We already can cleanup correctly after the last close of a video_device, but there is no top-level release yet. Anyway, I tried to use the serialization flag in bttv as well, but I ran into problems with videobuf. Basically when you need to wait for some event you should release the serialization lock and grab it after the event arrives. Unfortunately videobuf has no access to v4l2_device at the moment. If we would have that, then videobuf can just release the serialization lock while waiting for something to happen. While this code works like a charm with the radio devices, it probably won't work fine with complex devices. You'll have issues also with -alsa and -dvb parts that are present on the drivers. Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It behaves like two separate drivers (each with its own bind code at V4L core), but, as there's just one PCI bridge, and just one analog video decoder/analog audio decoder, the lock should cross between the different drivers. So, binding videobuf to v4l2_device won't solve the issue with most videobuf-aware drivers, nor the lock will work as expected, at least with most of the devices. Also, although this is not a real problem, your lock is too pedantic: it is blocking access to several ioctl's that don't need to be locked. For example, there are several ioctl's that just returns static: information: capabilities, supported video standards, etc. There are even some cases where dynamic ioctl's are welcome, like accepting QBUF/DQBUF without locking (or with a minimum lock), to allow different threads to handle the buffers. The big issue I see with this approach is that developers will become lazy on checking the locks inside the drivers: they'll just apply the flag and trust that all of their locking troubles were magically solved by the core. Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock, used internally on the driver. This approach will still be pedantic, as all ioctls will keep being serialized, but at least the driver will need to explicitly handle the lock, and the same lock can be used on other parts of the driver. -- Cheers, Mauro -- 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: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due to the async load, we'll need to add the same locking at *alsa and *dvb parts of em28xx. Yes, that is correct. The problem effects both dvb and alsa, although empirically it is more visible with the dvb case. Yet, in this specific case, as the errors are due to the reception of wrong data from tvp5150, maybe the problem is due to the lack of a proper lock at the i2c access. The problem is because hald sees the new device and is making v4l2 calls against the tvp5150 even though the gpio has been toggled over to digital mode. Hence an i2c lock won't help. We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution). This is what got me thinking a few weeks ago that perhaps the submodules should not be loaded asynchronously. In that case, at least the main em28xx module could continue to hold the lock while the submodules are still being loaded. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: V4L-DVB drivers and BKL
Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due to the async load, we'll need to add the same locking at *alsa and *dvb parts of em28xx. Yes, that is correct. The problem effects both dvb and alsa, although empirically it is more visible with the dvb case. In the case of the initialization, the lock is needed, even if we fing a way to load the module synchronously. Yet, in this specific case, as the errors are due to the reception of wrong data from tvp5150, maybe the problem is due to the lack of a proper lock at the i2c access. The problem is because hald sees the new device and is making v4l2 calls against the tvp5150 even though the gpio has been toggled over to digital mode. Hence an i2c lock won't help. If the i2c lock was toggled to digital mode, then it means that the i2c code is being in use simultaneously by analog and digital mode. It also means that an i2c IR device, or alsa will have troubles also. So, we really need one i2c lock that will protect the access to the I2C bus as a hole, including the i2c gate. We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution). Yes, this is also needed: we shouldn't let simultaneous stream access to the analog and digital mode at the same time, but I don't see, by itself, any problem on having both analog and digital nodes opened at the same time. So, the solution seems to lock some resources between digital/analog access. This is what got me thinking a few weeks ago that perhaps the submodules should not be loaded asynchronously. In that case, at least the main em28xx module could continue to hold the lock while the submodules are still being loaded. Devin -- Cheers, Mauro -- 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: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: If the i2c lock was toggled to digital mode, then it means that the i2c code is being in use simultaneously by analog and digital mode. It also means that an i2c IR device, or alsa will have troubles also. So, we really need one i2c lock that will protect the access to the I2C bus as a hole, including the i2c gate. Most i2c locks typically are only held for the duration of a single i2c transaction. What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone. We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution). Yes, this is also needed: we shouldn't let simultaneous stream access to the analog and digital mode at the same time, but I don't see, by itself, any problem on having both analog and digital nodes opened at the same time. So, the solution seems to lock some resources between digital/analog access. I think this is probably a bad idea. The additional granularity provides you little benefit, and you could easily end up in situations where power is being continuously being toggled on the decoder and demodulator as ioctls come in from both analog and digital. The solution would probably not be too bad if you're only talking about boards where everything is powered up all the time (like most of the PCI boards), but this approach would be horrific for any board where power management were a concern (e.g. USB devices). A fairly simple locking scheme at open() would prevent essentially all of race conditions, the change would only be in one or two places per bridge (as opposed to littering the code with locking logic), and the only constraint is that applications would have to be diligent in closing the device when its not in use. We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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
[cron job] v4l-dvb daily build 2.6.22 and up: ERRORS, 2.6.16-2.6.21: WARNINGS
This message is generated daily by a cron job that builds v4l-dvb for the kernels and architectures in the list below. Results of the daily build of v4l-dvb: date:Thu Apr 1 19:00:20 CEST 2010 path:http://www.linuxtv.org/hg/v4l-dvb changeset: 14536:a539e5b68945 git master: f6760aa024199cfbce564311dc4bc4d47b6fb349 git media-master: 8c69c6ed6c74c94fa7ad6fa24eda452e4b212d81 gcc version: i686-linux-gcc (GCC) 4.4.3 host hardware:x86_64 host os: 2.6.32.5 linux-2.6.32.6-armv5: OK linux-2.6.33-armv5: OK linux-2.6.34-rc1-armv5: OK linux-2.6.32.6-armv5-davinci: WARNINGS linux-2.6.33-armv5-davinci: WARNINGS linux-2.6.34-rc1-armv5-davinci: WARNINGS linux-2.6.32.6-armv5-ixp: WARNINGS linux-2.6.33-armv5-ixp: WARNINGS linux-2.6.34-rc1-armv5-ixp: WARNINGS linux-2.6.32.6-armv5-omap2: WARNINGS linux-2.6.33-armv5-omap2: WARNINGS linux-2.6.34-rc1-armv5-omap2: WARNINGS linux-2.6.22.19-i686: WARNINGS linux-2.6.23.17-i686: WARNINGS linux-2.6.24.7-i686: WARNINGS linux-2.6.25.20-i686: WARNINGS linux-2.6.26.8-i686: WARNINGS linux-2.6.27.44-i686: WARNINGS linux-2.6.28.10-i686: WARNINGS linux-2.6.29.1-i686: WARNINGS linux-2.6.30.10-i686: WARNINGS linux-2.6.31.12-i686: WARNINGS linux-2.6.32.6-i686: WARNINGS linux-2.6.33-i686: WARNINGS linux-2.6.34-rc1-i686: WARNINGS linux-2.6.32.6-m32r: OK linux-2.6.33-m32r: OK linux-2.6.34-rc1-m32r: OK linux-2.6.32.6-mips: WARNINGS linux-2.6.33-mips: WARNINGS linux-2.6.34-rc1-mips: WARNINGS linux-2.6.32.6-powerpc64: WARNINGS linux-2.6.33-powerpc64: WARNINGS linux-2.6.34-rc1-powerpc64: WARNINGS linux-2.6.22.19-x86_64: WARNINGS linux-2.6.23.17-x86_64: WARNINGS linux-2.6.24.7-x86_64: WARNINGS linux-2.6.25.20-x86_64: WARNINGS linux-2.6.26.8-x86_64: WARNINGS linux-2.6.27.44-x86_64: WARNINGS linux-2.6.28.10-x86_64: WARNINGS linux-2.6.29.1-x86_64: WARNINGS linux-2.6.30.10-x86_64: WARNINGS linux-2.6.31.12-x86_64: WARNINGS linux-2.6.32.6-x86_64: WARNINGS linux-2.6.33-x86_64: WARNINGS linux-2.6.34-rc1-x86_64: WARNINGS linux-git-armv5: OK linux-git-armv5-davinci: OK linux-git-armv5-ixp: OK linux-git-armv5-omap2: OK linux-git-i686: WARNINGS linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: WARNINGS linux-git-x86_64: WARNINGS spec: ERRORS spec-git: OK sparse: ERRORS linux-2.6.16.62-i686: WARNINGS linux-2.6.17.14-i686: WARNINGS linux-2.6.18.8-i686: WARNINGS linux-2.6.19.7-i686: WARNINGS linux-2.6.20.21-i686: WARNINGS linux-2.6.21.7-i686: WARNINGS linux-2.6.16.62-x86_64: WARNINGS linux-2.6.17.14-x86_64: WARNINGS linux-2.6.18.8-x86_64: WARNINGS linux-2.6.19.7-x86_64: WARNINGS linux-2.6.20.21-x86_64: WARNINGS linux-2.6.21.7-x86_64: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2 The V4L-DVB specification from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- 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: [RFC] Serialization flag example
On Thursday 01 April 2010 20:24:12 Mauro Carvalho Chehab wrote: Hans, Hans Verkuil wrote: I made a quick implementation which is available here: http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize It's pretty easy to use and it also gives you a very simple way to block access to the video device nodes until all have been allocated by simply taking the serialization lock and holding it until we are done with the initialization. I converted radio-mr800.c and ivtv. That said, almost all drivers that register multiple device nodes probably suffer from a race condition when one of the device node registrations returns an error and all devices have to be unregistered and the driver needs to release all resources. Currently most if not all drivers just release resources and free the memory. But if an application managed to open one device before the driver removes it again, then we have almost certainly a crash. It is possible to do this correctly in the driver, but it really needs core support where a release callback can be installed in v4l2_device that is called when the last video_device is closed by the application. We already can cleanup correctly after the last close of a video_device, but there is no top-level release yet. Anyway, I tried to use the serialization flag in bttv as well, but I ran into problems with videobuf. Basically when you need to wait for some event you should release the serialization lock and grab it after the event arrives. Unfortunately videobuf has no access to v4l2_device at the moment. If we would have that, then videobuf can just release the serialization lock while waiting for something to happen. While this code works like a charm with the radio devices, it probably won't work fine with complex devices. Nor was it meant to. Although ivtv is a pretty complex device and it works fine there. But yes, when you also have alsa, dvb or other parts then you will have to think more carefully and possibly abandon the core serialization altogether. However, of the almost 80 drivers we have it is my conservative estimate that about 75% of those fall in the 'simple' device category, at least when it comes to locking. I would like to see a simple, but good, locking scheme for those 60 drivers. And the remaining 20 can take care of their own locking. You'll have issues also with -alsa and -dvb parts that are present on the drivers. Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It behaves like two separate drivers (each with its own bind code at V4L core), but, as there's just one PCI bridge, and just one analog video decoder/analog audio decoder, the lock should cross between the different drivers. So, binding videobuf to v4l2_device won't solve the issue with most videobuf-aware drivers, nor the lock will work as expected, at least with most of the devices. As I said, you have to enable this serialization explicitly. And obviously you shouldn't enable it mindlessly. Also, although this is not a real problem, your lock is too pedantic: it is blocking access to several ioctl's that don't need to be locked. For example, there are several ioctl's that just returns static: information: capabilities, supported video standards, etc. There are even some cases where dynamic ioctl's are welcome, like accepting QBUF/DQBUF without locking (or with a minimum lock), to allow different threads to handle the buffers. Which is why videobuf should be aware of such a global lock so that it can release it when it has to wait. The big issue I see with this approach is that developers will become lazy on checking the locks inside the drivers: they'll just apply the flag and trust that all of their locking troubles were magically solved by the core. Well, for simple drivers (i.e. the vast majority) that is indeed the case. Locking is hard. If this can be moved into the core for most drivers, then that is good. I like it if developers can be lazy. Less chance of bugs. And mind that this is exactly the situation we have now with ioctl and the BKL! Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock, used internally on the driver. This approach will still be pedantic, as all ioctls will keep being serialized, but at least the driver will need to explicitly handle the lock, and the same lock can be used on other parts of the driver. Well, I guess you could add a 'struct mutex *serialize;' field to v4l2_device that drivers can set. But I don't see much of a difference in practice. Regarding the 'pedantic approach': we can easily move the locking to video_ioctl2: struct video_device *vdev = video_devdata(file); int serialize = must_serialize_ioctl(vdev, cmd); if (serialize) mutex_lock(vdev-v4l2_dev-serialize_lock); /* Handles
Re: V4L-DVB drivers and BKL
On Thursday 01 April 2010 20:29:52 Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due to the async load, we'll need to add the same locking at *alsa and *dvb parts of em28xx. Yes, that is correct. The problem effects both dvb and alsa, although empirically it is more visible with the dvb case. Yet, in this specific case, as the errors are due to the reception of wrong data from tvp5150, maybe the problem is due to the lack of a proper lock at the i2c access. The problem is because hald sees the new device and is making v4l2 calls against the tvp5150 even though the gpio has been toggled over to digital mode. Hence an i2c lock won't help. We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution). This is what got me thinking a few weeks ago that perhaps the submodules should not be loaded asynchronously. In that case, at least the main em28xx module could continue to hold the lock while the submodules are still being loaded. What was the reason behind the asynchronous loading? In general it simplifies things a lot if you load modules up front. Regards, Hans Devin -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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: V4L-DVB drivers and BKL
Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: If the i2c lock was toggled to digital mode, then it means that the i2c code is being in use simultaneously by analog and digital mode. It also means that an i2c IR device, or alsa will have troubles also. So, we really need one i2c lock that will protect the access to the I2C bus as a hole, including the i2c gate. Most i2c locks typically are only held for the duration of a single i2c transaction. What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone. The i2c open/close should be part of the transaction. Of course, there's no need to send a command to open an already opened gate (yet, from some sniff dumps, it seems that some drivers for other OS's do it for every single i2c access). We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution). Yes, this is also needed: we shouldn't let simultaneous stream access to the analog and digital mode at the same time, but I don't see, by itself, any problem on having both analog and digital nodes opened at the same time. So, the solution seems to lock some resources between digital/analog access. I think this is probably a bad idea. The additional granularity provides you little benefit, and you could easily end up in situations where power is being continuously being toggled on the decoder and demodulator as ioctls come in from both analog and digital. The solution would probably not be too bad if you're only talking about boards where everything is powered up all the time (like most of the PCI boards), but this approach would be horrific for any board where power management were a concern (e.g. USB devices). A fairly simple locking scheme at open() would prevent essentially all of race conditions, the change would only be in one or two places per bridge (as opposed to littering the code with locking logic), and the only constraint is that applications would have to be diligent in closing the device when its not in use. We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code. For sure a lock at the open() is simple, but I suspect that this may cause some troubles with applications that may just open everything on startup (even letting the device unused). Just as one example of such apps, kmix, pulseaudio and other alsa mixers love to keep the mixer node opened, even if nobody is using it. -- Cheers, Mauro -- 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: V4L-DVB drivers and BKL
On Thursday 01 April 2010 20:56:19 Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 2:42 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: If the i2c lock was toggled to digital mode, then it means that the i2c code is being in use simultaneously by analog and digital mode. It also means that an i2c IR device, or alsa will have troubles also. So, we really need one i2c lock that will protect the access to the I2C bus as a hole, including the i2c gate. Most i2c locks typically are only held for the duration of a single i2c transaction. What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone. We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution). Yes, this is also needed: we shouldn't let simultaneous stream access to the analog and digital mode at the same time, but I don't see, by itself, any problem on having both analog and digital nodes opened at the same time. So, the solution seems to lock some resources between digital/analog access. I think this is probably a bad idea. The additional granularity provides you little benefit, and you could easily end up in situations where power is being continuously being toggled on the decoder and demodulator as ioctls come in from both analog and digital. The solution would probably not be too bad if you're only talking about boards where everything is powered up all the time (like most of the PCI boards), but this approach would be horrific for any board where power management were a concern (e.g. USB devices). A fairly simple locking scheme at open() would prevent essentially all of race conditions, the change would only be in one or two places per bridge (as opposed to littering the code with locking logic), and the only constraint is that applications would have to be diligent in closing the device when its not in use. I agree. The biggest problem with v4l-dvb devices is driver complexity. It has never been performance. Reducing that complexity by moving some of that into the core is a good thing in my view. We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code. Right. Hans Devin -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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: V4L-DVB drivers and BKL
Hans Verkuil wrote: On Thursday 01 April 2010 20:29:52 Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 1:36 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: If you take a look at em28xx-dvb, it is not lock-protected. If the bug is due to the async load, we'll need to add the same locking at *alsa and *dvb parts of em28xx. Yes, that is correct. The problem effects both dvb and alsa, although empirically it is more visible with the dvb case. Yet, in this specific case, as the errors are due to the reception of wrong data from tvp5150, maybe the problem is due to the lack of a proper lock at the i2c access. The problem is because hald sees the new device and is making v4l2 calls against the tvp5150 even though the gpio has been toggled over to digital mode. Hence an i2c lock won't help. We would need to implement proper locking of analog versus digital mode, which unfortunately would either result in hald getting back -EBUSY on open of the V4L device or the DVB module loading being deferred while the v4l side of the board is in use (neither of which is a very good solution). This is what got me thinking a few weeks ago that perhaps the submodules should not be loaded asynchronously. In that case, at least the main em28xx module could continue to hold the lock while the submodules are still being loaded. What was the reason behind the asynchronous loading? In general it simplifies things a lot if you load modules up front. The reason is to avoid a dead lock: driver A depends on symbols on B (the other driver init code) that depends on symbols at A (core stuff, locks, etc). There are other approaches to avoid this trouble, like the attach method used by the DVB modules, but an asynchronous (and parallel) load offers another advantage: it speeds up boot time, as other processors can take care of the load of the additonal modules. -- Cheers, Mauro -- 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: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 5:16 PM, Mauro Carvalho Chehab mche...@redhat.com What was the reason behind the asynchronous loading? In general it simplifies things a lot if you load modules up front. The reason is to avoid a dead lock: driver A depends on symbols on B (the other driver init code) that depends on symbols at A (core stuff, locks, etc). I believe these problems can be avoided with a common entry point for initializing the DVB submodule (where the loading of the subordinate module sets a pointer to a function for the main module to call). In fact, doesn't em28xx do that today with it's init function for its submodules? There are other approaches to avoid this trouble, like the attach method used by the DVB modules, but an asynchronous (and parallel) load offers another advantage: it speeds up boot time, as other processors can take care of the load of the additonal modules. I think though that we need to favor stability/reliability over performance. In this case, I have seen a number of bridges where having a -dvb.ko exposes this race, and I would much rather have it take another 200ms to load the driver than continue to deal with intermittent problems with hardware being in an unknown state. Don't quote me on this number, but on em28xx I run into problems about 20% of the time where the dvb device fails to get created successfully because of this race (forcing me to reboot the system). Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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: [RFC] Serialization flag example
Hans Verkuil wrote:
Maybe a better alternative would be to pass to the V4L2 core, optionally,
some lock,
used internally on the driver. This approach will still be pedantic, as all
ioctls will
keep being serialized, but at least the driver will need to explicitly
handle the lock,
and the same lock can be used on other parts of the driver.
Well, I guess you could add a 'struct mutex *serialize;' field to v4l2_device
that drivers can set. But I don't see much of a difference in practice.
It makes easier to implement more complex approaches, where you may need to use
more
locks. Also, It makes no sense to make a DVB code or an alsa code dependent on
a V4L
header, just because it needs to see a mutex.
Also, a mutex at the driver need to be initialized inside the driver. It is not
just one
flag that someone writing a new driver will clone without really understanding
what
it is doing.
Regarding the 'pedantic approach': we can easily move the locking to
video_ioctl2:
struct video_device *vdev = video_devdata(file);
int serialize = must_serialize_ioctl(vdev, cmd);
if (serialize)
mutex_lock(vdev-v4l2_dev-serialize_lock);
/* Handles IOCTL */
err = __video_do_ioctl(file, cmd, parg);
if (serialize)
mutex_unlock(vdev-v4l2_dev-serialize_lock);
And must_serialize_ioctl() looks like this:
static int must_serialize_ioctl(struct video_device *vdev, int cmd)
{
if (!vdev-v4l2_dev || !vdev-v4l2_dev-fl_serialize)
return 0;
switch (cmd) {
case VIDIOC_QUERYCAP:
case VIDIOC_ENUM_FMT:
...
return 0;
}
return 1;
}
Basically the CAP (capability) ioctls and all ENUM ioctls do not have to be
serialized. I am not sure whether the streaming ioctls should also be included
here. I can't really grasp the consequences of whatever choice we make. If we
want to be compatible with what happens today with ioctl and the BKL, then we
need to lock and have videobuf unlock whenever it has to wait for an event.
I suspect that the list of must have is driver-dependent.
--
Cheers,
Mauro
--
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: V4L-DVB drivers and BKL
On Thu, Apr 1, 2010 at 5:07 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: Most i2c locks typically are only held for the duration of a single i2c transaction. What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone. The i2c open/close should be part of the transaction. Of course, there's no need to send a command to open an already opened gate (yet, from some sniff dumps, it seems that some drivers for other OS's do it for every single i2c access). I'm not even talking about i2c gate control, which is a whole separate case where it is applied inconsistently across drivers. Even under Linux, we have lots of cases where there are double opens and double closes of the i2c gate, depending on whether the developer is controlling the gate from the tuner driver or the demodulator. What I'm getting at though is that the lock granularity today is typically at the i2c transaction level, so something like an demod driver attempting to set two disparate registers is likely to be two i2c transactions. Without moving the locking into the caller, the other half of the driver can take control between those two transactions. And moving the logic into the caller means we will have to litter the code all over the place with lock/unlock calls. We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code. For sure a lock at the open() is simple, but I suspect that this may cause some troubles with applications that may just open everything on startup (even letting the device unused). Just as one example of such apps, kmix, pulseaudio and other alsa mixers love to keep the mixer node opened, even if nobody is using it. I'm frankly far less worried about the ALSA devices than I am about DVB versus V4L Vdeo/VBI, based on all the feedback I see from real users. The cases where we are getting continuously burned are MythTV users who don't have their input groups properly defined and as a result MythTV attempts to use both digital and analog at the same time (input groups themselves are really a hack to deal with the fact that the Linux kernel doesn't have any way to inform userland of the relationships). And the more I think about it, we can probably even implement the locking itself in the V4L and DVB core (further reducing the risk of some bridge maintainer screwing it up). All the bridge driver would have to do is declare the relationship between the DVB and V4L devices (both video and vbi), and the enforcement of the locking could be abstracted out. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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
Aw: Re: V4L-DVB drivers and BKL
Hi! - Original Nachricht Von: Mauro Carvalho Chehab mche...@redhat.com An: Devin Heitmueller dheitmuel...@kernellabs.com Datum: 02.04.2010 01:10 Betreff: Re: V4L-DVB drivers and BKL Devin Heitmueller wrote: On Thu, Apr 1, 2010 at 5:07 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: Most i2c locks typically are only held for the duration of a single i2c transaction. What you are proposing would likely result in just about every function having to explicitly lock/unlock, which just seems bound to be error prone. The i2c open/close should be part of the transaction. Of course, there's no need to send a command to open an already opened gate (yet, from some sniff dumps, it seems that some drivers for other OS's do it for every single i2c access). I'm not even talking about i2c gate control, which is a whole separate case where it is applied inconsistently across drivers. Even under Linux, we have lots of cases where there are double opens and double closes of the i2c gate, depending on whether the developer is controlling the gate from the tuner driver or the demodulator. What I'm getting at though is that the lock granularity today is typically at the i2c transaction level, so something like an demod driver attempting to set two disparate registers is likely to be two i2c transactions. Without moving the locking into the caller, the other half of the driver can take control between those two transactions. And moving the logic into the caller means we will have to litter the code all over the place with lock/unlock calls. I agree with a caller logic. Yet, even moving to the caller, an i2c lock is still needed. For example, i2c IR events are polling at interrupt time, so, they can happen anytime, including in the middle of one i2c transaction (by transaction, I mean gate control+i2c read/write ops go get/set a single register). We've got enough power management problems as it is without adding lots additional complexity with little benefit and only increasing the likelihood of buggy code. For sure a lock at the open() is simple, but I suspect that this may cause some troubles with applications that may just open everything on startup (even letting the device unused). Just as one example of such apps, kmix, pulseaudio and other alsa mixers love to keep the mixer node opened, even if nobody is using it. I'm frankly far less worried about the ALSA devices than I am about DVB versus V4L Vdeo/VBI, based on all the feedback I see from real users. Ok, but alsa driver may also try to access things like i2c. For example, msp34xx audio controls are reached via I2C. The cases where we are getting continuously burned are MythTV users who don't have their input groups properly defined and as a result MythTV attempts to use both digital and analog at the same time (input groups themselves are really a hack to deal with the fact that the Linux kernel doesn't have any way to inform userland of the relationships). We see the same on other OSs as well. In fact, to use digital and analog at once is totally valid. It is much too short to think about a device with a single hybrid tuner, best known meanwhile for causing troubles. The Medion Quad (md8080) on the saa713x with two hybrid tuners, two DVB-S tuners, four digital and two analog demodulators with two dedicated PCI bridges, with its restrictions too, is already ancient stuff. And the more I think about it, we can probably even implement the locking itself in the V4L and DVB core (further reducing the risk of some bridge maintainer screwing it up). All the bridge driver would have to do is declare the relationship between the DVB and V4L devices (both video and vbi), and the enforcement of the locking could be abstracted out. On older dual tuner, triple and quad stuff it needs granularity through each driver down to the physically existing device. No app on any OS seems to have it right. In best case they have some framework around to ask the user about his knowledge for what is a go and what a no go. Newer hardware can really do triple stuff at once for example. Nothing much left to configure wrong from the user and the app. But, to start simple, if a single bridge can pass DVB-T and DVB-S at once is as important to know these days as all details about tuners, demodulators and SECs on a given device. That mixture of old and new will continue over years anyway and to escape with some easy rules doesn't look simple. Some either digital or analog does not exist, except for a single hybrid tuner. Even then, on such a simplest hybrid device, the tuner can be in digital mode and without any problems you can have analog video from external inputs at once. I agree on having some sort of resource locking in core, but this type of lock between radio/audio/analog/analog mpeg-encoded/digital/vbi/... is
[PATCH 2/2] device_attributes: add sysfs_attr_init() for dynamic attributes
Made necessary by 6992f5334995af474c2b58d010d08bc597f0f2fe. Prevents further
key xxx not in .data bug-reports. Although some attributes could probably be
converted to static ones, this is left for people having hardware to test.
Found by this semantic patch:
@ init @
type T;
identifier A;
@@
T {
...
struct device_attribute A;
...
};
@ main extends init @
expression E;
statement S;
identifier err;
T *name;
@@
... when != sysfs_attr_init(name-A.attr);
(
+ sysfs_attr_init(name-A.attr);
if (device_create_file(E, name-A))
S
|
+ sysfs_attr_init(name-A.attr);
err = device_create_file(E, name-A);
)
While reviewing, I put the initialization to apropriate places.
Signed-off-by: Wolfram Sang w.s...@pengutronix.de
Cc: Eric W. Biederman ebied...@xmission.com
Cc: Greg KH gre...@suse.de
Cc: Andrew Morton a...@linux-foundation.org
---
drivers/macintosh/windfarm_core.c |1 +
drivers/media/video/pvrusb2/pvrusb2-sysfs.c |8
drivers/platform/x86/intel_menlow.c |1 +
drivers/video/fsl-diu-fb.c |1 +
4 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/drivers/macintosh/windfarm_core.c
b/drivers/macintosh/windfarm_core.c
index 419795f..f447642 100644
--- a/drivers/macintosh/windfarm_core.c
+++ b/drivers/macintosh/windfarm_core.c
@@ -209,6 +209,7 @@ int wf_register_control(struct wf_control *new_ct)
kref_init(new_ct-ref);
list_add(new_ct-link, wf_controls);
+ sysfs_attr_init(new_ct-attr.attr);
new_ct-attr.attr.name = new_ct-name;
new_ct-attr.attr.mode = 0644;
new_ct-attr.show = wf_show_control;
diff --git a/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
b/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
index 6c23456..71f5056 100644
--- a/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
+++ b/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
@@ -423,10 +423,12 @@ static void pvr2_sysfs_add_debugifc(struct pvr2_sysfs
*sfp)
dip = kzalloc(sizeof(*dip),GFP_KERNEL);
if (!dip) return;
+ sysfs_attr_init(dip-attr_debugcmd.attr);
dip-attr_debugcmd.attr.name = debugcmd;
dip-attr_debugcmd.attr.mode = S_IRUGO|S_IWUSR|S_IWGRP;
dip-attr_debugcmd.show = debugcmd_show;
dip-attr_debugcmd.store = debugcmd_store;
+ sysfs_attr_init(dip-attr_debuginfo.attr);
dip-attr_debuginfo.attr.name = debuginfo;
dip-attr_debuginfo.attr.mode = S_IRUGO;
dip-attr_debuginfo.show = debuginfo_show;
@@ -644,6 +646,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
return;
}
+ sysfs_attr_init(sfp-attr_v4l_minor_number.attr);
sfp-attr_v4l_minor_number.attr.name = v4l_minor_number;
sfp-attr_v4l_minor_number.attr.mode = S_IRUGO;
sfp-attr_v4l_minor_number.show = v4l_minor_number_show;
@@ -658,6 +661,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-v4l_minor_number_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_v4l_radio_minor_number.attr);
sfp-attr_v4l_radio_minor_number.attr.name = v4l_radio_minor_number;
sfp-attr_v4l_radio_minor_number.attr.mode = S_IRUGO;
sfp-attr_v4l_radio_minor_number.show = v4l_radio_minor_number_show;
@@ -672,6 +676,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-v4l_radio_minor_number_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_unit_number.attr);
sfp-attr_unit_number.attr.name = unit_number;
sfp-attr_unit_number.attr.mode = S_IRUGO;
sfp-attr_unit_number.show = unit_number_show;
@@ -685,6 +690,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-unit_number_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_bus_info.attr);
sfp-attr_bus_info.attr.name = bus_info_str;
sfp-attr_bus_info.attr.mode = S_IRUGO;
sfp-attr_bus_info.show = bus_info_show;
@@ -699,6 +705,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-bus_info_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_hdw_name.attr);
sfp-attr_hdw_name.attr.name = device_hardware_type;
sfp-attr_hdw_name.attr.mode = S_IRUGO;
sfp-attr_hdw_name.show = hdw_name_show;
@@ -713,6 +720,7 @@ static void class_dev_create(struct pvr2_sysfs *sfp,
sfp-hdw_name_created_ok = !0;
}
+ sysfs_attr_init(sfp-attr_hdw_desc.attr);
sfp-attr_hdw_desc.attr.name = device_hardware_description;
sfp-attr_hdw_desc.attr.mode = S_IRUGO;
sfp-attr_hdw_desc.show = hdw_desc_show;
diff --git a/drivers/platform/x86/intel_menlow.c
b/drivers/platform/x86/intel_menlow.c
index f0a90a6..90ba5d7 100644
--- a/drivers/platform/x86/intel_menlow.c
+++ b/drivers/platform/x86/intel_menlow.c
@@ -396,6 +396,7 @@ static int intel_menlow_add_one_attribute(char *name, int
Re: [PATCH 00/15] ir-core: Several improvements to allow adding LIRC and decoder plugins
On Thu, Apr 1, 2010 at 1:56 PM, Mauro Carvalho Chehab mche...@redhat.com wrote: This series of 15 patches improves support for IR, as discussed at the What are the goals for the architecture of an in-kernel IR system? thread. It basically adds a raw decoder layer at ir-core, allowing decoders to plug into IR core, and preparing for the addition of a lirc_dev driver that will allow raw IR codes to be sent to userspace. There's no lirc patch in this series. I have also a few other patches from David Härdeman that I'm about to test/review probably later today, but as I prefer to first merge what I have at V4L/DVB tree, before applying them. Has anyone ported the MSMCE driver onto these patches yet? That would be a good check to make sure that rc-core has the necessary API. Cooler if it works both through LIRC and with an internal protocol decoder. The MSMCE driver in my old patches was very simplified, it removed about half of the code from the LIRC version. There are two patches on this series that deserve a better analysis, IMO: - V4L/DVB: ir-core: rename sysfs remote controller class from ir to rc As discussed, IR is not a good name, as this infrastructure could later be used by other types of Remote Controllers, as it has nothing that is specific to IR inside the code, except for the name. So, I'm proposing to replace the sysfs notes do rc, instead of ir. The sooner we do such changes, the better, as userspace apps using it are still under development. So, an API change is still possible, without causing much hurt. Also, as some RC devices allow RC code transmission, we probably need to add a TX node somewhere, associated with the same RX part (as some devices don't allow simultaneous usage of TX and RX). So, we have a few alternatives for the RC device sysfs node: a) /sys/class/rc/rc0 |-- rx --- tx b) /sys/class/rc/rcrcv0 /sys/class/rc/rctx0 c) /sys/class/rc/rc0 and have there the RX and TX nodes/attributes mixed. IMO, (b) is a bad idea, so, I am between (a) and (c). - V4L/DVB: input: Add support for EVIO[CS]GKEYCODEBIG Adds two new ioctls in order to handle with big keycode tables. As already said, we'll need another ioctl, in order to get the maximum keycode supported by a given device. I didn't wrote the patch for the new ioctl yet. This patch will probably have a small conflict with upstream input, but I prefer to keep it on my tree and fix the upstream conflicts when submiting it, as the rest of the new IR code is also on my tree, and this patch is needed to procced with the IR code development. Mauro Carvalho Chehab (15): V4L/DVB: ir-core: be less pedantic with RC protocol name V4L/DVB: saa7134: use a full scancode table for M135A V4L/DVB: saa7134: add code to allow changing IR protocol V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core V4L/DVB: ir-core: add two functions to report keyup/keydown events V4L/DVB: ir-core/saa7134: Move ir keyup/keydown code to the ir-core V4L/DVB: saa7134: don't wait too much to generate an IR event on raw_decode V4L/DVB: ir-core: dynamically load the compiled IR protocols V4L/DVB: ir-core: prepare to add more operations for ir decoders V4L/DVB: ir-nec-decoder: Add sysfs node to enable/disable per irrcv V4L/DVB: saa7134: clear warning noise V4L/DVB: ir-core: rename sysfs remote controller class from ir to rc V4L/DVB: ir-core: Add callbacks for input/evdev open/close on IR core V4L/DVB: cx88: Only start IR if the input device is opened V4L/DVB: input: Add support for EVIO[CS]GKEYCODEBIG drivers/input/evdev.c | 39 +++ drivers/input/input.c | 260 ++-- drivers/media/IR/Kconfig | 9 + drivers/media/IR/Makefile | 3 +- drivers/media/IR/ir-keymaps.c | 98 drivers/media/IR/ir-keytable.c | 75 ++- drivers/media/IR/ir-nec-decoder.c | 351 +++ drivers/media/IR/ir-raw-event.c | 231 ++ drivers/media/IR/ir-sysfs.c | 29 ++- drivers/media/video/cx88/cx88-input.c | 69 +- drivers/media/video/cx88/cx88-video.c | 6 +- drivers/media/video/cx88/cx88.h | 6 +- drivers/media/video/saa7134/saa7134-core.c | 2 +- drivers/media/video/saa7134/saa7134-input.c | 207 +++-- drivers/media/video/saa7134/saa7134.h | 4 +- include/linux/input.h | 40 +++- include/media/ir-common.h | 9 +- include/media/ir-core.h | 59 +- 18 files changed, 1368 insertions(+), 129 deletions(-) create mode 100644 drivers/media/IR/ir-nec-decoder.c create mode 100644 drivers/media/IR/ir-raw-event.c -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to
