Is anyone planning to review this? I could really use some more eyeballs looking at this! Regards, Hans On 02/06/2014 12:02 PM, Hans Verkuil wrote: > This patch series is v2 of the previous series: > > http://www.spinics.net/lists/linux-media/msg72166.html > > Patches 1-6 are unchanged, 7-10 are new. > > This patch series fixes a series of bugs in vb2. Recently I have been > converting the saa7134 driver to vb2 and as part of that work I discovered > that the op calls were not properly balanced, which caused saa7134 to > fail. > > Based on that I did more debugging and I found lots of different issues > with vb2 when it comes to balancing ops. This patch series fixes them. > Many thanks to Pawel Osciak for a good brainstorm session. > > Patch 1 adds debugging code to check for unbalanced calls. I used this > when testing since without this it is very hard to verify correctness. > It is currently turned on when CONFIG_VIDEO_ADV_DEBUG is set, but perhaps > this should be placed under a vb2 specific debug option? > > The next patch changes the buf_finish return type to void. It must not > fail, and you can't do anything useful if it does anyway. > > Note that I really would like to do the same for stop_streaming. The > stop_streaming error is currently ignored, and there are some drivers > that can actually return an error (waiting for an interruptible mutex > for example). Opinions are welcome. > > Patch 3 just improves some comments. > > Patches 4 and 5 fix several unbalanced ops. > > Patch 6 fixes a regression (must go to 3.14!). > > Patch 7 just renames queue_count to owned_by_drv_count. The old name > suggests the number of buffers in the queue_list, but that's not what > it is. Since the next patch will actually add a real count for the > number of buffers in the queue_list I decided to just rename this, > thus freeing up the name 'queue_count'. > > Patch 8 fixes a bug in the handling of start_streaming: before that op > is called any prequeued buffers are handed over to the driver. But if > start_streaming returns an error, then those buffers are not reclaimed. > Since start_streaming failed, q->streaming is 0, so stop_streaming isn't > called either. > > There are two reasons for an error in start_streaming: either a real > error when setting up the DMA engine occurred or there were not enough > buffers queued for the DMA engine to start (start_streaming returns > -ENOBUFS in that case). It makes sense to require that drivers return > queued buffers back to vb2 in case of an error, but not if they also > have to do that in case of insufficient buffers. So this patch replaces > the -ENOBUFS mechanism by a vb2_queue field that tells vb2 what the > minimum number of buffers is. > > Now if start_streaming returns an error the vb2 core will check if there > are still buffers owned by the driver and if so produce a warning and > reclaim those buffers. The same is done when the vb2_queue is freed. > > This ensures that the prepare/finish memops are correctly called and > the state of all the buffers is consistent. > > Patch 9 fixes vivi for this start_streaming issue. Note that there are > many drivers that do not clean up properly in case of an error during > start_streaming. > > The final patch attempts to fix another issue: I noticed that in the few > drivers that implement VIDIOC_CREATE_BUFS the v4l2_format is just used as-is, > i.e. no checks are done whether the contents of the struct makes sense or not. > > This patch adds a number of sanity check to see if the buffer size related > values make sense. > > I have been testing these vb2 changes with vivi (vmalloc based) and an > out-of-tree PCI driver (dma-sg based), with the mmap/userptr and read/write > streaming modes. I hope to test this also for a dma-contig based driver. > I have no way of testing with dmabuf, though. Does anyone know of a simple > way to obtain dmabufs from somewhere so they can be passed to a v4l driver? > It would be great to add a --stream-dmabuf option for v4l2-ctl. > > I have to admit that I was a bit disappointed by all these vb2 issues... > > Regards, > > Hans > > -- > 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 > -- 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
