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