[videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Aguirre, Sergio]

Hi all,

While working on an old port of the omap3 camera-isp driver,
I have faced some problem.

Basically, when calling VIDIOC_REQBUFS with a certain buffer
Count, we had a software limit for total size, calculated depending on:

  Total bytesize = bytesperline x height x count

So, we had an arbitrary limit to, say 32 MB, which was generic.

Now, we want to condition it ONLY when MMAP buffers will be used.
Meaning, we don't want to keep that policy when the kernel is not
allocating the space

But the thing is that, according to videobuf documentation, buf_setup is
the one who should put a RAM usage limit. BUT the memory type passed to
reqbufs is not propagated to buf_setup, therefore forcing me to go to a
non-standard memory limitation in my reqbufs callback function, instead
of doing it properly inside buf_setup.

Is this scenario a good consideration to change buf_setup API, and
propagate buffers memory type aswell?

I'll appreciate your inputs on this matter.

Regards,
Sergio

Re: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Mauro Carvalho Chehab]

Aguirre, Sergio wrote:
> Hi all,
> 
> While working on an old port of the omap3 camera-isp driver,
> I have faced some problem.
> 
> Basically, when calling VIDIOC_REQBUFS with a certain buffer
> Count, we had a software limit for total size, calculated depending on:
> 
>   Total bytesize = bytesperline x height x count
> 
> So, we had an arbitrary limit to, say 32 MB, which was generic.
> 
> Now, we want to condition it ONLY when MMAP buffers will be used.
> Meaning, we don't want to keep that policy when the kernel is not
> allocating the space
> 
> But the thing is that, according to videobuf documentation, buf_setup is
> the one who should put a RAM usage limit. BUT the memory type passed to
> reqbufs is not propagated to buf_setup, therefore forcing me to go to a
> non-standard memory limitation in my reqbufs callback function, instead
> of doing it properly inside buf_setup.
> 
> Is this scenario a good consideration to change buf_setup API, and
> propagate buffers memory type aswell?

I don't see any problem on propagating the memory type to buffer_setup, if
this is really needed. Yet, I can't see why you would restrict the buffer
size to 32 MB on one case, and not restrict the size at all with non-MMAP
types.

> I'll appreciate your inputs on this matter.
> 
> Regards,
> Sergio
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro

RE: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Aguirre, Sergio]

> -----Original Message-----
> From: Mauro Carvalho Chehab [mailto:mchehab@redhat.com]
> Sent: Wednesday, May 05, 2010 6:24 PM
> To: Aguirre, Sergio
> Cc: linux-media@vger.kernel.org
> Subject: Re: [videobuf] Query: Condition bytesize limit in
> videobuf_reqbufs -> buf_setup() call?
> 
> Aguirre, Sergio wrote:
> > Hi all,
> >
> > While working on an old port of the omap3 camera-isp driver,
> > I have faced some problem.
> >
> > Basically, when calling VIDIOC_REQBUFS with a certain buffer
> > Count, we had a software limit for total size, calculated depending on:
> >
> >   Total bytesize = bytesperline x height x count
> >
> > So, we had an arbitrary limit to, say 32 MB, which was generic.
> >
> > Now, we want to condition it ONLY when MMAP buffers will be used.
> > Meaning, we don't want to keep that policy when the kernel is not
> > allocating the space
> >
> > But the thing is that, according to videobuf documentation, buf_setup is
> > the one who should put a RAM usage limit. BUT the memory type passed to
> > reqbufs is not propagated to buf_setup, therefore forcing me to go to a
> > non-standard memory limitation in my reqbufs callback function, instead
> > of doing it properly inside buf_setup.
> >
> > Is this scenario a good consideration to change buf_setup API, and
> > propagate buffers memory type aswell?
> 
> I don't see any problem on propagating the memory type to buffer_setup, if
> this is really needed. Yet, I can't see why you would restrict the buffer
> size to 32 MB on one case, and not restrict the size at all with non-MMAP
> types.

