Hello, On 12/6/2012 2:23 AM, Laurent Pinchart wrote:
Hi Hans, On Tuesday 04 December 2012 16:48:40 Hans Verkuil wrote: > (repost after accidentally using HTML formatting) > > This needs a good review. The change is minor, but as I am not a mm expert, > I'd like to get some more feedback on this. The dma-sg change has been > successfully tested on our hardware, but I don't have any hardware to test > the vmalloc change. > > Note that the 'write' attribute is still stored internally and used to tell > whether set_page_dirty_lock() should be called during put_userptr. > > It is my understanding that that still makes sense, so I didn't change that. > > Regards, > > Hans > > --- start patch --- > > When calling get_user_pages for output buffers, the 'write' argument is set > to 0 (since the DMA isn't writing to memory), This can cause unexpected > results: > > If you calloc() buffer memory and do not fill that memory afterwards, then > the kernel assigns most of that memory to one single physical 'zero' page. > > If you queue that buffer to the V4L2 driver, then it will call > get_user_pages and store the results. Next you dequeue it, fill the buffer > and queue it again. Now the V4L2 core code sees the same userptr and length > and expects it to be the same buffer that it got before and it will reuse > the results of the previous get_user_pages call. But that still points to > zero pages, whereas userspace filled it up and so changed the buffer to use > different pages. In other words, the pages the V4L2 core knows about are no > longer correct. > > The solution is to always set 'write' to 1 as this will force the kernel to > fault in proper pages. I'm wondering if we should really force faulting pages. The buffer given to the driver might be real read-only memory, in which case faulting the pages would probably hurt (agreed, that's pretty unlikely, but it's still a valid use case according to the API). Moreover, this wouldn't fix all similar userptr-related issues. An application could remap a completely different memory area to the same userspace address between two QBUF calls, and videobuf2 would not handle that properly. That's also a valid use case according to the API, and it would be pretty difficult to handle it correctly in an efficient way on the kernel side. I think we could modify the spec to forbid mapping changes between QBUF calls, and in that case the zero-page mapping situation you described could be forbidden as well. If we clearly document it in the spec we could push the responsibility of faulting the pages to userspace.
I've spent some time thinking on this issue and I came to the conclusion that it might be a good idea extend the v4l2 spec with such case. Changing the write parameter for get_user_pages() is only a workaround, which doesn't even work for all possible use cases, so I agree with Laurent that user ptr usage should be much better documented. I only wonder if it a good idea to require faulting in pages from the user space application. Can we assume that memset(ptr, 0, buf_size) will do the job in all cases? I expect yes, but I would like to be really sure,
this will be probably put into the documentation for the reference code. Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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
