[libcamera-devel] [PATCH] cam: Fail capture if to few Requests asked for

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2021-02-08 13:36:39 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Mon, Feb 08, 2021 at 10:44:16AM +0100, Niklas Söderlund wrote:
> > Running cam with the --capture=N option only queues N Requests to the
> > camera. If N is less then the number of requests needed by the camera to
> > operate cam may deadlock waiting for the camera to complete requests
> > while the camera waits for the application to give it more buffers.
> > 
> > Fix this by adding a check in cam to no attempt a capture session if to
> > few requests are requested.
> > 
> > Reported-by: Sebastian Fricke <sebastian.fricke at posteo.net>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/capture.cpp | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 7b55fc6770225e2a..18aa97c7cdb55dcc 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -90,6 +90,13 @@ int Capture::capture(FrameBufferAllocator *allocator)
> >  		nbuffers = std::min(nbuffers, allocated);
> >  	}
> >  
> > +	if (captureLimit_ && captureLimit_ < nbuffers) {
> > +		std::cerr << "Camera requiers at least " << nbuffers
> > +			  << " reqests to function, " << captureLimit_
> > +			  << " requested" << std::endl;
> > +		return -EINVAL;
> > +	}
> 
> I'm afraid this isn't right. The pipeline handler may not require
> nbuffers to be queued to operate properly, and this also won't handle
> the end of stream case where some drivers will require enqueing more
> buffers to push the last buffers out.

First off, I agree we should move towards not requiring more then 1 
Request queued for a camera to operate popery. But that is the design we 
have today and why we added nbuffers. I think it make sens to express 
this in cam until we rework our pipelines to not need nbuffers and 
instead add scratch buffers or something else.

Further more I don't think we should handle scenarios where we need to 
queue more buffers at stream stop. Is this not something v4l2-compliance 
checks for? And if we have a pipeline where this does not work we should 
fix it in the kernel. But for start conditions there is code in vb2 that 
deals with N buffers needs to be queued for capture to start so we need 
to handle that somehow in libcamera. Right now this is pushed to 
applications and until we rework this I think cam should act on the 
information it have instead of dead locking.

> 
> We need a more complex solution for this, involving an explicit queue
> depth.

Is this not more needed for operational guarantees of 3A algorithms 
unless we are going to have a larger memory footprint by using internal 
buffers to keep the algorithms running?

> 
> > +
> >  	/*
> >  	 * TODO: make cam tool smarter to support still capture by for
> >  	 * example pushing a button. For now run all streams all the time.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund