Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Kazunori Kobayashi]

Hi,

I have a question about memory freeing by calling REQBUF(0) before all the
dmabuf fds exported with VIDIOC_EXPBUF are closed.

In calling REQBUF(0), videobuf2-core returns -EBUSY when the reference count
of a vb2 buffer is more than 1. When dmabuf fds are not exported (usual V4L2_MEMORY_MMAP case),
the check is no problem, but when dmabuf fds are exported and some of them are
not closed (in other words the references to that memory are left),
we cannot succeed in calling REQBUF(0) despite being able to free the memory
after all the references are dropped.

Actually REQBUF(0) does not force a vb2 buffer to be freed but decreases
the refcount of it. Also all the vb2 memory allocators that support dmabuf exporting
(dma-contig, dma-sg, vmalloc) implements memory freeing by release() of dma_buf_ops,
so I think there is no need to return -EBUSY when exporting dmabuf fds.

Could you please tell me what you think?

The code that I am talking about is in drivers/media/v4l2-core/videobuf2-core.c:

   if (*count == 0 || q->num_buffers != 0 || q->memory != memory) {
          /*
           * We already have buffers allocated, so first check if they
           * are not in use and can be freed.
           */
          mutex_lock(&q->mmap_lock);
          if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
                  mutex_unlock(&q->mmap_lock);
                  dprintk(1, "memory in use, cannot free\n");
                  return -EBUSY;
          }

Regards,
Kobayashi
-- 
---------------------------------
IGEL Co.,Ltd. Kazunori Kobayashi
kkobayas@igel.co.jp
http://www.igel.co.jp/

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Laurent Pinchart]

Hello Kobayashi-san,

(CC'ing Hans Verkuil and Marek Szyprowski)

On Wednesday 27 Jul 2016 16:51:47 Kazunori Kobayashi wrote:
> Hi,
> 
> I have a question about memory freeing by calling REQBUF(0) before all the
> dmabuf fds exported with VIDIOC_EXPBUF are closed.
> 
> In calling REQBUF(0), videobuf2-core returns -EBUSY when the reference count
> of a vb2 buffer is more than 1. When dmabuf fds are not exported (usual
> V4L2_MEMORY_MMAP case), the check is no problem, but when dmabuf fds are
> exported and some of them are not closed (in other words the references to
> that memory are left), we cannot succeed in calling REQBUF(0) despite being
> able to free the memory after all the references are dropped.
> 
> Actually REQBUF(0) does not force a vb2 buffer to be freed but decreases
> the refcount of it. Also all the vb2 memory allocators that support dmabuf
> exporting (dma-contig, dma-sg, vmalloc) implements memory freeing by
> release() of dma_buf_ops, so I think there is no need to return -EBUSY when
> exporting dmabuf fds.
> 
> Could you please tell me what you think?

I think you're right. vb2 allocates the vb2_buffer and the memops-specific 
structure separately. videobuf2-core.c will free the vb2_buffer instance, but 
won't touch the memops-specific structure or the buffer memory. Both of these 
are reference-counted in the memops allocators. We could thus allow REQBUFS(0) 
to proceed even when buffers have been exported (or at least after fixing the 
small issues we'll run into, I have a feeling that this is too easy to be 
true).

Hans, Marek, any opinion on this ?

> The code that I am talking about is in
> drivers/media/v4l2-core/videobuf2-core.c:
> 
>    if (*count == 0 || q->num_buffers != 0 || q->memory != memory) {
>           /*
>            * We already have buffers allocated, so first check if they
>            * are not in use and can be freed.
>            */
>           mutex_lock(&q->mmap_lock);
>           if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
>                   mutex_unlock(&q->mmap_lock);
>                   dprintk(1, "memory in use, cannot free\n");
>                   return -EBUSY;
>           }

-- 
Regards,

Laurent Pinchart

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Hans Verkuil]

