[libcamera-devel] [PATCH v2 11/11] libcamera: ipu3: Share parameter and statistic buffers with IPA
Jacopo Mondi
jacopo at jmondi.org
Thu Jan 7 17:28:17 CET 2021
Hi Niklas,
On Tue, Dec 29, 2020 at 05:03:18PM +0100, Niklas Söderlund wrote:
> Use the IPU3Frames helper to share parameters and statistics buffers
> with the IPA. Track which parameter and statistic buffer is used for
> which request and make sure the parameter buffers is filled in by the
> IPA before it's needed and that the statistic buffer is consumed and
> meta data generated before completing the request.
>
> With this change the IPU3 pipeline is prepared to fully operate with an
> IPA component.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - Update commit message.
> - s/frameInfo_/frameInfos_/
> - Refactor away isComplete variable.
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 115 +++++++++++++++++++++++++--
> 1 file changed, 107 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 141066c528890c8e..6cd1879a76b195bf 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -29,6 +29,7 @@
> #include "libcamera/internal/v4l2_controls.h"
>
> #include "cio2.h"
> +#include "frames.h"
> #include "imgu.h"
>
> namespace libcamera {
> @@ -60,6 +61,8 @@ public:
>
> void imguOutputBufferReady(FrameBuffer *buffer);
> void cio2BufferReady(FrameBuffer *buffer);
> + void paramBufferReady(FrameBuffer *buffer);
> + void statBufferReady(FrameBuffer *buffer);
>
> CIO2Device cio2_;
> ImgUDevice *imgu_;
> @@ -69,6 +72,7 @@ public:
> Stream rawStream_;
>
> std::unique_ptr<DelayedControls> delayedCtrls_;
> + std::unique_ptr<IPU3Frames> frameInfos_;
>
> private:
> void actOnIpa(unsigned int id, const IPAOperationData &op);
> @@ -602,6 +606,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>
> data->ipa_->mapBuffers(ipaBuffers_);
>
> + data->frameInfos_->init(imgu->paramBuffers_, imgu->statBuffers_);
> +
> return 0;
> }
>
> @@ -609,6 +615,8 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
>
> + data->frameInfos_->clear();
> +
> std::vector<unsigned int> ids;
> for (IPABuffer &ipabuf : ipaBuffers_)
> ids.push_back(ipabuf.id);
> @@ -720,6 +728,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> IPU3CameraData *data = cameraData(camera);
> int error = 0;
>
> + IPU3Frames::Info *info = data->frameInfos_->create(request);
> + if (!info)
> + return -ENOENT;
> +
> /*
> * Queue a buffer on the CIO2, using the raw stream buffer provided in
> * the request, if any, or a CIO2 internal buffer otherwise.
> @@ -729,7 +741,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> if (!rawBuffer)
> return -ENOMEM;
>
> + info->rawBuffer = rawBuffer;
> +
> /* Queue all buffers from the request aimed for the ImgU. */
> + bool onlyRaw = true;
> for (auto it : request->buffers()) {
> const Stream *stream = it.first;
> FrameBuffer *buffer = it.second;
> @@ -744,6 +759,23 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>
> if (ret < 0)
> error = ret;
> +
> + onlyRaw = false;
> + }
> +
> + /* If request only contains a raw buffer do not involve IPA. */
> + if (onlyRaw) {
> + info->paramDequeued = true;
> + info->metadataProcessed = true;
> + } else {
> + IPAOperationData op;
> + op.operation = IPU3_IPA_EVENT_FILL_PARAMS;
> + op.data = { info->id, info->paramBuffer->cookie() };
> + op.controls = { request->controls() };
I'm a bit skeptical about this one. I would leave controls out of the
IPA interface for the moment. We have towards the API a mixed
v4l2/libcamera controls interface which might require a second
thought, and not all the controls have to be sent to the IPA. I would
drop this for the moment.
> + data->ipa_->processEvent(op);
> +
> + data->imgu_->param_->queueBuffer(info->paramBuffer);
> + data->imgu_->stat_->queueBuffer(info->statBuffer);
> }
>
> return error;
> @@ -893,6 +925,10 @@ int PipelineHandlerIPU3::registerCameras()
> &IPU3CameraData::imguOutputBufferReady);
> data->imgu_->viewfinder_->bufferReady.connect(data.get(),
> &IPU3CameraData::imguOutputBufferReady);
> + data->imgu_->param_->bufferReady.connect(data.get(),
> + &IPU3CameraData::paramBufferReady);
> + data->imgu_->stat_->bufferReady.connect(data.get(),
> + &IPU3CameraData::statBufferReady);
>
> /* Create and register the Camera instance. */
> std::string cameraId = cio2->sensor()->id();
> @@ -922,10 +958,12 @@ int IPU3CameraData::loadIPA()
>
> ipa_->init(IPASettings{});
>
> + frameInfos_ = std::make_unique<IPU3Frames>();
> +
> return 0;
> }
>
> -void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,
> +void IPU3CameraData::actOnIpa(unsigned int id,
> const IPAOperationData &action)
> {
> switch (action.operation) {
> @@ -934,6 +972,27 @@ void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,
> delayedCtrls_->push(controls);
> break;
> }
> + case IPU3_IPA_ACTION_PARAM_FILLED: {
> + IPU3Frames::Info *info = frameInfos_->find(id);
> + if (!info)
> + break;
> +
> + info->paramFilled = true;
> + break;
> + }
> + case IPU3_IPA_ACTION_METADATA_READY: {
The corresponding action when notifying IPA about tha availability of
the metadata buffer is called IPU3_IPA_EVENT_STAT_READY. The ipa uses
the terms 'parseStatistics'. Can we stabilize on one name, being it
metadata, STATS or Statistics ?
> + IPU3Frames::Info *info = frameInfos_->find(id);
> + if (!info)
> + break;
> +
> + Request *request = info->request;
> + request->metadata() = action.controls[0];
> + info->metadataProcessed = true;
> + if (frameInfos_->tryComplete(info))
> + pipe_->completeRequest(request);
> +
> + break;
> + }
> default:
> LOG(IPU3, Error) << "Unknown action " << action.operation;
> break;
> @@ -954,13 +1013,16 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> {
> Request *request = buffer->request();
>
> - if (!pipe_->completeBuffer(request, buffer))
> - /* Request not completed yet, return here. */
> + pipe_->completeBuffer(request, buffer);
> +
> + IPU3Frames::Info *info = frameInfos_->find(buffer);
> + if (!info)
> return;
>
> - /* Mark the request as complete. */
> request->metadata().set(controls::draft::PipelineDepth, 3);
> - pipe_->completeRequest(request);
> +
> + if (frameInfos_->tryComplete(info))
> + pipe_->completeRequest(request);
> }
>
> /**
> @@ -976,6 +1038,10 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> return;
>
> + IPU3Frames::Info *info = frameInfos_->find(buffer);
> + if (!info)
> + return;
> +
> Request *request = buffer->request();
>
> /*
> @@ -983,17 +1049,50 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> * now as there's no need for ImgU processing.
> */
> if (request->findBuffer(&rawStream_)) {
> - bool isComplete = pipe_->completeBuffer(request, buffer);
> - if (isComplete) {
I would be kept it. We're familiar with the fact completeBuffer()
return 1 if the request is complete, but it's not intuitive and
'isComplete' helps clarifying it.
> + if (pipe_->completeBuffer(request, buffer)) {
> request->metadata().set(controls::draft::PipelineDepth, 2);
> - pipe_->completeRequest(request);
> + if (frameInfos_->tryComplete(info))
> + pipe_->completeRequest(request);
> return;
> }
> }
>
> + if (!info->paramFilled)
> + LOG(IPU3, Info)
> + << "Parameters not ready on time for id " << info->id;
Can this happen ?
Currently it's not super clear to me how parameters are expected to be
elaborated by the IPA, we currently send the IPU3_IPA_EVENT_FILL_PARAMS
in queueRequestDevice(), which immediately returns the
IPU3_IPA_ACTION_PARAM_FILLED to the pipeline (and a set of sensor
controls immediately after).
Isn't actually the parsing of the metadata produced by a capture,
combined with the user controls, that should generate the new set of
parameters ?
It does not make any difference here actually as they're empty.
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> +
> imgu_->input_->queueBuffer(buffer);
> }
>
> +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> +{
> + if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> + return;
> +
> + IPU3Frames::Info *info = frameInfos_->find(buffer);
> + if (!info)
> + return;
> +
> + info->paramDequeued = true;
> + if (frameInfos_->tryComplete(info))
> + pipe_->completeRequest(buffer->request());
> +}
> +
> +void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> +{
> + if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> + return;
> +
> + IPU3Frames::Info *info = frameInfos_->find(buffer);
> + if (!info)
> + return;
> +
> + IPAOperationData op;
> + op.operation = IPU3_IPA_EVENT_STAT_READY;
> + op.data = { info->id, info->statBuffer->cookie() };
> + ipa_->processEvent(op);
> +}
> +
> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
>
> } /* namespace libcamera */
> --
> 2.29.2
>
> _______________________________________________
> 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