[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Avoid race when processing parameter buffers
Sebastian Fricke
sebastian.fricke at posteo.net
Tue Jan 19 07:23:30 CET 2021
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.
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>
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
More information about the libcamera-devel
mailing list