On 07/27/2016 02:57 PM, Laurent Pinchart wrote:
> Hello Kobayashi-san,
> 
> (CC'ing Hans Verkuil and Marek Szyprowski)
> 
> On Wednesday 27 Jul 2016 16:51:47 Kazunori Kobayashi wrote:
>> Hi,
>>
>> I have a question about memory freeing by calling REQBUF(0) before all the
>> dmabuf fds exported with VIDIOC_EXPBUF are closed.
>>
>> In calling REQBUF(0), videobuf2-core returns -EBUSY when the reference count
>> of a vb2 buffer is more than 1. When dmabuf fds are not exported (usual
>> V4L2_MEMORY_MMAP case), the check is no problem, but when dmabuf fds are
>> exported and some of them are not closed (in other words the references to
>> that memory are left), we cannot succeed in calling REQBUF(0) despite being
>> able to free the memory after all the references are dropped.
>>
>> Actually REQBUF(0) does not force a vb2 buffer to be freed but decreases
>> the refcount of it. Also all the vb2 memory allocators that support dmabuf
>> exporting (dma-contig, dma-sg, vmalloc) implements memory freeing by
>> release() of dma_buf_ops, so I think there is no need to return -EBUSY when
>> exporting dmabuf fds.
>>
>> Could you please tell me what you think?
> 
> I think you're right. vb2 allocates the vb2_buffer and the memops-specific 
> structure separately. videobuf2-core.c will free the vb2_buffer instance, but 
> won't touch the memops-specific structure or the buffer memory. Both of these 
> are reference-counted in the memops allocators. We could thus allow REQBUFS(0) 
> to proceed even when buffers have been exported (or at least after fixing the 
> small issues we'll run into, I have a feeling that this is too easy to be 
> true).
> 
> Hans, Marek, any opinion on this ?

What is the use-case for this? What you are doing here is to either free all
existing buffers or reallocate buffers. We can decide to rely on refcounting,
but then you would create a second set of buffers (when re-allocating) or
leave a lot of unfreed memory behind. That's pretty hard on the memory usage.

I think the EBUSY is there to protect the user against him/herself: i.e. don't
call this unless you know all refs are closed.

Given the typical large buffersizes we're talking about, I think that EBUSY
makes sense.

Regards,

	Hans

> 
>> The code that I am talking about is in
>> drivers/media/v4l2-core/videobuf2-core.c:
>>
>>    if (*count == 0 || q->num_buffers != 0 || q->memory != memory) {
>>           /*
>>            * We already have buffers allocated, so first check if they
>>            * are not in use and can be freed.
>>            */
>>           mutex_lock(&q->mmap_lock);
>>           if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
>>                   mutex_unlock(&q->mmap_lock);
>>                   dprintk(1, "memory in use, cannot free\n");
>>                   return -EBUSY;
>>           }
> 

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Laurent Pinchart]

Hi Hans,

On Monday 01 Aug 2016 12:56:55 Hans Verkuil wrote:
> On 07/27/2016 02:57 PM, Laurent Pinchart wrote:
> > On Wednesday 27 Jul 2016 16:51:47 Kazunori Kobayashi wrote:
> >> Hi,
> >> 
> >> I have a question about memory freeing by calling REQBUF(0) before all
> >> the dmabuf fds exported with VIDIOC_EXPBUF are closed.
> >> 
> >> In calling REQBUF(0), videobuf2-core returns -EBUSY when the reference
> >> count of a vb2 buffer is more than 1. When dmabuf fds are not exported
> >> (usual V4L2_MEMORY_MMAP case), the check is no problem, but when dmabuf
> >> fds are exported and some of them are not closed (in other words the
> >> references to that memory are left), we cannot succeed in calling
> >> REQBUF(0) despite being able to free the memory after all the references
> >> are dropped.
> >> 
> >> Actually REQBUF(0) does not force a vb2 buffer to be freed but decreases
> >> the refcount of it. Also all the vb2 memory allocators that support
> >> dmabuf exporting (dma-contig, dma-sg, vmalloc) implements memory freeing
> >> by release() of dma_buf_ops, so I think there is no need to return -EBUSY
> >> when exporting dmabuf fds.
> >> 
> >> Could you please tell me what you think?
> > 
> > I think you're right. vb2 allocates the vb2_buffer and the memops-specific
> > structure separately. videobuf2-core.c will free the vb2_buffer instance,
> > but won't touch the memops-specific structure or the buffer memory. Both
> > of these are reference-counted in the memops allocators. We could thus
> > allow REQBUFS(0) to proceed even when buffers have been exported (or at
> > least after fixing the small issues we'll run into, I have a feeling that
> > this is too easy to be true).
> > 
> > Hans, Marek, any opinion on this ?
> 
> What is the use-case for this? What you are doing here is to either free all
> existing buffers or reallocate buffers. We can decide to rely on
> refcounting, but then you would create a second set of buffers (when
> re-allocating) or leave a lot of unfreed memory behind. That's pretty hard
> on the memory usage.

