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

[Jean-Michel Hautbois]

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 :-).

--