[libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests in the case a buffer shortage
Jacopo Mondi
jacopo at jmondi.org
Tue May 11 12:35:33 CEST 2021
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...
> + }
> +
> + 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
More information about the libcamera-devel
mailing list