Ok, my reason for doing that is because I thought that there should be a memory limit in whichever place you're doing the buffer allocations.

MMAP is allocating buffers in kernel, so kernel should provide a memory restriction, if applies.

USERPTR is allocating buffers in userspace, so userspace should provide a memory restriction, if applies.

Please correct me if my reasoning is not correct.

Regards,
Sergio

> 
> > I'll appreciate your inputs on this matter.
> >
> > Regards,
> > Sergio
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> 
> Cheers,
> Mauro

Re: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Mauro Carvalho Chehab]

Aguirre, Sergio wrote:
> 
>> -----Original Message-----
>> From: Mauro Carvalho Chehab [mailto:mchehab@redhat.com]
>> Sent: Wednesday, May 05, 2010 6:24 PM
>> To: Aguirre, Sergio
>> Cc: linux-media@vger.kernel.org
>> Subject: Re: [videobuf] Query: Condition bytesize limit in
>> videobuf_reqbufs -> buf_setup() call?
>>
>> Aguirre, Sergio wrote:
>>> Hi all,
>>>
>>> While working on an old port of the omap3 camera-isp driver,
>>> I have faced some problem.
>>>
>>> Basically, when calling VIDIOC_REQBUFS with a certain buffer
>>> Count, we had a software limit for total size, calculated depending on:
>>>
>>>   Total bytesize = bytesperline x height x count
>>>
>>> So, we had an arbitrary limit to, say 32 MB, which was generic.
>>>
>>> Now, we want to condition it ONLY when MMAP buffers will be used.
>>> Meaning, we don't want to keep that policy when the kernel is not
>>> allocating the space
>>>
>>> But the thing is that, according to videobuf documentation, buf_setup is
>>> the one who should put a RAM usage limit. BUT the memory type passed to
>>> reqbufs is not propagated to buf_setup, therefore forcing me to go to a
>>> non-standard memory limitation in my reqbufs callback function, instead
>>> of doing it properly inside buf_setup.
>>>
>>> Is this scenario a good consideration to change buf_setup API, and
>>> propagate buffers memory type aswell?
>> I don't see any problem on propagating the memory type to buffer_setup, if
>> this is really needed. Yet, I can't see why you would restrict the buffer
>> size to 32 MB on one case, and not restrict the size at all with non-MMAP
>> types.
> 
> Ok, my reason for doing that is because I thought that there should be a memory limit in whichever place you're doing the buffer allocations.
> 
> MMAP is allocating buffers in kernel, so kernel should provide a memory restriction, if applies.
> 
> USERPTR is allocating buffers in userspace, so userspace should provide a memory restriction, if applies.
> 
> Please correct me if my reasoning is not correct.
>

Your rationale makes some sense.

For the effects of the remaining discussion, let's forget about videobuf-vmalloc,
as I'm assuming your troubles are with a driver using videobuf-contig 
or videobuf-dma_sg.

With all memory types, kernel will need to command DMA transfers to those 
buffers. It still keeps sense to have a restriction on kernelspace, 
to avoid, for example, the risk of  userspace to fill all DMA capable 
memory with video buffers, preventing its usage by other subsystems 
(usb, disk, net, etc). So, such limit should still be enforced in kernel, 
otherwise, you may open an space for DoS attacks.

The limit between read(), MMAP, USERPTR and OVERLAY might eventually be different, but
I can't see why you might want to put different limits for each case, as the resource
that this check is protecting is the DMA-capable memory area. No matter how you're allocating
it, you'll need to enforce the same degree of protection.

-- 

Cheers,
Mauro

Re: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Laurent Pinchart]

Hi Sergio,

