[libcamera-devel] [PATCH v2 11/11] libcamera: ipu3: Share parameter and statistic buffers with IPA
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Feb 3 16:04:43 CET 2021
Hi Jacopo and Laurent,
Thanks for your feedback.
On 2021-01-10 22:59:57 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Thu, Jan 07, 2021 at 05:28:17PM +0100, Jacopo Mondi wrote:
> > 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_;
>
> Can this be embedded instead of allocated dynamically ?
Good point, yes it can.
>
> > >
> > > 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;
>
> -ENOBUFS ?
Sure.
>
> > > +
> > > /*
> > > * 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;
>
> I don't think that's right. Even if we only capture raw frames, the IPA
> has to run or all the algorithms will stop.
Understood this whole logic will need to be reworked for v2 anyway.
>
> > > + } 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.
>
> I'm not sure to agree here. The controls that are received as part of
> the request are all libcamera controls, and the IPA needs them. The
> separate set of controls for the sensor is currently using V4L2 controls
> and that will change, but it's separate from the controls stored in the
> request.
>
> > > + data->ipa_->processEvent(op);
> > > +
> > > + data->imgu_->param_->queueBuffer(info->paramBuffer);
>
> That's not right. The processEvent() call is not synchronous, the
> parameters buffer will be filled later, you're queuing an empty buffer
> here.
>
> Furthermore, even if processEvent() was synchronous, you would end up
> requesting ImgU parameters too early if the application has a large
> queue of requests.
>
> Controls need to be sent to the IPA as soon as the request is received,
> but ISP parameters should be requested only when we get close to needing
> them.
As discussed offline this will all be reworked for next version. It will
not get us all the way but a good step forward in the right direction.
>
> > > + 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 ?
>
> Isn't "statistics" (or "stats") used for statistics computed by the ImgU
> and sent by the pipeline handler to the IPA, and "metadata" used for
> request metadata computed by the IPA and sent by the IPA to the pipeline
> handler ?
Yes.
>
> > > + 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.
>
> Do you think we should add
>
> enum class RequestStatus {
> RequestPending,
> RequestComplete,
> };
>
> to the PipelineHandler class, and use it as the return type for
> completeBuffer() ? This could would then become
>
> if (pipe_->completeBuffer(request, buffer) == RequestComplete) {
>
> It's more explicit, at the cost of being longer.
I don't like the class and to no surprise I prefer what is done in this
patch ;-) I will settle on the middle ground and restore the isComplete
so we can bikeshed about this in a different series.
>
> > > + 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 ?
Yes, if the IPA is to slow signaling it has processed the param buffer.
This race between the IPA and V4L2 will however be reworked for next
version so hopefully this can't happen after v3 :-)
> >
> > 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>
Thank you. As I need to rework much of this patch I will not be able to
collect your tag.
> >
> > > +
> > > 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 */
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list