Re: SoC i.mx35 userptr method failure while running capture-example utility
Hi Guennadi/Laurent, On Wed, May 2, 2012 at 4:20 AM, Guennadi Liakhovetski g.liakhovet...@gmx.de wrote: Hi Alex On Tue, 1 May 2012, Alex Gershgorin wrote: Hi everyone, I use user-space utility from http://git.linuxtv.org/v4l-utils.git/blob/HEAD:/contrib/test/capture-example.c I made two small changes in this application and this is running on i.MX35 SoC fmt.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB565; fmt.fmt.pix.field = V4L2_FIELD_ANY; When MMAP method is used everything works fine, problem occurs when using USERPTR method this can see bellow : ./capture-example -u -f -d /dev/video0 mx3-camera mx3-camera.0: MX3 Camera driver attached to camera 0 Failed acquiring VMA for vaddr 0x76cd9008 VIDIOC_QBUF error 22, Invalid arg It doesn't surprise me, that this doesn't work. capture-example allocates absolutely normal user-space buffers, when called with USERPTR, and those buffers are very likely discontiguous. Whereas mx3-camera needs physically contiguous buffers, so, this can only fail. This means, you either have to use MMAP or you need to allocate USERPTR buffers in a special way to guarantee their contiguity. Even I am facing a similar issue when ported to VB2 for USERPTR. How do we ensure the buffers not discontiguous. Would cmem assure it? Thx, --Prabhakar Lad Unable to handle kernel NULL pointer dereference at virtual address This, however, is bad and is a bug in the driver. The capture-example should just fail nicely with no trouble. I'll add it to my TODO and will try to find some time to debug and fix this, however, I'd be more than happy if someone else would beat me on this ;-) Thanks Guennadi pgd = 80004000 [] *pgd= Internal error: Oops: 817 [#1] ARM CPU: 0 Not tainted (3.4.0-rc5+ #2283 PC is at mx3_videobuf_release+0x9c/0x10c LR is at mx3_videobuf_release+0x20/0x10c pc : [802cd92c] lr : [802cd8b0] psr: 0093 sp : 86db3e00 ip : 86db3e00 fp : 86db3e2c r10: 86ff6b20 r9 : 86817200 r8 : r7 : 86ff568c r6 : r5 : 8801a000 r4 : 86da3000 r3 : 6013 r2 : 86da3264 r1 : r0 : Flags: nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 00c5387d Table: 86dcc008 DAC: 0015 Process capture-example (pid: 52, stack limit = 0x86db2268) Stack: (0x86db3e00 to 0x86db4000) 3e00: 6013 86ff568c 0002 86ff56ac 3e20: 86db3e64 86db3e30 802c8978 802cd89c 80099414 86db3e84 86ff568c 3e40: 86dc9a80 8801a03c 80491828 86817200 86ff6b20 86db3e7c 86db3e68 3e60: 802c9a1c 802c8930 802ce048 86ff5600 86db3e9c 86db3e80 802cca14 802c9a00 3e80: 86ff5800 86dc9a80 0008 86dc9a88 86db3eb4 86db3ea0 802b936c 802cc9d0 3ea0: 86dc9a80 86ff6b20 86db3ef4 86db3eb8 80082f00 802b932c 3ec0: 86d35010 86d7f000 86dc9a80 86d59000 86d90120 000c 3ee0: 86db2000 86db3f14 86db3ef8 8007ff58 80082df0 86d59000 3f00: 0001 86db3f3c 86db3f18 8001c72c 8007fee4 86d59000 86d82000 3f20: 0100 76ef1770 00f8 8000e564 86db3f4c 86db3f40 8001c7a8 8001c6b4 3f40: 86db3f7c 86db3f50 8001dab4 8001c78c 7eb002b8 0001 0004 3f60: 86db3fa4 86db3f70 800824fc 00f8 86db3f94 86db3f80 8001dfc0 8001d8c4 3f80: 000a3d78 86db3fa4 86db3f98 8001e004 8001df4c 86db3fa8 3fa0: 8000e3e0 8001dff8 000a3d78 76ef1770 0001 000a3d64 0008 0001 3fc0: 000a3d78 76ef1770 76ef1770 00f8 76e1d248 9ecc 7eb02954 3fe0: 76f2e000 7eb02908 76de14dc 76e4f3d4 6010 0001 Backtrace: [802cd890] (mx3_videobuf_release+0x0/0x10c) from [802c8978] (__vb2_queue_free+0x54/0x15c) r8: r7:86ff56ac r6:0002 r5: r4:86ff568c [802c8924] (__vb2_queue_free+0x0/0x15c) from [802c9a1c] (vb2_queue_release+0x28/0x2c) [802c99f4] (vb2_queue_release+0x0/0x2c) from [802cca14] (soc_camera_close+0x50/0xac) r4:86ff5600 r3:802ce048 [802cc9c4] (soc_camera_close+0x0/0xac) from [802b936c] (v4l2_release+0x4c/0x6c) r7:86dc9a88 r6:0008 r5:86dc9a80 r4:86ff5800 [802b9320] (v4l2_release+0x0/0x6c) from [80082f00] (fput+0x11c/0x204) r5:86ff6b20 r4:86dc9a80 [80082de4] (fput+0x0/0x204) from [8007ff58] (filp_close+0x80/0x8c) [8007fed8] (filp_close+0x0/0x8c) from [8001c72c] (put_files_struct+0x84/0xd8) r6:0001 r5: r4:86d59000 r3: [8001c6a8] (put_files_struct+0x0/0xd8) from [8001c7a8] (exit_files+0x28/0x2c) r8:8000e564 r7:00f8 r6:76ef1770 r5:0100 r4:86d82000 r3:86d59000 [8001c780] (exit_files+0x0/0x2c) from [8001dab4] (do_exit+0x1fc/0x688) [8001d8b8] (do_exit+0x0/0x688) from [8001dfc0] (do_group_exit+0x80/0xac) r7:00f8 [8001df40] (do_group_exit+0x0/0xac) from [8001e004] (sys_exit_group+0x18/0x24) r4:000a3d78 r3: [8001dfec] (sys_exit_group+0x0/0x24) from [8000e3e0] (ret_fast_syscall+0x0/0x30) Code: 05852024 e5941268 e5940264 e2842f99 (e581)
Re: [RFC] [media] cx231xx: restore tuner settings on first open
On Mon June 18 2012 06:49:58 David Dillow wrote: Without this patch, MythTV requires some workarounds to be able to capture analog TV on my HVR-850. I need to keep the v4l device open via 'sleep 365d /dev/video0' or some other mechanism or there will be no recording. Also, I need to run vq4l2 and change the standard away and back to NTSC or I'll get garbage. Both of these can be traced to the settings (or lack there of) on the tuner. The first problem is that we will tell applications that we are in NTSC mode for the HVR-850 when they open the device for the first time after plugging it in, but never actually set the TDA18271 to NTSC, so it defaults to PAL_I. Applications may decide to not change the signal standard, as they have been told it is already in the desired state. This could be resolved by setting it during init (as we claim in the query results), as it seems to survive putting the tuner into standby mode. That's correct. Drivers must set an initial tuner standard and frequency. It doesn't matter whether that's PAL or NTSC, or what frequency, as long as there is something setup initially. The second problem is a bad interaction between driver behavior and MythTV. MythTV opens the video device, tunes to the desired channel, then closes the device. It then reopens the device and starts recording. While this works for my WinTV-FM PCI card, the cx231xx driver puts the tuner into standby after MythTV closes the device, and the tuner looses the frequency when waking back up. That sounds like a bug in the tuner driver: it should remember and restore the last set frequency when waking up. I solved both of these issues by just setting the standard and frequency when opening the device for the first user. This fixes the immediate problem, but I'm not sure it's the right fix, and I'm a bit uncomfortable faking a call into the ioctl() routines. What does the V4L2 API spec say about tuning frequency being persistent when there are no users of a video capture device? Is MythTV wrong to have that assumption, or is cx231xx wrong to not restore the frequency when a user first opens the device? Tuner standards and frequencies must be persistent. So cx231xx is wrong. Actually, all V4L2 settings must in general be persistent (there are some per-filehandle settings when dealing with low-level subdev setups or mem2mem devices). Either way, I think MythTV should keep the device open until it is done with it, as that would avoid added latency from putting the tuner to sleep and waking it back up. But, I think we should address the issue in the driver if it is not living up to the guarantees of the API. From what I can tell it is a bug in the tda tuner (not restoring the frequency) and cx231xx (not setting the initial standard and possibly frequency). Regards, Hans diff --git a/drivers/media/video/cx231xx/cx231xx-video.c b/drivers/media/video/cx231xx/cx231xx-video.c index 7f916f0..2794396 100644 --- a/drivers/media/video/cx231xx/cx231xx-video.c +++ b/drivers/media/video/cx231xx/cx231xx-video.c @@ -2190,6 +2190,12 @@ static int cx231xx_v4l2_open(struct file *filp) filp-private_data = fh; if (fh-type == V4L2_BUF_TYPE_VIDEO_CAPTURE dev-users == 0) { + struct v4l2_frequency freq = { + .tuner = 0, + .type = V4L2_TUNER_ANALOG_TV, + .frequency = dev-ctl_freq, + }; + dev-width = norm_maxw(dev); dev-height = norm_maxh(dev); @@ -2214,6 +2220,11 @@ static int cx231xx_v4l2_open(struct file *filp) /* device needs to be initialized before isoc transfer */ dev-video_input = dev-video_input 2 ? 2 : dev-video_input; + /* Restore standard and channel, as they may be lost when + * the tuner went to sleep. + */ + vidioc_s_std(filp, fh, dev-norm); + vidioc_s_frequency(filp, fh, freq); } if (fh-radio) { cx231xx_videodbg(video_open: setting radio device\n); -- 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 -- 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] s5p-fimc: Fix control creation function
Fixed the size of the V4L2_CID_COLORFX control cluster. Prior to this fix V4L2_CID_ROTATE was also icluded in the cluster preventing applications from enabling rotation. Reported-by: Marek Szyprowski m.szyprow...@samsung.com Signed-off-by: Kamil Debski k.deb...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/media/video/s5p-fimc/fimc-core.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c index fedcd56..92fc5a2 100644 --- a/drivers/media/video/s5p-fimc/fimc-core.c +++ b/drivers/media/video/s5p-fimc/fimc-core.c @@ -615,7 +615,7 @@ int fimc_ctrls_create(struct fimc_ctx *ctx) ctx-effect.type = FIMC_REG_CIIMGEFF_FIN_BYPASS; if (!handler-error) { - v4l2_ctrl_cluster(3, ctrls-colorfx); + v4l2_ctrl_cluster(2, ctrls-colorfx); ctrls-ready = true; } -- 1.7.0.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: [RFCv1 PATCH 04/32] v4l2-ioctl.c: v4l2-ioctl: add debug and callback/offset functionality.
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:26 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Add the necessary plumbing to make it possible to replace the switch by a table driven implementation. Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-ioctl.c | 91 -- 1 file changed, 78 insertions(+), 13 deletions(-) diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index a9602db..a4115ce 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -396,12 +396,22 @@ struct v4l2_ioctl_info { unsigned int ioctl; u32 flags; const char * const name; + union { + u32 offset; + int (*func)(const struct v4l2_ioctl_ops *ops, + struct file *file, void *fh, void *p); + }; + void (*debug)(const void *arg); }; /* This control needs a priority check */ #define INFO_FL_PRIO (1 0) /* This control can be valid if the filehandle passes a control handler. */ #define INFO_FL_CTRL (1 1) +/* This is a standard ioctl, no need for special code */ +#define INFO_FL_STD (1 2) +/* This is ioctl has its own function */ +#define INFO_FL_FUNC (1 3) /* Zero struct from after the field to the end */ #define INFO_FL_CLEAR(v4l2_struct, field)\ ((offsetof(struct v4l2_struct, field) + \ @@ -414,6 +424,24 @@ struct v4l2_ioctl_info { .name = #_ioctl,\ } +#define IOCTL_INFO_STD(_ioctl, _vidioc, _debug, _flags) \ + [_IOC_NR(_ioctl)] = { \ + .ioctl = _ioctl,\ + .flags = _flags | INFO_FL_STD, \ + .name = #_ioctl,\ + .offset = offsetof(struct v4l2_ioctl_ops, _vidioc), \ + .debug = _debug,\ + } + +#define IOCTL_INFO_FNC(_ioctl, _func, _debug, _flags) \ + [_IOC_NR(_ioctl)] = { \ + .ioctl = _ioctl,\ + .flags = _flags | INFO_FL_FUNC, \ + .name = #_ioctl,\ + .func = _func, \ + .debug = _debug,\ + } + static struct v4l2_ioctl_info v4l2_ioctls[] = { IOCTL_INFO(VIDIOC_QUERYCAP, 0), IOCTL_INFO(VIDIOC_ENUM_FMT, INFO_FL_CLEAR(v4l2_fmtdesc, type)), @@ -512,7 +540,7 @@ bool v4l2_is_known_ioctl(unsigned int cmd) external ioctl messages as well as internal V4L ioctl */ void v4l_printk_ioctl(unsigned int cmd) { - char *dir, *type; + const char *dir, *type; switch (_IOC_TYPE(cmd)) { case 'd': @@ -523,10 +551,11 @@ void v4l_printk_ioctl(unsigned int cmd) type = v4l2; break; } - printk(%s, v4l2_ioctls[_IOC_NR(cmd)].name); + pr_cont(%s, v4l2_ioctls[_IOC_NR(cmd)].name); return; default: type = unknown; + break; } switch (_IOC_DIR(cmd)) { @@ -536,7 +565,7 @@ void v4l_printk_ioctl(unsigned int cmd) case _IOC_READ | _IOC_WRITE: dir = rw; break; default: dir = *ERR*; break; } - printk(%s ioctl '%c', dir=%s, #%d (0x%08x), + pr_cont(%s ioctl '%c', dir=%s, #%d (0x%08x), type, _IOC_TYPE(cmd), dir, _IOC_NR(cmd), cmd); } EXPORT_SYMBOL(v4l_printk_ioctl); @@ -546,6 +575,9 @@ static long __video_do_ioctl(struct file *file, { struct video_device *vfd = video_devdata(file); const struct v4l2_ioctl_ops *ops = vfd-ioctl_ops; + bool write_only = false; + struct v4l2_ioctl_info default_info; + const struct v4l2_ioctl_info *info; void *fh = file-private_data; struct v4l2_fh *vfh = NULL; int use_fh_prio = 0; @@ -563,23 +595,40 @@ static long __video_do_ioctl(struct file *file, } if (v4l2_is_known_ioctl(cmd)) { - struct v4l2_ioctl_info *info = v4l2_ioctls[_IOC_NR(cmd)]; + info = v4l2_ioctls[_IOC_NR(cmd)]; if (!test_bit(_IOC_NR(cmd), vfd-valid_ioctls) !((info-flags INFO_FL_CTRL) vfh vfh-ctrl_handler)) - return -ENOTTY; + goto error; if (use_fh_prio (info-flags INFO_FL_PRIO)) { ret = v4l2_prio_check(vfd-prio, vfh-prio); if (ret) - return ret; +
Re: [RFCv1 PATCH 19/32] v4l2-dev.c: add debug sysfs entry.
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:41 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-dev.c | 24 1 file changed, 24 insertions(+) diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c index 1500208..5c0bb18 100644 --- a/drivers/media/video/v4l2-dev.c +++ b/drivers/media/video/v4l2-dev.c @@ -46,6 +46,29 @@ static ssize_t show_index(struct device *cd, return sprintf(buf, %i\n, vdev-index); } +static ssize_t show_debug(struct device *cd, + struct device_attribute *attr, char *buf) +{ + struct video_device *vdev = to_video_device(cd); + + return sprintf(buf, %i\n, vdev-debug); +} + +static ssize_t set_debug(struct device *cd, struct device_attribute *attr, +const char *buf, size_t len) +{ + struct video_device *vdev = to_video_device(cd); + int res = 0; + u16 value; + + res = kstrtou16(buf, 0, value); + if (res) + return res; + + vdev-debug = value; Can't this race with the various vdev-debug tests we have in the V4L core ? + return len; +} + static ssize_t show_name(struct device *cd, struct device_attribute *attr, char *buf) { @@ -56,6 +79,7 @@ static ssize_t show_name(struct device *cd, static struct device_attribute video_device_attrs[] = { __ATTR(name, S_IRUGO, show_name, NULL), + __ATTR(debug, 0644, show_debug, set_debug), __ATTR(index, S_IRUGO, show_index, NULL), __ATTR_NULL }; -- 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: [RFCv1 PATCH 20/32] v4l2-ioctl: remove v4l_(i2c_)print_ioctl
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:42 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com v4l_i2c_print_ioctl wasn't used and v4l_print_ioctl could be replaced by v4l_i2c_printk_ioctl. I suppose you meant v4l_printk_ioctl herE. Signed-off-by: Hans Verkuil hans.verk...@cisco.com -- 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: [RFCv1 PATCH 30/32] v4l2-ioctl.c: shorten the lines of the table.
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:52 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Use some macro magic to reduce the length of the lines in the table. This makes it more readable. It indeed shortens the lines, but to be honest I find the result less readable. Signed-off-by: Hans Verkuil hans.verk...@cisco.com -- 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: [RFCv1 PATCH 29/32] v4l2-dev.c: also add debug support for the fops.
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:51 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-dev.c | 41 ++--- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c index 5c0bb18..54f387d 100644 --- a/drivers/media/video/v4l2-dev.c +++ b/drivers/media/video/v4l2-dev.c @@ -305,6 +305,9 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf, ret = vdev-fops-read(filp, buf, sz, off); if (test_bit(V4L2_FL_LOCK_ALL_FOPS, vdev-flags)) mutex_unlock(vdev-lock); + if (vdev-debug) As vdev-debug is a bitmask, shouldn't we add an fops debug bit ? + pr_info(%s: read: %zd (%d)\n, + video_device_node_name(vdev), sz, ret); Shouldn't we use KERN_DEBUG instead of KERN_INFO ? BTW, what about replacing the pr_* calls with dev_* calls ? return ret; } @@ -323,6 +326,9 @@ static ssize_t v4l2_write(struct file *filp, const char __user *buf, ret = vdev-fops-write(filp, buf, sz, off); if (test_bit(V4L2_FL_LOCK_ALL_FOPS, vdev-flags)) mutex_unlock(vdev-lock); + if (vdev-debug) + pr_info(%s: write: %zd (%d)\n, + video_device_node_name(vdev), sz, ret); return ret; } @@ -339,6 +345,9 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll) ret = vdev-fops-poll(filp, poll); if (test_bit(V4L2_FL_LOCK_ALL_FOPS, vdev-flags)) mutex_unlock(vdev-lock); + if (vdev-debug) + pr_info(%s: poll: %08x\n, + video_device_node_name(vdev), ret); return ret; } @@ -348,20 +357,14 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) int ret = -ENODEV; if (vdev-fops-unlocked_ioctl) { - bool locked = false; + struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd); - if (vdev-lock) { - /* always lock unless the cmd is marked as don't use lock */ - locked = !v4l2_is_known_ioctl(cmd) || - !test_bit(_IOC_NR(cmd), vdev-disable_locking); - - if (locked mutex_lock_interruptible(vdev-lock)) - return -ERESTARTSYS; - } + if (lock mutex_lock_interruptible(lock)) + return -ERESTARTSYS; if (video_is_registered(vdev)) ret = vdev-fops-unlocked_ioctl(filp, cmd, arg); - if (locked) - mutex_unlock(vdev-lock); + if (lock) + mutex_unlock(lock); } else if (vdev-fops-ioctl) { /* This code path is a replacement for the BKL. It is a major * hack but it will have to do for those drivers that are not @@ -409,12 +412,17 @@ static unsigned long v4l2_get_unmapped_area(struct file *filp, unsigned long flags) { struct video_device *vdev = video_devdata(filp); + int ret; if (!vdev-fops-get_unmapped_area) return -ENOSYS; if (!video_is_registered(vdev)) return -ENODEV; - return vdev-fops-get_unmapped_area(filp, addr, len, pgoff, flags); + ret = vdev-fops-get_unmapped_area(filp, addr, len, pgoff, flags); + if (vdev-debug) + pr_info(%s: get_unmapped_area (%d)\n, + video_device_node_name(vdev), ret); + return ret; } #endif @@ -432,6 +440,9 @@ static int v4l2_mmap(struct file *filp, struct vm_area_struct *vm) ret = vdev-fops-mmap(filp, vm); if (test_bit(V4L2_FL_LOCK_ALL_FOPS, vdev-flags)) mutex_unlock(vdev-lock); + if (vdev-debug) + pr_info(%s: mmap (%d)\n, + video_device_node_name(vdev), ret); return ret; } @@ -470,6 +481,9 @@ err: /* decrease the refcount in case of an error */ if (ret) video_put(vdev); + if (vdev-debug) + pr_info(%s: open (%d)\n, + video_device_node_name(vdev), ret); return ret; } @@ -489,6 +503,9 @@ static int v4l2_release(struct inode *inode, struct file *filp) /* decrease the refcount unconditionally since the release() return value is ignored. */ video_put(vdev); + if (vdev-debug) + pr_info(%s: release\n, + video_device_node_name(vdev)); return ret; } -- 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: [RFCv1 PATCH 28/32] vivi: use vb2 helper functions.
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:50 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/vivi.c | 160 ++--- 1 file changed, 21 insertions(+), 139 deletions(-) diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c index 1e4da5e..1e8c4f3 100644 --- a/drivers/media/video/vivi.c +++ b/drivers/media/video/vivi.c @@ -767,7 +767,13 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt, struct vivi_dev *dev = vb2_get_drv_priv(vq); unsigned long size; - size = dev-width * dev-height * dev-pixelsize; + if (fmt) + size = fmt-fmt.pix.sizeimage; + else + size = dev-width * dev-height * dev-pixelsize; + + if (size == 0) + return -EINVAL; If I'm not mistaken, this is a bug fix to properly support CREATE_BUF, right ? If so it should be split to its own patch. if (0 == *nbuffers) *nbuffers = 32; -- 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: [RFCv1 PATCH 25/32] create_bufs: handle count == 0.
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:47 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- Documentation/DocBook/media/v4l/vidioc-create-bufs.xml |8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml index 765549f..afdba4d 100644 --- a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml +++ b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml @@ -97,7 +97,13 @@ information./para row entry__u32/entry entrystructfieldcount/structfield/entry - entryThe number of buffers requested or granted./entry + entryThe number of buffers requested or granted. If count == 0, then + constantVIDIOC_CREATE_BUFS/constant will set structfieldindex/structfield + to the starting buffer index, I find starting buffer index a bit unclear in this context, as we don't create any buffer. and it will check the validity of + structfieldmemory/structfield and structfieldformat.type/structfield. + If those are invalid -1 is returned and errno is set to EINVAL;, + otherwise constantVIDIOC_CREATE_BUFS/constant returns 0. It will + never set errno to EBUSY; in this particular case./entry /row row entry__u32/entry -- 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: [RFCv1 PATCH 27/32] vivi: embed struct video_device instead of allocating it.
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:49 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/vivi.c | 25 +++-- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c index 8dd5ae6..1e4da5e 100644 --- a/drivers/media/video/vivi.c +++ b/drivers/media/video/vivi.c [snip] @@ -1080,7 +1078,6 @@ static int vidioc_enum_input(struct file *file, void *priv, return -EINVAL; inp-type = V4L2_INPUT_TYPE_CAMERA; - inp-std = V4L2_STD_525_60; Doesn't this belong to the previous patch ? sprintf(inp-name, Camera %u, inp-index); return 0; } -- 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 4/5] usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework
Hi Laurent, Can you please review this patch and let me know if any modifications are required.. -Original Message- From: Bhupesh SHARMA Sent: Friday, June 01, 2012 3:09 PM To: laurent.pinch...@ideasonboard.com; linux-...@vger.kernel.org Cc: ba...@ti.com; linux-media@vger.kernel.org; gre...@linuxfoundation.org; Bhupesh SHARMA Subject: [PATCH 4/5] usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework This patch reworks the videobuffer management logic present in the UVC webcam gadget and ports it to use the more apt videobuf2 framework for video buffer management. To support routing video data captured from a real V4L2 video capture device with a zero copy operation on videobuffers (as they pass from the V4L2 domain to UVC domain via a user-space application), we need to support USER_PTR IO method at the UVC gadget side. So the V4L2 capture device driver can still continue to use MMAO IO method and now the user-space application can just pass a pointer to the video buffers being DeQueued from the V4L2 device side while Queueing them at the UVC gadget end. This ensures that we have a zero-copy design as the videobuffers pass from the V4L2 capture device to the UVC gadget. Note that there will still be a need to apply UVC specific payload headers on top of each UVC payload data, which will still require a copy operation to be performed in the 'encode' routines of the UVC gadget. Signed-off-by: Bhupesh Sharma bhupesh.sha...@st.com --- drivers/usb/gadget/Kconfig |1 + drivers/usb/gadget/uvc_queue.c | 524 +++- drivers/usb/gadget/uvc_queue.h | 25 +-- drivers/usb/gadget/uvc_v4l2.c | 35 ++-- drivers/usb/gadget/uvc_video.c | 17 +- 5 files changed, 184 insertions(+), 418 deletions(-) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 1f93861..5a351f8 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -967,6 +967,7 @@ endif config USB_G_WEBCAM tristate USB Webcam Gadget depends on VIDEO_DEV + select VIDEOBUF2_VMALLOC help The Webcam Gadget acts as a composite USB Audio and Video Class device. It provides a userspace API to process UVC control requests diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c index 0cdf89d..907ece8 100644 --- a/drivers/usb/gadget/uvc_queue.c +++ b/drivers/usb/gadget/uvc_queue.c @@ -10,6 +10,7 @@ * (at your option) any later version. */ +#include linux/atomic.h #include linux/kernel.h #include linux/mm.h #include linux/list.h @@ -18,7 +19,8 @@ #include linux/videodev2.h #include linux/vmalloc.h #include linux/wait.h -#include linux/atomic.h + +#include media/videobuf2-vmalloc.h #include uvc.h @@ -28,271 +30,156 @@ * Video queues is initialized by uvc_queue_init(). The function performs * basic initialization of the uvc_video_queue struct and never fails. * - * Video buffer allocation and freeing are performed by uvc_alloc_buffers and - * uvc_free_buffers respectively. The former acquires the video queue lock, - * while the later must be called with the lock held (so that allocation can - * free previously allocated buffers). Trying to free buffers that are mapped - * to user space will return -EBUSY. - * - * Video buffers are managed using two queues. However, unlike most USB video - * drivers that use an in queue and an out queue, we use a main queue to hold - * all queued buffers (both 'empty' and 'done' buffers), and an irq queue to - * hold empty buffers. This design (copied from video-buf) minimizes locking - * in interrupt, as only one queue is shared between interrupt and user - * contexts. - * - * Use cases - * - - * - * Unless stated otherwise, all operations that modify the irq buffers queue - * are protected by the irq spinlock. - * - * 1. The user queues the buffers, starts streaming and dequeues a buffer. - * - *The buffers are added to the main and irq queues. Both operations are - *protected by the queue lock, and the later is protected by the irq - *spinlock as well. - * - *The completion handler fetches a buffer from the irq queue and fills it - *with video data. If no buffer is available (irq queue empty), the handler - *returns immediately. - * - *When the buffer is full, the completion handler removes it from the irq - *queue, marks it as ready (UVC_BUF_STATE_DONE) and wakes its wait queue. - *At that point, any process waiting on the buffer will be woken up. If a - *process tries to dequeue a buffer after it has been marked ready, the - *dequeing will succeed immediately. - * - * 2. Buffers are queued, user is waiting on a buffer and the device gets - *disconnected. - * - *When the device is disconnected, the kernel calls the completion handler - *with
RE: [PATCH 5/5] usb: gadget/uvc: Add support for 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command
Hi Laurent, Can you please review this patch and let me know if any modifications are required.. -Original Message- From: Bhupesh SHARMA Sent: Friday, June 01, 2012 3:09 PM To: laurent.pinch...@ideasonboard.com; linux-...@vger.kernel.org Cc: ba...@ti.com; linux-media@vger.kernel.org; gre...@linuxfoundation.org; Bhupesh SHARMA Subject: [PATCH 5/5] usb: gadget/uvc: Add support for 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command This patch adds the support in UVC webcam gadget design for providing USB_GADGET_DELAYED_STATUS in response to a set_interface(alt setting 1) command issue by the Host. The current UVC webcam gadget design generates a STREAMON event corresponding to a set_interface(alt setting 1) command from the Host. This STREAMON event will eventually be routed to a real V4L2 device. To start video streaming, it may be required to perform some register writes to a camera sensor device over slow external busses like I2C or SPI. So, it makes sense to ensure that we delay the STATUS stage of the set_interface(alt setting 1) command. Otherwise, a lot of ISOC IN tokens sent by the Host will be replied to by zero-length packets by the webcam device. On certain Hosts this may even lead to ISOC URBs been cancelled from the Host side. So, as soon as we finish doing all the streaming related stuff on the real V4L2 device, we call a STREAMON ioctl on the UVC side and from here we call the 'usb_composite_setup_continue' function to complete the status stage of the set_interface(alt setting 1) command. Further, we need to ensure that we queue no video buffers on the UVC webcam gadget, until we de-queue a video buffer from the V4L2 device. Also, we need to enable UVC video related stuff at the first QBUF ioctl call itself, as the application will call the STREAMON on UVC side only when it has dequeued sufficient buffers from the V4L2 side and queued them to the UVC gadget. So, the UVC video enable stuff cannot be done in STREAMON ioctl call. For the same we add two more UVC states: - PRE_STREAMING : not even a single buffer has been queued to UVC - BUF_QUEUED_STREAMING_OFF : one video buffer has been queued to UVC but we have not yet enabled STREAMING on UVC side. Signed-off-by: Bhupesh Sharma bhupesh.sha...@st.com --- drivers/usb/gadget/f_uvc.c| 17 - drivers/usb/gadget/uvc.h |3 +++ drivers/usb/gadget/uvc_v4l2.c | 38 -- 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c index 2a8bf06..3589ed0 100644 --- a/drivers/usb/gadget/f_uvc.c +++ b/drivers/usb/gadget/f_uvc.c @@ -272,6 +272,13 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) return 0; } +void uvc_function_setup_continue(struct uvc_device *uvc) +{ + struct usb_composite_dev *cdev = uvc-func.config-cdev; + + usb_composite_setup_continue(cdev); +} + static int uvc_function_get_alt(struct usb_function *f, unsigned interface) { @@ -334,7 +341,8 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) v4l2_event_queue(uvc-vdev, v4l2_event); uvc-state = UVC_STATE_CONNECTED; - break; + + return 0; case 1: if (uvc-state != UVC_STATE_CONNECTED) @@ -352,14 +360,13 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt) v4l2_event.type = UVC_EVENT_STREAMON; v4l2_event_queue(uvc-vdev, v4l2_event); - uvc-state = UVC_STATE_STREAMING; - break; + uvc-state = UVC_STATE_PRE_STREAMING; + + return USB_GADGET_DELAYED_STATUS; default: return -EINVAL; } - - return 0; } static void diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h index d78ea25..6cd1435 100644 --- a/drivers/usb/gadget/uvc.h +++ b/drivers/usb/gadget/uvc.h @@ -141,6 +141,8 @@ enum uvc_state { UVC_STATE_DISCONNECTED, UVC_STATE_CONNECTED, + UVC_STATE_PRE_STREAMING, + UVC_STATE_BUF_QUEUED_STREAMING_OFF, UVC_STATE_STREAMING, }; @@ -190,6 +192,7 @@ struct uvc_file_handle * Functions */ +extern void uvc_function_setup_continue(struct uvc_device *uvc); extern void uvc_endpoint_stream(struct uvc_device *dev); extern void uvc_function_connect(struct uvc_device *uvc); diff --git a/drivers/usb/gadget/uvc_v4l2.c b/drivers/usb/gadget/uvc_v4l2.c index 9c2b45b..5f23571 100644 --- a/drivers/usb/gadget/uvc_v4l2.c +++ b/drivers/usb/gadget/uvc_v4l2.c @@ -235,10 +235,36 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd, void *arg) } case VIDIOC_QBUF: + /* + * Theory of operation: + * -
Re: [RFCv1 PATCH 24/32] videobuf2-core: add helper functions.
Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:46 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Add helper functions to make it easier to adapt drivers to vb2. What about moving those functions to videobuf2-ioctl.c ? The helper functions are based on top of an existing vb2 core that isn't aware of queue ownership. It's not clear (to me at least) how the helpers will evolve and whether they will be used by all drivers or not, or whether part of what they do will get merged into the vb2 core. Splitting the helpers in a separate file would help not mixing code too much without really thinking about it. These helpers take care of core locking and check if the filehandle is the owner of the queue. This patch also adds support for count == 0 in create_bufs. Could you please split that to its own patch ? The addition of __verify_memory_type() should be split as well. Signed-off-by: Hans Verkuil hans.verk...@cisco.com -- 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: SoC i.mx35 userptr method failure while running capture-example utility
Hi, On Monday 18 June 2012 12:28:44 Prabhakar Lad wrote: On Wed, May 2, 2012 at 4:20 AM, Guennadi Liakhovetski wrote: On Tue, 1 May 2012, Alex Gershgorin wrote: Hi everyone, I use user-space utility from http://git.linuxtv.org/v4l-utils.git/blob/HEAD:/contrib/test/capture-ex ample.c I made two small changes in this application and this is running on i.MX35 SoC fmt.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB565; fmt.fmt.pix.field = V4L2_FIELD_ANY; When MMAP method is used everything works fine, problem occurs when using USERPTR method this can see bellow : ./capture-example -u -f -d /dev/video0 mx3-camera mx3-camera.0: MX3 Camera driver attached to camera 0 Failed acquiring VMA for vaddr 0x76cd9008 VIDIOC_QBUF error 22, Invalid arg It doesn't surprise me, that this doesn't work. capture-example allocates absolutely normal user-space buffers, when called with USERPTR, and those buffers are very likely discontiguous. Whereas mx3-camera needs physically contiguous buffers, so, this can only fail. This means, you either have to use MMAP or you need to allocate USERPTR buffers in a special way to guarantee their contiguity. Even I am facing a similar issue when ported to VB2 for USERPTR. How do we ensure the buffers not discontiguous. Would cmem assure it? CMEM is definitely not the way to go, it's an out-of-tree hack that should disappear once we get proper CMA and DMABUF support in the kernel. How to allocate memory depends on your use case(s). If you just need to capture to anonymous memory that will then be read by userspace you shouldn't use USERPTR, but MMAP. If you need to capture to device memory (for instance capturing directly to a frame buffer), you should export a DMABUF object on from the frame buffer driver (this isn't available in mainline yet, I'll try to send a patch soon) and then import it on the V4L2 side (not available in mainline yet either I'm afraid :-)). As an interim solution, mmap() your frame buffer to userspace and then use USERPTR. Unable to handle kernel NULL pointer dereference at virtual address This, however, is bad and is a bug in the driver. The capture-example should just fail nicely with no trouble. I'll add it to my TODO and will try to find some time to debug and fix this, however, I'd be more than happy if someone else would beat me on this ;-) Thanks Guennadi pgd = 80004000 [] *pgd= Internal error: Oops: 817 [#1] ARM CPU: 0Not tainted (3.4.0-rc5+ #2283 PC is at mx3_videobuf_release+0x9c/0x10c LR is at mx3_videobuf_release+0x20/0x10c pc : [802cd92c]lr : [802cd8b0]psr: 0093 sp : 86db3e00 ip : 86db3e00 fp : 86db3e2c r10: 86ff6b20 r9 : 86817200 r8 : r7 : 86ff568c r6 : r5 : 8801a000 r4 : 86da3000 r3 : 6013 r2 : 86da3264 r1 : r0 : Flags: nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 00c5387d Table: 86dcc008 DAC: 0015 Process capture-example (pid: 52, stack limit = 0x86db2268) Stack: (0x86db3e00 to 0x86db4000) 3e00: 6013 86ff568c 0002 86ff56ac 3e20: 86db3e64 86db3e30 802c8978 802cd89c 80099414 86db3e84 86ff568c 3e40: 86dc9a80 8801a03c 80491828 86817200 86ff6b20 86db3e7c 86db3e68 3e60: 802c9a1c 802c8930 802ce048 86ff5600 86db3e9c 86db3e80 802cca14 802c9a00 3e80: 86ff5800 86dc9a80 0008 86dc9a88 86db3eb4 86db3ea0 802b936c 802cc9d0 3ea0: 86dc9a80 86ff6b20 86db3ef4 86db3eb8 80082f00 802b932c 3ec0: 86d35010 86d7f000 86dc9a80 86d59000 86d90120 000c 3ee0: 86db2000 86db3f14 86db3ef8 8007ff58 80082df0 86d59000 3f00: 0001 86db3f3c 86db3f18 8001c72c 8007fee4 86d59000 86d82000 3f20: 0100 76ef1770 00f8 8000e564 86db3f4c 86db3f40 8001c7a8 8001c6b4 3f40: 86db3f7c 86db3f50 8001dab4 8001c78c 7eb002b8 0001 0004 3f60: 86db3fa4 86db3f70 800824fc 00f8 86db3f94 86db3f80 8001dfc0 8001d8c4 3f80: 000a3d78 86db3fa4 86db3f98 8001e004 8001df4c 86db3fa8 3fa0: 8000e3e0 8001dff8 000a3d78 76ef1770 0001 000a3d64 0008 0001 3fc0: 000a3d78 76ef1770 76ef1770 00f8 76e1d248 9ecc 7eb02954 3fe0: 76f2e000 7eb02908 76de14dc 76e4f3d4 6010 0001 Backtrace: [802cd890] (mx3_videobuf_release+0x0/0x10c) from [802c8978] (__vb2_queue_free+0x54/0x15c) r8: r7:86ff56ac r6:0002 r5: r4:86ff568c [802c8924] (__vb2_queue_free+0x0/0x15c) from [802c9a1c] (vb2_queue_release+0x28/0x2c) [802c99f4] (vb2_queue_release+0x0/0x2c) from [802cca14] (soc_camera_close+0x50/0xac) r4:86ff5600 r3:802ce048 [802cc9c4] (soc_camera_close+0x0/0xac) from [802b936c] (v4l2_release+0x4c/0x6c) r7:86dc9a88 r6:0008 r5:86dc9a80 r4:86ff5800 [802b9320] (v4l2_release+0x0/0x6c) from [80082f00]
Re: uvcvideo issue with kernel 3.5-rc2 and 3
Hi Philipp, On Sunday 17 June 2012 13:35:07 Philipp Dreimann wrote: Hello, my external webcam from Logitech (I guess it's a c910) stopped working using kernel 3.5-rc3.( 3.4 worked fine.) uvcvideo: Found UVC 1.00 device unnamed (046d:0821) input: UVC Camera (046d:0821) as /devices/pci:00/:00:1a.0/usb1/1-1/1-1.2/1-1.2:1.2/input/input14 usbcore: registered new interface driver uvcvideo USB Video Class driver (1.1.1) uvcvideo: Failed to query (GET_DEF) UVC control 2 on unit 2: -71 (exp. 2). uvcvideo: Failed to query (GET_DEF) UVC control 2 on unit 2: -71 (exp. 2). uvcvideo: Failed to query (GET_DEF) UVC control 3 on unit 2: -71 (exp. 2). uvcvideo: Failed to query (GET_DEF) UVC control 7 on unit 2: -71 (exp. 2). uvcvideo: Failed to query (GET_DEF) UVC control 11 on unit 2: -71 (exp. 1). uvcvideo: Failed to query (GET_DEF) UVC control 4 on unit 2: -71 (exp. 2). uvcvideo: Failed to query (GET_DEF) UVC control 5 on unit 2: -71 (exp. 1). uvcvideo: Failed to query (GET_CUR) UVC control 11 on unit 2: -71 (exp. 1). uvcvideo: Failed to query (GET_DEF) UVC control 8 on unit 2: -71 (exp. 2). uvcvideo: Failed to query (GET_DEF) UVC control 1 on unit 2: -71 (exp. 2). uvcvideo: Failed to set UVC probe control : -71 (exp. 26). uvcvideo: Failed to set UVC probe control : -71 (exp. 26). uvcvideo: Failed to set UVC probe control : -71 (exp. 26). uvcvideo: Failed to set UVC probe control : -71 (exp. 26). (the last line is being repeated...) This might be cause by a bug in the USB core or in the UVC driver. Would you be able to bisect the regression ? Or, alternatively, test the v3.4 uvcvideo driver on v3.5-rc3 ? Or the other way around, test the latest v4l tree on v3.4 (instructions regarding how to compile the v4l tree with a different kernel are available at http://linuxtv.org/wiki/index.php/How_to_Obtain,_Build_and_Install_V4L- DVB_Device_Drivers). I used cheese to test the webcam. My other webcam is working fine: uvcvideo: Found UVC 1.00 device Integrated Camera (04f2:b217) input: Integrated Camera as /devices/pci:00/:00:1a.0/usb1/1-1/1-1.6/1-1.6:1.0/input/input13 -- 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: [RFCv1 PATCH 18/32] v4l2-ioctl.c: finalize table conversion.
Em 18-06-2012 06:46, Laurent Pinchart escreveu: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:40 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-ioctl.c | 35 +-- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 0de31c4..6c91674 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -870,6 +870,11 @@ static void v4l_print_newline(const void *arg) pr_cont(\n); } +static void v4l_print_default(const void *arg) +{ +pr_cont(non-standard ioctl\n); I'd say driver-specific ioctl instead. non-standard may sound like an error to users. This message is useless as-is, as it provides no glue about what ioctl was called. You should either remove it or print the ioctl number, in hexa. Regards, 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 PULL FOR 3.6] V4L2 API cleanups
Hi Sakari, On Sunday 17 June 2012 10:54:32 Sakari Ailus wrote: On Sun, Jun 17, 2012 at 12:03:06AM +0200, Laurent Pinchart wrote: On Monday 11 June 2012 12:39:44 Sakari Ailus wrote: On Mon, Jun 11, 2012 at 09:50:54AM +0200, Laurent Pinchart wrote: On Sunday 10 June 2012 23:22:59 Sakari Ailus wrote: Hi Mauro, Here are two V4L2 API cleanup patches; the first removes __user from videodev2.h from a few places, making it possible to use the header file as such in user space, while the second one changes the v4l2_buffer.input field back to reserved. The following changes since commit 5472d3f17845c4398c6a510b46855820920c2181: [media] mt9m032: Implement V4L2_CID_PIXEL_RATE control (2012-05-24 09:27:24 -0300) are available in the git repository at: ssh://linuxtv.org/git/sailus/media_tree.git media-for-3.6 Sakari Ailus (2): v4l: Remove __user from interface structure definitions NAK, sorry. __user has a purpose, we need to add it where it's missing, not remove it where it's rightfully present. It's not quite as simple as adding __user everywhere it might belong to --- these structs are being used in kernel space, too. The structs that are part of the user space interface may at some point contain pointers to memory which is in user space. That is being dealt by video_usercopy(), so the individual drivers or the rest of the V4L2 framework always gets pointers pointing to kernel memory. Very good point, I haven't thought about that. I'm not sure how to deal with this, splitting structures in a __user and a non __user version isn't really a good option. Maybe the sparse tool should be somehow extended ? Wouldn't type casting in video_usercopy() just do the job? Albeit I'm far from certain it'd make the code better, just make the sparse warnings go away... For pointers that are completely handled in the v4l core (such as the v4l2_buffer and v4l2_ext_controls pointer fields), casting casting in video_usercopy() is enough as no driver will ever see the user pointers (we already cast in those cases). For pointers that are to be handled by drivers, I think we need to keep __user (or make the v4l core handle them). -- 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: SoC i.mx35 userptr method failure while running capture-example utility
Hi Laurent, On Mon, Jun 18, 2012 at 4:05 PM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: Hi, On Monday 18 June 2012 12:28:44 Prabhakar Lad wrote: On Wed, May 2, 2012 at 4:20 AM, Guennadi Liakhovetski wrote: On Tue, 1 May 2012, Alex Gershgorin wrote: Hi everyone, I use user-space utility from http://git.linuxtv.org/v4l-utils.git/blob/HEAD:/contrib/test/capture-ex ample.c I made two small changes in this application and this is running on i.MX35 SoC fmt.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB565; fmt.fmt.pix.field = V4L2_FIELD_ANY; When MMAP method is used everything works fine, problem occurs when using USERPTR method this can see bellow : ./capture-example -u -f -d /dev/video0 mx3-camera mx3-camera.0: MX3 Camera driver attached to camera 0 Failed acquiring VMA for vaddr 0x76cd9008 VIDIOC_QBUF error 22, Invalid arg It doesn't surprise me, that this doesn't work. capture-example allocates absolutely normal user-space buffers, when called with USERPTR, and those buffers are very likely discontiguous. Whereas mx3-camera needs physically contiguous buffers, so, this can only fail. This means, you either have to use MMAP or you need to allocate USERPTR buffers in a special way to guarantee their contiguity. Even I am facing a similar issue when ported to VB2 for USERPTR. How do we ensure the buffers not discontiguous. Would cmem assure it? CMEM is definitely not the way to go, it's an out-of-tree hack that should disappear once we get proper CMA and DMABUF support in the kernel. How to allocate memory depends on your use case(s). If you just need to capture to anonymous memory that will then be read by userspace you shouldn't use USERPTR, but MMAP. If you need to capture to device memory (for instance capturing directly to a frame buffer), you should export a DMABUF object on from the frame buffer driver (this isn't available in mainline yet, I'll try to send a patch soon) and then import it on the V4L2 side (not available in mainline yet either I'm afraid :-)). As an interim solution, mmap() your frame buffer to userspace and then use USERPTR. I have got MMAP working with videobuf2, with videobuf there was a support for userptr which I need to achieve this with videobuf2. Can you point me to your branch where you are working on it and also an example if you have to interim solution as you suggested above. Thx, --Prabhakar Lad Unable to handle kernel NULL pointer dereference at virtual address This, however, is bad and is a bug in the driver. The capture-example should just fail nicely with no trouble. I'll add it to my TODO and will try to find some time to debug and fix this, however, I'd be more than happy if someone else would beat me on this ;-) Thanks Guennadi pgd = 80004000 [] *pgd= Internal error: Oops: 817 [#1] ARM CPU: 0 Not tainted (3.4.0-rc5+ #2283 PC is at mx3_videobuf_release+0x9c/0x10c LR is at mx3_videobuf_release+0x20/0x10c pc : [802cd92c] lr : [802cd8b0] psr: 0093 sp : 86db3e00 ip : 86db3e00 fp : 86db3e2c r10: 86ff6b20 r9 : 86817200 r8 : r7 : 86ff568c r6 : r5 : 8801a000 r4 : 86da3000 r3 : 6013 r2 : 86da3264 r1 : r0 : Flags: nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user Control: 00c5387d Table: 86dcc008 DAC: 0015 Process capture-example (pid: 52, stack limit = 0x86db2268) Stack: (0x86db3e00 to 0x86db4000) 3e00: 6013 86ff568c 0002 86ff56ac 3e20: 86db3e64 86db3e30 802c8978 802cd89c 80099414 86db3e84 86ff568c 3e40: 86dc9a80 8801a03c 80491828 86817200 86ff6b20 86db3e7c 86db3e68 3e60: 802c9a1c 802c8930 802ce048 86ff5600 86db3e9c 86db3e80 802cca14 802c9a00 3e80: 86ff5800 86dc9a80 0008 86dc9a88 86db3eb4 86db3ea0 802b936c 802cc9d0 3ea0: 86dc9a80 86ff6b20 86db3ef4 86db3eb8 80082f00 802b932c 3ec0: 86d35010 86d7f000 86dc9a80 86d59000 86d90120 000c 3ee0: 86db2000 86db3f14 86db3ef8 8007ff58 80082df0 86d59000 3f00: 0001 86db3f3c 86db3f18 8001c72c 8007fee4 86d59000 86d82000 3f20: 0100 76ef1770 00f8 8000e564 86db3f4c 86db3f40 8001c7a8 8001c6b4 3f40: 86db3f7c 86db3f50 8001dab4 8001c78c 7eb002b8 0001 0004 3f60: 86db3fa4 86db3f70 800824fc 00f8 86db3f94 86db3f80 8001dfc0 8001d8c4 3f80: 000a3d78 86db3fa4 86db3f98 8001e004 8001df4c 86db3fa8 3fa0: 8000e3e0 8001dff8 000a3d78 76ef1770 0001 000a3d64 0008 0001 3fc0: 000a3d78 76ef1770 76ef1770 00f8 76e1d248 9ecc 7eb02954 3fe0: 76f2e000 7eb02908 76de14dc 76e4f3d4 6010 0001 Backtrace: [802cd890] (mx3_videobuf_release+0x0/0x10c) from [802c8978] (__vb2_queue_free+0x54/0x15c) r8: r7:86ff56ac r6:0002 r5: r4:86ff568c [802c8924]
Re: [RFCv1 PATCH 18/32] v4l2-ioctl.c: finalize table conversion.
Hi Mauro, On Monday 18 June 2012 07:50:35 Mauro Carvalho Chehab wrote: Em 18-06-2012 06:46, Laurent Pinchart escreveu: On Sunday 10 June 2012 12:25:40 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-ioctl.c | 35 +-- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 0de31c4..6c91674 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -870,6 +870,11 @@ static void v4l_print_newline(const void *arg) pr_cont(\n); } +static void v4l_print_default(const void *arg) +{ + pr_cont(non-standard ioctl\n); I'd say driver-specific ioctl instead. non-standard may sound like an error to users. This message is useless as-is, as it provides no glue about what ioctl was called. You should either remove it or print the ioctl number, in hexa. I think the ioctl number is already printed by the caller of v4l_print_default. -- 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: [PATCHv7 01/15] v4l: Add DMABUF as a memory type
Hi everyone, The automatic build system informed me that there is a shameful error in this patch. The declarations of fd fields in v4l2_plane32 and v4l2_buffer32 are missing. It breaks build process for 64-bit architectures. I am deeply sorry for posting a patch without testing it enough. The build-break will be fixed in the next version of the patchset. Regards, Tomasz Stanislawski On 06/14/2012 03:37 PM, Tomasz Stanislawski wrote: From: Sumit Semwal sumit.sem...@ti.com Adds DMABUF memory type to v4l framework. Also adds the related file descriptor in v4l2_plane and v4l2_buffer. Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com [original work in the PoC for buffer sharing] Signed-off-by: Sumit Semwal sumit.sem...@ti.com Signed-off-by: Sumit Semwal sumit.sem...@linaro.org Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com --- drivers/media/video/v4l2-compat-ioctl32.c | 16 drivers/media/video/v4l2-ioctl.c |1 + include/linux/videodev2.h |7 +++ 3 files changed, 24 insertions(+) diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c index 5327ad3..d33ab18 100644 --- a/drivers/media/video/v4l2-compat-ioctl32.c +++ b/drivers/media/video/v4l2-compat-ioctl32.c @@ -348,6 +348,9 @@ static int get_v4l2_plane32(struct v4l2_plane *up, struct v4l2_plane32 *up32, up_pln = compat_ptr(p); if (put_user((unsigned long)up_pln, up-m.userptr)) return -EFAULT; + } else if (memory == V4L2_MEMORY_DMABUF) { + if (copy_in_user(up-m.fd, up32-m.fd, sizeof(int))) + return -EFAULT; } else { if (copy_in_user(up-m.mem_offset, up32-m.mem_offset, sizeof(__u32))) @@ -371,6 +374,11 @@ static int put_v4l2_plane32(struct v4l2_plane *up, struct v4l2_plane32 *up32, if (copy_in_user(up32-m.mem_offset, up-m.mem_offset, sizeof(__u32))) return -EFAULT; + /* For DMABUF, driver might've set up the fd, so copy it back. */ + if (memory == V4L2_MEMORY_DMABUF) + if (copy_in_user(up32-m.fd, up-m.fd, + sizeof(int))) + return -EFAULT; return 0; } @@ -454,6 +462,10 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user if (get_user(kp-m.offset, up-m.offset)) return -EFAULT; break; + case V4L2_MEMORY_DMABUF: + if (get_user(kp-m.fd, up-m.fd)) + return -EFAULT; + break; } } @@ -518,6 +530,10 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user if (put_user(kp-m.offset, up-m.offset)) return -EFAULT; break; + case V4L2_MEMORY_DMABUF: + if (put_user(kp-m.fd, up-m.fd)) + return -EFAULT; + break; } } diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 91be4e8..31fc2ad 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -175,6 +175,7 @@ static const char *v4l2_memory_names[] = { [V4L2_MEMORY_MMAP]= mmap, [V4L2_MEMORY_USERPTR] = userptr, [V4L2_MEMORY_OVERLAY] = overlay, + [V4L2_MEMORY_DMABUF] = dmabuf, }; #define prt_names(a, arr) a) = 0) ((a) ARRAY_SIZE(arr))) ? \ diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 370d111..51b20f4 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -185,6 +185,7 @@ enum v4l2_memory { V4L2_MEMORY_MMAP = 1, V4L2_MEMORY_USERPTR = 2, V4L2_MEMORY_OVERLAY = 3, + V4L2_MEMORY_DMABUF = 4, }; /* see also http://vektor.theorem.ca/graphics/ycbcr/ */ @@ -591,6 +592,8 @@ struct v4l2_requestbuffers { * should be passed to mmap() called on the video node) * @userptr: when memory is V4L2_MEMORY_USERPTR, a userspace pointer * pointing to this plane + * @fd: when memory is V4L2_MEMORY_DMABUF, a userspace file + * descriptor associated with this plane * @data_offset: offset in the plane to the start of data; usually 0, * unless there is a header in front of the data * @@ -605,6 +608,7 @@ struct v4l2_plane { union { __u32 mem_offset; unsigned long userptr; + int fd; } m; __u32
Re: [RFCv1 PATCH 18/32] v4l2-ioctl.c: finalize table conversion.
On Mon June 18 2012 11:46:57 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:40 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-ioctl.c | 35 +-- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 0de31c4..6c91674 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -870,6 +870,11 @@ static void v4l_print_newline(const void *arg) pr_cont(\n); } +static void v4l_print_default(const void *arg) +{ + pr_cont(non-standard ioctl\n); I'd say driver-specific ioctl instead. non-standard may sound like an error to users. Much better, thank you! Regards, Hans -- 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] v4l: added V4L2_BUF_FLAG_EOS flag indicating the last frame in the stream
Em 22-05-2012 12:33, Andrzej Hajda escreveu: Some devices requires indicator if the buffer is the last one in the stream. Applications and drivers can use this flag in such case. Signed-off-by: Andrzej Hajda a.ha...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- Documentation/DocBook/media/v4l/io.xml |7 +++ Documentation/DocBook/media/v4l/vidioc-qbuf.xml |2 ++ include/linux/videodev2.h |1 + 3 files changed, 10 insertions(+), 0 deletions(-) diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml index fd6aca2..dcbf1e0 100644 --- a/Documentation/DocBook/media/v4l/io.xml +++ b/Documentation/DocBook/media/v4l/io.xml @@ -956,6 +956,13 @@ Typically applications shall use this flag for output buffers if the data in this buffer has not been created by the CPU but by some DMA-capable unit, in which case caches have not been used./entry /row + row + entryconstantV4L2_BUF_FLAG_EOS/constant/entry + entry0x2000/entry + entryApplication should set this flag in the output buffer +in order to inform the driver about the last frame of the stream. Some +drivers may require it to properly finish processing the stream./entry This breaks backward compatibility, as applications written before this change won't set this flag. + /row /tbody /tgroup /table diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml index 9caa49a..ad49f7d 100644 --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml @@ -76,6 +76,8 @@ supports capturing from specific video inputs and you want to specify a video input, then structfieldflags/structfield should be set to constantV4L2_BUF_FLAG_INPUT/constant and the field structfieldinput/structfield must be initialized to the desired input. +Some drivers expects applications set constantV4L2_BUF_FLAG_EOS/constant +flag on the last buffer of the stream. The structfieldreserved/structfield field must be set to 0. When using the link linkend=planar-apismulti-planar API/link, the structfieldm.planes/structfield field must contain a userspace pointer diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 370d111..e44a7cd 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -676,6 +676,7 @@ struct v4l2_buffer { /* Cache handling flags */ #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN0x1000 +#define V4L2_BUF_FLAG_EOS0x2000 /* The last buffer in the stream */ /* * O V E R L A Y P R E V I E W -- 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: [RFCv1 PATCH 04/32] v4l2-ioctl.c: v4l2-ioctl: add debug and callback/offset functionality.
On Mon June 18 2012 11:47:07 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:26 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Add the necessary plumbing to make it possible to replace the switch by a table driven implementation. Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-ioctl.c | 91 -- 1 file changed, 78 insertions(+), 13 deletions(-) diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index a9602db..a4115ce 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -563,23 +595,40 @@ static long __video_do_ioctl(struct file *file, } if (v4l2_is_known_ioctl(cmd)) { - struct v4l2_ioctl_info *info = v4l2_ioctls[_IOC_NR(cmd)]; + info = v4l2_ioctls[_IOC_NR(cmd)]; if (!test_bit(_IOC_NR(cmd), vfd-valid_ioctls) !((info-flags INFO_FL_CTRL) vfh vfh-ctrl_handler)) - return -ENOTTY; + goto error; if (use_fh_prio (info-flags INFO_FL_PRIO)) { ret = v4l2_prio_check(vfd-prio, vfh-prio); if (ret) - return ret; + goto error; } + } else { + default_info.ioctl = cmd; + default_info.flags = 0; + default_info.debug = NULL; + info = default_info; } - if ((vfd-debug V4L2_DEBUG_IOCTL) - !(vfd-debug V4L2_DEBUG_IOCTL_ARG)) { + write_only = _IOC_DIR(cmd) == _IOC_WRITE; + if (info-debug write_only vfd-debug V4L2_DEBUG_IOCTL) { v4l_print_ioctl(vfd-name, cmd); - printk(KERN_CONT \n); + pr_cont(: ); + info-debug(arg); + } Shouldn't you print the ioctl name and information even if info-debug is NULL ? The 'info-debug' test is temporary. Once the conversion is finished this test will be removed since then info-debug will never be NULL. + if (info-flags INFO_FL_STD) { + typedef int (*vidioc_op)(struct file *file, void *fh, void *p); + const void *p = vfd-ioctl_ops; + const vidioc_op *vidioc = p + info-offset; + + ret = (*vidioc)(file, fh, arg); + goto error; + } else if (info-flags INFO_FL_FUNC) { + ret = info-func(ops, file, fh, arg); + goto error; } switch (cmd) { @@ -2100,10 +2149,26 @@ static long __video_do_ioctl(struct file *file, break; } /* switch */ - if (vfd-debug V4L2_DEBUG_IOCTL_ARG) { - if (ret 0) { - v4l_print_ioctl(vfd-name, cmd); - printk(KERN_CONT error %ld\n, ret); +error: This isn't an error, is it ? I'd call it done instead. True, I'll change this. + if (vfd-debug) { + if (write_only vfd-debug V4L2_DEBUG_IOCTL) { vfd-debug is a bitmask (or at least is documented as being a bitmask in include/media/v4l2-ioctl.h). Yeah, and that makes no sense. Having V4L2_DEBUG_IOCTL_ARG without V4L2_DEBUG_IOCTL is undefined. It is much more logical to have 0 for no debugging, 1 for just ioctl name debugging and = 2 for extensive debugging. It's just weird to have to set the debug sysfs entry to 0, 1 or 3. In a later patch I want to change the few drivers that use this accordingly. + if (ret) Shouldn't you test for 0 instead ? Driver-specific ioctls might return a 0 value in case of success. Good catch. Yes, I need to do that. + pr_info(%s: error %ld\n, + video_device_node_name(vfd), ret); + return ret; + } + v4l_print_ioctl(vfd-name, cmd); + if (ret) + pr_cont(: error %ld\n, ret); + else if (vfd-debug == V4L2_DEBUG_IOCTL) + pr_cont(\n); + else if (!info-debug) + return ret; + else if (_IOC_DIR(cmd) == _IOC_NONE) + info-debug(arg); + else { + pr_cont(: ); + info-debug(arg); } Ouch. What are you trying to do here ? Can't we simplify debug messages ? It's simplified a bit in a later patch when the temporary info-debug test can be dropped. Regards, Hans -- 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: [RFCv1 PATCH 19/32] v4l2-dev.c: add debug sysfs entry.
On Mon June 18 2012 11:48:45 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:41 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-dev.c | 24 1 file changed, 24 insertions(+) diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c index 1500208..5c0bb18 100644 --- a/drivers/media/video/v4l2-dev.c +++ b/drivers/media/video/v4l2-dev.c @@ -46,6 +46,29 @@ static ssize_t show_index(struct device *cd, return sprintf(buf, %i\n, vdev-index); } +static ssize_t show_debug(struct device *cd, +struct device_attribute *attr, char *buf) +{ + struct video_device *vdev = to_video_device(cd); + + return sprintf(buf, %i\n, vdev-debug); +} + +static ssize_t set_debug(struct device *cd, struct device_attribute *attr, + const char *buf, size_t len) +{ + struct video_device *vdev = to_video_device(cd); + int res = 0; + u16 value; + + res = kstrtou16(buf, 0, value); + if (res) + return res; + + vdev-debug = value; Can't this race with the various vdev-debug tests we have in the V4L core ? Not really. You may have a short race where you set it, but some core test is just reading it and gets the old value. But that's no problem in this case. The worst that can happen is that you get one more or one less debug message in the log. It probably deserves a comment, though. Regards, Hans -- 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: [RFCv1 PATCH 20/32] v4l2-ioctl: remove v4l_(i2c_)print_ioctl
On Mon June 18 2012 11:50:38 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:42 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com v4l_i2c_print_ioctl wasn't used and v4l_print_ioctl could be replaced by v4l_i2c_printk_ioctl. I suppose you meant v4l_printk_ioctl herE. Yes indeed. Regards, Hans Signed-off-by: Hans Verkuil hans.verk...@cisco.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: [RFCv1 PATCH 30/32] v4l2-ioctl.c: shorten the lines of the table.
On Mon June 18 2012 11:57:24 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:52 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Use some macro magic to reduce the length of the lines in the table. This makes it more readable. It indeed shortens the lines, but to be honest I find the result less readable. What do you think, should I just keep those long lines? I've tried splitting them up, but that too made it harder to read. Regards, Hans Signed-off-by: Hans Verkuil hans.verk...@cisco.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: [RFCv1 PATCH 19/32] v4l2-dev.c: add debug sysfs entry.
Hi Hans, On Monday 18 June 2012 13:30:18 Hans Verkuil wrote: On Mon June 18 2012 11:48:45 Laurent Pinchart wrote: On Sunday 10 June 2012 12:25:41 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-dev.c | 24 1 file changed, 24 insertions(+) diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c index 1500208..5c0bb18 100644 --- a/drivers/media/video/v4l2-dev.c +++ b/drivers/media/video/v4l2-dev.c @@ -46,6 +46,29 @@ static ssize_t show_index(struct device *cd, return sprintf(buf, %i\n, vdev-index); } +static ssize_t show_debug(struct device *cd, + struct device_attribute *attr, char *buf) +{ + struct video_device *vdev = to_video_device(cd); + + return sprintf(buf, %i\n, vdev-debug); +} + +static ssize_t set_debug(struct device *cd, struct device_attribute *attr, +const char *buf, size_t len) +{ + struct video_device *vdev = to_video_device(cd); + int res = 0; + u16 value; + + res = kstrtou16(buf, 0, value); + if (res) + return res; + + vdev-debug = value; Can't this race with the various vdev-debug tests we have in the V4L core ? Not really. You may have a short race where you set it, but some core test is just reading it and gets the old value. But that's no problem in this case. The worst that can happen is that you get one more or one less debug message in the log. You test the debug value several times in the __video_do_ioctl() function. I haven't checked in details whether changing the value between the two tests could for instance lead to a KERN_CONT print without a previous non-KERN_CONT message. That won't crash the machine :-) but it should still be avoided. It probably deserves a comment, though. -- 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: [RFCv1 PATCH 29/32] v4l2-dev.c: also add debug support for the fops.
On Mon June 18 2012 12:01:47 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:51 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-dev.c | 41 ++--- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c index 5c0bb18..54f387d 100644 --- a/drivers/media/video/v4l2-dev.c +++ b/drivers/media/video/v4l2-dev.c @@ -305,6 +305,9 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf, ret = vdev-fops-read(filp, buf, sz, off); if (test_bit(V4L2_FL_LOCK_ALL_FOPS, vdev-flags)) mutex_unlock(vdev-lock); + if (vdev-debug) As vdev-debug is a bitmask, shouldn't we add an fops debug bit ? I actually want to move away from the bitmask idea. I've never really liked it here. + pr_info(%s: read: %zd (%d)\n, + video_device_node_name(vdev), sz, ret); Shouldn't we use KERN_DEBUG instead of KERN_INFO ? BTW, what about replacing the pr_* calls with dev_* calls ? KERN_DEBUG vs KERN_INFO is actually a good question. My reasoning is that you explicitly enable logging, and so you really want to see it in the log, so we use KERN_INFO. With KERN_DEBUG you might have the situation where the debug level of the logging is disabled, so the messages are ignored. However, if people disagree with this, then I'm happy to move it back to KERN_DEBUG. With regards to dev_ vs pr_: I'd have to test this to see what dev_ prints as prefix. Regards, Hans -- 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: [RFCv1 PATCH 28/32] vivi: use vb2 helper functions.
On Mon June 18 2012 12:08:10 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:50 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/vivi.c | 160 ++--- 1 file changed, 21 insertions(+), 139 deletions(-) diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c index 1e4da5e..1e8c4f3 100644 --- a/drivers/media/video/vivi.c +++ b/drivers/media/video/vivi.c @@ -767,7 +767,13 @@ static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt, struct vivi_dev *dev = vb2_get_drv_priv(vq); unsigned long size; - size = dev-width * dev-height * dev-pixelsize; + if (fmt) + size = fmt-fmt.pix.sizeimage; + else + size = dev-width * dev-height * dev-pixelsize; + + if (size == 0) + return -EINVAL; If I'm not mistaken, this is a bug fix to properly support CREATE_BUF, right ? If so it should be split to its own patch. OK. Hans -- 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: [RFCv1 PATCH 25/32] create_bufs: handle count == 0.
On Mon June 18 2012 12:11:27 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:47 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- Documentation/DocBook/media/v4l/vidioc-create-bufs.xml |8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml index 765549f..afdba4d 100644 --- a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml +++ b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml @@ -97,7 +97,13 @@ information./para row entry__u32/entry entrystructfieldcount/structfield/entry - entryThe number of buffers requested or granted./entry + entryThe number of buffers requested or granted. If count == 0, then + constantVIDIOC_CREATE_BUFS/constant will set structfieldindex/structfield + to the starting buffer index, I find starting buffer index a bit unclear in this context, as we don't create any buffer. How about: ... will set index to the current number of created buffers, Better alternatives welcome :-) Hans and it will check the validity of + structfieldmemory/structfield and structfieldformat.type/structfield. + If those are invalid -1 is returned and errno is set to EINVAL;, + otherwise constantVIDIOC_CREATE_BUFS/constant returns 0. It will + never set errno to EBUSY; in this particular case./entry /row row entry__u32/entry -- 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: [RFCv1 PATCH 27/32] vivi: embed struct video_device instead of allocating it.
On Mon June 18 2012 12:13:38 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:49 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/vivi.c | 25 +++-- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c index 8dd5ae6..1e4da5e 100644 --- a/drivers/media/video/vivi.c +++ b/drivers/media/video/vivi.c [snip] @@ -1080,7 +1078,6 @@ static int vidioc_enum_input(struct file *file, void *priv, return -EINVAL; inp-type = V4L2_INPUT_TYPE_CAMERA; - inp-std = V4L2_STD_525_60; Doesn't this belong to the previous patch ? Indeed. Hans -- 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: [RFCv1 PATCH 24/32] videobuf2-core: add helper functions.
On Mon June 18 2012 12:23:26 Laurent Pinchart wrote: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:46 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Add helper functions to make it easier to adapt drivers to vb2. What about moving those functions to videobuf2-ioctl.c ? The helper functions are based on top of an existing vb2 core that isn't aware of queue ownership. It's not clear (to me at least) how the helpers will evolve and whether they will be used by all drivers or not, or whether part of what they do will get merged into the vb2 core. Splitting the helpers in a separate file would help not mixing code too much without really thinking about it. Sounds reasonable. One thing that I want to think about when preparing RFCv2 is whether the new queue_lock in video_device shouldn't be moved to vb2_queue instead, since it is a per-vb2_queue lock. I need to check whether that still makes it possible to split it up like you suggest. It almost certainly will make sense to do this. These helpers take care of core locking and check if the filehandle is the owner of the queue. This patch also adds support for count == 0 in create_bufs. Could you please split that to its own patch ? The addition of __verify_memory_type() should be split as well. OK. Regards, Hans Signed-off-by: Hans Verkuil hans.verk...@cisco.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: [RFCv1 PATCH 18/32] v4l2-ioctl.c: finalize table conversion.
On Mon June 18 2012 12:50:35 Mauro Carvalho Chehab wrote: Em 18-06-2012 06:46, Laurent Pinchart escreveu: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:40 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-ioctl.c | 35 +-- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 0de31c4..6c91674 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -870,6 +870,11 @@ static void v4l_print_newline(const void *arg) pr_cont(\n); } +static void v4l_print_default(const void *arg) +{ + pr_cont(non-standard ioctl\n); I'd say driver-specific ioctl instead. non-standard may sound like an error to users. This message is useless as-is, as it provides no glue about what ioctl was called. You should either remove it or print the ioctl number, in hexa. That ioctl number is already printed in front of this message. Regards, Hans -- 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] v4l: added V4L2_BUF_FLAG_EOS flag indicating the last frame in the stream
On Mon, 2012-06-18 at 08:24 -0300, Mauro Carvalho Chehab wrote: Em 22-05-2012 12:33, Andrzej Hajda escreveu: Some devices requires indicator if the buffer is the last one in the stream. Applications and drivers can use this flag in such case. Signed-off-by: Andrzej Hajda a.ha...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- Documentation/DocBook/media/v4l/io.xml |7 +++ Documentation/DocBook/media/v4l/vidioc-qbuf.xml |2 ++ include/linux/videodev2.h |1 + 3 files changed, 10 insertions(+), 0 deletions(-) diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml index fd6aca2..dcbf1e0 100644 --- a/Documentation/DocBook/media/v4l/io.xml +++ b/Documentation/DocBook/media/v4l/io.xml @@ -956,6 +956,13 @@ Typically applications shall use this flag for output buffers if the data in this buffer has not been created by the CPU but by some DMA-capable unit, in which case caches have not been used./entry /row + row + entryconstantV4L2_BUF_FLAG_EOS/constant/entry + entry0x2000/entry + entryApplication should set this flag in the output buffer +in order to inform the driver about the last frame of the stream. Some +drivers may require it to properly finish processing the stream./entry This breaks backward compatibility, as applications written before this change won't set this flag. I am preparing a new patch which will use VIDIOC_ENCODER_CMD with command V4L2_ENC_CMD_STOP and a new flag V4L2_ENC_CMD_STOP_AFTER_NEXT_FRAME, according to suggestions by Hans Verkuil. Discussion is at thread started from the parent email (subject [PATCH 0/2] s5p-mfc: added encoder support for end of stream handling). + /row /tbody /tgroup /table diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml index 9caa49a..ad49f7d 100644 --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml @@ -76,6 +76,8 @@ supports capturing from specific video inputs and you want to specify a video input, then structfieldflags/structfield should be set to constantV4L2_BUF_FLAG_INPUT/constant and the field structfieldinput/structfield must be initialized to the desired input. +Some drivers expects applications set constantV4L2_BUF_FLAG_EOS/constant +flag on the last buffer of the stream. The structfieldreserved/structfield field must be set to 0. When using the link linkend=planar-apismulti-planar API/link, the structfieldm.planes/structfield field must contain a userspace pointer diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 370d111..e44a7cd 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -676,6 +676,7 @@ struct v4l2_buffer { /* Cache handling flags */ #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x1000 +#define V4L2_BUF_FLAG_EOS 0x2000 /* The last buffer in the stream */ /* *O V E R L A Y P R E V I E W Regards Andrzej -- 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: [RFCv1 PATCH 18/32] v4l2-ioctl.c: finalize table conversion.
Em 18-06-2012 08:49, Hans Verkuil escreveu: On Mon June 18 2012 12:50:35 Mauro Carvalho Chehab wrote: Em 18-06-2012 06:46, Laurent Pinchart escreveu: Hi Hans, Thanks for the patch. On Sunday 10 June 2012 12:25:40 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-ioctl.c | 35 +-- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 0de31c4..6c91674 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -870,6 +870,11 @@ static void v4l_print_newline(const void *arg) pr_cont(\n); } +static void v4l_print_default(const void *arg) +{ + pr_cont(non-standard ioctl\n); I'd say driver-specific ioctl instead. non-standard may sound like an error to users. This message is useless as-is, as it provides no glue about what ioctl was called. You should either remove it or print the ioctl number, in hexa. That ioctl number is already printed in front of this message. Hmm... should we print it when the ioctl is known? IMHO, the behavior should be to either print the ioctl name or its number, as those debug messages are generally big. So, better to not pollute it with duplicated information. Regards, Hans -- 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: [RFCv1 PATCH 29/32] v4l2-dev.c: also add debug support for the fops.
Hi Hans, On Monday 18 June 2012 13:40:24 Hans Verkuil wrote: On Mon June 18 2012 12:01:47 Laurent Pinchart wrote: On Sunday 10 June 2012 12:25:51 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-dev.c | 41 ++--- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c index 5c0bb18..54f387d 100644 --- a/drivers/media/video/v4l2-dev.c +++ b/drivers/media/video/v4l2-dev.c @@ -305,6 +305,9 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf, ret = vdev-fops-read(filp, buf, sz, off); if (test_bit(V4L2_FL_LOCK_ALL_FOPS, vdev-flags)) mutex_unlock(vdev-lock); + if (vdev-debug) As vdev-debug is a bitmask, shouldn't we add an fops debug bit ? I actually want to move away from the bitmask idea. I've never really liked it here. Would using dev_dbg with dynamic printk instead of creating our own logging system be an option ? + pr_info(%s: read: %zd (%d)\n, + video_device_node_name(vdev), sz, ret); Shouldn't we use KERN_DEBUG instead of KERN_INFO ? BTW, what about replacing the pr_* calls with dev_* calls ? KERN_DEBUG vs KERN_INFO is actually a good question. My reasoning is that you explicitly enable logging, and so you really want to see it in the log, so we use KERN_INFO. With KERN_DEBUG you might have the situation where the debug level of the logging is disabled, so the messages are ignored. However, if people disagree with this, then I'm happy to move it back to KERN_DEBUG. On embedded systems KERN_INFO will be printed to the serial console. Interleaving kernel messages with application output during capture result in a mess. If someone enables debugging I expect him/her to know enough to get the kernel log debug messages. With regards to dev_ vs pr_: I'd have to test this to see what dev_ prints as prefix. -- 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: [RFCv1 PATCH 25/32] create_bufs: handle count == 0.
Hi Hans, On Monday 18 June 2012 13:43:22 Hans Verkuil wrote: On Mon June 18 2012 12:11:27 Laurent Pinchart wrote: On Sunday 10 June 2012 12:25:47 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- Documentation/DocBook/media/v4l/vidioc-create-bufs.xml |8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml index 765549f..afdba4d 100644 --- a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml +++ b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml @@ -97,7 +97,13 @@ information./para row entry__u32/entry entrystructfieldcount/structfield/entry - entryThe number of buffers requested or granted./entry + entryThe number of buffers requested or granted. If count == 0, then + constantVIDIOC_CREATE_BUFS/constant will set structfieldindex/structfield + to the starting buffer index, I find starting buffer index a bit unclear in this context, as we don't create any buffer. How about: ... will set index to the current number of created buffers, Sounds good to me for now (but it won't be true anymore when we'll support deleting buffers individually). Better alternatives welcome :-) -- 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: [RFCv1 PATCH 29/32] v4l2-dev.c: also add debug support for the fops.
On Mon June 18 2012 14:19:51 Laurent Pinchart wrote: Hi Hans, On Monday 18 June 2012 13:40:24 Hans Verkuil wrote: On Mon June 18 2012 12:01:47 Laurent Pinchart wrote: On Sunday 10 June 2012 12:25:51 Hans Verkuil wrote: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com --- drivers/media/video/v4l2-dev.c | 41 ++--- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c index 5c0bb18..54f387d 100644 --- a/drivers/media/video/v4l2-dev.c +++ b/drivers/media/video/v4l2-dev.c @@ -305,6 +305,9 @@ static ssize_t v4l2_read(struct file *filp, char __user *buf, ret = vdev-fops-read(filp, buf, sz, off); if (test_bit(V4L2_FL_LOCK_ALL_FOPS, vdev-flags)) mutex_unlock(vdev-lock); + if (vdev-debug) As vdev-debug is a bitmask, shouldn't we add an fops debug bit ? I actually want to move away from the bitmask idea. I've never really liked it here. Would using dev_dbg with dynamic printk instead of creating our own logging system be an option ? I don't think so. There are lots of printk message you'd all have to enable. You just want to have a standard method of debugging which ioctls are called that anyone can use and that's consistent for all v4l drivers. This does just that. + pr_info(%s: read: %zd (%d)\n, + video_device_node_name(vdev), sz, ret); Shouldn't we use KERN_DEBUG instead of KERN_INFO ? BTW, what about replacing the pr_* calls with dev_* calls ? KERN_DEBUG vs KERN_INFO is actually a good question. My reasoning is that you explicitly enable logging, and so you really want to see it in the log, so we use KERN_INFO. With KERN_DEBUG you might have the situation where the debug level of the logging is disabled, so the messages are ignored. However, if people disagree with this, then I'm happy to move it back to KERN_DEBUG. On embedded systems KERN_INFO will be printed to the serial console. Interleaving kernel messages with application output during capture result in a mess. If someone enables debugging I expect him/her to know enough to get the kernel log debug messages. OK, I'll change this back to KERN_DEBUG. Good argument. Regards, Hans -- 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/1] [media] s5p-fimc: Add missing static storage class
Em 11-06-2012 01:27, Sachin Kamat escreveu: On 28/05/2012, Sylwester Nawrocki snj...@gmail.com wrote: On 05/26/2012 05:11 PM, Sachin Kamat wrote: Fixes the following sparse warnings: Hi Sachin. Thanks, in case somebody else applies this patch before I do: Acked-by: Sylwester Nawrocki s.nawro...@samsung.com Thanks Sylwester. I would just change the summary line to: s5p-fimc: Add missing static storage class specifiers when applying this patch. Applied with the above change. Ok. -- Regards, Sylwester -- 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] [media] cx231xx: restore tuner settings on first open
On Mon, 2012-06-18 at 09:29 +0200, Hans Verkuil wrote: On Mon June 18 2012 06:49:58 David Dillow wrote: What does the V4L2 API spec say about tuning frequency being persistent when there are no users of a video capture device? Is MythTV wrong to have that assumption, or is cx231xx wrong to not restore the frequency when a user first opens the device? Tuner standards and frequencies must be persistent. So cx231xx is wrong. Actually, all V4L2 settings must in general be persistent (there are some per-filehandle settings when dealing with low-level subdev setups or mem2mem devices). Is there a document somewhere I can reference; I need to go through the cx231xx driver and make sure it is doing the right things and it would be handy to have a checklist. Either way, I think MythTV should keep the device open until it is done with it, as that would avoid added latency from putting the tuner to sleep and waking it back up. But, I think we should address the issue in the driver if it is not living up to the guarantees of the API. From what I can tell it is a bug in the tda tuner (not restoring the frequency) and cx231xx (not setting the initial standard and possibly frequency). Ok, I'll break this up and have a go at a proper fix. Thanks for the pointers on the persistence of parameters. Dave -- 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] [media] cx231xx: restore tuner settings on first open
On Mon, Jun 18, 2012 at 10:15 AM, David Dillow d...@thedillows.org wrote: Tuner standards and frequencies must be persistent. So cx231xx is wrong. Actually, all V4L2 settings must in general be persistent (there are some per-filehandle settings when dealing with low-level subdev setups or mem2mem devices). Is there a document somewhere I can reference; I need to go through the cx231xx driver and make sure it is doing the right things and it would be handy to have a checklist. This isn't just the cx231xx driver. Almost all USB devices that have tuners which support power management have this problem (em28xx/au0828/cx231xx with Xceive or tda18271 tuners). Really the tuner_core should be resending the set_params call to the tuner after waking it up (it shouldn't be the tuner's responsibility to remember its settings). The other thing to watch out for is that the API is pretty brittle in terms of cases where initializing the tuner takes a long time. In the case of the Xceive tuners it can take upwards of ten seconds on devices with slow i2c busses (au0828 in particular due to a hardware bug in clock stretching), and the 18271 initialization has a calibration loop which can take 5-10 seconds. Hence you would have to be very careful in terms of figuring out where to put the call to reset the tuner, since you don't want every call to v4l2-ctl taking ten seconds. And of course you don't want to leave the tuner running if it's no longer in use (since it will run down the battery on laptops), although figuring out when that is can be nearly impossible if the sequence of events is a series of calls to the v4l2-ctl command line tool followed by reading from the device node. I'm certainly in favor of this long-standing problem being fixed, but it will take considerable investigation to figure out the right place to make the change. 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
[PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns
Rearrange the CFA interpolation coefficients table based on the Bayer pattern. Modifying the table during streaming isn't supported anymore, but didn't make sense in the first place anyway. Support for non-Bayer CFA patterns is dropped as they were not correctly supported, and have never been tested. Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com --- drivers/media/video/omap3isp/isppreview.c | 118 1 files changed, 67 insertions(+), 51 deletions(-) Jean-Philippe, Could you please test this patch on your hardware ? diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c index 8a4935e..bfa3107 100644 --- a/drivers/media/video/omap3isp/isppreview.c +++ b/drivers/media/video/omap3isp/isppreview.c @@ -309,36 +309,6 @@ preview_config_dcor(struct isp_prev_device *prev, const void *prev_dcor) } /* - * preview_config_cfa - Configures the CFA Interpolation parameters. - * @prev_cfa: Structure containing the CFA interpolation table, CFA format - *in the image, vertical and horizontal gradient threshold. - */ -static void -preview_config_cfa(struct isp_prev_device *prev, const void *prev_cfa) -{ - struct isp_device *isp = to_isp_device(prev); - const struct omap3isp_prev_cfa *cfa = prev_cfa; - unsigned int i; - - isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR, - ISPPRV_PCR_CFAFMT_MASK, - cfa-format ISPPRV_PCR_CFAFMT_SHIFT); - - isp_reg_writel(isp, - (cfa-gradthrs_vert ISPPRV_CFA_GRADTH_VER_SHIFT) | - (cfa-gradthrs_horz ISPPRV_CFA_GRADTH_HOR_SHIFT), - OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA); - - isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR, - OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR); - - for (i = 0; i OMAP3ISP_PREV_CFA_TBL_SIZE; i++) { - isp_reg_writel(isp, cfa-table[i], - OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA); - } -} - -/* * preview_config_gammacorrn - Configures the Gamma Correction table values * @gtable: Structure containing the table for red, blue, green gamma table. */ @@ -813,7 +783,7 @@ static const struct preview_update update_attrs[] = { FIELD_SIZEOF(struct prev_params, hmed), offsetof(struct omap3isp_prev_update_config, hmed), }, /* OMAP3ISP_PREV_CFA */ { - preview_config_cfa, + NULL, NULL, offsetof(struct prev_params, cfa), FIELD_SIZEOF(struct prev_params, cfa), @@ -1043,42 +1013,88 @@ preview_config_ycpos(struct isp_prev_device *prev, static void preview_config_averager(struct isp_prev_device *prev, u8 average) { struct isp_device *isp = to_isp_device(prev); - struct prev_params *params; - int reg = 0; - params = (prev-params.active OMAP3ISP_PREV_CFA) - ? prev-params.params[0] : prev-params.params[1]; - - if (params-cfa.format == OMAP3ISP_CFAFMT_BAYER) - reg = ISPPRV_AVE_EVENDIST_2 ISPPRV_AVE_EVENDIST_SHIFT | - ISPPRV_AVE_ODDDIST_2 ISPPRV_AVE_ODDDIST_SHIFT | - average; - else if (params-cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON) - reg = ISPPRV_AVE_EVENDIST_3 ISPPRV_AVE_EVENDIST_SHIFT | - ISPPRV_AVE_ODDDIST_3 ISPPRV_AVE_ODDDIST_SHIFT | - average; - isp_reg_writel(isp, reg, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE); + isp_reg_writel(isp, ISPPRV_AVE_EVENDIST_2 ISPPRV_AVE_EVENDIST_SHIFT | + ISPPRV_AVE_ODDDIST_2 ISPPRV_AVE_ODDDIST_SHIFT | + average, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE); } + +#define OMAP3ISP_PREV_CFA_BLK_SIZE (OMAP3ISP_PREV_CFA_TBL_SIZE / 4) + /* * preview_config_input_format - Configure the input format * @prev: The preview engine * @format: Format on the preview engine sink pad * - * Enable CFA interpolation for Bayer formats and disable it for greyscale - * formats. + * Enable and configure CFA interpolation for Bayer formats and disable it for + * greyscale formats. + * + * The CFA table is organised in four blocks, one per Bayer component. The + * hardware expects blocks to follow the Bayer order of the input data, while + * the driver stores the table in GRBG order in memory. The blocks need to be + * reordered to support non-GRBG Bayer patterns. */ static void preview_config_input_format(struct isp_prev_device *prev, const struct v4l2_mbus_framefmt *format) { + static const unsigned int cfa_coef_order[4][4] = { + { 0, 1, 2, 3 }, /* GRBG */ + { 1, 0, 3, 2 }, /* RGGB */ + { 2, 3, 0, 1 }, /* BGGR */ + { 3, 2, 1, 0 }, /* GBRG */ + }; struct isp_device *isp = to_isp_device(prev); + struct prev_params *params;
Re: [RFC] [media] cx231xx: restore tuner settings on first open
On Mon June 18 2012 16:15:40 David Dillow wrote: On Mon, 2012-06-18 at 09:29 +0200, Hans Verkuil wrote: On Mon June 18 2012 06:49:58 David Dillow wrote: What does the V4L2 API spec say about tuning frequency being persistent when there are no users of a video capture device? Is MythTV wrong to have that assumption, or is cx231xx wrong to not restore the frequency when a user first opens the device? Tuner standards and frequencies must be persistent. So cx231xx is wrong. Actually, all V4L2 settings must in general be persistent (there are some per-filehandle settings when dealing with low-level subdev setups or mem2mem devices). Is there a document somewhere I can reference; I need to go through the cx231xx driver and make sure it is doing the right things and it would be handy to have a checklist. By far the easiest method is to run v4l2-compliance from the v4l-utils repository on the driver. It's not 100%, but it is achieving pretty good coverage. It's purpose is to verify all the fields are properly filled in, all the latest frameworks are used, and everything is according to spec. Make sure you compile the v4l-utils repository yourself to be sure you use the latest version of this utility. In principle the V4L2 spec should contain all that information, but it is sometimes buries in the text and there are some things that are only available in the collective memory of the developers :-) Either way, I think MythTV should keep the device open until it is done with it, as that would avoid added latency from putting the tuner to sleep and waking it back up. But, I think we should address the issue in the driver if it is not living up to the guarantees of the API. From what I can tell it is a bug in the tda tuner (not restoring the frequency) and cx231xx (not setting the initial standard and possibly frequency). Ok, I'll break this up and have a go at a proper fix. Thanks for the pointers on the persistence of parameters. As Devin mentioned, putting the fix in tuner-core is a better approach since that fixes it for all such tuners. And a quite separate problem is when to put a tuner to sleep. That's a very dark corner in V4L2. For that someone would have to write an RFC, outlining the various options and starting a proper discussion on how to solve this. Regards, Hans Dave -- 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 -- 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] [media] cx231xx: restore tuner settings on first open
On Mon, 2012-06-18 at 10:23 -0400, Devin Heitmueller wrote: This isn't just the cx231xx driver. Almost all USB devices that have tuners which support power management have this problem (em28xx/au0828/cx231xx with Xceive or tda18271 tuners). Really the tuner_core should be resending the set_params call to the tuner after waking it up (it shouldn't be the tuner's responsibility to remember its settings). The other thing to watch out for is that the API is pretty brittle in terms of cases where initializing the tuner takes a long time. In the case of the Xceive tuners it can take upwards of ten seconds on devices with slow i2c busses (au0828 in particular due to a hardware bug in clock stretching), and the 18271 initialization has a calibration loop which can take 5-10 seconds. Hence you would have to be very careful in terms of figuring out where to put the call to reset the tuner, since you don't want every call to v4l2-ctl taking ten seconds. And of course you don't want to leave the tuner running if it's no longer in use (since it will run down the battery on laptops), although figuring out when that is can be nearly impossible if the sequence of events is a series of calls to the v4l2-ctl command line tool followed by reading from the device node. I'm certainly in favor of this long-standing problem being fixed, but it will take considerable investigation to figure out the right place to make the change. Hmm, it sounds like perhaps changing the standby call in the tuner core to asynchronously power down the tuner may be the way to go -- ie, when we tell it to standby, it will do a schedule_work for some 10 seconds later to really pull it down. If we get a resume call prior to then, we'll just cancel the work, otherwise we wait for the work to finish and then issue the resume. Does that sound reasonable? -- 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] [media] cx231xx: restore tuner settings on first open
On Mon, Jun 18, 2012 at 10:32 AM, David Dillow d...@thedillows.org wrote: Hmm, it sounds like perhaps changing the standby call in the tuner core to asynchronously power down the tuner may be the way to go -- ie, when we tell it to standby, it will do a schedule_work for some 10 seconds later to really pull it down. If we get a resume call prior to then, we'll just cancel the work, otherwise we wait for the work to finish and then issue the resume. Does that sound reasonable? At face value it sounds reasonable, except the approach breaks down as soon as you have hybrid tuners which support both analog and digital. Because the digital side of the tuner isn't tied into tuner-core, you'll break in the following situation: Start using analog Stop using analog [schedule_work() call] Start using digital Timer pops and powers down the tuner even though it's in use for ATSC or ClearQAM Again, I'm not proposing a solution, but just poking a fatal hole in your proposal (believe me, I had considered the same approach when first looking at the problem). 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: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date:Mon Jun 18 19:00:12 CEST 2012 git hash:5472d3f17845c4398c6a510b46855820920c2181 gcc version: i686-linux-gcc (GCC) 4.7.1 host hardware:x86_64 host os: 3.4.07-marune linux-git-arm-eabi-davinci: WARNINGS linux-git-arm-eabi-exynos: WARNINGS linux-git-arm-eabi-omap: WARNINGS linux-git-i686: WARNINGS linux-git-m32r: WARNINGS linux-git-mips: WARNINGS linux-git-powerpc64: WARNINGS linux-git-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-x86_64: WARNINGS linux-2.6.35.3-x86_64: WARNINGS linux-2.6.36-x86_64: WARNINGS linux-2.6.37-x86_64: WARNINGS linux-2.6.38.2-x86_64: WARNINGS linux-2.6.39.1-x86_64: ERRORS linux-3.0-x86_64: ERRORS linux-3.1-x86_64: ERRORS linux-3.2.1-x86_64: ERRORS linux-3.3-x86_64: ERRORS linux-3.4-x86_64: 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-i686: WARNINGS linux-2.6.35.3-i686: WARNINGS linux-2.6.36-i686: WARNINGS linux-2.6.37-i686: WARNINGS linux-2.6.38.2-i686: WARNINGS linux-2.6.39.1-i686: ERRORS linux-3.0-i686: ERRORS linux-3.1-i686: ERRORS linux-3.2.1-i686: ERRORS linux-3.3-i686: ERRORS linux-3.4-i686: WARNINGS apps: WARNINGS spec-git: WARNINGS sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Monday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Monday.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
[PATCH 1 of 3] media: gpio-ir-recv: fix missing udev by-path entry
Add missing information so that udev can create an entry for gpio-ir-recv under /dev/input/by-path/ . Cc: Mauro Carvalho Chehab mche...@infradead.org Cc: Ravi Kumar V kumar...@codeaurora.org Cc: linux-media@vger.kernel.org Signed-off-by: Benoît Thébaudeau benoit.thebaud...@advansee.com --- .../drivers/media/rc/gpio-ir-recv.c|6 ++ 1 file changed, 6 insertions(+) diff --git linux-next-HEAD-6c86b58.orig/drivers/media/rc/gpio-ir-recv.c linux-next-HEAD-6c86b58/drivers/media/rc/gpio-ir-recv.c index 0d87545..b41e13c 100644 --- linux-next-HEAD-6c86b58.orig/drivers/media/rc/gpio-ir-recv.c +++ linux-next-HEAD-6c86b58/drivers/media/rc/gpio-ir-recv.c @@ -82,10 +82,16 @@ static int __devinit gpio_ir_recv_probe(struct platform_device *pdev) goto err_allocate_device; } + rcdev-priv = gpio_dev; rcdev-driver_type = RC_DRIVER_IR_RAW; rcdev-allowed_protos = RC_TYPE_ALL; rcdev-input_name = GPIO_IR_DEVICE_NAME; + rcdev-input_phys = GPIO_IR_DEVICE_NAME /input0; rcdev-input_id.bustype = BUS_HOST; + rcdev-input_id.vendor = 0x0001; + rcdev-input_id.product = 0x0001; + rcdev-input_id.version = 0x0100; + rcdev-dev.parent = pdev-dev; rcdev-driver_name = GPIO_IR_DRIVER_NAME; rcdev-map_name = RC_MAP_EMPTY; -- 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 0 of 3] media: gpio-ir-recv: several improvements
Hi all, This is a set of 3 small improvements for gpio-ir-recv. Regards, Benoît Thébaudeau -- 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 of 3] media: gpio-ir-recv: add map name
Make it possible for gpio-ir-recv users to choose a map name. Cc: Mauro Carvalho Chehab mche...@infradead.org Cc: Ravi Kumar V kumar...@codeaurora.org Cc: linux-media@vger.kernel.org Signed-off-by: Benoît Thébaudeau benoit.thebaud...@advansee.com --- .../drivers/media/rc/gpio-ir-recv.c|2 +- .../include/media/gpio-ir-recv.h |1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git linux-next-HEAD-6c86b58.orig/drivers/media/rc/gpio-ir-recv.c linux-next-HEAD-6c86b58/drivers/media/rc/gpio-ir-recv.c index b41e13c..15e346e 100644 --- linux-next-HEAD-6c86b58.orig/drivers/media/rc/gpio-ir-recv.c +++ linux-next-HEAD-6c86b58/drivers/media/rc/gpio-ir-recv.c @@ -93,7 +93,7 @@ static int __devinit gpio_ir_recv_probe(struct platform_device *pdev) rcdev-input_id.version = 0x0100; rcdev-dev.parent = pdev-dev; rcdev-driver_name = GPIO_IR_DRIVER_NAME; - rcdev-map_name = RC_MAP_EMPTY; + rcdev-map_name = pdata-map_name ?: RC_MAP_EMPTY; gpio_dev-rcdev = rcdev; gpio_dev-gpio_nr = pdata-gpio_nr; diff --git linux-next-HEAD-6c86b58.orig/include/media/gpio-ir-recv.h linux-next-HEAD-6c86b58/include/media/gpio-ir-recv.h index 67797bf..91546f3 100644 --- linux-next-HEAD-6c86b58.orig/include/media/gpio-ir-recv.h +++ linux-next-HEAD-6c86b58/include/media/gpio-ir-recv.h @@ -16,6 +16,7 @@ struct gpio_ir_recv_platform_data { int gpio_nr; bool active_low; + const char *map_name; }; #endif /* __GPIO_IR_RECV_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 3 of 3] media: gpio-ir-recv: switch to module_platform_driver
Cc: Mauro Carvalho Chehab mche...@infradead.org Cc: Ravi Kumar V kumar...@codeaurora.org Cc: linux-media@vger.kernel.org Signed-off-by: Benoît Thébaudeau benoit.thebaud...@advansee.com --- .../drivers/media/rc/gpio-ir-recv.c| 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git linux-next-HEAD-6c86b58.orig/drivers/media/rc/gpio-ir-recv.c linux-next-HEAD-6c86b58/drivers/media/rc/gpio-ir-recv.c index 15e346e..59fe60c 100644 --- linux-next-HEAD-6c86b58.orig/drivers/media/rc/gpio-ir-recv.c +++ linux-next-HEAD-6c86b58/drivers/media/rc/gpio-ir-recv.c @@ -194,18 +194,7 @@ static struct platform_driver gpio_ir_recv_driver = { #endif }, }; - -static int __init gpio_ir_recv_init(void) -{ - return platform_driver_register(gpio_ir_recv_driver); -} -module_init(gpio_ir_recv_init); - -static void __exit gpio_ir_recv_exit(void) -{ - platform_driver_unregister(gpio_ir_recv_driver); -} -module_exit(gpio_ir_recv_exit); +module_platform_driver(gpio_ir_recv_driver); MODULE_DESCRIPTION(GPIO IR Receiver driver); MODULE_LICENSE(GPL v2); -- 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 0/12] struct i2c_algo_bit_data cleanup on several drivers
Hi Mauro, This patchset cleans the i2c part of some drivers. This issue was recently reported by Dan Carpenter [1], and revealed wrong (and harmless) usage of struct i2c_algo_bit. Also, I properly assigned bus-i2c_rc (return code variable) and replaced struct memcpy with struct assignment. The latter is, in my opinion, a much safer way for struct filling and I'm not aware of any drawbacks. The patches are based on today's linux-next; I hope this is okey. As I don't own any of these devices, I can't test the changes beyond compilation. Ezequiel Garcia (12): cx25821: Replace struct memcpy with struct assignment cx25821: Remove useless struct i2c_algo_bit_data usage cx25821: Use i2c_rc properly to store i2c register status cx231xx: Replace struct memcpy with struct assignment cx231xx: Remove useless struct i2c_algo_bit_data usage cx231xx: Use i2c_rc properly to store i2c register status cx23885: Replace struct memcpy with struct assignment cx23885: Remove useless struct i2c_algo_bit_data cx23885: Use i2c_rc properly to store i2c register status saa7164: Replace struct memcpy with struct assignment saa7164: Remove useless struct i2c_algo_bit_data saa7164: Use i2c_rc properly to store i2c register status drivers/media/video/cx231xx/cx231xx-i2c.c | 10 +++--- drivers/media/video/cx231xx/cx231xx.h |2 -- drivers/media/video/cx23885/cx23885-i2c.c | 12 +++- drivers/media/video/cx23885/cx23885.h |2 -- drivers/media/video/cx25821/cx25821-i2c.c | 12 +++- drivers/media/video/cx25821/cx25821.h |2 -- drivers/media/video/saa7164/saa7164-i2c.c | 13 +++-- drivers/media/video/saa7164/saa7164.h |2 -- 8 files changed, 12 insertions(+), 43 deletions(-) Thanks, Ezequiel. [1] http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/49553 -- 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 01/12] saa7164: Use i2c_rc properly to store i2c register status
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com --- drivers/media/video/saa7164/saa7164-i2c.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/saa7164/saa7164-i2c.c b/drivers/media/video/saa7164/saa7164-i2c.c index 26148f7..536f7dc 100644 --- a/drivers/media/video/saa7164/saa7164-i2c.c +++ b/drivers/media/video/saa7164/saa7164-i2c.c @@ -123,7 +123,7 @@ int saa7164_i2c_register(struct saa7164_i2c *bus) bus-i2c_algo.data = bus; bus-i2c_adap.algo_data = bus; i2c_set_adapdata(bus-i2c_adap, bus); - i2c_add_adapter(bus-i2c_adap); + bus-i2c_rc = i2c_add_adapter(bus-i2c_adap); bus-i2c_client.adapter = bus-i2c_adap; -- 1.7.4.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 02/12] saa7164: Remove useless struct i2c_algo_bit_data
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com --- drivers/media/video/saa7164/saa7164-i2c.c |4 drivers/media/video/saa7164/saa7164.h |1 - 2 files changed, 0 insertions(+), 5 deletions(-) diff --git a/drivers/media/video/saa7164/saa7164-i2c.c b/drivers/media/video/saa7164/saa7164-i2c.c index 536f7dc..8df15ca 100644 --- a/drivers/media/video/saa7164/saa7164-i2c.c +++ b/drivers/media/video/saa7164/saa7164-i2c.c @@ -109,9 +109,6 @@ int saa7164_i2c_register(struct saa7164_i2c *bus) memcpy(bus-i2c_adap, saa7164_i2c_adap_template, sizeof(bus-i2c_adap)); - memcpy(bus-i2c_algo, saa7164_i2c_algo_template, - sizeof(bus-i2c_algo)); - memcpy(bus-i2c_client, saa7164_i2c_client_template, sizeof(bus-i2c_client)); @@ -120,7 +117,6 @@ int saa7164_i2c_register(struct saa7164_i2c *bus) strlcpy(bus-i2c_adap.name, bus-dev-name, sizeof(bus-i2c_adap.name)); - bus-i2c_algo.data = bus; bus-i2c_adap.algo_data = bus; i2c_set_adapdata(bus-i2c_adap, bus); bus-i2c_rc = i2c_add_adapter(bus-i2c_adap); diff --git a/drivers/media/video/saa7164/saa7164.h b/drivers/media/video/saa7164/saa7164.h index 8d120e3..fc1f854 100644 --- a/drivers/media/video/saa7164/saa7164.h +++ b/drivers/media/video/saa7164/saa7164.h @@ -251,7 +251,6 @@ struct saa7164_i2c { /* I2C I/O */ struct i2c_adapter i2c_adap; - struct i2c_algo_bit_datai2c_algo; struct i2c_client i2c_client; u32 i2c_rc; }; -- 1.7.4.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 09/12] cx231xx: Replace struct memcpy with struct assignment
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com --- drivers/media/video/cx231xx/cx231xx-i2c.c |6 ++ 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/media/video/cx231xx/cx231xx-i2c.c b/drivers/media/video/cx231xx/cx231xx-i2c.c index f5f4844..2b2e91e 100644 --- a/drivers/media/video/cx231xx/cx231xx-i2c.c +++ b/drivers/media/video/cx231xx/cx231xx-i2c.c @@ -499,10 +499,8 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus) BUG_ON(!dev-cx231xx_send_usb_command); - memcpy(bus-i2c_adap, cx231xx_adap_template, sizeof(bus-i2c_adap)); - memcpy(bus-i2c_client, cx231xx_client_template, - sizeof(bus-i2c_client)); - + bus-i2c_adap = cx231xx_adap_template; + bus-i2c_client = cx231xx_client_template; bus-i2c_adap.dev.parent = dev-udev-dev; strlcpy(bus-i2c_adap.name, bus-dev-name, sizeof(bus-i2c_adap.name)); -- 1.7.4.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 08/12] cx231xx: Remove useless struct i2c_algo_bit_data usage
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com --- drivers/media/video/cx231xx/cx231xx-i2c.c |2 -- drivers/media/video/cx231xx/cx231xx.h |2 -- 2 files changed, 0 insertions(+), 4 deletions(-) diff --git a/drivers/media/video/cx231xx/cx231xx-i2c.c b/drivers/media/video/cx231xx/cx231xx-i2c.c index 8064119..f5f4844 100644 --- a/drivers/media/video/cx231xx/cx231xx-i2c.c +++ b/drivers/media/video/cx231xx/cx231xx-i2c.c @@ -500,7 +500,6 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus) BUG_ON(!dev-cx231xx_send_usb_command); memcpy(bus-i2c_adap, cx231xx_adap_template, sizeof(bus-i2c_adap)); - memcpy(bus-i2c_algo, cx231xx_algo, sizeof(bus-i2c_algo)); memcpy(bus-i2c_client, cx231xx_client_template, sizeof(bus-i2c_client)); @@ -508,7 +507,6 @@ int cx231xx_i2c_register(struct cx231xx_i2c *bus) strlcpy(bus-i2c_adap.name, bus-dev-name, sizeof(bus-i2c_adap.name)); - bus-i2c_algo.data = bus; bus-i2c_adap.algo_data = bus; i2c_set_adapdata(bus-i2c_adap, dev-v4l2_dev); bus-i2c_rc = i2c_add_adapter(bus-i2c_adap); diff --git a/drivers/media/video/cx231xx/cx231xx.h b/drivers/media/video/cx231xx/cx231xx.h index e174475..a89d020 100644 --- a/drivers/media/video/cx231xx/cx231xx.h +++ b/drivers/media/video/cx231xx/cx231xx.h @@ -26,7 +26,6 @@ #include linux/types.h #include linux/ioctl.h #include linux/i2c.h -#include linux/i2c-algo-bit.h #include linux/workqueue.h #include linux/mutex.h @@ -481,7 +480,6 @@ struct cx231xx_i2c { /* i2c i/o */ struct i2c_adapter i2c_adap; - struct i2c_algo_bit_data i2c_algo; struct i2c_client i2c_client; u32 i2c_rc; -- 1.7.4.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 10/12] cx25821: Use i2c_rc properly to store i2c register status
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com --- drivers/media/video/cx25821/cx25821-i2c.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/cx25821/cx25821-i2c.c b/drivers/media/video/cx25821/cx25821-i2c.c index 6311180..398e0e6 100644 --- a/drivers/media/video/cx25821/cx25821-i2c.c +++ b/drivers/media/video/cx25821/cx25821-i2c.c @@ -319,7 +319,7 @@ int cx25821_i2c_register(struct cx25821_i2c *bus) bus-i2c_algo.data = bus; bus-i2c_adap.algo_data = bus; i2c_set_adapdata(bus-i2c_adap, dev-v4l2_dev); - i2c_add_adapter(bus-i2c_adap); + bus-i2c_rc = i2c_add_adapter(bus-i2c_adap); bus-i2c_client.adapter = bus-i2c_adap; -- 1.7.4.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 05/12] cx23885: Remove useless struct i2c_algo_bit_data
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com --- drivers/media/video/cx23885/cx23885-i2c.c |3 --- drivers/media/video/cx23885/cx23885.h |2 -- drivers/media/video/saa7164/saa7164.h |1 - 3 files changed, 0 insertions(+), 6 deletions(-) diff --git a/drivers/media/video/cx23885/cx23885-i2c.c b/drivers/media/video/cx23885/cx23885-i2c.c index 7a93fc6..1404050 100644 --- a/drivers/media/video/cx23885/cx23885-i2c.c +++ b/drivers/media/video/cx23885/cx23885-i2c.c @@ -318,8 +318,6 @@ int cx23885_i2c_register(struct cx23885_i2c *bus) memcpy(bus-i2c_adap, cx23885_i2c_adap_template, sizeof(bus-i2c_adap)); - memcpy(bus-i2c_algo, cx23885_i2c_algo_template, - sizeof(bus-i2c_algo)); memcpy(bus-i2c_client, cx23885_i2c_client_template, sizeof(bus-i2c_client)); @@ -328,7 +326,6 @@ int cx23885_i2c_register(struct cx23885_i2c *bus) strlcpy(bus-i2c_adap.name, bus-dev-name, sizeof(bus-i2c_adap.name)); - bus-i2c_algo.data = bus; bus-i2c_adap.algo_data = bus; i2c_set_adapdata(bus-i2c_adap, dev-v4l2_dev); bus-i2c_rc = i2c_add_adapter(bus-i2c_adap); diff --git a/drivers/media/video/cx23885/cx23885.h b/drivers/media/video/cx23885/cx23885.h index d884784..3cf397f 100644 --- a/drivers/media/video/cx23885/cx23885.h +++ b/drivers/media/video/cx23885/cx23885.h @@ -21,7 +21,6 @@ #include linux/pci.h #include linux/i2c.h -#include linux/i2c-algo-bit.h #include linux/kdev_t.h #include linux/slab.h @@ -246,7 +245,6 @@ struct cx23885_i2c { /* i2c i/o */ struct i2c_adapter i2c_adap; - struct i2c_algo_bit_data i2c_algo; struct i2c_client i2c_client; u32i2c_rc; diff --git a/drivers/media/video/saa7164/saa7164.h b/drivers/media/video/saa7164/saa7164.h index fc1f854..35219b9 100644 --- a/drivers/media/video/saa7164/saa7164.h +++ b/drivers/media/video/saa7164/saa7164.h @@ -46,7 +46,6 @@ #include linux/pci.h #include linux/i2c.h -#include linux/i2c-algo-bit.h #include linux/kdev_t.h #include linux/mutex.h #include linux/crc32.h -- 1.7.4.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 12/12] cx25821: Replace struct memcpy with struct assignment
Signed-off-by: Ezequiel Garcia elezegar...@gmail.com --- drivers/media/video/cx25821/cx25821-i2c.c |7 ++- 1 files changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/media/video/cx25821/cx25821-i2c.c b/drivers/media/video/cx25821/cx25821-i2c.c index 431fa7f..8a823b8 100644 --- a/drivers/media/video/cx25821/cx25821-i2c.c +++ b/drivers/media/video/cx25821/cx25821-i2c.c @@ -305,11 +305,8 @@ int cx25821_i2c_register(struct cx25821_i2c *bus) dprintk(1, %s(bus = %d)\n, __func__, bus-nr); - memcpy(bus-i2c_adap, cx25821_i2c_adap_template, - sizeof(bus-i2c_adap)); - memcpy(bus-i2c_client, cx25821_i2c_client_template, - sizeof(bus-i2c_client)); - + bus-i2c_adap = cx25821_i2c_adap_template; + bus-i2c_client = cx25821_i2c_client_template; bus-i2c_adap.dev.parent = dev-pci-dev; strlcpy(bus-i2c_adap.name, bus-dev-name, sizeof(bus-i2c_adap.name)); -- 1.7.4.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] media: add Analog Devices ADV7393 video encoder driver
Add ADV7393 I²C-based video encoder driver. This driver has been tested on custom hardware. It has been tested for composite output. It is derived from the ADV7343 driver. Cc: Mauro Carvalho Chehab mche...@infradead.org Cc: linux-media@vger.kernel.org Signed-off-by: Benoît Thébaudeau benoit.thebaud...@advansee.com --- .../drivers/media/video/Kconfig|9 + .../drivers/media/video/Makefile |1 + .../drivers/media/video/adv7393.c | 487 .../drivers/media/video/adv7393_regs.h | 188 .../include/media/adv7393.h| 28 ++ .../include/media/v4l2-chip-ident.h|3 + 6 files changed, 716 insertions(+) create mode 100644 linux-next-HEAD-6c86b58/drivers/media/video/adv7393.c create mode 100644 linux-next-HEAD-6c86b58/drivers/media/video/adv7393_regs.h create mode 100644 linux-next-HEAD-6c86b58/include/media/adv7393.h diff --git linux-next-HEAD-6c86b58.orig/drivers/media/video/Kconfig linux-next-HEAD-6c86b58/drivers/media/video/Kconfig index 99937c9..d00dee9 100644 --- linux-next-HEAD-6c86b58.orig/drivers/media/video/Kconfig +++ linux-next-HEAD-6c86b58/drivers/media/video/Kconfig @@ -461,6 +461,15 @@ config VIDEO_ADV7343 To compile this driver as a module, choose M here: the module will be called adv7343. +config VIDEO_ADV7393 + tristate ADV7393 video encoder + depends on I2C + help + Support for Analog Devices I2C bus based ADV7393 encoder. + + To compile this driver as a module, choose M here: the + module will be called adv7393. + config VIDEO_AK881X tristate AK8813/AK8814 video encoders depends on I2C diff --git linux-next-HEAD-6c86b58.orig/drivers/media/video/Makefile linux-next-HEAD-6c86b58/drivers/media/video/Makefile index d209de0..b7da9fa 100644 --- linux-next-HEAD-6c86b58.orig/drivers/media/video/Makefile +++ linux-next-HEAD-6c86b58/drivers/media/video/Makefile @@ -45,6 +45,7 @@ obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o obj-$(CONFIG_VIDEO_ADV7183) += adv7183.o obj-$(CONFIG_VIDEO_ADV7343) += adv7343.o +obj-$(CONFIG_VIDEO_ADV7393) += adv7393.o obj-$(CONFIG_VIDEO_VPX3220) += vpx3220.o obj-$(CONFIG_VIDEO_VS6624) += vs6624.o obj-$(CONFIG_VIDEO_BT819) += bt819.o diff --git linux-next-HEAD-6c86b58/drivers/media/video/adv7393.c linux-next-HEAD-6c86b58/drivers/media/video/adv7393.c new file mode 100644 index 000..ca72486 --- /dev/null +++ linux-next-HEAD-6c86b58/drivers/media/video/adv7393.c @@ -0,0 +1,487 @@ +/* + * adv7393 - ADV7393 Video Encoder Driver + * + * The encoder hardware does not support SECAM. + * + * Copyright (C) 2010-2012 ADVANSEE - http://www.advansee.com/ + * Benoît Thébaudeau benoit.thebaud...@advansee.com + * + * Based on ADV7343 driver, + * + * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.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. + * + * This program is distributed .as is. WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include linux/kernel.h +#include linux/init.h +#include linux/ctype.h +#include linux/slab.h +#include linux/i2c.h +#include linux/device.h +#include linux/delay.h +#include linux/module.h +#include linux/videodev2.h +#include linux/uaccess.h + +#include media/adv7393.h +#include media/v4l2-device.h +#include media/v4l2-chip-ident.h +#include media/v4l2-ctrls.h + +#include adv7393_regs.h + +MODULE_DESCRIPTION(ADV7393 video encoder driver); +MODULE_LICENSE(GPL); + +static bool debug; +module_param(debug, bool, 0644); +MODULE_PARM_DESC(debug, Debug level 0-1); + +struct adv7393_state { + struct v4l2_subdev sd; + struct v4l2_ctrl_handler hdl; + u8 reg00; + u8 reg01; + u8 reg02; + u8 reg35; + u8 reg80; + u8 reg82; + u32 output; + v4l2_std_id std; +}; + +static inline struct adv7393_state *to_state(struct v4l2_subdev *sd) +{ + return container_of(sd, struct adv7393_state, sd); +} + +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl) +{ + return container_of(ctrl-handler, struct adv7393_state, hdl)-sd; +} + +static inline int adv7393_write(struct v4l2_subdev *sd, u8 reg, u8 value) +{ + struct i2c_client *client = v4l2_get_subdevdata(sd); + + return i2c_smbus_write_byte_data(client, reg, value); +} + +static const u8 adv7393_init_reg_val[] = { + ADV7393_SOFT_RESET, ADV7393_SOFT_RESET_DEFAULT, + ADV7393_POWER_MODE_REG, ADV7393_POWER_MODE_REG_DEFAULT, + + ADV7393_HD_MODE_REG1, ADV7393_HD_MODE_REG1_DEFAULT, + ADV7393_HD_MODE_REG2,
RE: [PATCH 0/12] struct i2c_algo_bit_data cleanup on several drivers
Thanks Ezequiel and Dan. The changes look ok. I'll have someone check out the changes on the CX devices. Rgds, Palash -Original Message- From: Ezequiel Garcia [mailto:elezegar...@gmail.com] Sent: Monday, June 18, 2012 12:23 PM To: Mauro Carvalho Chehab Cc: linux-media; Dan Carpenter; Palash Bandyopadhyay; st...@kernellabs.com Subject: [PATCH 0/12] struct i2c_algo_bit_data cleanup on several drivers Hi Mauro, This patchset cleans the i2c part of some drivers. This issue was recently reported by Dan Carpenter [1], and revealed wrong (and harmless) usage of struct i2c_algo_bit. Also, I properly assigned bus-i2c_rc (return code variable) and replaced struct memcpy with struct assignment. The latter is, in my opinion, a much safer way for struct filling and I'm not aware of any drawbacks. The patches are based on today's linux-next; I hope this is okey. As I don't own any of these devices, I can't test the changes beyond compilation. Ezequiel Garcia (12): cx25821: Replace struct memcpy with struct assignment cx25821: Remove useless struct i2c_algo_bit_data usage cx25821: Use i2c_rc properly to store i2c register status cx231xx: Replace struct memcpy with struct assignment cx231xx: Remove useless struct i2c_algo_bit_data usage cx231xx: Use i2c_rc properly to store i2c register status cx23885: Replace struct memcpy with struct assignment cx23885: Remove useless struct i2c_algo_bit_data cx23885: Use i2c_rc properly to store i2c register status saa7164: Replace struct memcpy with struct assignment saa7164: Remove useless struct i2c_algo_bit_data saa7164: Use i2c_rc properly to store i2c register status drivers/media/video/cx231xx/cx231xx-i2c.c | 10 +++--- drivers/media/video/cx231xx/cx231xx.h |2 -- drivers/media/video/cx23885/cx23885-i2c.c | 12 +++- drivers/media/video/cx23885/cx23885.h |2 -- drivers/media/video/cx25821/cx25821-i2c.c | 12 +++- drivers/media/video/cx25821/cx25821.h |2 -- drivers/media/video/saa7164/saa7164-i2c.c | 13 +++-- drivers/media/video/saa7164/saa7164.h |2 -- 8 files changed, 12 insertions(+), 43 deletions(-) Thanks, Ezequiel. [1] http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructure/49553 Conexant E-mail Firewall (Conexant.Com) made the following annotations - ** Legal Disclaimer This email may contain confidential and privileged material for the sole use of the intended recipient. Any unauthorized review, use or distribution by others is strictly prohibited. If you have received the message in error, please advise the sender by reply email and delete the message. Thank you. ** - -- 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 0/12] struct i2c_algo_bit_data cleanup on several drivers
On Mon, Jun 18, 2012 at 04:23:14PM -0300, Ezequiel Garcia wrote: Hi Mauro, This patchset cleans the i2c part of some drivers. This issue was recently reported by Dan Carpenter [1], and revealed wrong (and harmless) usage of struct i2c_algo_bit. How is this harmless? We are setting the function pointers to something completely bogus. It seems like a bad thing. regards, dan carpenter -- 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 0/12] struct i2c_algo_bit_data cleanup on several drivers
On Mon, Jun 18, 2012 at 5:56 PM, Dan Carpenter dan.carpen...@oracle.com wrote: On Mon, Jun 18, 2012 at 04:23:14PM -0300, Ezequiel Garcia wrote: Hi Mauro, This patchset cleans the i2c part of some drivers. This issue was recently reported by Dan Carpenter [1], and revealed wrong (and harmless) usage of struct i2c_algo_bit. How is this harmless? We are setting the function pointers to something completely bogus. It seems like a bad thing. You're right, but that wrongly assigned struct algo_bit_data is never *ever* used, since it is not registered. So, I meant it was harmless in that way, perhaps it wasn't the right term. Of course, I can be wrong. Ezequiel. -- 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 0/12] struct i2c_algo_bit_data cleanup on several drivers
On Mon, Jun 18, 2012 at 06:00:52PM -0300, Ezequiel Garcia wrote: On Mon, Jun 18, 2012 at 5:56 PM, Dan Carpenter dan.carpen...@oracle.com wrote: On Mon, Jun 18, 2012 at 04:23:14PM -0300, Ezequiel Garcia wrote: Hi Mauro, This patchset cleans the i2c part of some drivers. This issue was recently reported by Dan Carpenter [1], and revealed wrong (and harmless) usage of struct i2c_algo_bit. How is this harmless? We are setting the function pointers to something completely bogus. It seems like a bad thing. You're right, but that wrongly assigned struct algo_bit_data is never *ever* used, since it is not registered. So, I meant it was harmless in that way, perhaps it wasn't the right term. No no. I didn't realize these files didn't call i2c_add_bus(). Harmless is the right term. regards, dan carpenter -- 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 4/5] usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework
Hi Bhupesh, Thanks for the patch. It looks quite good, please see below for various small comments. On Friday 01 June 2012 15:08:57 Bhupesh Sharma wrote: This patch reworks the videobuffer management logic present in the UVC webcam gadget and ports it to use the more apt videobuf2 framework for video buffer management. To support routing video data captured from a real V4L2 video capture device with a zero copy operation on videobuffers (as they pass from the V4L2 domain to UVC domain via a user-space application), we need to support USER_PTR IO method at the UVC gadget side. So the V4L2 capture device driver can still continue to use MMAO IO method s/MMAO/MMAP/ and now the user-space application can just pass a pointer to the video buffers being DeQueued from the V4L2 device side while Queueing them at the I don't think dequeued and queueing need capitals :-) UVC gadget end. This ensures that we have a zero-copy design as the videobuffers pass from the V4L2 capture device to the UVC gadget. Note that there will still be a need to apply UVC specific payload headers on top of each UVC payload data, which will still require a copy operation to be performed in the 'encode' routines of the UVC gadget. Signed-off-by: Bhupesh Sharma bhupesh.sha...@st.com --- drivers/usb/gadget/Kconfig |1 + drivers/usb/gadget/uvc_queue.c | 524 ++--- drivers/usb/gadget/uvc_queue.h | 25 +-- drivers/usb/gadget/uvc_v4l2.c | 35 ++-- drivers/usb/gadget/uvc_video.c | 17 +- 5 files changed, 184 insertions(+), 418 deletions(-) [snip] diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c index 0cdf89d..907ece8 100644 --- a/drivers/usb/gadget/uvc_queue.c +++ b/drivers/usb/gadget/uvc_queue.c [snip] This part is a bit difficult to review, as git tried too hard to create a universal diff where your patch really replaces the code. I'll remove the - lines to make the comments as readable as possible. +/* --- -- + * videobuf2 queue operations */ + +static int uvc_queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt, + unsigned int *nbuffers, unsigned int *nplanes, + unsigned int sizes[], void *alloc_ctxs[]) { + struct uvc_video_queue *queue = vb2_get_drv_priv(vq); + struct uvc_video *video = + container_of(queue, struct uvc_video, queue); No need for a line split. + if (*nbuffers UVC_MAX_VIDEO_BUFFERS) + *nbuffers = UVC_MAX_VIDEO_BUFFERS; + *nplanes = 1; + + sizes[0] = video-imagesize; return 0; } +static int uvc_buffer_prepare(struct vb2_buffer *vb) { + struct uvc_video_queue *queue = vb2_get_drv_priv(vb-vb2_queue); + struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf); + if (vb-v4l2_buf.type == V4L2_BUF_TYPE_VIDEO_OUTPUT + vb2_get_plane_payload(vb, 0) vb2_plane_size(vb, 0)) { Please align vb2 with the vb- on the previous line. Have you by any chance found some inspiration in drivers/media/video/uvc/uvc_queue.c ? :-) It would probably make sense to move this check to vb2 core, but that's outside of the scope of this patch. + uvc_trace(UVC_TRACE_CAPTURE, [E] Bytes used out of bounds.\n); + return -EINVAL; + } + if (unlikely(queue-flags UVC_QUEUE_DISCONNECTED)) + return -ENODEV; + buf-state = UVC_BUF_STATE_QUEUED; + buf-mem = vb2_plane_vaddr(vb, 0); + buf-length = vb2_plane_size(vb, 0); + if (vb-v4l2_buf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE) + buf-bytesused = 0; + else + buf-bytesused = vb2_get_plane_payload(vb, 0); The driver doesn't support the capture type at the moment so this might be a bit overkill, but I think it's a good idea to support capture in the queue imeplementation. I plan to try and merge the uvcvideo and uvcgadget queue implementations at some point. + return 0; +} +static void uvc_buffer_queue(struct vb2_buffer *vb) +{ + struct uvc_video_queue *queue = vb2_get_drv_priv(vb-vb2_queue); + struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf); + unsigned long flags; + spin_lock_irqsave(queue-irqlock, flags); + if (likely(!(queue-flags UVC_QUEUE_DISCONNECTED))) { + list_add_tail(buf-queue, queue-irqqueue); + } else { + /* If the device is disconnected return the buffer to userspace + * directly. The next QBUF call will fail with -ENODEV. + */ + buf-state = UVC_BUF_STATE_ERROR; + vb2_buffer_done(buf-buf, VB2_BUF_STATE_ERROR); } + spin_unlock_irqrestore(queue-irqlock, flags); } +static struct vb2_ops uvc_queue_qops = { + .queue_setup = uvc_queue_setup, +
Re: [PATCH] make VIDEO_MEDIA depends on DVB_CORE only (removing depends VIDEO_DEV)
Em 07-06-2012 22:55, cheng renquan escreveu: I think the root cause is VIDEO_MEDIA depending on VIDEO_DEV or DVB_CORE; since MEDIA_TUNER is depending on VIDEO_MEDIA; I have VIDEO_DEV but not DVB_CORE, hence should be no VIDEO_MEDIA, config MEDIA_TUNER tristate default VIDEO_MEDIA I2C depends on VIDEO_MEDIA I2C diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig index 9575db4..1b35dae 100644 --- a/drivers/media/Kconfig +++ b/drivers/media/Kconfig @@ -99,7 +99,7 @@ config DVB_NET config VIDEO_MEDIA tristate - default (DVB_CORE (VIDEO_DEV = n)) || (VIDEO_DEV (DVB_CORE = n)) || (DVB_CORE VIDEO_DEV) + default DVB_CORE Sorry, but this is wrong and will break compilation for analog drivers. I think my RFC patches should fix this issue[1] and [2]. If not, they will at the final version, after removing drivers/media/video and drivers/media/dvb, and re-writing the dependency chain (planned as part 2 and 3 of my patch series). [1] http://www.spinics.net/lists/linux-media/msg48451.html [2] http://www.spinics.net/lists/linux-media/msg48974.html Regards, Mauro comment Multimedia drivers On Thu, Jun 7, 2012 at 4:09 PM, VDR User user@gmail.com wrote: On Thu, Jun 7, 2012 at 2:53 PM, cheng renquan crq...@gmail.com wrote: till recently I found that also chosen those media tuner modules, $ grep MEDIA_TUNER /boot/config CONFIG_MEDIA_TUNER=m # CONFIG_MEDIA_TUNER_CUSTOMISE is not set CONFIG_MEDIA_TUNER_SIMPLE=m CONFIG_MEDIA_TUNER_TDA8290=m CONFIG_MEDIA_TUNER_TDA827X=m CONFIG_MEDIA_TUNER_TDA18271=m CONFIG_MEDIA_TUNER_TDA9887=m CONFIG_MEDIA_TUNER_TEA5761=m CONFIG_MEDIA_TUNER_TEA5767=m CONFIG_MEDIA_TUNER_MT20XX=m CONFIG_MEDIA_TUNER_XC2028=m CONFIG_MEDIA_TUNER_XC5000=m CONFIG_MEDIA_TUNER_XC4000=m CONFIG_MEDIA_TUNER_MC44S803=m as I understand, MEDIA_TUNER is for some tv adapters but I don't have such hardware, to disable them I need to enable MEDIA_TUNER_CUSTOMISE, then a menu Customize TV tuners becomes visible then I need to enter that menu and disable all the tuners one-by-one; this looks not convenient, I hate that too so you're not alone. I've just gotten into the habit of having to manually disabling everything I don't need as opposed to only needing to enable what I do need. :\ -- 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 -- 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: [RFCv2 PATCH 4/6] videodev2.h: add frequency band information.
Em 28-05-2012 07:46, Hans Verkuil escreveu: From: Hans Verkuil hans.verk...@cisco.com Signed-off-by: Hans Verkuil hans.verk...@cisco.com Acked-by: Hans de Goede hdego...@redhat.com --- include/linux/videodev2.h | 19 +-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 2339678..013ee46 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -2023,7 +2023,8 @@ struct v4l2_tuner { __u32 audmode; __s32 signal; __s32 afc; - __u32 reserved[4]; + __u32 band; + __u32 reserved[3]; }; struct v4l2_modulator { @@ -2033,7 +2034,8 @@ struct v4l2_modulator { __u32 rangelow; __u32 rangehigh; __u32 txsubchans; - __u32 reserved[4]; + __u32 band; + __u32 reserved[3]; }; /* Flags for the 'capability' field */ @@ -2048,6 +2050,11 @@ struct v4l2_modulator { #define V4L2_TUNER_CAP_RDS 0x0080 #define V4L2_TUNER_CAP_RDS_BLOCK_IO 0x0100 #define V4L2_TUNER_CAP_RDS_CONTROLS 0x0200 +#define V4L2_TUNER_CAP_BAND_FM_EUROPE_US 0x0001 +#define V4L2_TUNER_CAP_BAND_FM_JAPAN 0x0002 +#define V4L2_TUNER_CAP_BAND_FM_RUSSIAN 0x0004 +#define V4L2_TUNER_CAP_BAND_FM_WEATHER 0x0008 +#define V4L2_TUNER_CAP_BAND_AM_MW0x0010 Frequency band is already specified by rangelow/rangehigh. Why do you need to duplicate this information? /* Flags for the 'rxsubchans' field */ #define V4L2_TUNER_SUB_MONO 0x0001 @@ -2065,6 +2072,14 @@ struct v4l2_modulator { #define V4L2_TUNER_MODE_LANG1 0x0003 #define V4L2_TUNER_MODE_LANG1_LANG2 0x0004 +/* Values for the 'band' field */ +#define V4L2_TUNER_BAND_DEFAULT 0 What does default mean? +#define V4L2_TUNER_BAND_FM_EUROPE_US 1 /* 87.5 Mhz - 108 MHz */ EUROPE_US is a bad name for this range. According with Wikipedia, this range is used at ITU region 1 (Europe/Africa), while America uses ITU region 2 (88-108). In Brazil, the range from 87.5-88 were added several years ago, so it is currently at the ITU region 1 range, just like in US. I don't doubt that there are still some places at the 88-108 MHz range. +#define V4L2_TUNER_BAND_FM_JAPAN 2 /* 76 MHz - 90 MHz */ This is currently true, but wikipedia points that they may increase it (from 76MHz to 108MHz?) after the end of NTSC broadcast. The DTV range there starts at channel 14 (473 MHz and upper). Maybe they may reserve the channel 7-13 range (VHF High - starting at 177 MHz) like Brazil for DTV. Anyway, what I mean is that calling a frequency range with a Country name is dangerous, as frequency ranges can vary from time to time. +#define V4L2_TUNER_BAND_FM_RUSSIAN3 /* 65.8 MHz - 74 MHz */ AFAIKT, this is wrong. The range used there is 65.8-104MHz. It used to be 65.8 to 100 MHz. Also, other ex-soviet countries are still using such range. +#define V4L2_TUNER_BAND_FM_WEATHER4 /* 162.4 MHz - 162.55 MHz */ +#define V4L2_TUNER_BAND_AM_MW 5 + struct v4l2_frequency { __u32 tuner; __u32 type; /* enum v4l2_tuner_type */ Regards, 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: SoC i.mx35 userptr method failure while running capture-example utility
Hi, On Monday 18 June 2012 16:26:01 Prabhakar Lad wrote: On Mon, Jun 18, 2012 at 4:05 PM, Laurent Pinchart wrote: On Monday 18 June 2012 12:28:44 Prabhakar Lad wrote: On Wed, May 2, 2012 at 4:20 AM, Guennadi Liakhovetski wrote: On Tue, 1 May 2012, Alex Gershgorin wrote: Hi everyone, I use user-space utility from http://git.linuxtv.org/v4l-utils.git/blob/HEAD:/contrib/test/capture- example.c I made two small changes in this application and this is running on i.MX35 SoC fmt.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB565; fmt.fmt.pix.field = V4L2_FIELD_ANY; When MMAP method is used everything works fine, problem occurs when using USERPTR method this can see bellow : ./capture-example -u -f -d /dev/video0 mx3-camera mx3-camera.0: MX3 Camera driver attached to camera 0 Failed acquiring VMA for vaddr 0x76cd9008 VIDIOC_QBUF error 22, Invalid arg It doesn't surprise me, that this doesn't work. capture-example allocates absolutely normal user-space buffers, when called with USERPTR, and those buffers are very likely discontiguous. Whereas mx3- camera needs physically contiguous buffers, so, this can only fail. This means, you either have to use MMAP or you need to allocate USERPTR buffers in a special way to guarantee their contiguity. Even I am facing a similar issue when ported to VB2 for USERPTR. How do we ensure the buffers not discontiguous. Would cmem assure it? CMEM is definitely not the way to go, it's an out-of-tree hack that should disappear once we get proper CMA and DMABUF support in the kernel. How to allocate memory depends on your use case(s). If you just need to capture to anonymous memory that will then be read by userspace you shouldn't use USERPTR, but MMAP. If you need to capture to device memory (for instance capturing directly to a frame buffer), you should export a DMABUF object on from the frame buffer driver (this isn't available in mainline yet, I'll try to send a patch soon) and then import it on the V4L2 side (not available in mainline yet either I'm afraid :-)). As an interim solution, mmap() your frame buffer to userspace and then use USERPTR. I have got MMAP working with videobuf2, with videobuf there was a support for userptr which I need to achieve this with videobuf2. videobuf2 supports USERPTR as well. If the memory pointed to by your user pointer matches the device requirements, USERPTR should work out of the box. Can you point me to your branch where you are working on it and also an example if you have to interim solution as you suggested above. I currently have no public branch with the FBDEV DMABUF patches. I need to clean them up and submit them upstream, I hope to do that in the next couple of days. Regarding the interim solution, just mmap() your frame buffer to userspace and pass that to your V4L driver using USERPTR. That's what embedded systems usually do nowadays. -- 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] Revert [media] media: mx2_camera: Fix mbus format handling
Hi Mauro, On Fri, May 25, 2012 at 7:06 PM, Guennadi Liakhovetski g.liakhovet...@gmx.de wrote: This reverts commit d509835e32bd761a2b7b446034a273da568e5573. That commit breaks support for the generic pass-through mode in the driver for formats, not natively supported by it. Besides due to a merge conflict it also breaks driver compilation: drivers/media/video/mx2_camera.c: In function 'mx2_camera_set_bus_param': drivers/media/video/mx2_camera.c:937: error: 'pixfmt' undeclared (first use in this function) drivers/media/video/mx2_camera.c:937: error: (Each undeclared identifier is reported only once drivers/media/video/mx2_camera.c:937: error: for each function it appears in.) Can this be applied? It is breaking mxs_defconfig for one month now. Thanks, Fabio Estevam -- 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] Revert [media] media: mx2_camera: Fix mbus format handling
Em 18-06-2012 21:55, Fabio Estevam escreveu: Hi Mauro, On Fri, May 25, 2012 at 7:06 PM, Guennadi Liakhovetski g.liakhovet...@gmx.de wrote: This reverts commit d509835e32bd761a2b7b446034a273da568e5573. That commit breaks support for the generic pass-through mode in the driver for formats, not natively supported by it. Besides due to a merge conflict it also breaks driver compilation: drivers/media/video/mx2_camera.c: In function 'mx2_camera_set_bus_param': drivers/media/video/mx2_camera.c:937: error: 'pixfmt' undeclared (first use in this function) drivers/media/video/mx2_camera.c:937: error: (Each undeclared identifier is reported only once drivers/media/video/mx2_camera.c:937: error: for each function it appears in.) Can this be applied? It is breaking mxs_defconfig for one month now. It were applied today at my -next tree. Thanks, Fabio Estevam -- 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] [media] cx231xx: restore tuner settings on first open
On Mon, 2012-06-18 at 10:36 -0400, Devin Heitmueller wrote: On Mon, Jun 18, 2012 at 10:32 AM, David Dillow d...@thedillows.org wrote: Hmm, it sounds like perhaps changing the standby call in the tuner core to asynchronously power down the tuner may be the way to go -- ie, when we tell it to standby, it will do a schedule_work for some 10 seconds later to really pull it down. If we get a resume call prior to then, we'll just cancel the work, otherwise we wait for the work to finish and then issue the resume. Does that sound reasonable? At face value it sounds reasonable, except the approach breaks down as soon as you have hybrid tuners which support both analog and digital. Because the digital side of the tuner isn't tied into tuner-core, you'll break in the following situation: Start using analog Stop using analog [schedule_work() call] Start using digital Timer pops and powers down the tuner even though it's in use for ATSC or ClearQAM Again, I'm not proposing a solution, but just poking a fatal hole in your proposal (believe me, I had considered the same approach when first looking at the problem). No worries, I just started looking at V4L stuff on Friday, so I expect to make some mistakes... It sounds like we want a solution that * lives in core code * doesn't require tuner drivers to save state * manages hybrid tuners appropriately * allows for gradual API change-over (no flag day for tuners or for capture devices) * has a reasonable grace period before putting tuner to standby Aside from the entering standby issue, fixing the loss of mode/frequency looks like it may be fixable by just having the capture cards explicitly request the tuner become active -- the tuner core will already restore those for us. It's just that few cards do that today. Is it safe to say that the tuner core will know about all hybrid tuners, even if it doesn't know if the digital side is still in use? Can a single tuner be used for both digital and analog tuning at the same time? I'm wondering if just having a simple counter would work, with the digital side calling for power just as the capture side should already be doing. If the count is non-zero on a standby call, don't do anything. If it goes to zero, then schedule the HW standby. The challenge would seem to be getting the DVB system to call into the tuner-core (or other proper place). So much to learn... thank you for your patience, Dave -- 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