On Friday 23 April 2010 08:21:14 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 double checked this: it has nothing to do with my patches. It has been in
videobuf for ages.
What happens is that videobuf_read_one takes vb_lock and calls buffer_prepare.
This will call __videobuf_iolock in videobuf_dma_sg which calls
videobuf_dma_init_user. And this takes the mm->mmap_sem semaphore.
This happens in more places (e.g. videobuf_qbuf does the same).
To prevent these messages we would have to teach the lock validator about this.
Read also linux/rwsem.h and Documentation/lockdep-design.txt.
The lock validator relies on fixed lock ordering (e.g. first take the mmap_sem
lock, then vb_lock), but we definitely use different lock ordering at the moment.
In any case, this is not something introduced by my patches.
We should probably introduce down_read_nested as part of the future refactoring
patches to fix these messages. The order in which these locks are taken must
definitely be fixed.
Regards,
Hans
>
> > 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.
>
> Regards,
>
> Hans
>
> > This is probably the most complex case. You need to use xawtv (without libv4l2
> > patches) and either a bttv or saa7134 card.
> >
> > I tried to run a test here today, with a saa7134, plus xawtv3 from:
> > http://git.linuxtv.org/mchehab/xawtv3.git
> > (this version is basically Gerd's original one, plus some patches from Fedora and
> > two fixes patches I had to do to make it compile)
> >
> > Unfortunately, my video adapter driver didn't support overlay mode. I'll likely
> > try to re-test it with a different driver this weekend.
> >
> >
>
>
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
