[libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove bufferCount

[Jean-Michel Hautbois]

Hi David,

On 26/04/2021 12:46, David Plowman wrote:
> Hi everyone
> 
> On Mon, 26 Apr 2021 at 11:17, Jean-Michel Hautbois
> <jeanmichel.hautbois at ideasonboard.com> wrote:
>>
>> Hi,
>>
>> On 26/04/2021 11:28, Niklas Söderlund wrote:
>>> Hello,
>>>
>>> On 2021-04-26 06:19:57 +0300, Laurent Pinchart wrote:
>>>
>>> <snip>
>>>
>>>>> @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera
>>>>> *camera)
>>>>>  {
>>>>>     IPU3CameraData *data = cameraData(camera);
>>>>>     ImgUDevice *imgu = data->imgu_;
>>>>> -   unsigned int bufferCount;
>>>>> +   unsigned int bufferCount = data->properties_.get(properties::QueueDepth);
>>>>>     int ret;
>>>>>
>>>>> -   bufferCount = std::max({
>>>>> -           data->outStream_.configuration().bufferCount,
>>>>> -           data->vfStream_.configuration().bufferCount,
>>>>> -           data->rawStream_.configuration().bufferCount,
>>>>> -   });
>>>>
>>>> I'm not sure properties::QueueDepth is the right value here. This deals
>>>> with both allocation of stats and params buffers, which are internal,
>>>> and the number of V4L2 buffer slots for the ImgU input and output. For
>>>> the latter, we probably should overallocate, as underallocation would
>>>> mean trashing dmabuf mappings. For the former, we shouldn't allocate too
>>>> much to avoid wasting memory, so we should take into account how long
>>>> the IPA would need to hold on the params and stats buffers.
>>>>
>>>> Jacopo, Jean-Michel, Niklas, any comment ?
>>>
>>> I agree that QueueDepth is likely not the best fit here, but neither was
>>> bufferCount :-) IIRC this was done very early on where bufferCount was
>>> not really defined and added as a first steeping stone. I'm very happy
>>> to see this being thought about.
>>>
>>> Maybe the best solution for this series is to add some const numerical
>>> values to the IPU3 pipeline handler to replace the usage of bufferCount
>>> but not making it depending on QueueDepth?
>>>
>>> Maybe even separating it in two values as Laurent outlines, one for the
>>> output and viewfinder slots allocation and one for the raw stream? Maybe
>>> it make sens to link the raw streams count with the value used for the
>>> stats and param buffer counts?
>>
>> Indeed IPA may need some buffers, and even now it is not satisfying :-).
>> Overallocating is a need, and we should not go lower than the highest
>> number of frame delay in the delayedControls list. For IPU3, EXPOSURE
>> has a delay of 2, so we should not have less than 3 buffers to be
>> "comfortable".
>> My 2 cents :-).
> 
> I've been following this discussion and there seem to be a lot of
> queues and delays flying around! Maybe someone can help me to
> understand - at the moment I believe there is:
> 
> * A notion of how many buffers need to be allocated for each
> stream.The more you have, the more resilient processing tends to be to
> latencies and delays.
> 
> * This number can be different for different streams. You could
> imagine an application requiring fewer raw stream buffers, for
> example.
> 
> * There's a minimum number of buffers that are required for the
> pipeline to work. For the Raspberry Pi this is 1.
> 
> * I don't understand how delayed controls are related to this. They're
> purely to cope with the fact that the sensor has delays before
> settings that have been applied take effect, and that this delay may
> vary between controls. They also vary with the sensor, they're not
> fixed for a particular pipeline handler.

True, I read too fast sorry for having disturbed you :-/.
There is no relation with delayedControls.

JM