On Thursday 06 May 2010 01:29:54 Aguirre, Sergio wrote:
> > -----Original Message-----
> > From: Mauro Carvalho Chehab [mailto:mchehab@redhat.com]
> > Sent: Wednesday, May 05, 2010 6:24 PM
> > To: Aguirre, Sergio
> > Cc: linux-media@vger.kernel.org
> > Subject: Re: [videobuf] Query: Condition bytesize limit in
> > videobuf_reqbufs -> buf_setup() call?
> > 
> > Aguirre, Sergio wrote:
> > > Hi all,
> > > 
> > > While working on an old port of the omap3 camera-isp driver,
> > > I have faced some problem.
> > > 
> > > Basically, when calling VIDIOC_REQBUFS with a certain buffer
> > > 
> > > Count, we had a software limit for total size, calculated depending on:
> > >   Total bytesize = bytesperline x height x count
> > > 
> > > So, we had an arbitrary limit to, say 32 MB, which was generic.
> > > 
> > > Now, we want to condition it ONLY when MMAP buffers will be used.
> > > Meaning, we don't want to keep that policy when the kernel is not
> > > allocating the space
> > > 
> > > But the thing is that, according to videobuf documentation, buf_setup
> > > is the one who should put a RAM usage limit. BUT the memory type
> > > passed to reqbufs is not propagated to buf_setup, therefore forcing me
> > > to go to a non-standard memory limitation in my reqbufs callback
> > > function, instead of doing it properly inside buf_setup.
> > > 
> > > Is this scenario a good consideration to change buf_setup API, and
> > > propagate buffers memory type aswell?
> > 
> > I don't see any problem on propagating the memory type to buffer_setup,
> > if this is really needed. Yet, I can't see why you would restrict the
> > buffer size to 32 MB on one case, and not restrict the size at all with
> > non-MMAP types.
> 
> Ok, my reason for doing that is because I thought that there should be a
> memory limit in whichever place you're doing the buffer allocations.
> 
> MMAP is allocating buffers in kernel, so kernel should provide a memory
> restriction, if applies.
> 
> USERPTR is allocating buffers in userspace, so userspace should provide a
> memory restriction, if applies.

I agree with the intend here, but not with the current implementation which 
has a hardcoded arbitrary limit. Do you think it would be possible to compute 
a meaningful default limit in the V4L2 core, with a way for userspace to 
modify it (with root privileges of course) ?

-- 
Regards,

Laurent Pinchart

Re: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Mauro Carvalho Chehab]

Laurent Pinchart wrote:
> Hi Sergio,
> 
> On Thursday 06 May 2010 01:29:54 Aguirre, Sergio wrote:
>>> -----Original Message-----
>>> From: Mauro Carvalho Chehab [mailto:mchehab@redhat.com]
>>> Sent: Wednesday, May 05, 2010 6:24 PM
>>> To: Aguirre, Sergio
>>> Cc: linux-media@vger.kernel.org
>>> Subject: Re: [videobuf] Query: Condition bytesize limit in
>>> videobuf_reqbufs -> buf_setup() call?
>>>
>>> Aguirre, Sergio wrote:
>>>> Hi all,
>>>>
>>>> While working on an old port of the omap3 camera-isp driver,
>>>> I have faced some problem.
>>>>
>>>> Basically, when calling VIDIOC_REQBUFS with a certain buffer
>>>>
>>>> Count, we had a software limit for total size, calculated depending on:
>>>>   Total bytesize = bytesperline x height x count
>>>>
>>>> So, we had an arbitrary limit to, say 32 MB, which was generic.
>>>>
>>>> Now, we want to condition it ONLY when MMAP buffers will be used.
>>>> Meaning, we don't want to keep that policy when the kernel is not
>>>> allocating the space
>>>>
>>>> But the thing is that, according to videobuf documentation, buf_setup
>>>> is the one who should put a RAM usage limit. BUT the memory type
>>>> passed to reqbufs is not propagated to buf_setup, therefore forcing me
>>>> to go to a non-standard memory limitation in my reqbufs callback
>>>> function, instead of doing it properly inside buf_setup.
>>>>
>>>> Is this scenario a good consideration to change buf_setup API, and
>>>> propagate buffers memory type aswell?
>>> I don't see any problem on propagating the memory type to buffer_setup,
>>> if this is really needed. Yet, I can't see why you would restrict the
>>> buffer size to 32 MB on one case, and not restrict the size at all with
>>> non-MMAP types.
>> Ok, my reason for doing that is because I thought that there should be a
>> memory limit in whichever place you're doing the buffer allocations.
>>
>> MMAP is allocating buffers in kernel, so kernel should provide a memory
>> restriction, if applies.
>>
>> USERPTR is allocating buffers in userspace, so userspace should provide a
>> memory restriction, if applies.
> 
> I agree with the intend here, but not with the current implementation which 
> has a hardcoded arbitrary limit. Do you think it would be possible to compute 
> a meaningful default limit in the V4L2 core, with a way for userspace to 
> modify it (with root privileges of course) ?
> 
On almost all drivers, the limit is not arbitrary. It is a reasonable number
of buffers (like 16 buffers). A limit in terms of the number of buffers is
meaningful for V4L2 API, and also, has a "physical meaning": considering that
almost all drivers that use videobuf can do at maximum 30 fps, 16 buffers mean 
that the maximum delay that the driver will apply to the stream is 533 ms.

Some drivers even provide a modprobe parameter to allow changing this limit
(for example, bttv allows changing it up to 32 buffers), but only during
module load time. I can't foresee any use case where this maximum limit 
would need to be dynamically adjusted. Root can always change it by removing 
and re-inserting the module with a new maximum size.

-- 

Cheers,
Mauro

Re: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Laurent Pinchart]

Hi Mauro,

On Thursday 06 May 2010 14:38:36 Mauro Carvalho Chehab wrote:
> Laurent Pinchart wrote:
> > On Thursday 06 May 2010 01:29:54 Aguirre, Sergio wrote:
> >>> -----Original Message-----
> >>> From: Mauro Carvalho Chehab [mailto:mchehab@redhat.com]
> >>> Sent: Wednesday, May 05, 2010 6:24 PM
> >>> To: Aguirre, Sergio
> >>> Cc: linux-media@vger.kernel.org
> >>> Subject: Re: [videobuf] Query: Condition bytesize limit in
> >>> videobuf_reqbufs -> buf_setup() call?
> >>> 
> >>> Aguirre, Sergio wrote:
> >>>> Hi all,
> >>>> 
> >>>> While working on an old port of the omap3 camera-isp driver,
> >>>> I have faced some problem.
> >>>> 
> >>>> Basically, when calling VIDIOC_REQBUFS with a certain buffer
> >>>> 
> >>>> Count, we had a software limit for total size, calculated depending on:
> >>>>   Total bytesize = bytesperline x height x count
> >>>> 
> >>>> So, we had an arbitrary limit to, say 32 MB, which was generic.
> >>>> 
> >>>> Now, we want to condition it ONLY when MMAP buffers will be used.
> >>>> Meaning, we don't want to keep that policy when the kernel is not
> >>>> allocating the space
> >>>> 
> >>>> But the thing is that, according to videobuf documentation, buf_setup
> >>>> is the one who should put a RAM usage limit. BUT the memory type
> >>>> passed to reqbufs is not propagated to buf_setup, therefore forcing me
> >>>> to go to a non-standard memory limitation in my reqbufs callback
> >>>> function, instead of doing it properly inside buf_setup.
> >>>> 
> >>>> Is this scenario a good consideration to change buf_setup API, and
> >>>> propagate buffers memory type aswell?
> >>> 
> >>> I don't see any problem on propagating the memory type to buffer_setup,
> >>> if this is really needed. Yet, I can't see why you would restrict the
> >>> buffer size to 32 MB on one case, and not restrict the size at all with
> >>> non-MMAP types.
> >> 
> >> Ok, my reason for doing that is because I thought that there should be a
> >> memory limit in whichever place you're doing the buffer allocations.
> >> 
> >> MMAP is allocating buffers in kernel, so kernel should provide a memory
> >> restriction, if applies.
> >> 
> >> USERPTR is allocating buffers in userspace, so userspace should provide
> >> a memory restriction, if applies.
> > 
> > I agree with the intend here, but not with the current implementation
> > which has a hardcoded arbitrary limit. Do you think it would be possible
> > to compute a meaningful default limit in the V4L2 core, with a way for
> > userspace to modify it (with root privileges of course) ?
> 
> On almost all drivers, the limit is not arbitrary. It is a reasonable
> number of buffers (like 16 buffers). A limit in terms of the number of
> buffers is meaningful for V4L2 API, and also, has a "physical meaning":
> considering that almost all drivers that use videobuf can do at maximum 30
> fps, 16 buffers mean that the maximum delay that the driver will apply to
> the stream is 533 ms.
> 
> Some drivers even provide a modprobe parameter to allow changing this limit
> (for example, bttv allows changing it up to 32 buffers), but only during
> module load time. I can't foresee any use case where this maximum limit
> would need to be dynamically adjusted. Root can always change it by
> removing and re-inserting the module with a new maximum size.