Speaking of which, we have no way today to really limit memory usage. I wonder 
whether we should try to integrate support for resource limits in V4L2.

> I think the EBUSY is there to protect the user against him/herself: i.e.
> don't call this unless you know all refs are closed.
> 
> Given the typical large buffersizes we're talking about, I think that EBUSY
> makes sense.
>
> >> The code that I am talking about is in
> >> 
> >> drivers/media/v4l2-core/videobuf2-core.c:
> >>    if (*count == 0 || q->num_buffers != 0 || q->memory != memory) {
> >>           /*
> >>            * We already have buffers allocated, so first check if they
> >>            * are not in use and can be freed.
> >>            */
> >>           mutex_lock(&q->mmap_lock);
> >>           if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
> >>                   mutex_unlock(&q->mmap_lock);
> >>                   dprintk(1, "memory in use, cannot free\n");
> >>                   return -EBUSY;
> >>           }

-- 
Regards,

Laurent Pinchart

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Hans Verkuil]

On 08/01/2016 02:17 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 01 Aug 2016 12:56:55 Hans Verkuil wrote:
>> On 07/27/2016 02:57 PM, Laurent Pinchart wrote:
>>> On Wednesday 27 Jul 2016 16:51:47 Kazunori Kobayashi wrote:
>>>> Hi,
>>>>
>>>> I have a question about memory freeing by calling REQBUF(0) before all
>>>> the dmabuf fds exported with VIDIOC_EXPBUF are closed.
>>>>
>>>> In calling REQBUF(0), videobuf2-core returns -EBUSY when the reference
>>>> count of a vb2 buffer is more than 1. When dmabuf fds are not exported
>>>> (usual V4L2_MEMORY_MMAP case), the check is no problem, but when dmabuf
>>>> fds are exported and some of them are not closed (in other words the
>>>> references to that memory are left), we cannot succeed in calling
>>>> REQBUF(0) despite being able to free the memory after all the references
>>>> are dropped.
>>>>
>>>> Actually REQBUF(0) does not force a vb2 buffer to be freed but decreases
>>>> the refcount of it. Also all the vb2 memory allocators that support
>>>> dmabuf exporting (dma-contig, dma-sg, vmalloc) implements memory freeing
>>>> by release() of dma_buf_ops, so I think there is no need to return -EBUSY
>>>> when exporting dmabuf fds.
>>>>
>>>> Could you please tell me what you think?
>>>
>>> I think you're right. vb2 allocates the vb2_buffer and the memops-specific
>>> structure separately. videobuf2-core.c will free the vb2_buffer instance,
>>> but won't touch the memops-specific structure or the buffer memory. Both
>>> of these are reference-counted in the memops allocators. We could thus
>>> allow REQBUFS(0) to proceed even when buffers have been exported (or at
>>> least after fixing the small issues we'll run into, I have a feeling that
>>> this is too easy to be true).
>>>
>>> Hans, Marek, any opinion on this ?
>>
>> What is the use-case for this? What you are doing here is to either free all
>> existing buffers or reallocate buffers. We can decide to rely on
>> refcounting, but then you would create a second set of buffers (when
>> re-allocating) or leave a lot of unfreed memory behind. That's pretty hard
>> on the memory usage.
> 
> Speaking of which, we have no way today to really limit memory usage. I wonder 
> whether we should try to integrate support for resource limits in V4L2.

