| Message ID | 20120620061216.GA19245@google.com |
|---|---|
| State | New |
| Headers | show |
ping? On Tue, Jun 19, 2012 at 11:12 PM, Dima Zavin <dmitriyz@google.com> 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). > > Thoughts? > > --Dima > > Subject: [PATCH] v4l: vb2: unmap dmabufs on STREAM_OFF event > > Currently, if the user issues a STREAM_OFF request and then > tries to re-enqueue buffers, it will trigger a warning in > the vb2 allocators as the buffer would still be mapped > from before STREAM_OFF was called. The current expectation > is that buffers will be unmapped in dqbuf, but that will never > be called on the mapped buffers after a STREAM_OFF event. > > Cc: Sumit Semwal <sumit.semwal@ti.com> > Cc: Tomasz Stanislawski <t.stanislaws@samsung.com> > Signed-off-by: Dima Zavin <dima@android.com> > --- > drivers/media/video/videobuf2-core.c | 22 ++++++++++++++++++++-- > 1 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/video/videobuf2-core.c > b/drivers/media/video/videobuf2-core.c > index b431dc6..e2a8f12 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -1592,8 +1592,26 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > /* > * Reinitialize all buffers for next use. > */ > - for (i = 0; i < q->num_buffers; ++i) > - q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; > + for (i = 0; i < q->num_buffers; ++i) { > + struct vb2_buffer *vb = q->bufs[i]; > + int plane; > + > + vb->state = VB2_BUF_STATE_DEQUEUED; > + > + if (q->memory != V4L2_MEMORY_DMABUF) > + continue; > + > + for (plane = 0; plane < vb->num_planes; ++plane) { > + struct vb2_plane *p = &vb->planes[plane]; > + > + if (!p->mem_priv) > + continue; > + if (p->dbuf_mapped) { > + call_memop(q, unmap_dmabuf, p->mem_priv); > + p->dbuf_mapped = 0; > + } > + } > + } > } > > /** > -- > 1.7.7.3 > >
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). > > Thoughts? > > --Dima > > Subject: [PATCH] v4l: vb2: unmap dmabufs on STREAM_OFF event > > Currently, if the user issues a STREAM_OFF request and then > tries to re-enqueue buffers, it will trigger a warning in > the vb2 allocators as the buffer would still be mapped > from before STREAM_OFF was called. The current expectation > is that buffers will be unmapped in dqbuf, but that will never > be called on the mapped buffers after a STREAM_OFF event. > > Cc: Sumit Semwal <sumit.semwal@ti.com> > Cc: Tomasz Stanislawski <t.stanislaws@samsung.com> > Signed-off-by: Dima Zavin <dima@android.com> > --- > drivers/media/video/videobuf2-core.c | 22 ++++++++++++++++++++-- > 1 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c > index b431dc6..e2a8f12 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -1592,8 +1592,26 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > /* > * Reinitialize all buffers for next use. > */ > - for (i = 0; i < q->num_buffers; ++i) > - q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; > + for (i = 0; i < q->num_buffers; ++i) { > + struct vb2_buffer *vb = q->bufs[i]; > + int plane; > + > + vb->state = VB2_BUF_STATE_DEQUEUED; > + > + if (q->memory != V4L2_MEMORY_DMABUF) > + continue; > + > + for (plane = 0; plane < vb->num_planes; ++plane) { > + struct vb2_plane *p = &vb->planes[plane]; > + > + if (!p->mem_priv) > + continue; is the check above really needed? No check like this is done in vb2_dqbuf. > + if (p->dbuf_mapped) { If a buffer is queued then it is also mapped, so dbuf_mapped should be always be true here (at least in theory). > + call_memop(q, unmap_dmabuf, p->mem_priv); > + p->dbuf_mapped = 0; > + } > + } > + } > } > > /** Regards, Tomasz Stanislawski
Hi Dima and Tomasz, Sorry for the late reply. 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". Detaching the buffer is thus not strictly required. At first thought I agreed with you, as not deatching the buffer might keep resources allocated for much longer than needed. For instance, an application that stops the stream and expects to resume it later will usually not free the buffers (with VIDIOC_REQBUFS(0)) between VIDIOC_STREAMOFF and VIDIOC_STREAMON. Buffer will thus be referenced for longer than needed. However, to reuse the same buffer after restarting the stream, the application will need to keep the dmabuf fds around in order to queue them. Detaching the buffer will thus bring little benefit in terms of resource usage, as the open file handles will keep the buffer around anyway. If an application cares about that and closes all dmabuf fds after stopping the stream, I expect it to free the buffers as well. I don't have a very strong opinion about this, if you would rather detach the buffer at stream-off time I'm fine with that. > > Thoughts? > > > > --Dima > > > > Subject: [PATCH] v4l: vb2: unmap dmabufs on STREAM_OFF event > > > > Currently, if the user issues a STREAM_OFF request and then > > tries to re-enqueue buffers, it will trigger a warning in > > the vb2 allocators as the buffer would still be mapped > > from before STREAM_OFF was called. The current expectation > > is that buffers will be unmapped in dqbuf, but that will never > > be called on the mapped buffers after a STREAM_OFF event. > > > > Cc: Sumit Semwal <sumit.semwal@ti.com> > > Cc: Tomasz Stanislawski <t.stanislaws@samsung.com> > > Signed-off-by: Dima Zavin <dima@android.com> > > --- > > > > drivers/media/video/videobuf2-core.c | 22 ++++++++++++++++++++-- > > 1 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/video/videobuf2-core.c > > b/drivers/media/video/videobuf2-core.c index b431dc6..e2a8f12 100644 > > --- a/drivers/media/video/videobuf2-core.c > > +++ b/drivers/media/video/videobuf2-core.c > > @@ -1592,8 +1592,26 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > > > > /* > > * Reinitialize all buffers for next use. > > */ > > > > - for (i = 0; i < q->num_buffers; ++i) > > - q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; > > + for (i = 0; i < q->num_buffers; ++i) { > > + struct vb2_buffer *vb = q->bufs[i]; > > + int plane; > > + > > + vb->state = VB2_BUF_STATE_DEQUEUED; > > + > > + if (q->memory != V4L2_MEMORY_DMABUF) > > + continue; Don't we need to do something similat for USERPTR buffers as well ? They don't seem to get unpinned (put_userptr) at stream-off time. If we decide to detach the buffer as well as unmapping it, we could just call __vb2_buf_put and __vb2_buf_userptr put here. If we don't, the code might still be simplified by adding an argument to __vb2_buf_dmabuf_put to select whether to unmap and detach the buffer, or just unmap it. > > + for (plane = 0; plane < vb->num_planes; ++plane) { > > + struct vb2_plane *p = &vb->planes[plane]; > > + > > + if (!p->mem_priv) > > + continue; > > is the check above really needed? No check like this is done in > vb2_dqbuf. I think the check comes from __vb2_plane_dmabuf_put. If the buffer is not queued mem_priv will be NULL. However, that might be redundant with the next check > > + if (p->dbuf_mapped) { > > If a buffer is queued then it is also mapped, so dbuf_mapped > should be always be true here (at least in theory). The buffer might never have been queued. > > + call_memop(q, unmap_dmabuf, p->mem_priv); > > + p->dbuf_mapped = 0; > > + } > > + } > > + } > > > > } > > > > /**
On Tue 26 June 2012 11:11:06 Laurent Pinchart wrote: > Hi Dima and Tomasz, > > Sorry for the late reply. > > 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. 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. Regards, Hans > Detaching the buffer is thus not strictly required. At first thought I agreed > with you, as not deatching the buffer might keep resources allocated for much > longer than needed. For instance, an application that stops the stream and > expects to resume it later will usually not free the buffers (with > VIDIOC_REQBUFS(0)) between VIDIOC_STREAMOFF and VIDIOC_STREAMON. Buffer will > thus be referenced for longer than needed. > > However, to reuse the same buffer after restarting the stream, the application > will need to keep the dmabuf fds around in order to queue them. Detaching the > buffer will thus bring little benefit in terms of resource usage, as the open > file handles will keep the buffer around anyway. If an application cares about > that and closes all dmabuf fds after stopping the stream, I expect it to free > the buffers as well. > > I don't have a very strong opinion about this, if you would rather detach the > buffer at stream-off time I'm fine with that. > > > > Thoughts? > > > > > > --Dima > > > > > > Subject: [PATCH] v4l: vb2: unmap dmabufs on STREAM_OFF event > > > > > > Currently, if the user issues a STREAM_OFF request and then > > > tries to re-enqueue buffers, it will trigger a warning in > > > the vb2 allocators as the buffer would still be mapped > > > from before STREAM_OFF was called. The current expectation > > > is that buffers will be unmapped in dqbuf, but that will never > > > be called on the mapped buffers after a STREAM_OFF event. > > > > > > Cc: Sumit Semwal <sumit.semwal@ti.com> > > > Cc: Tomasz Stanislawski <t.stanislaws@samsung.com> > > > Signed-off-by: Dima Zavin <dima@android.com> > > > --- > > > > > > drivers/media/video/videobuf2-core.c | 22 ++++++++++++++++++++-- > > > 1 files changed, 20 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/video/videobuf2-core.c > > > b/drivers/media/video/videobuf2-core.c index b431dc6..e2a8f12 100644 > > > --- a/drivers/media/video/videobuf2-core.c > > > +++ b/drivers/media/video/videobuf2-core.c > > > @@ -1592,8 +1592,26 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > > > > > > /* > > > * Reinitialize all buffers for next use. > > > */ > > > > > > - for (i = 0; i < q->num_buffers; ++i) > > > - q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; > > > + for (i = 0; i < q->num_buffers; ++i) { > > > + struct vb2_buffer *vb = q->bufs[i]; > > > + int plane; > > > + > > > + vb->state = VB2_BUF_STATE_DEQUEUED; > > > + > > > + if (q->memory != V4L2_MEMORY_DMABUF) > > > + continue; > > Don't we need to do something similat for USERPTR buffers as well ? They don't > seem to get unpinned (put_userptr) at stream-off time. > > If we decide to detach the buffer as well as unmapping it, we could just call > __vb2_buf_put and __vb2_buf_userptr put here. If we don't, the code might > still be simplified by adding an argument to __vb2_buf_dmabuf_put to select > whether to unmap and detach the buffer, or just unmap it. > > > > + for (plane = 0; plane < vb->num_planes; ++plane) { > > > + struct vb2_plane *p = &vb->planes[plane]; > > > + > > > + if (!p->mem_priv) > > > + continue; > > > > is the check above really needed? No check like this is done in > > vb2_dqbuf. > > I think the check comes from __vb2_plane_dmabuf_put. If the buffer is not > queued mem_priv will be NULL. However, that might be redundant with the next > check > > > > + if (p->dbuf_mapped) { > > > > If a buffer is queued then it is also mapped, so dbuf_mapped > > should be always be true here (at least in theory). > > The buffer might never have been queued. > > > > + call_memop(q, unmap_dmabuf, p->mem_priv); > > > + p->dbuf_mapped = 0; > > > + } > > > + } > > > + } > > > > > > } > > > > > > /** > >
On Tue, Jun 26, 2012 at 1:40 AM, Tomasz Stanislawski <t.stanislaws@samsung.com> 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). >> >> Thoughts? >> >> --Dima >> >> Subject: [PATCH] v4l: vb2: unmap dmabufs on STREAM_OFF event >> >> Currently, if the user issues a STREAM_OFF request and then >> tries to re-enqueue buffers, it will trigger a warning in >> the vb2 allocators as the buffer would still be mapped >> from before STREAM_OFF was called. The current expectation >> is that buffers will be unmapped in dqbuf, but that will never >> be called on the mapped buffers after a STREAM_OFF event. >> >> Cc: Sumit Semwal <sumit.semwal@ti.com> >> Cc: Tomasz Stanislawski <t.stanislaws@samsung.com> >> Signed-off-by: Dima Zavin <dima@android.com> >> --- >> drivers/media/video/videobuf2-core.c | 22 ++++++++++++++++++++-- >> 1 files changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c >> index b431dc6..e2a8f12 100644 >> --- a/drivers/media/video/videobuf2-core.c >> +++ b/drivers/media/video/videobuf2-core.c >> @@ -1592,8 +1592,26 @@ static void __vb2_queue_cancel(struct vb2_queue *q) >> /* >> * Reinitialize all buffers for next use. >> */ >> - for (i = 0; i < q->num_buffers; ++i) >> - q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; >> + for (i = 0; i < q->num_buffers; ++i) { >> + struct vb2_buffer *vb = q->bufs[i]; >> + int plane; >> + >> + vb->state = VB2_BUF_STATE_DEQUEUED; >> + >> + if (q->memory != V4L2_MEMORY_DMABUF) >> + continue; >> + >> + for (plane = 0; plane < vb->num_planes; ++plane) { >> + struct vb2_plane *p = &vb->planes[plane]; >> + >> + if (!p->mem_priv) >> + continue; > > is the check above really needed? No check like this is done in > vb2_dqbuf. I added it because queue_cancel gets called in release, so you never know what the state of the buffers will be. If we called req_bufs to allocate the buffer descriptors and then call release, then won't we have the vb2_buffer structs but nothing in mem_priv since we haven't attached a dmabuf yet? > >> + if (p->dbuf_mapped) { > > If a buffer is queued then it is also mapped, so dbuf_mapped > should be always be true here (at least in theory). This loop doesn't check for what the buffer status was, it just iterates over the entire queue. The buffer may have been put into STATE_ERROR, and then you don't know if it was ever mapped, etc. It should always be safe to just follow the flag that says you mapped it and unmap it. If you think that this should always be true, we can change it to: if (!WARN_ON(!p->dbuf_mapped)) or something like that. Thanks! --Dima > >> + call_memop(q, unmap_dmabuf, p->mem_priv); >> + p->dbuf_mapped = 0; >> + } >> + } >> + } >> } >> >> /** > > Regards, > Tomasz Stanislawski
Hans and Laurent, Thanks for the feedback. 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: >> Hi Dima and Tomasz, >> >> Sorry for the late reply. >> >> 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? > 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! --Dima > > Regards, > > Hans > >> Detaching the buffer is thus not strictly required. At first thought I agreed >> with you, as not deatching the buffer might keep resources allocated for much >> longer than needed. For instance, an application that stops the stream and >> expects to resume it later will usually not free the buffers (with >> VIDIOC_REQBUFS(0)) between VIDIOC_STREAMOFF and VIDIOC_STREAMON. Buffer will >> thus be referenced for longer than needed. >> >> However, to reuse the same buffer after restarting the stream, the application >> will need to keep the dmabuf fds around in order to queue them. Detaching the >> buffer will thus bring little benefit in terms of resource usage, as the open >> file handles will keep the buffer around anyway. If an application cares about >> that and closes all dmabuf fds after stopping the stream, I expect it to free >> the buffers as well. >> >> I don't have a very strong opinion about this, if you would rather detach the >> buffer at stream-off time I'm fine with that. >> >> > > Thoughts? >> > > >> > > --Dima >> > > >> > > Subject: [PATCH] v4l: vb2: unmap dmabufs on STREAM_OFF event >> > > >> > > Currently, if the user issues a STREAM_OFF request and then >> > > tries to re-enqueue buffers, it will trigger a warning in >> > > the vb2 allocators as the buffer would still be mapped >> > > from before STREAM_OFF was called. The current expectation >> > > is that buffers will be unmapped in dqbuf, but that will never >> > > be called on the mapped buffers after a STREAM_OFF event. >> > > >> > > Cc: Sumit Semwal <sumit.semwal@ti.com> >> > > Cc: Tomasz Stanislawski <t.stanislaws@samsung.com> >> > > Signed-off-by: Dima Zavin <dima@android.com> >> > > --- >> > > >> > > drivers/media/video/videobuf2-core.c | 22 ++++++++++++++++++++-- >> > > 1 files changed, 20 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/drivers/media/video/videobuf2-core.c >> > > b/drivers/media/video/videobuf2-core.c index b431dc6..e2a8f12 100644 >> > > --- a/drivers/media/video/videobuf2-core.c >> > > +++ b/drivers/media/video/videobuf2-core.c >> > > @@ -1592,8 +1592,26 @@ static void __vb2_queue_cancel(struct vb2_queue *q) >> > > >> > > /* >> > > * Reinitialize all buffers for next use. >> > > */ >> > > >> > > - for (i = 0; i < q->num_buffers; ++i) >> > > - q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; >> > > + for (i = 0; i < q->num_buffers; ++i) { >> > > + struct vb2_buffer *vb = q->bufs[i]; >> > > + int plane; >> > > + >> > > + vb->state = VB2_BUF_STATE_DEQUEUED; >> > > + >> > > + if (q->memory != V4L2_MEMORY_DMABUF) >> > > + continue; >> >> Don't we need to do something similat for USERPTR buffers as well ? They don't >> seem to get unpinned (put_userptr) at stream-off time. >> >> If we decide to detach the buffer as well as unmapping it, we could just call >> __vb2_buf_put and __vb2_buf_userptr put here. If we don't, the code might >> still be simplified by adding an argument to __vb2_buf_dmabuf_put to select >> whether to unmap and detach the buffer, or just unmap it. >> >> > > + for (plane = 0; plane < vb->num_planes; ++plane) { >> > > + struct vb2_plane *p = &vb->planes[plane]; >> > > + >> > > + if (!p->mem_priv) >> > > + continue; >> > >> > is the check above really needed? No check like this is done in >> > vb2_dqbuf. >> >> I think the check comes from __vb2_plane_dmabuf_put. If the buffer is not >> queued mem_priv will be NULL. However, that might be redundant with the next >> check >> >> > > + if (p->dbuf_mapped) { >> > >> > If a buffer is queued then it is also mapped, so dbuf_mapped >> > should be always be true here (at least in theory). >> >> The buffer might never have been queued. >> >> > > + call_memop(q, unmap_dmabuf, p->mem_priv); >> > > + p->dbuf_mapped = 0; >> > > + } >> > > + } >> > > + } >> > > >> > > } >> > > >> > > /** >> >>
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.
Hi Laurent, Hi Dima, On 06/27/2012 10:40 PM, Laurent Pinchart wrote: > 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 now we should decide what should be changed: the spec or vb2 ? Bringing the queue state back to *after REQBUFS* may make the next (STREAMON + QBUFs) very costly operations. Reattaching and mapping a DMABUF might trigger DMA allocation and *will* trigger creation of IOMMU mappings. In case of a user pointer, calling next get_user_pages may cause numerous fault events and will *create* new IOMMU mapping. Is there any need to do such a cleanup if the destruction of buffers and all caches can be explicitly executed by REQBUFS(count = 0) ? Maybe it would be easier to change the spec by removing "apart of ... in physical memory" part? STREAMOFF should mean stopping streaming, and that resources are no longer used. DMABUFs are unmapped but unmapping does not mean releasing. The exporter may keep the resource in its caches. Currently, vb2 does not follow the policy from the spec. The put_userptr ops is called on: - VIDIOC_REBUFS - VIDIOC_CREATE_BUFS - vb2_queue_release() which is usually called on close() syscall The put_userptr is not called and streamoff therefore the user pages are locked after STREAMOFF. BTW. What does 'buffer locked' mean? Does it mean that a buffer is pinned or referenced by a driver/HW? Regards, Tomasz Stanislawski >>> 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. >
Hi Tomasz, On Thursday 02 August 2012 18:31:13 Tomasz Stanislawski wrote: > On 06/27/2012 10:40 PM, Laurent Pinchart wrote: > > 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 now we should decide what should be changed: the spec or vb2 ? > Bringing the queue state back to *after REQBUFS* may make the > next (STREAMON + QBUFs) very costly operations. > > Reattaching and mapping a DMABUF might trigger DMA allocation and > *will* trigger creation of IOMMU mappings. In case of a user pointer, > calling next get_user_pages may cause numerous fault events and > will *create* new IOMMU mapping. > > Is there any need to do such a cleanup if the destruction of buffers > and all caches can be explicitly executed by REQBUFS(count = 0) ? STREAMOFF needs to pass ownership of all buffers to the application. In practice this means that buffers must then be ready to be passed to other devices, requeued to the same device, or destroyed completely. We can't keep the buffers in the V4L2 prepared state, as queueing them would then skip cache handling. Keeping the mapping around could be done though, but would not be compliant with the V4L2 spec as the DMABUF would then not be freed until we call REQBUFS(0). Changing the spec might be possible. I'll need to think more about this, but I'm not very fond of the way we allow a new DMABUF fd (as well as USERPTR pointer) to be associated with an existing buffer, replacing the currently associated fd/pointer. This makes the API asymetrical: it provides an explicit way to associate an fd/pointer with a buffer, but no explicit way to break that association. It's obviously too late to change this for USERPTR, but for DMABUF we could make the buffer/fd association immutable. This would require a way to selectively destroy buffers though, or at least to explicitly break the association. > Maybe it would be easier to change the spec by removing > "apart of ... in physical memory" part? > > STREAMOFF should mean stopping streaming, and that resources are no > longer used. DMABUFs are unmapped but unmapping does not mean releasing. > The exporter may keep the resource in its caches. If the DMABUF implementation follows the USERPTR spec, applications will expect a STREAMOFF call to release all DMABUF instances associated with the buffers. This means that a DMABUF that is only referenced by a V4L2 buffer will get destroyed by a STREAMOFF call. The more I think about it the more this sounds wrong to me. STREAMOFF has never been tasked with freeing resources. As we lack a way to selectively break the fd (or pointer) to buffer association created at buffer prepare or queue time, applications would have to call REQBUFS(0) to release all buffers, even if they will then want to start a new capture run. This might be costly (although probably not in the USERPTR and DMABUF cases), and doesn't allow to unmap DMABUF instances selectively. Maybe an UNPREPARE ioctl would be needed ? > Currently, vb2 does not follow the policy from the spec. > The put_userptr ops is called on: > - VIDIOC_REBUFS > - VIDIOC_CREATE_BUFS > - vb2_queue_release() which is usually called on close() syscall > > The put_userptr is not called and streamoff therefore the user pages > are locked after STREAMOFF. > > BTW. What does 'buffer locked' mean? > > Does it mean that a buffer is pinned or referenced by a driver/HW? In this context I think it refers to pinning pages in memory.
diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index b431dc6..e2a8f12 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -1592,8 +1592,26 @@ static void __vb2_queue_cancel(struct vb2_queue *q) /* * Reinitialize all buffers for next use. */ - for (i = 0; i < q->num_buffers; ++i) - q->bufs[i]->state = VB2_BUF_STATE_DEQUEUED; + for (i = 0; i < q->num_buffers; ++i) { + struct vb2_buffer *vb = q->bufs[i]; + int plane; + + vb->state = VB2_BUF_STATE_DEQUEUED; + + if (q->memory != V4L2_MEMORY_DMABUF) + continue; + + for (plane = 0; plane < vb->num_planes; ++plane) { + struct vb2_plane *p = &vb->planes[plane]; + + if (!p->mem_priv) + continue; + if (p->dbuf_mapped) { + call_memop(q, unmap_dmabuf, p->mem_priv); + p->dbuf_mapped = 0; + } + } + } } /**
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). Thoughts? --Dima Subject: [PATCH] v4l: vb2: unmap dmabufs on STREAM_OFF event Currently, if the user issues a STREAM_OFF request and then tries to re-enqueue buffers, it will trigger a warning in the vb2 allocators as the buffer would still be mapped from before STREAM_OFF was called. The current expectation is that buffers will be unmapped in dqbuf, but that will never be called on the mapped buffers after a STREAM_OFF event. Cc: Sumit Semwal <sumit.semwal@ti.com> Cc: Tomasz Stanislawski <t.stanislaws@samsung.com> Signed-off-by: Dima Zavin <dima@android.com> --- drivers/media/video/videobuf2-core.c | 22 ++++++++++++++++++++-- 1 files changed, 20 insertions(+), 2 deletions(-)