I wasn't talking about the limit on the number of buffers, but on the amount 
of memory. That's what Sergio was mentioning, and that's what is done in the 
OMAP3 ISP driver.

-- 
Regards,

Laurent Pinchart

Re: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Mauro Carvalho Chehab]

Laurent Pinchart wrote:
> Hi Mauro,
> 
> On Thursday 06 May 2010 14:38:36 Mauro Carvalho Chehab wrote:
>> Laurent Pinchart wrote:
>>> On Thursday 06 May 2010 01:29:54 Aguirre, Sergio wrote:
>>>>> -----Original Message-----
>>>>> From: Mauro Carvalho Chehab [mailto:mchehab@redhat.com]
>>>>> Sent: Wednesday, May 05, 2010 6:24 PM
>>>>> To: Aguirre, Sergio
>>>>> Cc: linux-media@vger.kernel.org
>>>>> Subject: Re: [videobuf] Query: Condition bytesize limit in
>>>>> videobuf_reqbufs -> buf_setup() call?
>>>>>
>>>>> Aguirre, Sergio wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> While working on an old port of the omap3 camera-isp driver,
>>>>>> I have faced some problem.
>>>>>>
>>>>>> Basically, when calling VIDIOC_REQBUFS with a certain buffer
>>>>>>
>>>>>> Count, we had a software limit for total size, calculated depending on:
>>>>>>   Total bytesize = bytesperline x height x count
>>>>>>
>>>>>> So, we had an arbitrary limit to, say 32 MB, which was generic.
>>>>>>
>>>>>> Now, we want to condition it ONLY when MMAP buffers will be used.
>>>>>> Meaning, we don't want to keep that policy when the kernel is not
>>>>>> allocating the space
>>>>>>
>>>>>> But the thing is that, according to videobuf documentation, buf_setup
>>>>>> is the one who should put a RAM usage limit. BUT the memory type
>>>>>> passed to reqbufs is not propagated to buf_setup, therefore forcing me
>>>>>> to go to a non-standard memory limitation in my reqbufs callback
>>>>>> function, instead of doing it properly inside buf_setup.
>>>>>>
>>>>>> Is this scenario a good consideration to change buf_setup API, and
>>>>>> propagate buffers memory type aswell?
>>>>> I don't see any problem on propagating the memory type to buffer_setup,
>>>>> if this is really needed. Yet, I can't see why you would restrict the
>>>>> buffer size to 32 MB on one case, and not restrict the size at all with
>>>>> non-MMAP types.
>>>> Ok, my reason for doing that is because I thought that there should be a
>>>> memory limit in whichever place you're doing the buffer allocations.
>>>>
>>>> MMAP is allocating buffers in kernel, so kernel should provide a memory
>>>> restriction, if applies.
>>>>
>>>> USERPTR is allocating buffers in userspace, so userspace should provide
>>>> a memory restriction, if applies.
>>> I agree with the intend here, but not with the current implementation
>>> which has a hardcoded arbitrary limit. Do you think it would be possible
>>> to compute a meaningful default limit in the V4L2 core, with a way for
>>> userspace to modify it (with root privileges of course) ?
>> On almost all drivers, the limit is not arbitrary. It is a reasonable
>> number of buffers (like 16 buffers). A limit in terms of the number of
>> buffers is meaningful for V4L2 API, and also, has a "physical meaning":
>> considering that almost all drivers that use videobuf can do at maximum 30
>> fps, 16 buffers mean that the maximum delay that the driver will apply to
>> the stream is 533 ms.
>>
>> Some drivers even provide a modprobe parameter to allow changing this limit
>> (for example, bttv allows changing it up to 32 buffers), but only during
>> module load time. I can't foresee any use case where this maximum limit
>> would need to be dynamically adjusted. Root can always change it by
>> removing and re-inserting the module with a new maximum size.
> 
> I wasn't talking about the limit on the number of buffers, but on the amount 
> of memory. That's what Sergio was mentioning, and that's what is done in the 
> OMAP3 ISP driver.

