[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Avoid race when processing parameter buffers
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Jan 19 13:06:15 CET 2021
Hi Sebastian,
Thanks for you feedback.
On 2021-01-19 07:23:30 +0100, Sebastian Fricke wrote:
> Hello Niklas,
>
> I have tested your patch on my board. The tests I performed were:
> `LIBCAMERA_LOG_LEVELS=0 cam --camera=1 --capture=200 --file=~/test.raw`
> * without the delayed ctrls and without this patch
> * with the delayed ctrls and without this patch
> * with the delayed ctrls and with this patch
>
> Here are some snippets of my tests: https://paste.debian.net/1181697/
>
> All in all, it looks way smoother with this patch.
Nice catch!
This patch mitigates the problem you describe but reveals another
problem in cam's event loop. I believe the problem in cam is the true
problem in your report. While this patch is still needed for the
original problem in the RkISP1 pipeline handler.
I will post patches shortly to solve this issue as well in cam.
>
> There is one thing that I noticed, that we might have to address in
> another set of patches. When I run the command:
> `cam --camera=1 --capture=200`, the cam command stops at 200 frames
> without issues. If I start this command:
> `cam --camera=1 --capture=200 --file=test.raw`, the cam command keeps
> running until I `ctrl+c` it. While the output looks like this:
> ```
> 3969.152837 (4.29 fps) stream0 seq: 000381 bytesused: 5529600
> [1:06:10.137876560] [7219] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video4[out]: Queueing buffer 0
> [1:06:10.138091809] [7219] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[cap]: Queueing buffer 0
> [1:06:10.138238808] [7219] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video1[cap]: Queueing buffer 0
> [1:06:10.151352654] [7219] DEBUG V4L2 v4l2_videodevice.cpp:1509 /dev/video3[cap]: Dequeuing buffer 0
> [1:06:10.151674944] [7219] DEBUG V4L2 v4l2_videodevice.cpp:1509 /dev/video4[out]: Dequeuing buffer 0
> [1:06:10.153216102] [7219] DEBUG DelayedControls delayed_controls.cpp:209 frame 412 started
> [1:06:10.153342685] [7219] DEBUG DelayedControls delayed_controls.cpp:239 Queue is empty, auto queue no-op.
> [1:06:10.186488090] [7219] DEBUG DelayedControls delayed_controls.cpp:209 frame 413 started
> [1:06:10.186597173] [7219] DEBUG DelayedControls delayed_controls.cpp:239 Queue is empty, auto queue no-op.
> [1:06:10.217897505] [7219] DEBUG V4L2 v4l2_videodevice.cpp:1509 /dev/video1[cap]: Dequeuing buffer 0
> [1:06:10.218134628] [7219] DEBUG Request request.cpp:266 Request has completed - cookie: 0
> [1:06:10.219763286] [7219] DEBUG DelayedControls delayed_controls.cpp:209 frame 414 started
> [1:06:10.219870619] [7219] DEBUG DelayedControls delayed_controls.cpp:239 Queue is empty, auto queue no-op.
> [1:06:10.253048107] [7219] DEBUG DelayedControls delayed_controls.cpp:209 frame 415 started
> [1:06:10.253153690] [7219] DEBUG DelayedControls delayed_controls.cpp:239 Queue is empty, auto queue no-op.
> [1:06:10.286324762] [7219] DEBUG DelayedControls delayed_controls.cpp:209 frame 416 started
> [1:06:10.286432094] [7219] DEBUG DelayedControls delayed_controls.cpp:239 Queue is empty, auto queue no-op.
> [1:06:10.319640791] [7219] DEBUG DelayedControls delayed_controls.cpp:209 frame 417 started
> [1:06:10.319792457] [7219] DEBUG DelayedControls delayed_controls.cpp:239 Queue is empty, auto queue no-op.
> [1:06:10.352930278] [7219] DEBUG DelayedControls
> delayed_controls.cpp:209 frame 418 started
> [1:06:10.353086319] [7219] DEBUG DelayedControls delayed_controls.cpp:239 Queue is empty, auto queue no-op.
> [1:06:10.386194683] [7219] DEBUG DelayedControls delayed_controls.cpp:209 frame 419 started
> [1:06:10.386338182] [7219] DEBUG DelayedControls delayed_controls.cpp:239 Queue is empty, auto queue no-op.
> 3969.452358 (3.34 fps) stream0 seq: 000390 bytesused: 5529600
> ```
>
> Anyway, this seems to be another issue, I have corrected two small typos
> below. And if you like to you can add a:
> Tested-by: Sebastian Fricke <sebastian.fricke at posteo.net>
Thanks.
>
> Greetings,
> Sebastian
>
> On 18.01.2021 15:06, Niklas Söderlund wrote:
> > When moving the pipeline away from the Timeline design it was discovered
> > that the design of queuing the buffers to the device as soon as possible
> > was not the best idea. The parameter buffers where queued to the device
>
> s/where queued/were queued/
>
> > before the IPA had processed them and this violates the V4L2 API.
> >
> > Fix this by waiting to queue any buffer to the device until the IPA have
>
> s/until the IPA have/until the IPA has/
>
> > filled in the parameters buffer.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > Hello,
> >
> > This patch is based on-top of '[PATCH v5 0/8] libcamera: Add helper for
> > controls that take effect with a delay' as the change is much cleaner
> > on-top of the removal of the Timeline.
> > ---
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 41 ++++++++----------------
> > 1 file changed, 13 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 58ae8f98a5c683d5..e85979a719b7ced6 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -51,7 +51,6 @@ struct RkISP1FrameInfo {
> > FrameBuffer *mainPathBuffer;
> > FrameBuffer *selfPathBuffer;
> >
> > - bool paramFilled;
> > bool paramDequeued;
> > bool metadataProcessed;
> > };
> > @@ -219,7 +218,6 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
> > info->mainPathBuffer = mainPathBuffer;
> > info->selfPathBuffer = selfPathBuffer;
> > info->statBuffer = statBuffer;
> > - info->paramFilled = false;
> > info->paramDequeued = false;
> > info->metadataProcessed = false;
> >
> > @@ -322,9 +320,20 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
> > break;
> > }
> > case RKISP1_IPA_ACTION_PARAM_FILLED: {
> > + PipelineHandlerRkISP1 *pipe = static_cast<PipelineHandlerRkISP1 *>(pipe_);
> > RkISP1FrameInfo *info = frameInfo_.find(frame);
> > - if (info)
> > - info->paramFilled = true;
> > + if (!info)
> > + break;
> > +
> > + pipe->param_->queueBuffer(info->paramBuffer);
> > + pipe->stat_->queueBuffer(info->statBuffer);
> > +
> > + if (info->mainPathBuffer)
> > + mainPath_->queueBuffer(info->mainPathBuffer);
> > +
> > + if (info->selfPathBuffer)
> > + selfPath_->queueBuffer(info->selfPathBuffer);
> > +
> > break;
> > }
> > case RKISP1_IPA_ACTION_METADATA:
> > @@ -852,15 +861,6 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> > op.controls = { request->controls() };
> > data->ipa_->processEvent(op);
> >
> > - param_->queueBuffer(info->paramBuffer);
> > - stat_->queueBuffer(info->statBuffer);
> > -
> > - if (info->mainPathBuffer)
> > - mainPath_.queueBuffer(info->mainPathBuffer);
> > -
> > - if (info->selfPathBuffer)
> > - selfPath_.queueBuffer(info->selfPathBuffer);
> > -
> > data->frame_++;
> >
> > return 0;
> > @@ -1009,7 +1009,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> > mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> > selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> > stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> > - isp_->frameStart.connect(this, &PipelineHandlerRkISP1::frameStart);
> > param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
> >
> > /*
> > @@ -1097,20 +1096,6 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> > data->ipa_->processEvent(op);
> > }
> >
> > -void PipelineHandlerRkISP1::frameStart(uint32_t sequence)
> > -{
> > - if (!activeCamera_)
> > - return;
> > -
> > - RkISP1CameraData *data = cameraData(activeCamera_);
> > -
> > - RkISP1FrameInfo *info = data->frameInfo_.find(sequence);
> > - if (!info || !info->paramFilled)
> > - LOG(RkISP1, Info)
> > - << "Parameters not ready on time for frame "
> > - << sequence;
> > -}
> > -
> > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
> >
> > } /* namespace libcamera */
> > --
> > 2.30.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list