Re: [PATCH 6 of 9] videobuf-vmalloc.c: Fix hack of postponing mmap on remap failure

[Mauro Carvalho Chehab <mchehab@infradead.org>]

On Wed, 9 Apr 2008 14:26:46 +0100
"Aidan Thornton" <makosoft@googlemail.com> wrote:

> On Tue, Apr 8, 2008 at 9:50 PM, Mauro Carvalho Chehab
> <mchehab@infradead.org> wrote:
> > > >  With 6/9 or with this patch, the allocation will happen when you try to use an
> >  > >  mmap command. This can happen on streamon, reqbufs, dqbuf, qbuf or reqbuf.
> >  > >
> >  > >  Sorry, but I think I missed your point.
> >  >
> >  > I think I agree with Brandon. Allocating the buffers in reqbufs is
> >  > what the existing em28xx driver does. It saves a bit of complexity in
> >  > that you just have to allocate the buffers from one place and it seems
> >  > to be what the v4l2 API expects. (Apps have to call reqbufs first, but
> >  > the API doesn't put any real restrictions on what order they
> >  > mmap/munmap or queue/dequeue the buffers afterwards, and the API also
> >  > forbids apps from calling reqbufs with the existing buffers mapped or
> >  > streaming turned on which makes life easier). It's what I was going to
> >  > suggest, actually.
> >
> >  I'm still missed. I don't see what's the difference on what patch 6/9 were
> >  expecting to do and the proposed patch, in terms of adding a restriction of not
> >  accepting other mmap commands, without getting a REQBUFS.
> >
> >  The current way that videobuf works is the one that userspace apps expect. If
> >  we change videobuf behavior, for a given userspace API command, we risk to
> >  break some userspace app.
> 
> Either way, mmap isn't allowed without a REQBUFS first, and videobuf
> enforces this. The question is, what orderings of mmap/munmap and
> streamon/streamoff are allowed once you've called REQBUFS - if I'm
> reading the docs correctly, you're allowed to interleave mmap/munmap
> and streamon/streamoff in whatever order you like, don't have to mmap
> buffers before queuing them, etc. In practice, apps don't, but the
> possible combinations at least need to be handled cleanly.

One patch that worked is this one:
	http://linuxtv.org/hg/~mchehab/tm6010/rev/e1a2b9e33bd2

I tried first to use spinlock before vfree(), but kernel complained. I suspect
that vfree() can sleep.

So, what the above patch does is to streamoff() before unmapping. This seems to
be correct to my eyes, since there's no sense of keeping streamon() if the
buffers are unmapped.

I didn't pushed it on em28xx yet, since I'm trying to fix another bug there.

> In what way do you expect userspace apps to break?
> 
> >
> >  > >  A fix could be to disable IRQ during that interval of time, and/or protecting
> >  > >  with a spinlock.
> >  >
> >  > What you're doing in the attached patch won't trigger the issue, since
> >  > you're calling VIDIOC_STREAMOFF before munmap and that dequeues all
> >  > buffers. Try removing the calls to stop_capturing and start_capturing
> >  > around uninit_device / init_device. I haven't tried it, but you should
> >  > be prepared to get a kernel panic.
> >
> >  The tests I did covers the issue of unmapping/remapping. As we've already
> >  discussed, something else should be added to allow unmapping without streamoff.
> >
> >  The same kind of issue seems to apply also to videobuf-dma-sg. In fact, it is
> >  even worse with dma: since buffers will be filled without CPU, just disabling
> >  IRQ, or running a spinlock doesn't solve. You'll need to stop DMA before unmapping.
> 
> Actually, videobuf-dma-sg works in a quite interesting way - it
> actually treats mmap'ed IO essentially the same as userptr IO, just
> with some additional code to allocate the user pages.

I've started to write a patch to add the same functionality. Didn't finished
yet. I'm not sure if it would be important to provide userspace mmapped IO. I
suspect that most apps are using read() or kernel mmap() interfaces.

> Shutting off DMA on munmap wouldn't help, since the buffer could be a userptr one in
> some anonymous mapping entirely unrelated to the device.  I'm not sure
> whether or not this is an issue; I'd have to check the small print on
> the code that pins the pages in RAM, and I can't find it.

I suspect that the issue exists with DMA, although there's a sync routine
there, that may help to avoid such issues. But I think that the same strategy of
doing a streamoff before unmap should be used for kernel mmapped memory.

> (There's also some code to allocate bounce buffers in the mmap case if
> it doesn't have a userland address for the buffer yet. I'm not
> convinced this actually works.)

It should work. you can test it with "capture_example -u". Unfortunately, I
can't test here right now, since the desktop I use here for testing PCI devices
is burned. I'll need to save some money to buy another motherboard or to
replace the machine.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list