I'm opposed to that. We had drivers in the past that did that (perhaps there
are still a few old ones left), but I removed those checks. In practice this all
depends on the use-case. And if you try to allocate more buffers than there is
memory, you just get ENOMEM. Which is what it is there for.

After all, how to decide what limit to use? If someone wants to use all 32 buffers
for some reason, or allocate larger buffers than strictly needed, then they
should be able to do so. Perhaps you want to be able to capture a burst of
high quality snapshots without having to process them immediately. Or other
video streams are going to be composed into the buffers so you need to make
them larger.

The only real limits are the amount of physical (DMAable) memory and the
artificial 32 buffer limit which we already know is insufficient in
certain use-cases.

Regards,

	Hans

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Laurent Pinchart]

Hi Hans,

On Monday 01 Aug 2016 14:27:48 Hans Verkuil wrote:
> On 08/01/2016 02:17 PM, Laurent Pinchart wrote:
> > On Monday 01 Aug 2016 12:56:55 Hans Verkuil wrote:
> >> On 07/27/2016 02:57 PM, Laurent Pinchart wrote:
> >>> On Wednesday 27 Jul 2016 16:51:47 Kazunori Kobayashi wrote:
> >>>> Hi,
> >>>> 
> >>>> I have a question about memory freeing by calling REQBUF(0) before all
> >>>> the dmabuf fds exported with VIDIOC_EXPBUF are closed.
> >>>> 
> >>>> In calling REQBUF(0), videobuf2-core returns -EBUSY when the reference
> >>>> count of a vb2 buffer is more than 1. When dmabuf fds are not exported
> >>>> (usual V4L2_MEMORY_MMAP case), the check is no problem, but when dmabuf
> >>>> fds are exported and some of them are not closed (in other words the
> >>>> references to that memory are left), we cannot succeed in calling
> >>>> REQBUF(0) despite being able to free the memory after all the
> >>>> references are dropped.
> >>>> 
> >>>> Actually REQBUF(0) does not force a vb2 buffer to be freed but
> >>>> decreases the refcount of it. Also all the vb2 memory allocators that
> >>>> support dmabuf exporting (dma-contig, dma-sg, vmalloc) implements
> >>>> memory freeing by release() of dma_buf_ops, so I think there is no need
> >>>> to return -EBUSY when exporting dmabuf fds.
> >>>> 
> >>>> Could you please tell me what you think?
> >>> 
> >>> I think you're right. vb2 allocates the vb2_buffer and the
> >>> memops-specific structure separately. videobuf2-core.c will free the
> >>> vb2_buffer instance, but won't touch the memops-specific structure or
> >>> the buffer memory. Both of these are reference-counted in the memops
> >>> allocators. We could thus allow REQBUFS(0) to proceed even when buffers
> >>> have been exported (or at least after fixing the small issues we'll run
> >>> into, I have a feeling that this is too easy to be true).
> >>> 
> >>> Hans, Marek, any opinion on this ?
> >> 
> >> What is the use-case for this? What you are doing here is to either free
> >> all existing buffers or reallocate buffers. We can decide to rely on
> >> refcounting, but then you would create a second set of buffers (when
> >> re-allocating) or leave a lot of unfreed memory behind. That's pretty
> >> hard
> >> on the memory usage.
> > 
> > Speaking of which, we have no way today to really limit memory usage. I
> > wonder whether we should try to integrate support for resource limits in
> > V4L2.
>
> I'm opposed to that. We had drivers in the past that did that (perhaps there
> are still a few old ones left), but I removed those checks. In practice
> this all depends on the use-case. And if you try to allocate more buffers
> than there is memory, you just get ENOMEM. Which is what it is there for.
> 
> After all, how to decide what limit to use? If someone wants to use all 32
> buffers for some reason, or allocate larger buffers than strictly needed,
> then they should be able to do so. Perhaps you want to be able to capture a
> burst of high quality snapshots without having to process them immediately.
> Or other video streams are going to be composed into the buffers so you
> need to make them larger.
> 
> The only real limits are the amount of physical (DMAable) memory and the
> artificial 32 buffer limit which we already know is insufficient in
> certain use-cases.

