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

[Nícolas F. R. A. Prado]

Hi,

Em 2021-04-26 06:28, Niklas Söderlund escreveu:

> 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?

Okay, this makes perfect sense. So if I'm understanding it correctly, in general
we don't want to use QueueDepth in importBuffers() and allocateBuffers().
importBuffers() should be on the bigger side, since having it less than the
amount of requests queued decreases performance. allocateBuffers() on the other
hand is just for internal buffers, so it doesn't need to scale with the amount
of requests sent, and how many are needed concerns only the IPA.

Now, my first question is: in the ideal world, importBuffers() should
always "overallocate", meaning that however many requests are sent to the
camera simultaneousy, there are always more buffer slots, so performance is
unaffected, but in reality there are limited resources so we should create a
fixed amount of buffer slots, and by "overallocate" you just meant keep it on
the bigger side, right Laurent?

So I guess my doubt is: how many are we talking about? Does importBuffers(5) and
allocateBuffers(3) sound reasonable? Or maybe we could keep allocateBuffers at
4, for now, since I'm not sure if the IPA would work with less, and 4 was the
amount used all along so probably a safer bet.

Thanks,
Nícolas