Hi Dima, On Tuesday 26 June 2012 13:53:34 Dima Zavin wrote: > On Tue, Jun 26, 2012 at 2:40 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > > On Tue 26 June 2012 11:11:06 Laurent Pinchart wrote: > >> On Tuesday 26 June 2012 10:40:44 Tomasz Stanislawski wrote: > >> > Hi Dima Zavin, > >> > Thank you for the patch and for a ping remainder :). > >> > > >> > You are right. The unmap is missing in __vb2_queue_cancel. > >> > I will apply your fix into next version of V4L2 support for dmabuf. > >> > > >> > Please refer to some comments below. > >> > > >> > On 06/20/2012 08:12 AM, Dima Zavin wrote: > >> > > Tomasz, > >> > > > >> > > I've encountered an issue with this patch when userspace does several > >> > > stream_on/stream_off cycles. When the user tries to qbuf a buffer > >> > > after doing stream_off, we trigger the "dmabuf already pinned" > >> > > warning since we didn't unmap the buffer as dqbuf was never called. > >> > > > >> > > The below patch adds calls to unmap in queue_cancel, but my feeling > >> > > is that we probably should be calling detach too (i.e. put_dmabuf). > >> > >> According to the V4L2 specification, the "VIDIOC_STREAMOFF ioctl, apart > >> of aborting or finishing any DMA in progress, unlocks any user pointer > >> buffers locked in physical memory, and it removes all buffers from the > >> incoming and outgoing queues". > > > > Correct. And what that means in practice is that after a streamoff all > > buffers are returned to the state they had just before STREAMON was > > called. > > That can't be right. The buffers had to have been returned to the > state just *after REQBUFS*, not just *before STREAMON*. You need to > re-enqueue buffers before calling STREAMON. I assume that's what you > meant? Your interpretation is correct. > > So after STREAMOFF you can immediately queue all buffers again with QBUF > > and call STREAMON to restart streaming. No mmap or other operations > > should be required. This behavior must be kept. > > > > VIDIOC_REQBUFS() or a close() are the only two operations that will > > actually free the buffers completely. > > > > In practice, a STREAMOFF is either followed by a STREAMON at a later time, > > or almost immediately followed by REQBUFS or close() to tear down the > > buffers. So I don't think the buffers should be detached at streamoff. > > I agree. I was leaning this way which is why I left it out of my patch > and wanted to hear your guys' opinion as you are much more familiar > with the intended behavior than I am. > > Thanks! You're welcome. Thank you for reporting the problem and providing a patch. -- Regards, Laurent Pinchart -- 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
