Re: [PATCHv7 03/15] v4l: vb2: add support for shared buffer (dma_buf)

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@xs4all.nl> 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