Hans Verkuil wrote:
> On Friday 23 April 2010 08:08:01 Mauro Carvalho Chehab wrote:
>> Hans Verkuil wrote:
>>> The following changes since commit 975b06b6c01ba2da4d26a7ba6ea783d5f670aa7d:
>>> Jonathan Corbet (1):
>>> V4L/DVB: ov7670: silence some compiler warnings
>>>
>>> are available in the git repository at:
>>>
>>> git://linuxtv.org/hverkuil/v4l-dvb-videobuf.git videobuf
>>>
>>> Hans Verkuil (8):
>>> v4l videobuf: remove unused mmap callback.
>>> v4l videobuf: remove mmap_free callback
>>> v4l videobuf: remove unused is_mmapped field
>>> v4l videobuf: use struct videobuf_buffer * instead of void * for videobuf_alloc
>>> v4l videobuf: rename .vmalloc to .vaddr
>>> v4l videobuf: rename videobuf_queue_to_vmalloc to videobuf_queue_to_vaddr
>>> v4l videobuf: add videobuf_buffer *buf as argument to mmap_mapper
>>> v4l videobuf: move video_copy_to_user and copy_stream to core.
>>>
>>> drivers/media/video/videobuf-core.c | 95 +++++++++++++++-----
>>> drivers/media/video/videobuf-dma-contig.c | 109 +++--------------------
>>> drivers/media/video/videobuf-dma-sg.c | 139 ++++++++---------------------
>>> drivers/media/video/videobuf-dvb.c | 2 +-
>>> drivers/media/video/videobuf-vmalloc.c | 104 ++--------------------
>>> include/media/videobuf-core.h | 29 ++-----
>>> 6 files changed, 137 insertions(+), 341 deletions(-)
>>>
>>> Tested with cx88-blackbird, bttv, saa7134-empress, dvb-ttpci, vivi.
>>> The tests were done with userptr, mmap and read().
>>>
>>> There is a lot more that needs to be done, but this is a good start.
>> I got this error when testing the patches with a saa7134 card:
>>
>> [ 9171.989707] =======================================================
>> [ 9171.992696] [ INFO: possible circular locking dependency detected ]
>> [ 9171.992696] 2.6.33 #3
>> [ 9171.992696] -------------------------------------------------------
>> [ 9171.992696] xawtv/13330 is trying to acquire lock:
>> [ 9171.992696] (&mm->mmap_sem){++++++}, at: [<f80f6c50>] videobuf_dma_init_user+0x30/0x70 [videobuf_dma_sg]
>> [ 9171.992696]
>> [ 9171.992696] but task is already holding lock:
>> [ 9171.992696] (&q->vb_lock){+.+.+.}, at: [<f80ed8bf>] videobuf_read_one+0x3f/0x340 [videobuf_core]
>> [ 9171.992696]
>> [ 9171.992696] which lock already depends on the new lock.
>> [ 9171.992696]
>> [ 9171.992696]
>> [ 9171.992696] the existing dependency chain (in reverse order) is:
>> [ 9171.992696]
>> [ 9171.992696] -> #1 (&q->vb_lock){+.+.+.}:
>> [ 9171.992696] [<c1078b3d>] __lock_acquire+0xd1d/0x1220
>> [ 9171.992696] [<c10790cd>] lock_acquire+0x8d/0x100
>> [ 9171.992696] [<c1499f6c>] __mutex_lock_common+0x4c/0x370
>> [ 9171.992696] [<c149a36f>] mutex_lock_nested+0x3f/0x50
>> [ 9171.992696] [<f80ec3c9>] videobuf_mmap_mapper+0x39/0xf0 [videobuf_core]
>> [ 9171.992696] [<f83ce5e9>] video_mmap+0x29/0x40 [saa7134]
>> [ 9171.992696] [<f82df23b>] v4l2_mmap+0x3b/0x40 [videodev]
>> [ 9171.992696] [<c10ee292>] mmap_region+0x342/0x480
>> [ 9171.992696] [<c10ee62f>] do_mmap_pgoff+0x25f/0x300
>> [ 9171.992696] [<c10ee869>] sys_mmap_pgoff+0x199/0x1c0
>> [ 9171.992696] [<c1002fff>] sysenter_do_call+0x12/0x38
>> [ 9171.992696]
>> [ 9171.992696] -> #0 (&mm->mmap_sem){++++++}:
>> [ 9171.992696] [<c1078ddf>] __lock_acquire+0xfbf/0x1220
>> [ 9171.992696] [<c10790cd>] lock_acquire+0x8d/0x100
>> [ 9171.992696] [<c149a6ee>] down_read+0x4e/0x60
>> [ 9171.992696] [<f80f6c50>] videobuf_dma_init_user+0x30/0x70 [videobuf_dma_sg]
>> [ 9171.992696] [<f80f71c4>] __videobuf_iolock+0xc4/0x110 [videobuf_dma_sg]
>> [ 9171.992696] [<f80ec343>] videobuf_iolock+0x33/0x80 [videobuf_core]
>> [ 9171.992696] [<f83cded6>] buffer_prepare+0x1d6/0x230 [saa7134]
>> [ 9171.992696] [<f80ed9f6>] videobuf_read_one+0x176/0x340 [videobuf_core]
>> [ 9171.992696] [<f83ce7cb>] video_read+0x8b/0xc0 [saa7134]
>> [ 9171.992696] [<f82df08d>] v4l2_read+0x5d/0x70 [videodev]
>> [ 9171.992696] [<c1107b3f>] vfs_read+0x9f/0x190
>> [ 9171.992696] [<c1108052>] sys_read+0x42/0x70
>> [ 9171.992696] [<c1002fff>] sysenter_do_call+0x12/0x38
>> [ 9171.992696]
>> [ 9171.992696] other info that might help us debug this:
>> [ 9171.992696]
>> [ 9171.992696] 1 lock held by xawtv/13330:
>> [ 9171.992696] #0: (&q->vb_lock){+.+.+.}, at: [<f80ed8bf>] videobuf_read_one+0x3f/0x340 [videobuf_core]
>> [ 9171.992696]
>> [ 9171.992696] stack backtrace:
>> [ 9171.992696] Pid: 13330, comm: xawtv Tainted: G C 2.6.33 #3
>> [ 9171.992696] Call Trace:
>> [ 9171.992696] [<c1498793>] ? printk+0x1d/0x22
>> [ 9171.992696] [<c1076cb2>] print_circular_bug+0xc2/0xd0
>> [ 9171.992696] [<c1078ddf>] __lock_acquire+0xfbf/0x1220
>> [ 9171.992696] [<c10790cd>] lock_acquire+0x8d/0x100
>> [ 9171.992696] [<f80f6c50>] ? videobuf_dma_init_user+0x30/0x70 [videobuf_dma_sg]
>> [ 9171.992696] [<c149a6ee>] down_read+0x4e/0x60
>> [ 9171.992696] [<f80f6c50>] ? videobuf_dma_init_user+0x30/0x70 [videobuf_dma_sg]
>> [ 9171.992696] [<f80f6c50>] videobuf_dma_init_user+0x30/0x70 [videobuf_dma_sg]
>> [ 9171.992696] [<f80f71c4>] __videobuf_iolock+0xc4/0x110 [videobuf_dma_sg]
>> [ 9171.992696] [<f80f6a22>] ? videobuf_dma_free+0x62/0xb0 [videobuf_dma_sg]
>> [ 9171.992696] [<c107787b>] ? trace_hardirqs_on+0xb/0x10
>> [ 9171.992696] [<f80f7100>] ? __videobuf_iolock+0x0/0x110 [videobuf_dma_sg]
>> [ 9171.992696] [<f80ec343>] videobuf_iolock+0x33/0x80 [videobuf_core]
>> [ 9171.992696] [<f83c786e>] ? saa7134_dma_free+0x4e/0x70 [saa7134]
>> [ 9171.992696] [<f83cded6>] buffer_prepare+0x1d6/0x230 [saa7134]
>> [ 9171.992696] [<c1075c5c>] ? lockdep_init_map+0x3c/0x110
>> [ 9171.992696] [<c12697a2>] ? __raw_spin_lock_init+0x32/0x60
>> [ 9171.992696] [<f80ed9f6>] videobuf_read_one+0x176/0x340 [videobuf_core]
>> [ 9171.992696] [<f83ce7cb>] video_read+0x8b/0xc0 [saa7134]
>> [ 9171.992696] [<c1107712>] ? rw_verify_area+0x62/0xd0
>> [ 9171.992696] [<f83ce740>] ? video_read+0x0/0xc0 [saa7134]
>> [ 9171.992696] [<f82df08d>] v4l2_read+0x5d/0x70 [videodev]
>> [ 9171.992696] [<c1107b3f>] vfs_read+0x9f/0x190
>> [ 9171.992696] [<c1067a1b>] ? up_read+0x1b/0x30
>> [ 9171.992696] [<f82df030>] ? v4l2_read+0x0/0x70 [videodev]
>> [ 9171.992696] [<c1098feb>] ? audit_syscall_entry+0x1db/0x200
>> [ 9171.992696] [<c1108052>] sys_read+0x42/0x70
>> [ 9171.992696] [<c1002fff>] sysenter_do_call+0x12/0x38
>>
>> Not sure if it is a regression or not.
>
> Can you retest this without my patches? My patches did not touch locking at all
> (just double checked this) and I am convinced this is an already existing bug.
I also think that this is an already existing issue (although it only appeared
after testing the videobuf patches - probably just a coincidence or some race
condition).
>> Had you test running the videobuf with both Overlay and mmap modes enabled?
>
> None of the hardware I currently use for testing can handle overlay (all nvidia
> GPUs). I do have a few old miniITX boards with VIA graphics output and I can try
> to test whether those support overlay. I won't be able to test this until next
> weekend, though.
Most of the patches on your series are pretty trivial and I doubt that they might
affect Overlay mode. The one that I'm more concerned is this one:
From 292378217a05fa887d87371067903abb9bc1b525 Mon Sep 17 00:00:00 2001
From: Hans Verkuil <hverkuil@xxxxxxxxx>
Date: Sun, 28 Mar 2010 14:09:05 +0200
Subject: v4l videobuf: add videobuf_buffer *buf as argument to mmap_mapper
Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>
mmap_mapper should operate on a buffer, not on a complete queue. So let
the videobuf-core find the correct buffer instead of duplicating that
code in each mmap_mapper implementation.
The dma-sg implementation has backwards compatibility code for handling
the V4L1_COMPAT layer. This code is now under the v4L1_COMPAT config option.
This patch assumes that only v4l1 compat needs to map more than one videobuf.
I never spent my time trying to understand the overlay mode implementation,
as I never found time (or interest) to add overlay support on any driver,
Yet, what concerns me is that, although I suspect that this is not the case,
eventually overlay mode might need this feature. So, the better is to do later
some tests with a device that is capable of working on overlay mode.
I tried to do such test before committing, but unfortunately, the machines I use
for test are all with Nvidia boards, and I couldn't find any driver that I could
compile with one of the OS's that is there to enable DGA support on it (DGA is
required by xawtv to enable overlay mode). I didn't test other applications, nor
I found any patch adding DRI support on xawtv.
I can't spend more time on it, holding my big queue, due to that. I'll apply the
patch and hope for the best. Eventually, someone with Intel or Via via adapters
will fill a bug if the breakage happens. I'd appreciate if you could later do your
tests with the VIA machine.
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