When the user running V4L2 applications has full control over the system, 
perhaps, but how about shared systems where the system administrator wants to 
control resource usage per user ? Containers also come to mind, per-cgroup 
memory limits should be honoured.

And even for single-user systems, having the system start trashing because a 
random 3rd party video application decided to allocate a large number of 
buffers for no good reason provides a pretty bad user experience.

-- 
Regards,

Laurent Pinchart

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Hans Verkuil]

On 08/01/2016 03:49 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 01 Aug 2016 14:27:48 Hans Verkuil wrote:
>> On 08/01/2016 02:17 PM, Laurent Pinchart wrote:
>>> On Monday 01 Aug 2016 12:56:55 Hans Verkuil wrote:
>>>> On 07/27/2016 02:57 PM, Laurent Pinchart wrote:
>>>>> On Wednesday 27 Jul 2016 16:51:47 Kazunori Kobayashi wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I have a question about memory freeing by calling REQBUF(0) before all
>>>>>> the dmabuf fds exported with VIDIOC_EXPBUF are closed.
>>>>>>
>>>>>> In calling REQBUF(0), videobuf2-core returns -EBUSY when the reference
>>>>>> count of a vb2 buffer is more than 1. When dmabuf fds are not exported
>>>>>> (usual V4L2_MEMORY_MMAP case), the check is no problem, but when dmabuf
>>>>>> fds are exported and some of them are not closed (in other words the
>>>>>> references to that memory are left), we cannot succeed in calling
>>>>>> REQBUF(0) despite being able to free the memory after all the
>>>>>> references are dropped.
>>>>>>
>>>>>> Actually REQBUF(0) does not force a vb2 buffer to be freed but
>>>>>> decreases the refcount of it. Also all the vb2 memory allocators that
>>>>>> support dmabuf exporting (dma-contig, dma-sg, vmalloc) implements
>>>>>> memory freeing by release() of dma_buf_ops, so I think there is no need
>>>>>> to return -EBUSY when exporting dmabuf fds.
>>>>>>
>>>>>> Could you please tell me what you think?
>>>>>
>>>>> I think you're right. vb2 allocates the vb2_buffer and the
>>>>> memops-specific structure separately. videobuf2-core.c will free the
>>>>> vb2_buffer instance, but won't touch the memops-specific structure or
>>>>> the buffer memory. Both of these are reference-counted in the memops
>>>>> allocators. We could thus allow REQBUFS(0) to proceed even when buffers
>>>>> have been exported (or at least after fixing the small issues we'll run
>>>>> into, I have a feeling that this is too easy to be true).
>>>>>
>>>>> Hans, Marek, any opinion on this ?
>>>>
>>>> What is the use-case for this? What you are doing here is to either free
>>>> all existing buffers or reallocate buffers. We can decide to rely on
>>>> refcounting, but then you would create a second set of buffers (when
>>>> re-allocating) or leave a lot of unfreed memory behind. That's pretty
>>>> hard
>>>> on the memory usage.
>>>
>>> Speaking of which, we have no way today to really limit memory usage. I
>>> wonder whether we should try to integrate support for resource limits in
>>> V4L2.
>>
>> I'm opposed to that. We had drivers in the past that did that (perhaps there
>> are still a few old ones left), but I removed those checks. In practice
>> this all depends on the use-case. And if you try to allocate more buffers
>> than there is memory, you just get ENOMEM. Which is what it is there for.
>>
>> After all, how to decide what limit to use? If someone wants to use all 32
>> buffers for some reason, or allocate larger buffers than strictly needed,
>> then they should be able to do so. Perhaps you want to be able to capture a
>> burst of high quality snapshots without having to process them immediately.
>> Or other video streams are going to be composed into the buffers so you
>> need to make them larger.
>>
>> The only real limits are the amount of physical (DMAable) memory and the
>> artificial 32 buffer limit which we already know is insufficient in
>> certain use-cases.
> 
> When the user running V4L2 applications has full control over the system, 
> perhaps, but how about shared systems where the system administrator wants to 
> control resource usage per user ? Containers also come to mind, per-cgroup 
> memory limits should be honoured.