The memory consumption is basically dictated by the maximum size of an image
(resolution x bpp / 8) and the number of buffers. An arbitrary value in terms
of megabytes is meaningless: it is just a random number above some reasonable limit.

The proper way to limit memory is to do something like:

#define MAX_HRES	1920
#define MAX_VRES	1650
#define MAX_DEPTH	3
#define MAX_BUFS	4

#define MAX_MEMSIZE	(MAX_HRES * MAX_VRES * MAX_DEPTH * MAX_BUFS)

buf_setup(...)
{
	if (size > MAX_MEMSIZE)
		fix_it_by_reducing_number_of_buffers();
}

-- 

Cheers,
Mauro

Re: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Laurent Pinchart]

Hi Mauro,

On Thursday 06 May 2010 15:23:10 Mauro Carvalho Chehab wrote:
> Laurent Pinchart wrote:
> > On Thursday 06 May 2010 14:38:36 Mauro Carvalho Chehab wrote:
> >> Laurent Pinchart wrote:
> >>> On Thursday 06 May 2010 01:29:54 Aguirre, Sergio wrote:
> >>>>> -----Original Message-----
> >>>>> From: Mauro Carvalho Chehab [mailto:mchehab@redhat.com]
> >>>>> Sent: Wednesday, May 05, 2010 6:24 PM
> >>>>> To: Aguirre, Sergio
> >>>>> Cc: linux-media@vger.kernel.org
> >>>>> Subject: Re: [videobuf] Query: Condition bytesize limit in
> >>>>> videobuf_reqbufs -> buf_setup() call?

[snip]

