<div dir="ltr"><div dir="ltr">Hi Jacopo, thank you for reviewing.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 11, 2021 at 12:57 AM 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 Hiro,<br>
sorry took me a while to re-read the whole discussion on the<br>
previous version and put me in the same perspective as you and<br>
Laurent had<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>
> +<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>
> + data->pendingRequests_.push(request);<br>
> + data->queuePendingRequests();<br>
<br>
I originally wrote this comment:<br>
<br>
----------------------------------------------------------<br>
This makes it impossible to return errors to the pipeline handler base<br>
class, even if those errors are not due to missing buffers but are<br>
reported by the video device, in example at cio2_.queueBuffer() time.<br>
I don't think that's desired, as errors would go unnonticed.<br>
<br>
I would<br>
<br>
int queueRequestDevice(Request *req)<br>
{<br>
pending.push(req);<br>
while(!pending.empty()) {<br>
<br>
r = pending.front()<br>
<br>
info = frameInfo.create();<br>
if (!info)<br>
break;<br>
<br>
pending.pop();<br>
rawB = request->findBuffer(req);<br>
raw = cio2.queue(rawB);<br>
if (!raw) {<br>
frameInfo.remove(info);<br>
return -ENOMEM;<br>
}<br>
<br>
info->rawBuffer = raw;<br>
<br>
...<br>
ipa_->processEvent(ev);<br>
}<br>
<br>
}<br>
<br>
<br>
To maintain the ability to return errors from the video device<br>
-------------------------------------------------------------<br>
<br>
But then I noticed CIO2::queueBuffer() is pretty idiotic, it behaves<br>
differently if the raw buffer is nullptr or not, and to work around<br>
this it cannot return an error code to signal an actual queueing<br>
error, but it just return nullptr which can mean either "I don't have<br>
memory available" or "there's been an error in queueing the buffer to<br>
the device". It's impossible to decouple the two error conditions and<br>
too much magic is happening in that function which makes it not easy<br>
predictable from the caller.<br>
<br>
So, I would start by splitting the buffer retrieval from the CIO2 from<br>
the buffer queueing as in:<br>
<br>
raw = request->findBuffer(req);<br>
if (!raw)<br>
raw = cio2->getRawBuffer();<br>
<br>
if (!raw)<br>
break;<br>
<br>
ret = cio2->queueBuffer(raw);<br>
if (ret)<br>
return ret;<br>
<br>
But as you don't have to solve all the issues you encounter, the patch<br>
is fine as it is with a little nit that I find<br>
<br></blockquote><div><br></div><div>Thanks for finding.</div><div>I am also thinking of how to report the error.</div><div>Since we delay queueing a request, it may be difficult to return the error properly with an associated request with the current solution.</div><div>I honestly have no idea.</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">
void funcA()<br>
{<br>
while(something) {<br>
code;<br>
}<br>
}<br>
<br>
void funcB()<br>
{<br>
funcA();<br>
}<br>
<br>
worse to read compared to unfolding the while loop in the caller.<br>
<br></blockquote><div><br></div><div>I think you are saying about queuePendingRequests().</div><div>queuePendingRequests() is called in queueRequestDevice() in this patch.</div><div>But it will be called from other places in 2/2.</div><div>So the current style is fine for you?</div><div><br></div><div>-Hiro</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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>