If an administrator explicitly restricts memory usage, then it would make sense
to honor that. Are those checks perhaps already done deep in the mm code? I've
no idea what would be involved.

> And even for single-user systems, having the system start trashing because a 
> random 3rd party video application decided to allocate a large number of 
> buffers for no good reason provides a pretty bad user experience.
> 

How would you know it is 'for no good reason'? Like any other application if it
uses too much memory either don't use it or fix it. It's not up to the kernel
to limit this arbitrarily.

My opinion, of course.

Regards,

	Hans

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Laurent Pinchart]

Hi Hans,

On Monday 01 Aug 2016 15:59:54 Hans Verkuil wrote:
> On 08/01/2016 03:49 PM, Laurent Pinchart wrote:
> > On Monday 01 Aug 2016 14:27:48 Hans Verkuil wrote:
> >> On 08/01/2016 02:17 PM, Laurent Pinchart wrote:
> >>> On Monday 01 Aug 2016 12:56:55 Hans Verkuil wrote:
> >>>> On 07/27/2016 02:57 PM, Laurent Pinchart wrote:
> >>>>> On Wednesday 27 Jul 2016 16:51:47 Kazunori Kobayashi wrote:
> >>>>>> Hi,
> >>>>>> 
> >>>>>> I have a question about memory freeing by calling REQBUF(0) before
> >>>>>> all the dmabuf fds exported with VIDIOC_EXPBUF are closed.
> >>>>>> 
> >>>>>> In calling REQBUF(0), videobuf2-core returns -EBUSY when the
> >>>>>> reference count of a vb2 buffer is more than 1. When dmabuf fds are
> >>>>>> not exported (usual V4L2_MEMORY_MMAP case), the check is no problem,
> >>>>>> but when dmabuf fds are exported and some of them are not closed (in
> >>>>>> other words the references to that memory are left), we cannot
> >>>>>> succeed in calling REQBUF(0) despite being able to free the memory
> >>>>>> after all the references are dropped.
> >>>>>> 
> >>>>>> Actually REQBUF(0) does not force a vb2 buffer to be freed but
> >>>>>> decreases the refcount of it. Also all the vb2 memory allocators that
> >>>>>> support dmabuf exporting (dma-contig, dma-sg, vmalloc) implements
> >>>>>> memory freeing by release() of dma_buf_ops, so I think there is no
> >>>>>> need to return -EBUSY when exporting dmabuf fds.
> >>>>>> 
> >>>>>> Could you please tell me what you think?
> >>>>> 
> >>>>> I think you're right. vb2 allocates the vb2_buffer and the
> >>>>> memops-specific structure separately. videobuf2-core.c will free the
> >>>>> vb2_buffer instance, but won't touch the memops-specific structure or
> >>>>> the buffer memory. Both of these are reference-counted in the memops
> >>>>> allocators. We could thus allow REQBUFS(0) to proceed even when
> >>>>> buffers have been exported (or at least after fixing the small issues
> >>>>> we'll run into, I have a feeling that this is too easy to be true).
> >>>>> 
> >>>>> Hans, Marek, any opinion on this ?
> >>>> 
> >>>> What is the use-case for this? What you are doing here is to either
> >>>> free all existing buffers or reallocate buffers. We can decide to rely
> >>>> on refcounting, but then you would create a second set of buffers (when
> >>>> re-allocating) or leave a lot of unfreed memory behind. That's pretty
> >>>> hard on the memory usage.
> >>> 
> >>> Speaking of which, we have no way today to really limit memory usage. I
> >>> wonder whether we should try to integrate support for resource limits in
> >>> V4L2.
> >> 
> >> I'm opposed to that. We had drivers in the past that did that (perhaps
> >> there are still a few old ones left), but I removed those checks. In
> >> practice this all depends on the use-case. And if you try to allocate
> >> more buffers than there is memory, you just get ENOMEM. Which is what it
> >> is there for.
> >> 
> >> After all, how to decide what limit to use? If someone wants to use all
> >> 32 buffers for some reason, or allocate larger buffers than strictly
> >> needed, then they should be able to do so. Perhaps you want to be able to
> >> capture a burst of high quality snapshots without having to process them
> >> immediately. Or other video streams are going to be composed into the
> >> buffers so you need to make them larger.
> >> 
> >> The only real limits are the amount of physical (DMAable) memory and the
> >> artificial 32 buffer limit which we already know is insufficient in
> >> certain use-cases.
> > 
> > When the user running V4L2 applications has full control over the system,
> > perhaps, but how about shared systems where the system administrator wants
> > to control resource usage per user ? Containers also come to mind,
> > per-cgroup memory limits should be honoured.
> 
> If an administrator explicitly restricts memory usage, then it would make
> sense to honor that. Are those checks perhaps already done deep in the mm
> code? I've no idea what would be involved.