> >>>>> I don't see any problem on propagating the memory type to
> >>>>> buffer_setup, if this is really needed. Yet, I can't see why you
> >>>>> would restrict the buffer size to 32 MB on one case, and not
> >>>>> restrict the size at all with non-MMAP types.
> >>>> 
> >>>> Ok, my reason for doing that is because I thought that there should be
> >>>> a memory limit in whichever place you're doing the buffer
> >>>> allocations.
> >>>> 
> >>>> MMAP is allocating buffers in kernel, so kernel should provide a
> >>>> memory restriction, if applies.
> >>>> 
> >>>> USERPTR is allocating buffers in userspace, so userspace should
> >>>> provide a memory restriction, if applies.
> >>> 
> >>> I agree with the intend here, but not with the current implementation
> >>> which has a hardcoded arbitrary limit. Do you think it would be
> >>> possible to compute a meaningful default limit in the V4L2 core, with
> >>> a way for userspace to modify it (with root privileges of course) ?
> >> 
> >> On almost all drivers, the limit is not arbitrary. It is a reasonable
> >> number of buffers (like 16 buffers). A limit in terms of the number of
> >> buffers is meaningful for V4L2 API, and also, has a "physical meaning":
> >> considering that almost all drivers that use videobuf can do at maximum
> >> 30 fps, 16 buffers mean that the maximum delay that the driver will
> >> apply to the stream is 533 ms.
> >> 
> >> Some drivers even provide a modprobe parameter to allow changing this
> >> limit (for example, bttv allows changing it up to 32 buffers), but only
> >> during module load time. I can't foresee any use case where this
> >> maximum limit would need to be dynamically adjusted. Root can always
> >> change it by removing and re-inserting the module with a new maximum
> >> size.
> > 
> > I wasn't talking about the limit on the number of buffers, but on the
> > amount of memory. That's what Sergio was mentioning, and that's what is
> > done in the OMAP3 ISP driver.
> 
> The memory consumption is basically dictated by the maximum size of an
> image (resolution x bpp / 8) and the number of buffers. An arbitrary value
> in terms of megabytes is meaningless: it is just a random number above
> some reasonable limit.
> 
> The proper way to limit memory is to do something like:
> 
> #define MAX_HRES	1920
> #define MAX_VRES	1650
> #define MAX_DEPTH	3
> #define MAX_BUFS	4
> 
> #define MAX_MEMSIZE	(MAX_HRES * MAX_VRES * MAX_DEPTH * MAX_BUFS)
> 
> buf_setup(...)
> {
> 	if (size > MAX_MEMSIZE)
> 		fix_it_by_reducing_number_of_buffers();
> }

That's what we do. The limit is this not a number of buffers, but a memory 
limit. You an allocate more than 4 buffers in small resolutions. This is still 
a somewhat arbitrary limit. The OMAP3 ISP driver has 7 video nodes (a 8th one 
is possible). If we let userspace application allocate 3 buffers of the 
maximum size (think about 4096x4096 and 2 bytes per pixel, could be even 
higher for some of the blocks), the total is 256MB. That's quite a lot. A 
device-global memory limit might need to be enforced.

-- 
Regards,

Laurent Pinchart

RE: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Pawel Osciak]

Hi,

>Aguirre, Sergio wrote:
>Basically, when calling VIDIOC_REQBUFS with a certain buffer
>Count, we had a software limit for total size, calculated depending on:
>
>  Total bytesize = bytesperline x height x count
>
>So, we had an arbitrary limit to, say 32 MB, which was generic.
>
>Now, we want to condition it ONLY when MMAP buffers will be used.
>Meaning, we don't want to keep that policy when the kernel is not
>allocating the space
>
>But the thing is that, according to videobuf documentation, buf_setup is
>the one who should put a RAM usage limit. BUT the memory type passed to
>reqbufs is not propagated to buf_setup, therefore forcing me to go to a
>non-standard memory limitation in my reqbufs callback function, instead
>of doing it properly inside buf_setup.

buf_setup is called during REQBUFS and is indeed the place to limit the
size and actually allocate the buffers as well, or at least try to do so,
as V4L2 API requires. This is not currently the case with videobuf, but
right now we are working to change it. buf_prepare() is called on QBUF
and it is definitely too late to do things like that then. It is the
REQBUFS that should be failing if the requested number of buffers is too
high.

You could also (although it's very hacky) just take one of the buffers
from vq passed to buf_setup and check its memory field.

>Is this scenario a good consideration to change buf_setup API, and
>propagate buffers memory type aswell?

I agree with Mauro that there should not be a restriction for MMAP only,
for the same reasons he already pointed out.

As I mentioned, an intensive work is underway to change the buf_* API in
videobuf at the moment, so if you have any recommendation/requirements
that you feel would be useful, please let us know.


Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

Re: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Mauro Carvalho Chehab]

