[libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests in the case a buffer shortage
Hirokazu Honda
hiroh at chromium.org
Wed May 12 07:32:06 CEST 2021
Hi Jacopo,
On Tue, May 11, 2021 at 7:34 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi again,
> sorry I rushed a bit in the previous reply
>
> On Wed, Apr 21, 2021 at 03:48:46PM +0900, Hirokazu Honda wrote:
> > PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a
> > request when there are not sufficient buffers for the request.
> > Since the request will be successful if it is queued later when
> > enough buffers are available. The requests failed due to a buffer
> > shortage should be stored and retried later in the FIFO order.
> > This introduces the queue in IPU3CameraData to do that.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +-
> > src/libcamera/pipeline/ipu3/frames.cpp | 4 +-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++-------
> > 3 files changed, 56 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp
> b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index 3cd777d1..8bbef174 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request
> *request, FrameBuffer *rawBuffer)
> > /* If no buffer is provided in the request, use an internal one. */
> > if (!buffer) {
> > if (availableBuffers_.empty()) {
> > - LOG(IPU3, Error) << "CIO2 buffer underrun";
> > + LOG(IPU3, Debug) << "CIO2 buffer underrun";
> > return nullptr;
> > }
> >
> > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp
> b/src/libcamera/pipeline/ipu3/frames.cpp
> > index a1b014ee..2c4fe508 100644
> > --- a/src/libcamera/pipeline/ipu3/frames.cpp
> > +++ b/src/libcamera/pipeline/ipu3/frames.cpp
> > @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request
> *request)
> > unsigned int id = request->sequence();
> >
> > if (availableParamBuffers_.empty()) {
> > - LOG(IPU3, Error) << "Parameters buffer underrun";
> > + LOG(IPU3, Debug) << "Parameters buffer underrun";
> > return nullptr;
> > }
> >
> > if (availableStatBuffers_.empty()) {
> > - LOG(IPU3, Error) << "Statistics buffer underrun";
> > + LOG(IPU3, Debug) << "Statistics buffer underrun";
> > return nullptr;
> > }
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 73306cea..3f311e58 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -66,6 +66,8 @@ public:
> > void cio2BufferReady(FrameBuffer *buffer);
> > void paramBufferReady(FrameBuffer *buffer);
> > void statBufferReady(FrameBuffer *buffer);
> > + void queuePendingRequests();
> > + void cancelPendingRequests();
> >
> > CIO2Device cio2_;
> > ImgUDevice *imgu_;
> > @@ -84,6 +86,8 @@ public:
> >
> > std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
> >
> > + std::queue<Request *> pendingRequests_;
> > +
> > private:
> > void queueFrameAction(unsigned int id,
> > const ipa::ipu3::IPU3Action &action);
> > @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > IPU3CameraData *data = cameraData(camera);
> > int ret = 0;
> >
> > + data->cancelPendingRequests();
> > +
> > data->ipa_->stop();
> >
> > ret |= data->imgu_->stop();
> > @@ -774,33 +780,60 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > freeBuffers(camera);
> > }
> >
> > -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request
> *request)
> > +void IPU3CameraData::cancelPendingRequests()
> > {
> > - IPU3CameraData *data = cameraData(camera);
> > + while (!pendingRequests_.empty()) {
> > + Request *request = pendingRequests_.front();
> >
> > - IPU3Frames::Info *info = data->frameInfos_.create(request);
> > - if (!info)
> > - return -ENOBUFS;
> > + for (auto it : request->buffers()) {
> > + FrameBuffer *buffer = it.second;
> > + buffer->cancel();
> > + pipe_->completeBuffer(request, buffer);
> > + }
> >
> > - /*
> > - * Queue a buffer on the CIO2, using the raw stream buffer
> provided in
> > - * the request, if any, or a CIO2 internal buffer otherwise.
> > - */
> > - FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);
> > - FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request,
> reqRawBuffer);
> > - if (!rawBuffer) {
> > - data->frameInfos_.remove(info);
> > - return -ENOMEM;
> > + pipe_->completeRequest(request);
> > + pendingRequests_.pop();
> > }
> > +}
> >
> > - info->rawBuffer = rawBuffer;
> > +void IPU3CameraData::queuePendingRequests()
> > +{
> > + while (!pendingRequests_.empty()) {
> > + Request *request = pendingRequests_.front();
> >
> > - ipa::ipu3::IPU3Event ev;
> > - ev.op = ipa::ipu3::EventProcessControls;
> > - ev.frame = info->id;
> > - ev.controls = request->controls();
> > - data->ipa_->processEvent(ev);
> > + IPU3Frames::Info *info = frameInfos_.create(request);
> > + if (!info)
> > + break;
> > +
> > + /*
> > + * Queue a buffer on the CIO2, using the raw stream buffer
> > + * provided in the request, if any, or a CIO2 internal
> buffer
> > + * otherwise.
> > + */
> > + FrameBuffer *reqRawBuffer =
> request->findBuffer(&rawStream_);
> > + FrameBuffer *rawBuffer = cio2_.queueBuffer(request,
> reqRawBuffer);
> > + if (!rawBuffer) {
> > + frameInfos_.remove(info);
> > + break;
>
> Replying here to the previous question about error signaling.
>
> Before this change if the cio2 had no buffer available or had errors
> when queuing to the device, it reported ENOMEM and the request failed at
> queueRequestDevice() time. Applications would detect the failure and
> likely stop.
>
> Now if we have no buffer or there's an error on the device, the
> requests remains in the queue, and it is re-tried later, something
> that might lead to try over and over the request even after an
> unrecoverable device error.
>
> I'm afraid this requires de-coupling the "no buffer available on CIO2"
> error from "device queue error on CIO2" one.
>
> If there are no buffers, then we can break and try later.
>
> If there's an actual device error the request should be completed with
> errors
> like you do in cancelPendingRequests() to signal that to applications.
> However this won't immediately notify to applications that
> queueRequest() has failed. I think that's fine, however now
> applications will start receiving sequences of failed requests if the
> error is not not recoverable and should be prepared to handle the
> situation. Ideally, we could possibily augment the error states with a
> "Fatal error on the device" state, that applications can detect and
> stop immediately from queuing any additional request. As we don't have
> that, I think for the moment is fine completing the request(s) in
> error state, but that at least should be signaled to applications...
>
>
I agree. Yeah, as you said, CIO2Device::queueBuffer() doesn't return a
error code. :(
We need to change the function, I am going to do it in a follow-up patch
with leaving todo comment in this patch series.
Is it okay for you?
-Hiro
> > + }
> > +
> > + info->rawBuffer = rawBuffer;
> >
> > + ipa::ipu3::IPU3Event ev;
> > + ev.op = ipa::ipu3::EventProcessControls;
> > + ev.frame = info->id;
> > + ev.controls = request->controls();
> > + ipa_->processEvent(ev);
> > +
> > + pendingRequests_.pop();
> > + }
> > +}
> > +
> > +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request
> *request)
> > +{
> > + IPU3CameraData *data = cameraData(camera);
>
> can we add on empty line here
>
> > + data->pendingRequests_.push(request);
> > + data->queuePendingRequests();
>
> and here to give the code some space ?
>
> Very minor, can be changed when applying
>
>
>
> > return 0;
> > }
> >
> > --
> > 2.31.1.368.gbe11c130af-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210512/9b81c9ae/attachment.htm>
More information about the libcamera-devel
mailing list