[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Do not wait for param buffer it it's not used

[Laurent Pinchart]

Hi Niklas,

On Wed, Mar 25, 2020 at 01:03:57PM +0100, Niklas Söderlund wrote:
> On 2020-03-25 12:41:54 +0200, Laurent Pinchart wrote:
> > On Wed, Mar 25, 2020 at 11:23:55AM +0100, Niklas Söderlund wrote:
> > > If the parameter buffer is not filled in by the IPA and therefor not
> > 
> > s/therefor/therefore/
> > 
> > > used, mark it as already dequeued. If not a request without a parameter
> > 
> > s/If not/Otherwise,/ (or "If not,")
> > 
> > > buffer will never complete.
> > 
> > I'm not sure to follow you there, parameters buffers are not provided in
> > requests. Is this about the IPA not providing a parameters buffer in
> > time ? Doesn't that introduce a bad race condition if we don't provide a
> > parameters buffer for every request ?
> 
> For RkISP1 when a request is queued to the pipeline it contains no 
> buffers for stats or params. So the first thing that happens at this 
> point is to internally associate buffers for this and inform the IPA 
> that for frame # please use this param buffer.
> 
> Then the timeline is used to schedule a point in time when this request 
> needs to be queued to hardware. When that time comes and the IPA have 
> not filled in the param buffer it should not be queued to hardware as we 
> don't know what parameters are in it. This is no problem for the 
> hardware as it don't need a param buffer queued to operate. But as we 
> have already associated a param buffer with the request we need mark 
> that we shall not wait for it to dequeue as we never queue it to 
> hardware.

What happens if

- request N is queued to the driver with no params buffer
- request N+1 is queued to the driver with a param buffer
- the driver executes request N

Won't the driver take the params buffer of request N+1 for request N ?

> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -351,12 +351,15 @@ protected:
> > >  		if (!info)
> > >  			LOG(RkISP1, Fatal) << "Frame not known";
> > >  
> > > -		if (info->paramFilled)
> > > +		if (info->paramFilled) {
> > >  			pipe_->param_->queueBuffer(info->paramBuffer);
> > > -		else
> > > +		} else {
> > > +			/* No need to dequeue param if it's never queued. */
> > > +			info->paramDequeued = true;
> > >  			LOG(RkISP1, Error)
> > >  				<< "Parameters not ready on time for frame "
> > >  				<< frame() << ", ignore parameters.";
> > 
> > How often does this occur ?
> 
> Without the ipa thread series I have never seen it.
> 
> With the ipa thread series I don't see it for cam runs, but for qcam I 
> see them at least every other frame.
> 
> If I add the opengl rendering series ontop (fps ~3 -> 30) I see them for 
> the first ~10 frames and they are gone.

That's still pretty bad though, it should be a very uncommon case.

> > 
> > > +		}
> > 
> > I think the logic should be simplified. Do we really have a need to
> > dequeue the parameters buffer to complete the request ? It seems we
> > should be able to just dequeue them when they're signalled, and put them
> > in a list of free parameters buffer for later use.
> 
> I think the current design is good. It makes sure all resources 
> associated with a request when it enters the pipeline are available 
> until the request is marked as completed and "exits" the pipeline 
> handler.

Acquiring a params buffer at the time the request is queued sounds good
to me, but I don't see a reason to delay request completion until the
params buffer is dequeued, as it's not needed. This will just delay
request completion for no real reason.

> I however think we should at some point create a helper class to track 
> requests and additional buffers added to it inside a pipeline handler, 
> as we have a similar need in IPU3.
> 
> > >  
> > >  		pipe_->stat_->queueBuffer(info->statBuffer);
> > >  		pipe_->video_->queueBuffer(info->videoBuffer);

-- 
Regards,

Laurent Pinchart