To be honest, I haven't checked.

> > And even for single-user systems, having the system start trashing because
> > a random 3rd party video application decided to allocate a large number
> > of buffers for no good reason provides a pretty bad user experience.
> 
> How would you know it is 'for no good reason'?

That's a good question. On one extreme an application trying to allocate 32 
50MB buffers would seem suspicious to me, but on the other hand it would be 
difficult to answer the question in a way that can be translated into code.

One thing we should perhaps do, though, is make sure that drivers limit the 
size of the buffers being allocated. If a device resolution is limited to VGA 
images, it makes little sense to allow VIDIOC_CREATE_BUF to allocate 20MB 
buffers. There could perhaps be use cases though, when sharing a buffer 
between devices.

> Like any other application if it uses too much memory either don't use it or
> fix it. It's not up to the kernel to limit this arbitrarily.
>
> My opinion, of course.

A valued opinion :-)

-- 
Regards,

Laurent Pinchart

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Steven Toth]

> That's a good question. On one extreme an application trying to allocate 32
> 50MB buffers would seem suspicious to me, but on the other hand it would be
> difficult to answer the question in a way that can be translated into code.

8k video in the ARGB 8bit 4:4:4 colorspace, would need a 126MB per frame buffer.

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Laurent Pinchart]

On Monday 01 Aug 2016 12:16:25 Steven Toth wrote:
> > That's a good question. On one extreme an application trying to allocate
> > 32 50MB buffers would seem suspicious to me, but on the other hand it
> > would be difficult to answer the question in a way that can be translated
> > into code.
> 
> 8k video in the ARGB 8bit 4:4:4 colorspace, would need a 126MB per frame
> buffer.

Hopefully not 32 of them though.

There's no easy way to compute an accurate limit, but maybe we could do 
improve the current situation with some heuristics based on the maximum 
resolution a device can capture.

-- 
Regards,

Laurent Pinchart

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Damian Hobson-Garcia]

Hi Hans,

On 2016-08-01 7:56 PM, Hans Verkuil wrote:
> 
> 
> On 07/27/2016 02:57 PM, Laurent Pinchart wrote:
>> Hello Kobayashi-san,
>>
>> (CC'ing Hans Verkuil and Marek Szyprowski)
>>
>> On Wednesday 27 Jul 2016 16:51:47 Kazunori Kobayashi wrote:
>>> Hi,
>>>
>>> I have a question about memory freeing by calling REQBUF(0) before all the
>>> dmabuf fds exported with VIDIOC_EXPBUF are closed.
>>>
>>> In calling REQBUF(0), videobuf2-core returns -EBUSY when the reference count
>>> of a vb2 buffer is more than 1. When dmabuf fds are not exported (usual
>>> V4L2_MEMORY_MMAP case), the check is no problem, but when dmabuf fds are
>>> exported and some of them are not closed (in other words the references to
>>> that memory are left), we cannot succeed in calling REQBUF(0) despite being
>>> able to free the memory after all the references are dropped.
>>>
>>> Actually REQBUF(0) does not force a vb2 buffer to be freed but decreases
>>> the refcount of it. Also all the vb2 memory allocators that support dmabuf
>>> exporting (dma-contig, dma-sg, vmalloc) implements memory freeing by
>>> release() of dma_buf_ops, so I think there is no need to return -EBUSY when
>>> exporting dmabuf fds.
>>>
>>> Could you please tell me what you think?
>>
>> I think you're right. vb2 allocates the vb2_buffer and the memops-specific 
>> structure separately. videobuf2-core.c will free the vb2_buffer instance, but 
>> won't touch the memops-specific structure or the buffer memory. Both of these 
>> are reference-counted in the memops allocators. We could thus allow REQBUFS(0) 
>> to proceed even when buffers have been exported (or at least after fixing the 
>> small issues we'll run into, I have a feeling that this is too easy to be 
>> true).
>>
>> Hans, Marek, any opinion on this ?
> 
> What is the use-case for this? What you are doing here is to either free all
> existing buffers or reallocate buffers. We can decide to rely on refcounting,
> but then you would create a second set of buffers (when re-allocating) or
> leave a lot of unfreed memory behind. That's pretty hard on the memory usage.

One use case is performing a buffer resize while actively streaming
buffers to the display.  It seems that it would be useful to re-allocate
new buffers (which will have some different properties) while one of the
old buffers is displayed on screen. When the new buffer replaces the old
one on screen, the old buffer(s) can be released, freeing the memory
while the resize appears seamless at the display.  This avoids the rush
to re-allocate and fill the first buffer after a resize with valid data
before the next vblank, which can cause flicker when it fails.

This method requires the application to properly close the dmabuf
handles when the buffers come off the screen, but Wayland, for example,
already provides a notification mechanism to achieve this.

Thanks,
Damian

> 
> I think the EBUSY is there to protect the user against him/herself: i.e. don't
> call this unless you know all refs are closed.
> 
> Given the typical large buffersizes we're talking about, I think that EBUSY
> makes sense.
> 
> Regards,
> 
> 	Hans
> 
>>
>>> The code that I am talking about is in
>>> drivers/media/v4l2-core/videobuf2-core.c:
>>>
>>>    if (*count == 0 || q->num_buffers != 0 || q->memory != memory) {
>>>           /*
>>>            * We already have buffers allocated, so first check if they
>>>            * are not in use and can be freed.
>>>            */
>>>           mutex_lock(&q->mmap_lock);
>>>           if (q->memory == VB2_MEMORY_MMAP && __buffers_in_use(q)) {
>>>                   mutex_unlock(&q->mmap_lock);
>>>                   dprintk(1, "memory in use, cannot free\n");
>>>                   return -EBUSY;
>>>           }
>>

Re: Memory freeing when dmabuf fds are exported with VIDIOC_EXPBUF

[Nicolas Dufresne]

I'd like to revive this discussion.

Le lundi 01 août 2016 à 12:56 +0200, Hans Verkuil a écrit :
> > 
> > Hans, Marek, any opinion on this ?
> 
> What is the use-case for this? What you are doing here is to either free all
> existing buffers or reallocate buffers. We can decide to rely on refcounting,
> but then you would create a second set of buffers (when re-allocating) or
> leave a lot of unfreed memory behind. That's pretty hard on the memory usage.
> 
> I think the EBUSY is there to protect the user against him/herself: i.e. don't
> call this unless you know all refs are closed.
> 
> Given the typical large buffersizes we're talking about, I think that EBUSY
> makes sense.

This is a userspace hell for the use case of seamless resolution
change. Let's say I'm rendering buffers from a V4L2 camera toward my
display KMS driver. While I'm streaming, the KMS driver will hold on
the last frame. This is required when your display is sourcing data
directly from your DMABuf, because the KMS render are not synchronized
with the V4L2 camera (you could have 24fps camera over a 60fps
display).

When its time to change the resolution, the fact that we can't let go
the DMABuf means that we need to reclaim the memory from KMS first. We
can't just take it back, we need to allocate a new buffer, copy using
the CPU that buffer data, setupe the DMABuf reference. that new buffer
for redraw and then releas

This operation is extremely slow, since it requires an allocation and a
CPU copy of the data. This is only needed because V4L2 is trying to
prevent over allocation. In this case, userspace is only holding on 1
of the frames, this is far from the dramatic memory waste that we are
describing here.

regards,
Nicolas