<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>