[{"id":16863,"web_url":"https://patchwork.libcamera.org/comment/16863/","msgid":"<20210510155829.qse7jnhygpx6pzg4@uno.localdomain>","date":"2021-05-10T15:58:29","subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n sorry took me a while to re-read the whole discussion on the\n previous version and put me in the same perspective as you and\n Laurent had\n\nOn Wed, Apr 21, 2021 at 03:48:46PM +0900, Hirokazu Honda wrote:\n> PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a\n> request when there are not sufficient buffers for the request.\n> Since the request will be successful if it is queued later when\n> enough buffers are available. The requests failed due to a buffer\n> shortage should be stored and retried later in the FIFO order.\n> This introduces the queue in IPU3CameraData to do that.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 2 +-\n> src/libcamera/pipeline/ipu3/frames.cpp | 4 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++-------\n> 3 files changed, 56 insertions(+), 23 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 3cd777d1..8bbef174 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)\n> \t/* If no buffer is provided in the request, use an internal one. */\n> \tif (!buffer) {\n> \t\tif (availableBuffers_.empty()) {\n> -\t\t\tLOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> +\t\t\tLOG(IPU3, Debug) << \"CIO2 buffer underrun\";\n> \t\t\treturn nullptr;\n> \t\t}\n>\n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> index a1b014ee..2c4fe508 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> \tunsigned int id = request->sequence();\n>\n> \tif (availableParamBuffers_.empty()) {\n> -\t\tLOG(IPU3, Error) << \"Parameters buffer underrun\";\n> +\t\tLOG(IPU3, Debug) << \"Parameters buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n>\n> \tif (availableStatBuffers_.empty()) {\n> -\t\tLOG(IPU3, Error) << \"Statistics buffer underrun\";\n> +\t\tLOG(IPU3, Debug) << \"Statistics buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 73306cea..3f311e58 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -66,6 +66,8 @@ public:\n> \tvoid cio2BufferReady(FrameBuffer *buffer);\n> \tvoid paramBufferReady(FrameBuffer *buffer);\n> \tvoid statBufferReady(FrameBuffer *buffer);\n> +\tvoid queuePendingRequests();\n> +\tvoid cancelPendingRequests();\n>\n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n> @@ -84,6 +86,8 @@ public:\n>\n> \tstd::unique_ptr ipa_;\n>\n> +\tstd::queue pendingRequests_;\n> +\n> private:\n> \tvoid queueFrameAction(unsigned int id,\n> \t\t\t const ipa::ipu3::IPU3Action &action);\n> @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tIPU3CameraData *data = cameraData(camera);\n> \tint ret = 0;\n>\n> +\tdata->cancelPendingRequests();\n> +\n> \tdata->ipa_->stop();\n>\n> \tret |= data->imgu_->stop();\n> @@ -774,33 +780,60 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tfreeBuffers(camera);\n> }\n>\n> -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> +void IPU3CameraData::cancelPendingRequests()\n> {\n> -\tIPU3CameraData *data = cameraData(camera);\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n>\n> -\tIPU3Frames::Info *info = data->frameInfos_.create(request);\n> -\tif (!info)\n> -\t\treturn -ENOBUFS;\n> +\t\tfor (auto it : request->buffers()) {\n> +\t\t\tFrameBuffer *buffer = it.second;\n> +\t\t\tbuffer->cancel();\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> +\t\t}\n>\n> -\t/*\n> -\t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n> -\t * the request, if any, or a CIO2 internal buffer otherwise.\n> -\t */\n> -\tFrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);\n> -\tFrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);\n> -\tif (!rawBuffer) {\n> -\t\tdata->frameInfos_.remove(info);\n> -\t\treturn -ENOMEM;\n> +\t\tpipe_->completeRequest(request);\n> +\t\tpendingRequests_.pop();\n> \t}\n> +}\n>\n> -\tinfo->rawBuffer = rawBuffer;\n> +void IPU3CameraData::queuePendingRequests()\n> +{\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n>\n> -\tipa::ipu3::IPU3Event ev;\n> -\tev.op = ipa::ipu3::EventProcessControls;\n> -\tev.frame = info->id;\n> -\tev.controls = request->controls();\n> -\tdata->ipa_->processEvent(ev);\n> +\t\tIPU3Frames::Info *info = frameInfos_.create(request);\n> +\t\tif (!info)\n> +\t\t\tbreak;\n> +\n> +\t\t/*\n> +\t\t * Queue a buffer on the CIO2, using the raw stream buffer\n> +\t\t * provided in the request, if any, or a CIO2 internal buffer\n> +\t\t * otherwise.\n> +\t\t */\n> +\t\tFrameBuffer *reqRawBuffer = request->findBuffer(&rawStream_);\n> +\t\tFrameBuffer *rawBuffer = cio2_.queueBuffer(request, reqRawBuffer);\n> +\t\tif (!rawBuffer) {\n> +\t\t\tframeInfos_.remove(info);\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tinfo->rawBuffer = rawBuffer;\n>\n> +\t\tipa::ipu3::IPU3Event ev;\n> +\t\tev.op = ipa::ipu3::EventProcessControls;\n> +\t\tev.frame = info->id;\n> +\t\tev.controls = request->controls();\n> +\t\tipa_->processEvent(ev);\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +}\n> +\n> +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> +\tIPU3CameraData *data = cameraData(camera);\n> +\tdata->pendingRequests_.push(request);\n> +\tdata->queuePendingRequests();\n\nI originally wrote this comment:\n\n----------------------------------------------------------\nThis makes it impossible to return errors to the pipeline handler base\nclass, even if those errors are not due to missing buffers but are\nreported by the video device, in example at cio2_.queueBuffer() time.\nI don't think that's desired, as errors would go unnonticed.\n\nI would\n\nint queueRequestDevice(Request *req)\n{\n pending.push(req);\n while(!pending.empty()) {\n\n r = pending.front()\n\n info = frameInfo.create();\n if (!info)\n break;\n\n pending.pop();\n rawB = request->findBuffer(req);\n raw = cio2.queue(rawB);\n if (!raw) {\n frameInfo.remove(info);\n return -ENOMEM;\n }\n\n info->rawBuffer = raw;\n\n ...\n ipa_->processEvent(ev);\n }\n\n}\n\n\nTo maintain the ability to return errors from the video device\n-------------------------------------------------------------\n\nBut then I noticed CIO2::queueBuffer() is pretty idiotic, it behaves\ndifferently if the raw buffer is nullptr or not, and to work around\nthis it cannot return an error code to signal an actual queueing\nerror, but it just return nullptr which can mean either \"I don't have\nmemory available\" or \"there's been an error in queueing the buffer to\nthe device\". It's impossible to decouple the two error conditions and\ntoo much magic is happening in that function which makes it not easy\npredictable from the caller.\n\nSo, I would start by splitting the buffer retrieval from the CIO2 from\nthe buffer queueing as in:\n\n raw = request->findBuffer(req);\n if (!raw)\n raw = cio2->getRawBuffer();\n\n if (!raw)\n break;\n\n ret = cio2->queueBuffer(raw);\n if (ret)\n return ret;\n\nBut as you don't have to solve all the issues you encounter, the patch\nis fine as it is with a little nit that I find\n\nvoid funcA()\n{\n while(something) {\n code;\n }\n}\n\nvoid funcB()\n{\n funcA();\n}\n\nworse to read compared to unfolding the while loop in the caller.\n\n\n> \treturn 0;\n> }\n>\n> --\n> 2.31.1.368.gbe11c130af-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C300FBF829\n\tfor ;\n\tMon, 10 May 2021 15:57:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33B37688E5;\n\tMon, 10 May 2021 17:57:46 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C6EA602BE\n\tfor ;\n\tMon, 10 May 2021 17:57:45 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 7327EE000F;\n\tMon, 10 May 2021 15:57:44 +0000 (UTC)"],"X-Originating-IP":"82.59.136.116","Date":"Mon, 10 May 2021 17:58:29 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210510155829.qse7jnhygpx6pzg4@uno.localdomain>","References":"<20210421064847.324118-1-hiroh@chromium.org>\n\t<20210421064847.324118-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421064847.324118-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16877,"web_url":"https://patchwork.libcamera.org/comment/16877/","msgid":"","date":"2021-05-11T04:18:53","subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for reviewing.\n\nOn Tue, May 11, 2021 at 12:57 AM Jacopo Mondi wrote:\n\n> Hi Hiro,\n> sorry took me a while to re-read the whole discussion on the\n> previous version and put me in the same perspective as you and\n> Laurent had\n>\n> On Wed, Apr 21, 2021 at 03:48:46PM +0900, Hirokazu Honda wrote:\n> > PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a\n> > request when there are not sufficient buffers for the request.\n> > Since the request will be successful if it is queued later when\n> > enough buffers are available. The requests failed due to a buffer\n> > shortage should be stored and retried later in the FIFO order.\n> > This introduces the queue in IPU3CameraData to do that.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +-\n> > src/libcamera/pipeline/ipu3/frames.cpp | 4 +-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++-------\n> > 3 files changed, 56 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp\n> b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index 3cd777d1..8bbef174 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request\n> *request, FrameBuffer *rawBuffer)\n> > /* If no buffer is provided in the request, use an internal one. */\n> > if (!buffer) {\n> > if (availableBuffers_.empty()) {\n> > - LOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> > + LOG(IPU3, Debug) << \"CIO2 buffer underrun\";\n> > return nullptr;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp\n> b/src/libcamera/pipeline/ipu3/frames.cpp\n> > index a1b014ee..2c4fe508 100644\n> > --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> > @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request\n> *request)\n> > unsigned int id = request->sequence();\n> >\n> > if (availableParamBuffers_.empty()) {\n> > - LOG(IPU3, Error) << \"Parameters buffer underrun\";\n> > + LOG(IPU3, Debug) << \"Parameters buffer underrun\";\n> > return nullptr;\n> > }\n> >\n> > if (availableStatBuffers_.empty()) {\n> > - LOG(IPU3, Error) << \"Statistics buffer underrun\";\n> > + LOG(IPU3, Debug) << \"Statistics buffer underrun\";\n> > return nullptr;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 73306cea..3f311e58 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -66,6 +66,8 @@ public:\n> > void cio2BufferReady(FrameBuffer *buffer);\n> > void paramBufferReady(FrameBuffer *buffer);\n> > void statBufferReady(FrameBuffer *buffer);\n> > + void queuePendingRequests();\n> > + void cancelPendingRequests();\n> >\n> > CIO2Device cio2_;\n> > ImgUDevice *imgu_;\n> > @@ -84,6 +86,8 @@ public:\n> >\n> > std::unique_ptr ipa_;\n> >\n> > + std::queue pendingRequests_;\n> > +\n> > private:\n> > void queueFrameAction(unsigned int id,\n> > const ipa::ipu3::IPU3Action &action);\n> > @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > IPU3CameraData *data = cameraData(camera);\n> > int ret = 0;\n> >\n> > + data->cancelPendingRequests();\n> > +\n> > data->ipa_->stop();\n> >\n> > ret |= data->imgu_->stop();\n> > @@ -774,33 +780,60 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > freeBuffers(camera);\n> > }\n> >\n> > -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request\n> *request)\n> > +void IPU3CameraData::cancelPendingRequests()\n> > {\n> > - IPU3CameraData *data = cameraData(camera);\n> > + while (!pendingRequests_.empty()) {\n> > + Request *request = pendingRequests_.front();\n> >\n> > - IPU3Frames::Info *info = data->frameInfos_.create(request);\n> > - if (!info)\n> > - return -ENOBUFS;\n> > + for (auto it : request->buffers()) {\n> > + FrameBuffer *buffer = it.second;\n> > + buffer->cancel();\n> > + pipe_->completeBuffer(request, buffer);\n> > + }\n> >\n> > - /*\n> > - * Queue a buffer on the CIO2, using the raw stream buffer\n> provided in\n> > - * the request, if any, or a CIO2 internal buffer otherwise.\n> > - */\n> > - FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);\n> > - FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request,\n> reqRawBuffer);\n> > - if (!rawBuffer) {\n> > - data->frameInfos_.remove(info);\n> > - return -ENOMEM;\n> > + pipe_->completeRequest(request);\n> > + pendingRequests_.pop();\n> > }\n> > +}\n> >\n> > - info->rawBuffer = rawBuffer;\n> > +void IPU3CameraData::queuePendingRequests()\n> > +{\n> > + while (!pendingRequests_.empty()) {\n> > + Request *request = pendingRequests_.front();\n> >\n> > - ipa::ipu3::IPU3Event ev;\n> > - ev.op = ipa::ipu3::EventProcessControls;\n> > - ev.frame = info->id;\n> > - ev.controls = request->controls();\n> > - data->ipa_->processEvent(ev);\n> > + IPU3Frames::Info *info = frameInfos_.create(request);\n> > + if (!info)\n> > + break;\n> > +\n> > + /*\n> > + * Queue a buffer on the CIO2, using the raw stream buffer\n> > + * provided in the request, if any, or a CIO2 internal\n> buffer\n> > + * otherwise.\n> > + */\n> > + FrameBuffer *reqRawBuffer =\n> request->findBuffer(&rawStream_);\n> > + FrameBuffer *rawBuffer = cio2_.queueBuffer(request,\n> reqRawBuffer);\n> > + if (!rawBuffer) {\n> > + frameInfos_.remove(info);\n> > + break;\n> > + }\n> > +\n> > + info->rawBuffer = rawBuffer;\n> >\n> > + ipa::ipu3::IPU3Event ev;\n> > + ev.op = ipa::ipu3::EventProcessControls;\n> > + ev.frame = info->id;\n> > + ev.controls = request->controls();\n> > + ipa_->processEvent(ev);\n> > +\n> > + pendingRequests_.pop();\n> > + }\n> > +}\n> > +\n> > +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request\n> *request)\n> > +{\n> > + IPU3CameraData *data = cameraData(camera);\n> > + data->pendingRequests_.push(request);\n> > + data->queuePendingRequests();\n>\n> I originally wrote this comment:\n>\n> ----------------------------------------------------------\n> This makes it impossible to return errors to the pipeline handler base\n> class, even if those errors are not due to missing buffers but are\n> reported by the video device, in example at cio2_.queueBuffer() time.\n> I don't think that's desired, as errors would go unnonticed.\n>\n> I would\n>\n> int queueRequestDevice(Request *req)\n> {\n> pending.push(req);\n> while(!pending.empty()) {\n>\n> r = pending.front()\n>\n> info = frameInfo.create();\n> if (!info)\n> break;\n>\n> pending.pop();\n> rawB = request->findBuffer(req);\n> raw = cio2.queue(rawB);\n> if (!raw) {\n> frameInfo.remove(info);\n> return -ENOMEM;\n> }\n>\n> info->rawBuffer = raw;\n>\n> ...\n> ipa_->processEvent(ev);\n> }\n>\n> }\n>\n>\n> To maintain the ability to return errors from the video device\n> -------------------------------------------------------------\n>\n> But then I noticed CIO2::queueBuffer() is pretty idiotic, it behaves\n> differently if the raw buffer is nullptr or not, and to work around\n> this it cannot return an error code to signal an actual queueing\n> error, but it just return nullptr which can mean either \"I don't have\n> memory available\" or \"there's been an error in queueing the buffer to\n> the device\". It's impossible to decouple the two error conditions and\n> too much magic is happening in that function which makes it not easy\n> predictable from the caller.\n>\n> So, I would start by splitting the buffer retrieval from the CIO2 from\n> the buffer queueing as in:\n>\n> raw = request->findBuffer(req);\n> if (!raw)\n> raw = cio2->getRawBuffer();\n>\n> if (!raw)\n> break;\n>\n> ret = cio2->queueBuffer(raw);\n> if (ret)\n> return ret;\n>\n> But as you don't have to solve all the issues you encounter, the patch\n> is fine as it is with a little nit that I find\n>\n>\nThanks for finding.\nI am also thinking of how to report the error.\nSince we delay queueing a request, it may be difficult to return the error\nproperly with an associated request with the current solution.\nI honestly have no idea.\n\n\n> void funcA()\n> {\n> while(something) {\n> code;\n> }\n> }\n>\n> void funcB()\n> {\n> funcA();\n> }\n>\n> worse to read compared to unfolding the while loop in the caller.\n>\n>\nI think you are saying about queuePendingRequests().\nqueuePendingRequests() is called in queueRequestDevice() in this patch.\nBut it will be called from other places in 2/2.\nSo the current style is fine for you?\n\n-Hiro\n\n>\n> > return 0;\n> > }\n> >\n> > --\n> > 2.31.1.368.gbe11c130af-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C59E6BF839\n\tfor ;\n\tTue, 11 May 2021 04:19:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C74D6891D;\n\tTue, 11 May 2021 06:19:07 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2992C61538\n\tfor ;\n\tTue, 11 May 2021 06:19:05 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id s7so16538904edq.12\n\tfor ;\n\tMon, 10 May 2021 21:19:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Or45MSCN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Hs4VruE+8E3eg2EWavac+GBy3dedyV3cBa77Hpx065M=;\n\tb=Or45MSCNwVTZ+iH/Gjub9tdG8p89Mv0nskR5jkwwivAdhNohHyuQYYog7RxRYoJiPt\n\tILjlk71dk1P9sohNLDzmxtZrfcZLOm/6isRtwErQkWCEfcImJ/ggaDwdGrGvvsJie16o\n\tB0vWCOECQe5bJMpKM7bG0UfcokEpDnYp3EJI8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Hs4VruE+8E3eg2EWavac+GBy3dedyV3cBa77Hpx065M=;\n\tb=WuDw0zM0Ifs6fFz6VmTmaZZIw0tEg3HNHYvq8lJjDOJ+735/l1/hWtSung3DeXBaxB\n\tBQZ7F7tdgQGZIVkzsSTvHwMKaRJ+Um5PQK6b84G0fc5XdMRz10vJRYdhaaUG1nYN0avO\n\tnTKXQQOKDZ7mkrxZu6SlJ6lLCucefDjjz0RacSzfEjAmLgZ7YuUUz/2xYlwql9f3ZBXc\n\tqmzdjwk+r0vAhvdAgVLO41EpWYDfgd3HPCxv9a6F45Xx3OZtzhYLK+SeHryQeZidjKkr\n\t1cvbnc53LJgl2WSTv1lTlh2X+hYKmsJhpJS4U+p5pjXoF4S2/6NyUOTpiQlWPfC5eUx5\n\tpWHQ==","X-Gm-Message-State":"AOAM531bSWm3EwTzbK0h5GzLU5rAFlrky507641u5iXJ/HJxaSAsl618\n\tyDboTTqJTjZM8WSs9q1qFQPeVaI+GsyfBP1B2csbrROQJaOCjQ==","X-Google-Smtp-Source":"ABdhPJwljZDyC1VcAv39nN00DxUGpl35jU9Lxhp13qXcQMVKNE0k/WyjusR3p34bNWknshQjzxPdmaSGVyYDqtwEefY=","X-Received":"by 2002:a50:bec1:: with SMTP id\n\te1mr33520863edk.116.1620706744679; \n\tMon, 10 May 2021 21:19:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421064847.324118-1-hiroh@chromium.org>\n\t<20210421064847.324118-2-hiroh@chromium.org>\n\t<20210510155829.qse7jnhygpx6pzg4@uno.localdomain>","In-Reply-To":"<20210510155829.qse7jnhygpx6pzg4@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 11 May 2021 13:18:53 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============8620302125368971933==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16891,"web_url":"https://patchwork.libcamera.org/comment/16891/","msgid":"<20210511100748.nvvcdpn457xsgqaf@uno.localdomain>","date":"2021-05-11T10:07:48","subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Tue, May 11, 2021 at 01:18:53PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for reviewing.\n>\n> On Tue, May 11, 2021 at 12:57 AM Jacopo Mondi wrote:\n>\n> > Hi Hiro,\n> > sorry took me a while to re-read the whole discussion on the\n> > previous version and put me in the same perspective as you and\n> > Laurent had\n> >\n> > On Wed, Apr 21, 2021 at 03:48:46PM +0900, Hirokazu Honda wrote:\n> > > PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a\n> > > request when there are not sufficient buffers for the request.\n> > > Since the request will be successful if it is queued later when\n> > > enough buffers are available. The requests failed due to a buffer\n> > > shortage should be stored and retried later in the FIFO order.\n> > > This introduces the queue in IPU3CameraData to do that.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +-\n> > > src/libcamera/pipeline/ipu3/frames.cpp | 4 +-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++-------\n> > > 3 files changed, 56 insertions(+), 23 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > index 3cd777d1..8bbef174 100644\n> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request\n> > *request, FrameBuffer *rawBuffer)\n> > > /* If no buffer is provided in the request, use an internal one. */\n> > > if (!buffer) {\n> > > if (availableBuffers_.empty()) {\n> > > - LOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> > > + LOG(IPU3, Debug) << \"CIO2 buffer underrun\";\n> > > return nullptr;\n> > > }\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp\n> > b/src/libcamera/pipeline/ipu3/frames.cpp\n> > > index a1b014ee..2c4fe508 100644\n> > > --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> > > @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request\n> > *request)\n> > > unsigned int id = request->sequence();\n> > >\n> > > if (availableParamBuffers_.empty()) {\n> > > - LOG(IPU3, Error) << \"Parameters buffer underrun\";\n> > > + LOG(IPU3, Debug) << \"Parameters buffer underrun\";\n> > > return nullptr;\n> > > }\n> > >\n> > > if (availableStatBuffers_.empty()) {\n> > > - LOG(IPU3, Error) << \"Statistics buffer underrun\";\n> > > + LOG(IPU3, Debug) << \"Statistics buffer underrun\";\n> > > return nullptr;\n> > > }\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 73306cea..3f311e58 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -66,6 +66,8 @@ public:\n> > > void cio2BufferReady(FrameBuffer *buffer);\n> > > void paramBufferReady(FrameBuffer *buffer);\n> > > void statBufferReady(FrameBuffer *buffer);\n> > > + void queuePendingRequests();\n> > > + void cancelPendingRequests();\n> > >\n> > > CIO2Device cio2_;\n> > > ImgUDevice *imgu_;\n> > > @@ -84,6 +86,8 @@ public:\n> > >\n> > > std::unique_ptr ipa_;\n> > >\n> > > + std::queue pendingRequests_;\n> > > +\n> > > private:\n> > > void queueFrameAction(unsigned int id,\n> > > const ipa::ipu3::IPU3Action &action);\n> > > @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > IPU3CameraData *data = cameraData(camera);\n> > > int ret = 0;\n> > >\n> > > + data->cancelPendingRequests();\n> > > +\n> > > data->ipa_->stop();\n> > >\n> > > ret |= data->imgu_->stop();\n> > > @@ -774,33 +780,60 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > > freeBuffers(camera);\n> > > }\n> > >\n> > > -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request\n> > *request)\n> > > +void IPU3CameraData::cancelPendingRequests()\n> > > {\n> > > - IPU3CameraData *data = cameraData(camera);\n> > > + while (!pendingRequests_.empty()) {\n> > > + Request *request = pendingRequests_.front();\n> > >\n> > > - IPU3Frames::Info *info = data->frameInfos_.create(request);\n> > > - if (!info)\n> > > - return -ENOBUFS;\n> > > + for (auto it : request->buffers()) {\n> > > + FrameBuffer *buffer = it.second;\n> > > + buffer->cancel();\n> > > + pipe_->completeBuffer(request, buffer);\n> > > + }\n> > >\n> > > - /*\n> > > - * Queue a buffer on the CIO2, using the raw stream buffer\n> > provided in\n> > > - * the request, if any, or a CIO2 internal buffer otherwise.\n> > > - */\n> > > - FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);\n> > > - FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request,\n> > reqRawBuffer);\n> > > - if (!rawBuffer) {\n> > > - data->frameInfos_.remove(info);\n> > > - return -ENOMEM;\n> > > + pipe_->completeRequest(request);\n> > > + pendingRequests_.pop();\n> > > }\n> > > +}\n> > >\n> > > - info->rawBuffer = rawBuffer;\n> > > +void IPU3CameraData::queuePendingRequests()\n> > > +{\n> > > + while (!pendingRequests_.empty()) {\n> > > + Request *request = pendingRequests_.front();\n> > >\n> > > - ipa::ipu3::IPU3Event ev;\n> > > - ev.op = ipa::ipu3::EventProcessControls;\n> > > - ev.frame = info->id;\n> > > - ev.controls = request->controls();\n> > > - data->ipa_->processEvent(ev);\n> > > + IPU3Frames::Info *info = frameInfos_.create(request);\n> > > + if (!info)\n> > > + break;\n> > > +\n> > > + /*\n> > > + * Queue a buffer on the CIO2, using the raw stream buffer\n> > > + * provided in the request, if any, or a CIO2 internal\n> > buffer\n> > > + * otherwise.\n> > > + */\n> > > + FrameBuffer *reqRawBuffer =\n> > request->findBuffer(&rawStream_);\n> > > + FrameBuffer *rawBuffer = cio2_.queueBuffer(request,\n> > reqRawBuffer);\n> > > + if (!rawBuffer) {\n> > > + frameInfos_.remove(info);\n> > > + break;\n> > > + }\n> > > +\n> > > + info->rawBuffer = rawBuffer;\n> > >\n> > > + ipa::ipu3::IPU3Event ev;\n> > > + ev.op = ipa::ipu3::EventProcessControls;\n> > > + ev.frame = info->id;\n> > > + ev.controls = request->controls();\n> > > + ipa_->processEvent(ev);\n> > > +\n> > > + pendingRequests_.pop();\n> > > + }\n> > > +}\n> > > +\n> > > +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request\n> > *request)\n> > > +{\n> > > + IPU3CameraData *data = cameraData(camera);\n> > > + data->pendingRequests_.push(request);\n> > > + data->queuePendingRequests();\n> >\n> > I originally wrote this comment:\n> >\n> > ----------------------------------------------------------\n> > This makes it impossible to return errors to the pipeline handler base\n> > class, even if those errors are not due to missing buffers but are\n> > reported by the video device, in example at cio2_.queueBuffer() time.\n> > I don't think that's desired, as errors would go unnonticed.\n> >\n> > I would\n> >\n> > int queueRequestDevice(Request *req)\n> > {\n> > pending.push(req);\n> > while(!pending.empty()) {\n> >\n> > r = pending.front()\n> >\n> > info = frameInfo.create();\n> > if (!info)\n> > break;\n> >\n> > pending.pop();\n> > rawB = request->findBuffer(req);\n> > raw = cio2.queue(rawB);\n> > if (!raw) {\n> > frameInfo.remove(info);\n> > return -ENOMEM;\n> > }\n> >\n> > info->rawBuffer = raw;\n> >\n> > ...\n> > ipa_->processEvent(ev);\n> > }\n> >\n> > }\n> >\n> >\n> > To maintain the ability to return errors from the video device\n> > -------------------------------------------------------------\n> >\n> > But then I noticed CIO2::queueBuffer() is pretty idiotic, it behaves\n> > differently if the raw buffer is nullptr or not, and to work around\n> > this it cannot return an error code to signal an actual queueing\n> > error, but it just return nullptr which can mean either \"I don't have\n> > memory available\" or \"there's been an error in queueing the buffer to\n> > the device\". It's impossible to decouple the two error conditions and\n> > too much magic is happening in that function which makes it not easy\n> > predictable from the caller.\n> >\n> > So, I would start by splitting the buffer retrieval from the CIO2 from\n> > the buffer queueing as in:\n> >\n> > raw = request->findBuffer(req);\n> > if (!raw)\n> > raw = cio2->getRawBuffer();\n> >\n> > if (!raw)\n> > break;\n> >\n> > ret = cio2->queueBuffer(raw);\n> > if (ret)\n> > return ret;\n> >\n> > But as you don't have to solve all the issues you encounter, the patch\n> > is fine as it is with a little nit that I find\n> >\n> >\n> Thanks for finding.\n> I am also thinking of how to report the error.\n> Since we delay queueing a request, it may be difficult to return the error\n> properly with an associated request with the current solution.\n> I honestly have no idea.\n\nDon't you have a Request * to return an error on if queuing failed ?\n\n>\n>\n> > void funcA()\n> > {\n> > while(something) {\n> > code;\n> > }\n> > }\n> >\n> > void funcB()\n> > {\n> > funcA();\n> > }\n> >\n> > worse to read compared to unfolding the while loop in the caller.\n> >\n> >\n> I think you are saying about queuePendingRequests().\n\nYes, I meant that function.\n\n> queuePendingRequests() is called in queueRequestDevice() in this patch.\n> But it will be called from other places in 2/2.\n\nOh sorry, I missed that !\n\n> So the current style is fine for you?\n\nConsidering 2/2 yes.\n\nPlease add\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> -Hiro\n>\n> >\n> > > return 0;\n> > > }\n> > >\n> > > --\n> > > 2.31.1.368.gbe11c130af-goog\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9C912BF829\n\tfor ;\n\tTue, 11 May 2021 10:07:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6768368918;\n\tTue, 11 May 2021 12:07:06 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE77B61538\n\tfor ;\n\tTue, 11 May 2021 12:07:05 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id E27AC24000A;\n\tTue, 11 May 2021 10:07:04 +0000 (UTC)"],"X-Originating-IP":"82.59.136.116","Date":"Tue, 11 May 2021 12:07:48 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210511100748.nvvcdpn457xsgqaf@uno.localdomain>","References":"<20210421064847.324118-1-hiroh@chromium.org>\n\t<20210421064847.324118-2-hiroh@chromium.org>\n\t<20210510155829.qse7jnhygpx6pzg4@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16894,"web_url":"https://patchwork.libcamera.org/comment/16894/","msgid":"<20210511103533.pknf3qhzjr3cu4p6@uno.localdomain>","date":"2021-05-11T10:35:33","subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi again,\n sorry I rushed a bit in the previous reply\n\nOn Wed, Apr 21, 2021 at 03:48:46PM +0900, Hirokazu Honda wrote:\n> PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a\n> request when there are not sufficient buffers for the request.\n> Since the request will be successful if it is queued later when\n> enough buffers are available. The requests failed due to a buffer\n> shortage should be stored and retried later in the FIFO order.\n> This introduces the queue in IPU3CameraData to do that.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 2 +-\n> src/libcamera/pipeline/ipu3/frames.cpp | 4 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++-------\n> 3 files changed, 56 insertions(+), 23 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 3cd777d1..8bbef174 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)\n> \t/* If no buffer is provided in the request, use an internal one. */\n> \tif (!buffer) {\n> \t\tif (availableBuffers_.empty()) {\n> -\t\t\tLOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> +\t\t\tLOG(IPU3, Debug) << \"CIO2 buffer underrun\";\n> \t\t\treturn nullptr;\n> \t\t}\n>\n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> index a1b014ee..2c4fe508 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> \tunsigned int id = request->sequence();\n>\n> \tif (availableParamBuffers_.empty()) {\n> -\t\tLOG(IPU3, Error) << \"Parameters buffer underrun\";\n> +\t\tLOG(IPU3, Debug) << \"Parameters buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n>\n> \tif (availableStatBuffers_.empty()) {\n> -\t\tLOG(IPU3, Error) << \"Statistics buffer underrun\";\n> +\t\tLOG(IPU3, Debug) << \"Statistics buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 73306cea..3f311e58 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -66,6 +66,8 @@ public:\n> \tvoid cio2BufferReady(FrameBuffer *buffer);\n> \tvoid paramBufferReady(FrameBuffer *buffer);\n> \tvoid statBufferReady(FrameBuffer *buffer);\n> +\tvoid queuePendingRequests();\n> +\tvoid cancelPendingRequests();\n>\n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n> @@ -84,6 +86,8 @@ public:\n>\n> \tstd::unique_ptr ipa_;\n>\n> +\tstd::queue pendingRequests_;\n> +\n> private:\n> \tvoid queueFrameAction(unsigned int id,\n> \t\t\t const ipa::ipu3::IPU3Action &action);\n> @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tIPU3CameraData *data = cameraData(camera);\n> \tint ret = 0;\n>\n> +\tdata->cancelPendingRequests();\n> +\n> \tdata->ipa_->stop();\n>\n> \tret |= data->imgu_->stop();\n> @@ -774,33 +780,60 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tfreeBuffers(camera);\n> }\n>\n> -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> +void IPU3CameraData::cancelPendingRequests()\n> {\n> -\tIPU3CameraData *data = cameraData(camera);\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n>\n> -\tIPU3Frames::Info *info = data->frameInfos_.create(request);\n> -\tif (!info)\n> -\t\treturn -ENOBUFS;\n> +\t\tfor (auto it : request->buffers()) {\n> +\t\t\tFrameBuffer *buffer = it.second;\n> +\t\t\tbuffer->cancel();\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> +\t\t}\n>\n> -\t/*\n> -\t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n> -\t * the request, if any, or a CIO2 internal buffer otherwise.\n> -\t */\n> -\tFrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);\n> -\tFrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);\n> -\tif (!rawBuffer) {\n> -\t\tdata->frameInfos_.remove(info);\n> -\t\treturn -ENOMEM;\n> +\t\tpipe_->completeRequest(request);\n> +\t\tpendingRequests_.pop();\n> \t}\n> +}\n>\n> -\tinfo->rawBuffer = rawBuffer;\n> +void IPU3CameraData::queuePendingRequests()\n> +{\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n>\n> -\tipa::ipu3::IPU3Event ev;\n> -\tev.op = ipa::ipu3::EventProcessControls;\n> -\tev.frame = info->id;\n> -\tev.controls = request->controls();\n> -\tdata->ipa_->processEvent(ev);\n> +\t\tIPU3Frames::Info *info = frameInfos_.create(request);\n> +\t\tif (!info)\n> +\t\t\tbreak;\n> +\n> +\t\t/*\n> +\t\t * Queue a buffer on the CIO2, using the raw stream buffer\n> +\t\t * provided in the request, if any, or a CIO2 internal buffer\n> +\t\t * otherwise.\n> +\t\t */\n> +\t\tFrameBuffer *reqRawBuffer = request->findBuffer(&rawStream_);\n> +\t\tFrameBuffer *rawBuffer = cio2_.queueBuffer(request, reqRawBuffer);\n> +\t\tif (!rawBuffer) {\n> +\t\t\tframeInfos_.remove(info);\n> +\t\t\tbreak;\n\nReplying here to the previous question about error signaling.\n\nBefore this change if the cio2 had no buffer available or had errors\nwhen queuing to the device, it reported ENOMEM and the request failed at\nqueueRequestDevice() time. Applications would detect the failure and\nlikely stop.\n\nNow if we have no buffer or there's an error on the device, the\nrequests remains in the queue, and it is re-tried later, something\nthat might lead to try over and over the request even after an\nunrecoverable device error.\n\nI'm afraid this requires de-coupling the \"no buffer available on CIO2\"\nerror from \"device queue error on CIO2\" one.\n\nIf there are no buffers, then we can break and try later.\n\nIf there's an actual device error the request should be completed with errors\nlike you do in cancelPendingRequests() to signal that to applications.\nHowever this won't immediately notify to applications that\nqueueRequest() has failed. I think that's fine, however now\napplications will start receiving sequences of failed requests if the\nerror is not not recoverable and should be prepared to handle the\nsituation. Ideally, we could possibily augment the error states with a\n\"Fatal error on the device\" state, that applications can detect and\nstop immediately from queuing any additional request. As we don't have\nthat, I think for the moment is fine completing the request(s) in\nerror state, but that at least should be signaled to applications...\n\n> +\t\t}\n> +\n> +\t\tinfo->rawBuffer = rawBuffer;\n>\n> +\t\tipa::ipu3::IPU3Event ev;\n> +\t\tev.op = ipa::ipu3::EventProcessControls;\n> +\t\tev.frame = info->id;\n> +\t\tev.controls = request->controls();\n> +\t\tipa_->processEvent(ev);\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +}\n> +\n> +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> +\tIPU3CameraData *data = cameraData(camera);\n\ncan we add on empty line here\n\n> +\tdata->pendingRequests_.push(request);\n> +\tdata->queuePendingRequests();\n\nand here to give the code some space ?\n\nVery minor, can be changed when applying\n\n\n\n> \treturn 0;\n> }\n>\n> --\n> 2.31.1.368.gbe11c130af-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4785FBF829\n\tfor ;\n\tTue, 11 May 2021 10:34:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C26016890C;\n\tTue, 11 May 2021 12:34:51 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C0C861538\n\tfor ;\n\tTue, 11 May 2021 12:34:50 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 8CC7C240009;\n\tTue, 11 May 2021 10:34:49 +0000 (UTC)"],"X-Originating-IP":"82.59.136.116","Date":"Tue, 11 May 2021 12:35:33 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210511103533.pknf3qhzjr3cu4p6@uno.localdomain>","References":"<20210421064847.324118-1-hiroh@chromium.org>\n\t<20210421064847.324118-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421064847.324118-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16909,"web_url":"https://patchwork.libcamera.org/comment/16909/","msgid":"","date":"2021-05-12T05:32:06","subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, May 11, 2021 at 7:34 PM Jacopo Mondi wrote:\n\n> Hi again,\n> sorry I rushed a bit in the previous reply\n>\n> On Wed, Apr 21, 2021 at 03:48:46PM +0900, Hirokazu Honda wrote:\n> > PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a\n> > request when there are not sufficient buffers for the request.\n> > Since the request will be successful if it is queued later when\n> > enough buffers are available. The requests failed due to a buffer\n> > shortage should be stored and retried later in the FIFO order.\n> > This introduces the queue in IPU3CameraData to do that.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +-\n> > src/libcamera/pipeline/ipu3/frames.cpp | 4 +-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++-------\n> > 3 files changed, 56 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp\n> b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index 3cd777d1..8bbef174 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request\n> *request, FrameBuffer *rawBuffer)\n> > /* If no buffer is provided in the request, use an internal one. */\n> > if (!buffer) {\n> > if (availableBuffers_.empty()) {\n> > - LOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> > + LOG(IPU3, Debug) << \"CIO2 buffer underrun\";\n> > return nullptr;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp\n> b/src/libcamera/pipeline/ipu3/frames.cpp\n> > index a1b014ee..2c4fe508 100644\n> > --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> > @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request\n> *request)\n> > unsigned int id = request->sequence();\n> >\n> > if (availableParamBuffers_.empty()) {\n> > - LOG(IPU3, Error) << \"Parameters buffer underrun\";\n> > + LOG(IPU3, Debug) << \"Parameters buffer underrun\";\n> > return nullptr;\n> > }\n> >\n> > if (availableStatBuffers_.empty()) {\n> > - LOG(IPU3, Error) << \"Statistics buffer underrun\";\n> > + LOG(IPU3, Debug) << \"Statistics buffer underrun\";\n> > return nullptr;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 73306cea..3f311e58 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -66,6 +66,8 @@ public:\n> > void cio2BufferReady(FrameBuffer *buffer);\n> > void paramBufferReady(FrameBuffer *buffer);\n> > void statBufferReady(FrameBuffer *buffer);\n> > + void queuePendingRequests();\n> > + void cancelPendingRequests();\n> >\n> > CIO2Device cio2_;\n> > ImgUDevice *imgu_;\n> > @@ -84,6 +86,8 @@ public:\n> >\n> > std::unique_ptr ipa_;\n> >\n> > + std::queue pendingRequests_;\n> > +\n> > private:\n> > void queueFrameAction(unsigned int id,\n> > const ipa::ipu3::IPU3Action &action);\n> > @@ -764,6 +768,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > IPU3CameraData *data = cameraData(camera);\n> > int ret = 0;\n> >\n> > + data->cancelPendingRequests();\n> > +\n> > data->ipa_->stop();\n> >\n> > ret |= data->imgu_->stop();\n> > @@ -774,33 +780,60 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > freeBuffers(camera);\n> > }\n> >\n> > -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request\n> *request)\n> > +void IPU3CameraData::cancelPendingRequests()\n> > {\n> > - IPU3CameraData *data = cameraData(camera);\n> > + while (!pendingRequests_.empty()) {\n> > + Request *request = pendingRequests_.front();\n> >\n> > - IPU3Frames::Info *info = data->frameInfos_.create(request);\n> > - if (!info)\n> > - return -ENOBUFS;\n> > + for (auto it : request->buffers()) {\n> > + FrameBuffer *buffer = it.second;\n> > + buffer->cancel();\n> > + pipe_->completeBuffer(request, buffer);\n> > + }\n> >\n> > - /*\n> > - * Queue a buffer on the CIO2, using the raw stream buffer\n> provided in\n> > - * the request, if any, or a CIO2 internal buffer otherwise.\n> > - */\n> > - FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);\n> > - FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request,\n> reqRawBuffer);\n> > - if (!rawBuffer) {\n> > - data->frameInfos_.remove(info);\n> > - return -ENOMEM;\n> > + pipe_->completeRequest(request);\n> > + pendingRequests_.pop();\n> > }\n> > +}\n> >\n> > - info->rawBuffer = rawBuffer;\n> > +void IPU3CameraData::queuePendingRequests()\n> > +{\n> > + while (!pendingRequests_.empty()) {\n> > + Request *request = pendingRequests_.front();\n> >\n> > - ipa::ipu3::IPU3Event ev;\n> > - ev.op = ipa::ipu3::EventProcessControls;\n> > - ev.frame = info->id;\n> > - ev.controls = request->controls();\n> > - data->ipa_->processEvent(ev);\n> > + IPU3Frames::Info *info = frameInfos_.create(request);\n> > + if (!info)\n> > + break;\n> > +\n> > + /*\n> > + * Queue a buffer on the CIO2, using the raw stream buffer\n> > + * provided in the request, if any, or a CIO2 internal\n> buffer\n> > + * otherwise.\n> > + */\n> > + FrameBuffer *reqRawBuffer =\n> request->findBuffer(&rawStream_);\n> > + FrameBuffer *rawBuffer = cio2_.queueBuffer(request,\n> reqRawBuffer);\n> > + if (!rawBuffer) {\n> > + frameInfos_.remove(info);\n> > + break;\n>\n> Replying here to the previous question about error signaling.\n>\n> Before this change if the cio2 had no buffer available or had errors\n> when queuing to the device, it reported ENOMEM and the request failed at\n> queueRequestDevice() time. Applications would detect the failure and\n> likely stop.\n>\n> Now if we have no buffer or there's an error on the device, the\n> requests remains in the queue, and it is re-tried later, something\n> that might lead to try over and over the request even after an\n> unrecoverable device error.\n>\n> I'm afraid this requires de-coupling the \"no buffer available on CIO2\"\n> error from \"device queue error on CIO2\" one.\n>\n> If there are no buffers, then we can break and try later.\n>\n> If there's an actual device error the request should be completed with\n> errors\n> like you do in cancelPendingRequests() to signal that to applications.\n> However this won't immediately notify to applications that\n> queueRequest() has failed. I think that's fine, however now\n> applications will start receiving sequences of failed requests if the\n> error is not not recoverable and should be prepared to handle the\n> situation. Ideally, we could possibily augment the error states with a\n> \"Fatal error on the device\" state, that applications can detect and\n> stop immediately from queuing any additional request. As we don't have\n> that, I think for the moment is fine completing the request(s) in\n> error state, but that at least should be signaled to applications...\n>\n>\nI agree. Yeah, as you said, CIO2Device::queueBuffer() doesn't return a\nerror code. :(\nWe need to change the function, I am going to do it in a follow-up patch\nwith leaving todo comment in this patch series.\nIs it okay for you?\n\n-Hiro\n\n\n> > + }\n> > +\n> > + info->rawBuffer = rawBuffer;\n> >\n> > + ipa::ipu3::IPU3Event ev;\n> > + ev.op = ipa::ipu3::EventProcessControls;\n> > + ev.frame = info->id;\n> > + ev.controls = request->controls();\n> > + ipa_->processEvent(ev);\n> > +\n> > + pendingRequests_.pop();\n> > + }\n> > +}\n> > +\n> > +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request\n> *request)\n> > +{\n> > + IPU3CameraData *data = cameraData(camera);\n>\n> can we add on empty line here\n>\n> > + data->pendingRequests_.push(request);\n> > + data->queuePendingRequests();\n>\n> and here to give the code some space ?\n>\n> Very minor, can be changed when applying\n>\n>\n>\n> > return 0;\n> > }\n> >\n> > --\n> > 2.31.1.368.gbe11c130af-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D52EBBF839\n\tfor ;\n\tWed, 12 May 2021 05:32:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4721968919;\n\tWed, 12 May 2021 07:32:19 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9348F602B6\n\tfor ;\n\tWed, 12 May 2021 07:32:17 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id f24so33117891ejc.6\n\tfor ;\n\tTue, 11 May 2021 22:32:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Yrhiy2Bb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Y6VFKN/wr5xxJN9l523kPyYNzj1aYA0zq8jDFGEYXK0=;\n\tb=Yrhiy2BbOWf4gKpa6TP66N5JBTj2pVfZAnwrnVmGc4D3K4ESVLdtPP0eKkLFjRmmun\n\tJqoXAJAO0VRFpM5/YXEK7+jbo5cK6NeT9reWz/Lfdf0IAQU+rqxqrODFO1NbWyLduRG4\n\tM5/l3fGXGRMBKwOm8QaD87UzqLJFBgql0zeBo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Y6VFKN/wr5xxJN9l523kPyYNzj1aYA0zq8jDFGEYXK0=;\n\tb=QZk56FOM1TmX11oSRlukdVXac8cL7Z3Zyh5MCSX0kf3dKQqK1wlHxrRnro7KW9k1st\n\tpqBNCDu2Kxl+pEj9PSGPSSad5073a1F6cOujT/ii4uNcTAt2lyJpH3FTnryoBVG2c7Tn\n\tKuD4NUh0s7oA9kGq2mQzWL9SInBgLQMZoH1ZISXehQOE4eYkBTk1BKGVdhUgYNug9t6G\n\tlFpvHQjK5qGzPr9mBt9kw3DqCJHnJhRMXWjj3dc7DruUklvA/AOrjzkBqRwTDD5cXGNS\n\teV5SZ1FHvwIG4Rvm9UsUTg8ClToeoVjADcv5ioN7T2BdU6/8i9hfC0iEJ5cEhA6NMyAu\n\t4T1w==","X-Gm-Message-State":"AOAM531csX0XhJZdiJMkD7NdMNaGZb7XNcFWupicmC9RgqWUmlvODNOX\n\ttO7xPfi7omF2aW8MdufJGyCq+LpdiIyCw9AttYp9LW0OEQI=","X-Google-Smtp-Source":"ABdhPJyNsOH9SXmSK7UsJOXjjXOckZL+ZdOoDe0GyFgi0RtG+Vo8NA+6Z91fRjyTv1i2fyb+4rAsGJsl8FI8uzwAdno=","X-Received":"by 2002:a17:907:209b:: with SMTP id\n\tpv27mr35874712ejb.475.1620797537215; \n\tTue, 11 May 2021 22:32:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421064847.324118-1-hiroh@chromium.org>\n\t<20210421064847.324118-2-hiroh@chromium.org>\n\t<20210511103533.pknf3qhzjr3cu4p6@uno.localdomain>","In-Reply-To":"<20210511103533.pknf3qhzjr3cu4p6@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 12 May 2021 14:32:06 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] pipeline: ipu3: Store requests\n\tin the case a buffer shortage","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============2422860038325428391==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]