Pawel Osciak wrote:
> Hi,
> 
>> Aguirre, Sergio wrote:
>> Basically, when calling VIDIOC_REQBUFS with a certain buffer
>> Count, we had a software limit for total size, calculated depending on:
>>
>>  Total bytesize = bytesperline x height x count
>>
>> So, we had an arbitrary limit to, say 32 MB, which was generic.
>>
>> Now, we want to condition it ONLY when MMAP buffers will be used.
>> Meaning, we don't want to keep that policy when the kernel is not
>> allocating the space
>>
>> But the thing is that, according to videobuf documentation, buf_setup is
>> the one who should put a RAM usage limit. BUT the memory type passed to
>> reqbufs is not propagated to buf_setup, therefore forcing me to go to a
>> non-standard memory limitation in my reqbufs callback function, instead
>> of doing it properly inside buf_setup.
> 
> buf_setup is called during REQBUFS and is indeed the place to limit the
> size and actually allocate the buffers as well, or at least try to do so,
> as V4L2 API requires. This is not currently the case with videobuf, but
> right now we are working to change it. 

I can't see the problem you're mentioning. Drivers apply (or should apply)
the maximum size limit at buffer setup. For example bttv driver seems to do
the right thing:

static unsigned int gbuffers = 8;
static unsigned int gbufsize = 0x208000;
...
MODULE_PARM_DESC(gbuffers,"number of capture buffers. range 2-32, default 8");
MODULE_PARM_DESC(gbufsize,"size of the capture buffers, default is 0x208000");

...
static int
buffer_setup(struct videobuf_queue *q, unsigned int *count, unsigned int *size)
{
        struct bttv_fh *fh = q->priv_data;

        *size = fh->fmt->depth*fh->width*fh->height >> 3;
        if (0 == *count)
                *count = gbuffers;
        if (*size * *count > gbuffers * gbufsize)
                *count = (gbuffers * gbufsize) / *size;
        return 0;
}

> buf_prepare() is called on QBUF
> and it is definitely too late to do things like that then. It is the
> REQBUFS that should be failing if the requested number of buffers is too
> high.

Yes, it is too late. Restrictions like that should be done at REQBUFS.

-- 

Cheers,
Mauro

RE: [videobuf] Query: Condition bytesize limit in videobuf_reqbufs -> buf_setup() call?

[Pawel Osciak]

> Mauro Carvalho Chehab [mailto:mchehab@redhat.com] wrote:
>
>Pawel Osciak wrote:
>> Hi,
>>
>>> Aguirre, Sergio wrote:
>>> Basically, when calling VIDIOC_REQBUFS with a certain buffer
>>> Count, we had a software limit for total size, calculated depending on:
>>>
>>>  Total bytesize = bytesperline x height x count
>>>
>>> So, we had an arbitrary limit to, say 32 MB, which was generic.
>>>
>>> Now, we want to condition it ONLY when MMAP buffers will be used.
>>> Meaning, we don't want to keep that policy when the kernel is not
>>> allocating the space
>>>
>>> But the thing is that, according to videobuf documentation, buf_setup is
>>> the one who should put a RAM usage limit. BUT the memory type passed to
>>> reqbufs is not propagated to buf_setup, therefore forcing me to go to a
>>> non-standard memory limitation in my reqbufs callback function, instead
>>> of doing it properly inside buf_setup.
>>
>> buf_setup is called during REQBUFS and is indeed the place to limit the
>> size and actually allocate the buffers as well, or at least try to do so,
>> as V4L2 API requires. This is not currently the case with videobuf, but
>> right now we are working to change it.
>
>I can't see the problem you're mentioning. Drivers apply (or should apply)
>the maximum size limit at buffer setup. For example bttv driver seems to do
>the right thing:

Not really a problem. You are right about setup. I meant that videobuf
behaves differently in regard to when it actually allocates the buffers.

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center