[{"id":26099,"web_url":"https://patchwork.libcamera.org/comment/26099/","msgid":"<20221216145230.hvdjalxjgez57zqy@uno.localdomain>","date":"2022-12-16T14:52:30","subject":"Re: [libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3:\n\tDon't rely on bufferCount","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul\n\nOn Fri, Dec 16, 2022 at 09:29:26PM +0900, Paul Elder via libcamera-devel wrote:\n> From: Nícolas F. R. A. Prado \n>\n> Currently the ipu3 pipeline handler relies on bufferCount to decide on\n> the number of V4L2 buffer slots to reserve as well as for the number of\n> buffers to allocate internally for the CIO2 raw buffers and\n> parameter-statistics ImgU buffer pairs. Instead, the number of internal\n> buffers should be the minimum required by the pipeline to keep the\n> requests flowing without frame drops, in order to avoid wasting memory,\n> while the number of V4L2 buffer slots should be greater than the\n> expected number of requests queued by the application, in order to avoid\n> thrashing dmabuf mappings, which would degrade performance.\n>\n> Stop relying on bufferCount for these numbers and instead set them to\n> appropriate, and independent, constants.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> Reviewed-by: Paul Elder \n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v9:\n> - rebase\n>\n> Changes in v8:\n> - New\n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 4 ++--\n> src/libcamera/pipeline/ipu3/cio2.h | 16 +++++++++++++++-\n> src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------\n> src/libcamera/pipeline/ipu3/imgu.h | 15 ++++++++++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------\n> 5 files changed, 45 insertions(+), 22 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index d4e523af..feb69991 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -335,13 +335,13 @@ int CIO2Device::exportBuffers(unsigned int count,\n> \treturn output_->exportBuffers(count, buffers);\n> }\n>\n> -int CIO2Device::start()\n> +int CIO2Device::start(unsigned int bufferSlotCount)\n> {\n> \tint ret = output_->exportBuffers(kBufferCount, &buffers_);\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> -\tret = output_->importBuffers(kBufferCount);\n> +\tret = output_->importBuffers(bufferSlotCount);\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index 68504a2d..8ed208d3 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -30,6 +30,20 @@ struct StreamConfiguration;\n> class CIO2Device\n> {\n> public:\n> +\t/*\n> +\t * This many internal buffers for the CIO2 ensures that the pipeline\n> +\t * runs smoothly, without frame drops. This number considers:\n> +\t * - one buffer being DMA'ed to in CIO2\n> +\t * - one buffer programmed by the CIO2 as the next buffer\n> +\t * - one buffer under processing in ImgU\n> +\t * - one extra idle buffer queued to CIO2, to account for possible\n> +\t * delays in requeuing the buffer from ImgU back to CIO2\n> +\t *\n> +\t * Transient situations can arise when one of the parts, CIO2 or ImgU,\n> +\t * finishes its processing first and experiences a lack of buffers, but\n> +\t * they will shortly after return to the state described above as the\n> +\t * other part catches up.\n> +\t */\n> \tstatic constexpr unsigned int kBufferCount = 4;\n>\n> \tCIO2Device();\n> @@ -48,7 +62,7 @@ public:\n> \tV4L2SubdeviceFormat getSensorFormat(const std::vector &mbusCodes,\n> \t\t\t\t\t const Size &size) const;\n>\n> -\tint start();\n> +\tint start(unsigned int bufferSlotCount);\n> \tint stop();\n>\n> \tCameraSensor *sensor() { return sensor_.get(); }\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 531879f1..fa920d87 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -576,22 +576,22 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> /**\n> * \\brief Allocate buffers for all the ImgU video devices\n> */\n> -int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> +int ImgUDevice::allocateBuffers(unsigned int bufferSlotCount)\n> {\n> \t/* Share buffers between CIO2 output and ImgU input. */\n> -\tint ret = input_->importBuffers(bufferCount);\n> +\tint ret = input_->importBuffers(bufferSlotCount);\n> \tif (ret) {\n> \t\tLOG(IPU3, Error) << \"Failed to import ImgU input buffers\";\n> \t\treturn ret;\n> \t}\n>\n> -\tret = param_->allocateBuffers(bufferCount, ¶mBuffers_);\n> +\tret = param_->allocateBuffers(kImgUInternalBufferCount, ¶mBuffers_);\n\nshouldn't stats and parameters use the same number of buffers\nallocated in the input and output devices ? If you run short of stats\nor param you will stall the pipeline, won't you ?\n\n> \tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU param buffers\";\n> \t\tgoto error;\n> \t}\n>\n> -\tret = stat_->allocateBuffers(bufferCount, &statBuffers_);\n> +\tret = stat_->allocateBuffers(kImgUInternalBufferCount, &statBuffers_);\n> \tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU stat buffers\";\n> \t\tgoto error;\n> @@ -602,13 +602,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> \t * corresponding stream is active or inactive, as the driver needs\n> \t * buffers to be requested on the V4L2 devices in order to operate.\n> \t */\n> -\tret = output_->importBuffers(bufferCount);\n> +\tret = output_->importBuffers(bufferSlotCount);\n> \tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to import ImgU output buffers\";\n> \t\tgoto error;\n> \t}\n>\n> -\tret = viewfinder_->importBuffers(bufferCount);\n> +\tret = viewfinder_->importBuffers(bufferSlotCount);\n> \tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to import ImgU viewfinder buffers\";\n> \t\tgoto error;\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> index 0af4dd8a..85873961 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -84,7 +84,7 @@ public:\n> \t\t\t\t\t outputFormat);\n> \t}\n>\n> -\tint allocateBuffers(unsigned int bufferCount);\n> +\tint allocateBuffers(unsigned int bufferSlotCount);\n> \tvoid freeBuffers();\n>\n> \tint start();\n> @@ -119,6 +119,19 @@ private:\n>\n> \tstd::string name_;\n> \tMediaDevice *media_;\n> +\n> +\t/*\n> +\t * This many internal buffers (or rather parameter and statistics buffer\n> +\t * pairs) for the ImgU ensures that the pipeline runs smoothly, without\n> +\t * frame drops. This number considers:\n> +\t * - three buffers queued to the CIO2 (Since these buffers are bound to\n> +\t * CIO2 buffers before queuing to the CIO2)\n> +\t * - one buffer under processing in ImgU\n> +\t *\n> +\t * \\todo Update this number when we make these buffers only get added to\n> +\t * the FrameInfo after the raw buffers are dequeued from CIO2.\n> +\t */\n> +\tstatic constexpr unsigned int kImgUInternalBufferCount = 4;\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index bab2db65..4d8fcfeb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -160,7 +160,7 @@ private:\n> \tint updateControls(IPU3CameraData *data);\n> \tint registerCameras();\n>\n> -\tint allocateBuffers(Camera *camera);\n> +\tint allocateBuffers(Camera *camera, unsigned int bufferSlotCount);\n> \tint freeBuffers(Camera *camera);\n>\n> \tImgUDevice imgu0_;\n> @@ -169,6 +169,8 @@ private:\n> \tMediaDevice *imguMediaDev_;\n>\n> \tstd::vector ipaBuffers_;\n> +\n> +\tstatic constexpr unsigned int kIPU3BufferSlotCount = 16;\n> };\n>\n> IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)\n> @@ -710,20 +712,14 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> * In order to be able to start the 'viewfinder' and 'stat' nodes, we need\n> * memory to be reserved.\n> */\n> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> +\t\t\t\t\t unsigned int bufferSlotCount)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \tImgUDevice *imgu = data->imgu_;\n> -\tunsigned int bufferCount;\n> \tint ret;\n>\n> -\tbufferCount = std::max({\n> -\t\tdata->outStream_.configuration().bufferCount,\n> -\t\tdata->vfStream_.configuration().bufferCount,\n> -\t\tdata->rawStream_.configuration().bufferCount,\n> -\t});\n> -\n> -\tret = imgu->allocateBuffers(bufferCount);\n> +\tret = imgu->allocateBuffers(bufferSlotCount);\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> @@ -781,7 +777,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \t\treturn ret;\n>\n> \t/* Allocate buffers for internal pipeline usage. */\n> -\tret = allocateBuffers(camera);\n> +\tret = allocateBuffers(camera, kIPU3BufferSlotCount);\n> \tif (ret)\n> \t\treturn ret;\n>\n> @@ -795,7 +791,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \t * Start the ImgU video devices, buffers will be queued to the\n> \t * ImgU output and viewfinder when requests will be queued.\n> \t */\n> -\tret = cio2->start();\n> +\tret = cio2->start(kIPU3BufferSlotCount);\n> \tif (ret)\n> \t\tgoto error;\n>\n> --\n> 2.35.1\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 00C3BC3200\n\tfor ;\n\tFri, 16 Dec 2022 14:52:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 690586339E;\n\tFri, 16 Dec 2022 15:52:33 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D72CD603D0\n\tfor ;\n\tFri, 16 Dec 2022 15:52:31 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4D31EFF808;\n\tFri, 16 Dec 2022 14:52:31 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671202353;\n\tbh=NS1ZldCfU4+KGAMMMRHdW29jnm9ZzRoMnvkcdisJmZI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=I20JE+0eoO5KHc0HbxokTTfqpFG2F1gTcNrEj6kGOmMfxqgky9aIe2XYKhFuu4aek\n\teyQTyzKdMRaPwTOBn7u3Ljm+45ze+udxrR6laUAErjicfNV6WMRWoW5Uiyl5ajI3LG\n\tgfSGoxXroCCL8slXYqs0dv1v3ZFMtFUW3NnU/xV8BgjlT93ig4xeFmnrkCE+fbxYfq\n\t+paBUP/XImPJ4SpAhGhaqtMBuB6tARV8T5r/Z1NHnqNS6LqdDUoHqNWvvY61TnUxk2\n\tWMrepQdviK32Hwqf7FnlGGR9/MuA+v/cqK7yrtrT7I3xuTONqB5cA9dt1pJvzaET2D\n\tvH8C5nvpU3mCA==","Date":"Fri, 16 Dec 2022 15:52:30 +0100","To":"Paul Elder ","Message-ID":"<20221216145230.hvdjalxjgez57zqy@uno.localdomain>","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221216122939.256534-6-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3:\n\tDon't rely on bufferCount","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26119,"web_url":"https://patchwork.libcamera.org/comment/26119/","msgid":"<0912d64a-ce44-6362-1f01-b089a816fd94@ideasonboard.com>","date":"2022-12-20T13:24:18","subject":"Re: [libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3:\n\tDon't rely on bufferCount","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 12/16/22 8:22 PM, Jacopo Mondi via libcamera-devel wrote:\n> Hi Paul\n>\n> On Fri, Dec 16, 2022 at 09:29:26PM +0900, Paul Elder via libcamera-devel wrote:\n>> From: Nícolas F. R. A. Prado \n>>\n>> Currently the ipu3 pipeline handler relies on bufferCount to decide on\n>> the number of V4L2 buffer slots to reserve as well as for the number of\n>> buffers to allocate internally for the CIO2 raw buffers and\n>> parameter-statistics ImgU buffer pairs. Instead, the number of internal\n>> buffers should be the minimum required by the pipeline to keep the\n>> requests flowing without frame drops, in order to avoid wasting memory,\n>> while the number of V4L2 buffer slots should be greater than the\n>> expected number of requests queued by the application, in order to avoid\n>> thrashing dmabuf mappings, which would degrade performance.\n>>\n>> Stop relying on bufferCount for these numbers and instead set them to\n>> appropriate, and independent, constants.\n>>\n>> Signed-off-by: Nícolas F. R. A. Prado \n>> Reviewed-by: Paul Elder \n>> Signed-off-by: Paul Elder \n>>\n>> ---\n>> Changes in v9:\n>> - rebase\n>>\n>> Changes in v8:\n>> - New\n>> ---\n>> src/libcamera/pipeline/ipu3/cio2.cpp | 4 ++--\n>> src/libcamera/pipeline/ipu3/cio2.h | 16 +++++++++++++++-\n>> src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------\n>> src/libcamera/pipeline/ipu3/imgu.h | 15 ++++++++++++++-\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------\n>> 5 files changed, 45 insertions(+), 22 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n>> index d4e523af..feb69991 100644\n>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n>> @@ -335,13 +335,13 @@ int CIO2Device::exportBuffers(unsigned int count,\n>> \treturn output_->exportBuffers(count, buffers);\n>> }\n>>\n>> -int CIO2Device::start()\n>> +int CIO2Device::start(unsigned int bufferSlotCount)\n>> {\n>> \tint ret = output_->exportBuffers(kBufferCount, &buffers_);\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>>\n>> -\tret = output_->importBuffers(kBufferCount);\n>> +\tret = output_->importBuffers(bufferSlotCount);\n>> \tif (ret)\n>> \t\tLOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n>> index 68504a2d..8ed208d3 100644\n>> --- a/src/libcamera/pipeline/ipu3/cio2.h\n>> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n>> @@ -30,6 +30,20 @@ struct StreamConfiguration;\n>> class CIO2Device\n>> {\n>> public:\n>> +\t/*\n>> +\t * This many internal buffers for the CIO2 ensures that the pipeline\n>> +\t * runs smoothly, without frame drops. This number considers:\n>> +\t * - one buffer being DMA'ed to in CIO2\n>> +\t * - one buffer programmed by the CIO2 as the next buffer\n>> +\t * - one buffer under processing in ImgU\n>> +\t * - one extra idle buffer queued to CIO2, to account for possible\n>> +\t * delays in requeuing the buffer from ImgU back to CIO2\n>> +\t *\n>> +\t * Transient situations can arise when one of the parts, CIO2 or ImgU,\n>> +\t * finishes its processing first and experiences a lack of buffers, but\n>> +\t * they will shortly after return to the state described above as the\n>> +\t * other part catches up.\n>> +\t */\n>> \tstatic constexpr unsigned int kBufferCount = 4;\n>>\n>> \tCIO2Device();\n>> @@ -48,7 +62,7 @@ public:\n>> \tV4L2SubdeviceFormat getSensorFormat(const std::vector &mbusCodes,\n>> \t\t\t\t\t const Size &size) const;\n>>\n>> -\tint start();\n>> +\tint start(unsigned int bufferSlotCount);\n>> \tint stop();\n>>\n>> \tCameraSensor *sensor() { return sensor_.get(); }\n>> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n>> index 531879f1..fa920d87 100644\n>> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n>> @@ -576,22 +576,22 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n>> /**\n>> * \\brief Allocate buffers for all the ImgU video devices\n>> */\n>> -int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n>> +int ImgUDevice::allocateBuffers(unsigned int bufferSlotCount)\n>> {\n>> \t/* Share buffers between CIO2 output and ImgU input. */\n>> -\tint ret = input_->importBuffers(bufferCount);\n>> +\tint ret = input_->importBuffers(bufferSlotCount);\n>> \tif (ret) {\n>> \t\tLOG(IPU3, Error) << \"Failed to import ImgU input buffers\";\n>> \t\treturn ret;\n>> \t}\n>>\n>> -\tret = param_->allocateBuffers(bufferCount, ¶mBuffers_);\n>> +\tret = param_->allocateBuffers(kImgUInternalBufferCount, ¶mBuffers_);\n> shouldn't stats and parameters use the same number of buffers\n> allocated in the input and output devices ? If you run short of stats\n> or param you will stall the pipeline, won't you ?\n\nI don't think so - I remember using only 1 parameter buffer while \nbuffers > 1 for input and output nodes for standalone IMGU streaming\n\nhttps://patchwork.libcamera.org/patch/16988/\n>\n>> \tif (ret < 0) {\n>> \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU param buffers\";\n>> \t\tgoto error;\n>> \t}\n>>\n>> -\tret = stat_->allocateBuffers(bufferCount, &statBuffers_);\n>> +\tret = stat_->allocateBuffers(kImgUInternalBufferCount, &statBuffers_);\n>> \tif (ret < 0) {\n>> \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU stat buffers\";\n>> \t\tgoto error;\n>> @@ -602,13 +602,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n>> \t * corresponding stream is active or inactive, as the driver needs\n>> \t * buffers to be requested on the V4L2 devices in order to operate.\n>> \t */\n>> -\tret = output_->importBuffers(bufferCount);\n>> +\tret = output_->importBuffers(bufferSlotCount);\n>> \tif (ret < 0) {\n>> \t\tLOG(IPU3, Error) << \"Failed to import ImgU output buffers\";\n>> \t\tgoto error;\n>> \t}\n>>\n>> -\tret = viewfinder_->importBuffers(bufferCount);\n>> +\tret = viewfinder_->importBuffers(bufferSlotCount);\n>> \tif (ret < 0) {\n>> \t\tLOG(IPU3, Error) << \"Failed to import ImgU viewfinder buffers\";\n>> \t\tgoto error;\n>> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n>> index 0af4dd8a..85873961 100644\n>> --- a/src/libcamera/pipeline/ipu3/imgu.h\n>> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n>> @@ -84,7 +84,7 @@ public:\n>> \t\t\t\t\t outputFormat);\n>> \t}\n>>\n>> -\tint allocateBuffers(unsigned int bufferCount);\n>> +\tint allocateBuffers(unsigned int bufferSlotCount);\n>> \tvoid freeBuffers();\n>>\n>> \tint start();\n>> @@ -119,6 +119,19 @@ private:\n>>\n>> \tstd::string name_;\n>> \tMediaDevice *media_;\n>> +\n>> +\t/*\n>> +\t * This many internal buffers (or rather parameter and statistics buffer\n>> +\t * pairs) for the ImgU ensures that the pipeline runs smoothly, without\n>> +\t * frame drops. This number considers:\n>> +\t * - three buffers queued to the CIO2 (Since these buffers are bound to\n>> +\t * CIO2 buffers before queuing to the CIO2)\n>> +\t * - one buffer under processing in ImgU\n>> +\t *\n>> +\t * \\todo Update this number when we make these buffers only get added to\n>> +\t * the FrameInfo after the raw buffers are dequeued from CIO2.\n>> +\t */\n>> +\tstatic constexpr unsigned int kImgUInternalBufferCount = 4;\n>> };\n>>\n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index bab2db65..4d8fcfeb 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -160,7 +160,7 @@ private:\n>> \tint updateControls(IPU3CameraData *data);\n>> \tint registerCameras();\n>>\n>> -\tint allocateBuffers(Camera *camera);\n>> +\tint allocateBuffers(Camera *camera, unsigned int bufferSlotCount);\n>> \tint freeBuffers(Camera *camera);\n>>\n>> \tImgUDevice imgu0_;\n>> @@ -169,6 +169,8 @@ private:\n>> \tMediaDevice *imguMediaDev_;\n>>\n>> \tstd::vector ipaBuffers_;\n>> +\n>> +\tstatic constexpr unsigned int kIPU3BufferSlotCount = 16;\n>> };\n>>\n>> IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)\n>> @@ -710,20 +712,14 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n>> * In order to be able to start the 'viewfinder' and 'stat' nodes, we need\n>> * memory to be reserved.\n>> */\n>> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n>> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n>> +\t\t\t\t\t unsigned int bufferSlotCount)\n>> {\n>> \tIPU3CameraData *data = cameraData(camera);\n>> \tImgUDevice *imgu = data->imgu_;\n>> -\tunsigned int bufferCount;\n>> \tint ret;\n>>\n>> -\tbufferCount = std::max({\n>> -\t\tdata->outStream_.configuration().bufferCount,\n>> -\t\tdata->vfStream_.configuration().bufferCount,\n>> -\t\tdata->rawStream_.configuration().bufferCount,\n>> -\t});\n>> -\n>> -\tret = imgu->allocateBuffers(bufferCount);\n>> +\tret = imgu->allocateBuffers(bufferSlotCount);\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>>\n>> @@ -781,7 +777,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n>> \t\treturn ret;\n>>\n>> \t/* Allocate buffers for internal pipeline usage. */\n>> -\tret = allocateBuffers(camera);\n>> +\tret = allocateBuffers(camera, kIPU3BufferSlotCount);\n>> \tif (ret)\n>> \t\treturn ret;\n>>\n>> @@ -795,7 +791,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n>> \t * Start the ImgU video devices, buffers will be queued to the\n>> \t * ImgU output and viewfinder when requests will be queued.\n>> \t */\n>> -\tret = cio2->start();\n>> +\tret = cio2->start(kIPU3BufferSlotCount);\n>> \tif (ret)\n>> \t\tgoto error;\n>>\n>> --\n>> 2.35.1\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 16F0CC3200\n\tfor ;\n\tTue, 20 Dec 2022 13:24:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FF3C6339B;\n\tTue, 20 Dec 2022 14:24:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A538F61507\n\tfor ;\n\tTue, 20 Dec 2022 14:24:25 +0100 (CET)","from [IPV6:2401:4900:1f3e:7d24:3f0:3e81:fb16:ab4d] (unknown\n\t[IPv6:2401:4900:1f3e:7d24:3f0:3e81:fb16:ab4d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E12F356D;\n\tTue, 20 Dec 2022 14:24:23 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671542667;\n\tbh=LCoiSBH5wG/Gg/AZjYFA5fKJBWdWdm5EyITwgIYH+AQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iiYwkspJpVaeYdon6hfx1BJiPYWErnwCErtJKL+ePOhH2KnaYft+72d8mppUyqSuf\n\tiGiXwNeIxl1lmCc3FCtbo02IBcSYBRSwx0ddcpq+cAwjJEzAV9liUCz+d0SO58s4OT\n\t7AYFD5T/n2kBOpsFu3mmVC0Nvf/EcBhoGeGvuZ3hYkB21f2qNCB5HKpTLBoXcliihX\n\tFr6SPGuWI4cpDg2iz8vPjIwyHvdx56Ld7U5ufGCCv1AD+1gYLL801L/wbwwZ4cJOz/\n\tYXwz07MsJ6IWLOToZ1UYHjBqXZzqHk7TpZKKitA35hruw6PJ9OFcyCwxf28PYAX7zB\n\tlRs6HqkyZYB4w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671542665;\n\tbh=LCoiSBH5wG/Gg/AZjYFA5fKJBWdWdm5EyITwgIYH+AQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=SFu2RezIeaqHs6Wmi0y4+AnEjibgTEqt+gbSw5YXS1xMgIePN4sT/bS0CZLjpYI0D\n\t21+nEDEdueN/ImC8kJEeTBW/TYr+8eqpe5tJZVqbcYSmPIkCw56ZMirdq5XMaQOYcd\n\tg+z/bJnmiuuEVPwpFlGKJpgODymo8z/zPa2U0YAc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SFu2RezI\"; dkim-atps=neutral","Message-ID":"<0912d64a-ce44-6362-1f01-b089a816fd94@ideasonboard.com>","Date":"Tue, 20 Dec 2022 18:54:18 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.1","Content-Language":"en-US","To":"Jacopo Mondi ,\n\tPaul Elder ","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-6-paul.elder@ideasonboard.com>\n\t<20221216145230.hvdjalxjgez57zqy@uno.localdomain>","In-Reply-To":"<20221216145230.hvdjalxjgez57zqy@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3:\n\tDon't rely on bufferCount","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26125,"web_url":"https://patchwork.libcamera.org/comment/26125/","msgid":"","date":"2022-12-21T10:39:21","subject":"Re: [libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3:\n\tDon't rely on bufferCount","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 12/16/22 5:59 PM, Paul Elder via libcamera-devel wrote:\n> From: Nícolas F. R. A. Prado \n>\n> Currently the ipu3 pipeline handler relies on bufferCount to decide on\n> the number of V4L2 buffer slots to reserve as well as for the number of\n> buffers to allocate internally for the CIO2 raw buffers and\n> parameter-statistics ImgU buffer pairs. Instead, the number of internal\n> buffers should be the minimum required by the pipeline to keep the\n> requests flowing without frame drops, in order to avoid wasting memory,\n\nThis is now sounding like the number of internal buffers should be \nallocated based on MinimumRequests property we just defined? Can it be \nthen pivoted to Minimum requests property ? I don't see possibility \nwhere  internal buffers needed to be allocated comes out to be less than \nthe camera's MinimumRequests to avoid frame drops.\n\n> while the number of V4L2 buffer slots should be greater than the\n> expected number of requests queued by the application, in order to avoid\n\ns/expected number of requests queued by the application/Minimum Requests \nneeded to be queued by the application/\n\nI think in this case we want a mutiplier to MinimumRequests so allocate \nlarger V4L2 slots, For e.g. in context of this patch,\n\n     kIPU3BufferSlotCount  =  4 * minimumRequests;\n\nMy goal is that, instead of defining arbitrary constants for both IMGU \nand CIO2 internal buffers , we better pivot the buffers counts to \nMinimumRequests so it would be more cleaner and logical ?  What do you \nthink?\n> thrashing dmabuf mappings, which would degrade performance.\n\nHave you checked any performance metrics with increasing internal \ncounts?  The comments  (and this series) overwhelming  says \"without \nframe drops\" throughout - but I am not sure if that's the case in \nreality. But the direction is more or less correct in my opinion.\n\n(The feedback applies to other platforms as well, regarding pivoting \nconstants the MinimumRequests property, so I rather not comment on them \nindividually)\n>\n> Stop relying on bufferCount for these numbers and instead set them to\n> appropriate, and independent, constants.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> Reviewed-by: Paul Elder \n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v9:\n> - rebase\n>\n> Changes in v8:\n> - New\n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 4 ++--\n> src/libcamera/pipeline/ipu3/cio2.h | 16 +++++++++++++++-\n> src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------\n> src/libcamera/pipeline/ipu3/imgu.h | 15 ++++++++++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------\n> 5 files changed, 45 insertions(+), 22 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index d4e523af..feb69991 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -335,13 +335,13 @@ int CIO2Device::exportBuffers(unsigned int count,\n> \treturn output_->exportBuffers(count, buffers);\n> }\n> \n> -int CIO2Device::start()\n> +int CIO2Device::start(unsigned int bufferSlotCount)\n> {\n> \tint ret = output_->exportBuffers(kBufferCount, &buffers_);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tret = output_->importBuffers(kBufferCount);\n> +\tret = output_->importBuffers(bufferSlotCount);\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index 68504a2d..8ed208d3 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -30,6 +30,20 @@ struct StreamConfiguration;\n> class CIO2Device\n> {\n> public:\n> +\t/*\n> +\t * This many internal buffers for the CIO2 ensures that the pipeline\n> +\t * runs smoothly, without frame drops. This number considers:\n> +\t * - one buffer being DMA'ed to in CIO2\n> +\t * - one buffer programmed by the CIO2 as the next buffer\n> +\t * - one buffer under processing in ImgU\n> +\t * - one extra idle buffer queued to CIO2, to account for possible\n> +\t * delays in requeuing the buffer from ImgU back to CIO2\n> +\t *\n> +\t * Transient situations can arise when one of the parts, CIO2 or ImgU,\n> +\t * finishes its processing first and experiences a lack of buffers, but\n> +\t * they will shortly after return to the state described above as the\n> +\t * other part catches up.\n> +\t */\n> \tstatic constexpr unsigned int kBufferCount = 4;\n> \n> \tCIO2Device();\n> @@ -48,7 +62,7 @@ public:\n> \tV4L2SubdeviceFormat getSensorFormat(const std::vector &mbusCodes,\n> \t\t\t\t\t const Size &size) const;\n> \n> -\tint start();\n> +\tint start(unsigned int bufferSlotCount);\n> \tint stop();\n> \n> \tCameraSensor *sensor() { return sensor_.get(); }\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 531879f1..fa920d87 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -576,22 +576,22 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> /**\n> * \\brief Allocate buffers for all the ImgU video devices\n> */\n> -int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> +int ImgUDevice::allocateBuffers(unsigned int bufferSlotCount)\n> {\n> \t/* Share buffers between CIO2 output and ImgU input. */\n> -\tint ret = input_->importBuffers(bufferCount);\n> +\tint ret = input_->importBuffers(bufferSlotCount);\n> \tif (ret) {\n> \t\tLOG(IPU3, Error) << \"Failed to import ImgU input buffers\";\n> \t\treturn ret;\n> \t}\n> \n> -\tret = param_->allocateBuffers(bufferCount, ¶mBuffers_);\n> +\tret = param_->allocateBuffers(kImgUInternalBufferCount, ¶mBuffers_);\n> \tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU param buffers\";\n> \t\tgoto error;\n> \t}\n> \n> -\tret = stat_->allocateBuffers(bufferCount, &statBuffers_);\n> +\tret = stat_->allocateBuffers(kImgUInternalBufferCount, &statBuffers_);\n> \tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU stat buffers\";\n> \t\tgoto error;\n> @@ -602,13 +602,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> \t * corresponding stream is active or inactive, as the driver needs\n> \t * buffers to be requested on the V4L2 devices in order to operate.\n> \t */\n> -\tret = output_->importBuffers(bufferCount);\n> +\tret = output_->importBuffers(bufferSlotCount);\n> \tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to import ImgU output buffers\";\n> \t\tgoto error;\n> \t}\n> \n> -\tret = viewfinder_->importBuffers(bufferCount);\n> +\tret = viewfinder_->importBuffers(bufferSlotCount);\n> \tif (ret < 0) {\n> \t\tLOG(IPU3, Error) << \"Failed to import ImgU viewfinder buffers\";\n> \t\tgoto error;\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> index 0af4dd8a..85873961 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -84,7 +84,7 @@ public:\n> \t\t\t\t\t outputFormat);\n> \t}\n> \n> -\tint allocateBuffers(unsigned int bufferCount);\n> +\tint allocateBuffers(unsigned int bufferSlotCount);\n> \tvoid freeBuffers();\n> \n> \tint start();\n> @@ -119,6 +119,19 @@ private:\n> \n> \tstd::string name_;\n> \tMediaDevice *media_;\n> +\n> +\t/*\n> +\t * This many internal buffers (or rather parameter and statistics buffer\n> +\t * pairs) for the ImgU ensures that the pipeline runs smoothly, without\n> +\t * frame drops. This number considers:\n> +\t * - three buffers queued to the CIO2 (Since these buffers are bound to\n> +\t * CIO2 buffers before queuing to the CIO2)\n> +\t * - one buffer under processing in ImgU\n> +\t *\n> +\t * \\todo Update this number when we make these buffers only get added to\n> +\t * the FrameInfo after the raw buffers are dequeued from CIO2.\n> +\t */\n> +\tstatic constexpr unsigned int kImgUInternalBufferCount = 4;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index bab2db65..4d8fcfeb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -160,7 +160,7 @@ private:\n> \tint updateControls(IPU3CameraData *data);\n> \tint registerCameras();\n> \n> -\tint allocateBuffers(Camera *camera);\n> +\tint allocateBuffers(Camera *camera, unsigned int bufferSlotCount);\n> \tint freeBuffers(Camera *camera);\n> \n> \tImgUDevice imgu0_;\n> @@ -169,6 +169,8 @@ private:\n> \tMediaDevice *imguMediaDev_;\n> \n> \tstd::vector ipaBuffers_;\n> +\n> +\tstatic constexpr unsigned int kIPU3BufferSlotCount = 16;\n> };\n> \n> IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)\n> @@ -710,20 +712,14 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> * In order to be able to start the 'viewfinder' and 'stat' nodes, we need\n> * memory to be reserved.\n> */\n> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> +\t\t\t\t\t unsigned int bufferSlotCount)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \tImgUDevice *imgu = data->imgu_;\n> -\tunsigned int bufferCount;\n> \tint ret;\n> \n> -\tbufferCount = std::max({\n> -\t\tdata->outStream_.configuration().bufferCount,\n> -\t\tdata->vfStream_.configuration().bufferCount,\n> -\t\tdata->rawStream_.configuration().bufferCount,\n> -\t});\n> -\n> -\tret = imgu->allocateBuffers(bufferCount);\n> +\tret = imgu->allocateBuffers(bufferSlotCount);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> @@ -781,7 +777,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \t\treturn ret;\n> \n> \t/* Allocate buffers for internal pipeline usage. */\n> -\tret = allocateBuffers(camera);\n> +\tret = allocateBuffers(camera, kIPU3BufferSlotCount);\n> \tif (ret)\n> \t\treturn ret;\n> \n> @@ -795,7 +791,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> \t * Start the ImgU video devices, buffers will be queued to the\n> \t * ImgU output and viewfinder when requests will be queued.\n> \t */\n> -\tret = cio2->start();\n> +\tret = cio2->start(kIPU3BufferSlotCount);\n> \tif (ret)\n> \t\tgoto error;\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 9147EC3200\n\tfor ;\n\tWed, 21 Dec 2022 10:39:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8630633A7;\n\tWed, 21 Dec 2022 11:39:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5291561F1A\n\tfor ;\n\tWed, 21 Dec 2022 11:39:29 +0100 (CET)","from [IPV6:2401:4900:1f3f:d076:4da6:b729:f032:ed0a] (unknown\n\t[IPv6:2401:4900:1f3f:d076:4da6:b729:f032:ed0a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D0142FB;\n\tWed, 21 Dec 2022 11:39:27 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671619170;\n\tbh=+J5SREtKYRLk6Dfho6dj8JQUhQ4L4X6yjmxLAwjyt7o=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=cMld6JkiIQ4WmldD4PZTcBDSxh+YdHoh3LaFVR89/CbxPMrn/fTrgLJRx2Svm8/3R\n\tBnpvm7xqjNsGceGA45QT7c02iF9D1CWk1DNBpwI6QYAjEdkZg1Q+ggKQWXdLN1wjWT\n\tTKx2RvcCU3KUz1s5x850l/waqEdHcaZyOrnElU70nA/9DDM1oGAjadymWZLA+ybccO\n\tGLpqtFQ5Rnjn1hKqvN+IYZmoEa4cucC311vAOYZzrscW0ZeqZumk84SA5DkTHhSAk7\n\tQOStWpnVZ31KcjyA1QKSljjUNTnPd0ZpSLWUlKojh5Hz7s7aLMdW9lpHbcSVm3dn7m\n\t6eu0qDI8LYexg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671619168;\n\tbh=+J5SREtKYRLk6Dfho6dj8JQUhQ4L4X6yjmxLAwjyt7o=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=RC4mps04Axkj85paD/BPrgNEvJepZeuTMBiIti1SQE6Mm3JAp7aH2owhm+o0ZTYku\n\tywsmW/kEoMcW+/fPyhkRNx5s9JWzetjIgqJXRJ8VO7gw6qONjxxiI8V4P30Pczmf5P\n\taPECUGPVqmZTmbJOTmXhMRZuWQSBldh6lSRn4iLs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RC4mps04\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 21 Dec 2022 16:09:21 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.1","Content-Language":"en-US","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-6-paul.elder@ideasonboard.com>","In-Reply-To":"<20221216122939.256534-6-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3:\n\tDon't rely on bufferCount","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26143,"web_url":"https://patchwork.libcamera.org/comment/26143/","msgid":"","date":"2022-12-23T22:28:11","subject":"Re: [libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3:\n\tDon't rely on bufferCount","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Dec 21, 2022 at 04:09:21PM +0530, Umang Jain wrote:\n> Hi Paul,\n> \n> On 12/16/22 5:59 PM, Paul Elder via libcamera-devel wrote:\n> > From: Nícolas F. R. A. Prado \n> > \n> > Currently the ipu3 pipeline handler relies on bufferCount to decide on\n> > the number of V4L2 buffer slots to reserve as well as for the number of\n> > buffers to allocate internally for the CIO2 raw buffers and\n> > parameter-statistics ImgU buffer pairs. Instead, the number of internal\n> > buffers should be the minimum required by the pipeline to keep the\n> > requests flowing without frame drops, in order to avoid wasting memory,\n> \n> This is now sounding like the number of internal buffers should be allocated\n> based on MinimumRequests property we just defined? Can it be then pivoted to\n> Minimum requests property ? I don't see possibility where  internal buffers\n> needed to be allocated comes out to be less than the camera's\n> MinimumRequests to avoid frame drops.\n\nIndeed I don't think there would be a situation where there would be\nless internal buffers than MinimumRequests, but I think (also as is seen\nthroughout this series) it's possible that the number of internal\nbuffers could be greater than MinimumRequests.\n\nI suppose it would be less duplication if the number of internal buffers\nwas defined based on MinimumRequests (eg. kIPU3BufferSlotCount =\nMinimumRequests + 1). Well, not the property directly but the quantity\nthat is fed to the property.\n\n> \n> > while the number of V4L2 buffer slots should be greater than the\n> > expected number of requests queued by the application, in order to avoid\n> \n> s/expected number of requests queued by the application/Minimum Requests\n> needed to be queued by the application/\n> \n> I think in this case we want a mutiplier to MinimumRequests so allocate\n> larger V4L2 slots, For e.g. in context of this patch,\n> \n>     kIPU3BufferSlotCount  =  4 * minimumRequests;\n> \n> My goal is that, instead of defining arbitrary constants for both IMGU and\n> CIO2 internal buffers , we better pivot the buffers counts to\n> MinimumRequests so it would be more cleaner and logical ?  What do you\n> think?\n\nI don't think the IMGU and CIO2 internal buffers are defined off of\narbitrary constants; they're intentional constants with (documented)\nrationales. But indeed I think there is benefit in defining them based\non MinimumRequests, such that they end up with the desired quantity.\n\n> > thrashing dmabuf mappings, which would degrade performance.\n> \n> Have you checked any performance metrics with increasing internal counts? \n\nI haven't yet. I don't have an IPU3 platform that I can test on anymore.\nI'll test it on imx8mp at least.\n\n> The comments  (and this series) overwhelming  says \"without frame drops\"\n> throughout - but I am not sure if that's the case in reality. But the\n> direction is more or less correct in my opinion.\n> \n> (The feedback applies to other platforms as well, regarding pivoting\n> constants the MinimumRequests property, so I rather not comment on them\n> individually)\n\n\nThanks,\n\nPaul\n\n> > \n> > Stop relying on bufferCount for these numbers and instead set them to\n> > appropriate, and independent, constants.\n> > \n> > Signed-off-by: Nícolas F. R. A. Prado \n> > Reviewed-by: Paul Elder \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v9:\n> > - rebase\n> > \n> > Changes in v8:\n> > - New\n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 4 ++--\n> > src/libcamera/pipeline/ipu3/cio2.h | 16 +++++++++++++++-\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------\n> > src/libcamera/pipeline/ipu3/imgu.h | 15 ++++++++++++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------\n> > 5 files changed, 45 insertions(+), 22 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index d4e523af..feb69991 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -335,13 +335,13 @@ int CIO2Device::exportBuffers(unsigned int count,\n> > \treturn output_->exportBuffers(count, buffers);\n> > }\n> > -int CIO2Device::start()\n> > +int CIO2Device::start(unsigned int bufferSlotCount)\n> > {\n> > \tint ret = output_->exportBuffers(kBufferCount, &buffers_);\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > -\tret = output_->importBuffers(kBufferCount);\n> > +\tret = output_->importBuffers(bufferSlotCount);\n> > \tif (ret)\n> > \t\tLOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > index 68504a2d..8ed208d3 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > @@ -30,6 +30,20 @@ struct StreamConfiguration;\n> > class CIO2Device\n> > {\n> > public:\n> > +\t/*\n> > +\t * This many internal buffers for the CIO2 ensures that the pipeline\n> > +\t * runs smoothly, without frame drops. This number considers:\n> > +\t * - one buffer being DMA'ed to in CIO2\n> > +\t * - one buffer programmed by the CIO2 as the next buffer\n> > +\t * - one buffer under processing in ImgU\n> > +\t * - one extra idle buffer queued to CIO2, to account for possible\n> > +\t * delays in requeuing the buffer from ImgU back to CIO2\n> > +\t *\n> > +\t * Transient situations can arise when one of the parts, CIO2 or ImgU,\n> > +\t * finishes its processing first and experiences a lack of buffers, but\n> > +\t * they will shortly after return to the state described above as the\n> > +\t * other part catches up.\n> > +\t */\n> > \tstatic constexpr unsigned int kBufferCount = 4;\n> > \tCIO2Device();\n> > @@ -48,7 +62,7 @@ public:\n> > \tV4L2SubdeviceFormat getSensorFormat(const std::vector &mbusCodes,\n> > \t\t\t\t\t const Size &size) const;\n> > -\tint start();\n> > +\tint start(unsigned int bufferSlotCount);\n> > \tint stop();\n> > \tCameraSensor *sensor() { return sensor_.get(); }\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 531879f1..fa920d87 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -576,22 +576,22 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> > /**\n> > * \\brief Allocate buffers for all the ImgU video devices\n> > */\n> > -int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> > +int ImgUDevice::allocateBuffers(unsigned int bufferSlotCount)\n> > {\n> > \t/* Share buffers between CIO2 output and ImgU input. */\n> > -\tint ret = input_->importBuffers(bufferCount);\n> > +\tint ret = input_->importBuffers(bufferSlotCount);\n> > \tif (ret) {\n> > \t\tLOG(IPU3, Error) << \"Failed to import ImgU input buffers\";\n> > \t\treturn ret;\n> > \t}\n> > -\tret = param_->allocateBuffers(bufferCount, ¶mBuffers_);\n> > +\tret = param_->allocateBuffers(kImgUInternalBufferCount, ¶mBuffers_);\n> > \tif (ret < 0) {\n> > \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU param buffers\";\n> > \t\tgoto error;\n> > \t}\n> > -\tret = stat_->allocateBuffers(bufferCount, &statBuffers_);\n> > +\tret = stat_->allocateBuffers(kImgUInternalBufferCount, &statBuffers_);\n> > \tif (ret < 0) {\n> > \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU stat buffers\";\n> > \t\tgoto error;\n> > @@ -602,13 +602,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> > \t * corresponding stream is active or inactive, as the driver needs\n> > \t * buffers to be requested on the V4L2 devices in order to operate.\n> > \t */\n> > -\tret = output_->importBuffers(bufferCount);\n> > +\tret = output_->importBuffers(bufferSlotCount);\n> > \tif (ret < 0) {\n> > \t\tLOG(IPU3, Error) << \"Failed to import ImgU output buffers\";\n> > \t\tgoto error;\n> > \t}\n> > -\tret = viewfinder_->importBuffers(bufferCount);\n> > +\tret = viewfinder_->importBuffers(bufferSlotCount);\n> > \tif (ret < 0) {\n> > \t\tLOG(IPU3, Error) << \"Failed to import ImgU viewfinder buffers\";\n> > \t\tgoto error;\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> > index 0af4dd8a..85873961 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.h\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> > @@ -84,7 +84,7 @@ public:\n> > \t\t\t\t\t outputFormat);\n> > \t}\n> > -\tint allocateBuffers(unsigned int bufferCount);\n> > +\tint allocateBuffers(unsigned int bufferSlotCount);\n> > \tvoid freeBuffers();\n> > \tint start();\n> > @@ -119,6 +119,19 @@ private:\n> > \tstd::string name_;\n> > \tMediaDevice *media_;\n> > +\n> > +\t/*\n> > +\t * This many internal buffers (or rather parameter and statistics buffer\n> > +\t * pairs) for the ImgU ensures that the pipeline runs smoothly, without\n> > +\t * frame drops. This number considers:\n> > +\t * - three buffers queued to the CIO2 (Since these buffers are bound to\n> > +\t * CIO2 buffers before queuing to the CIO2)\n> > +\t * - one buffer under processing in ImgU\n> > +\t *\n> > +\t * \\todo Update this number when we make these buffers only get added to\n> > +\t * the FrameInfo after the raw buffers are dequeued from CIO2.\n> > +\t */\n> > +\tstatic constexpr unsigned int kImgUInternalBufferCount = 4;\n> > };\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index bab2db65..4d8fcfeb 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -160,7 +160,7 @@ private:\n> > \tint updateControls(IPU3CameraData *data);\n> > \tint registerCameras();\n> > -\tint allocateBuffers(Camera *camera);\n> > +\tint allocateBuffers(Camera *camera, unsigned int bufferSlotCount);\n> > \tint freeBuffers(Camera *camera);\n> > \tImgUDevice imgu0_;\n> > @@ -169,6 +169,8 @@ private:\n> > \tMediaDevice *imguMediaDev_;\n> > \tstd::vector ipaBuffers_;\n> > +\n> > +\tstatic constexpr unsigned int kIPU3BufferSlotCount = 16;\n> > };\n> > IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)\n> > @@ -710,20 +712,14 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > * In order to be able to start the 'viewfinder' and 'stat' nodes, we need\n> > * memory to be reserved.\n> > */\n> > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> > +\t\t\t\t\t unsigned int bufferSlotCount)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tImgUDevice *imgu = data->imgu_;\n> > -\tunsigned int bufferCount;\n> > \tint ret;\n> > -\tbufferCount = std::max({\n> > -\t\tdata->outStream_.configuration().bufferCount,\n> > -\t\tdata->vfStream_.configuration().bufferCount,\n> > -\t\tdata->rawStream_.configuration().bufferCount,\n> > -\t});\n> > -\n> > -\tret = imgu->allocateBuffers(bufferCount);\n> > +\tret = imgu->allocateBuffers(bufferSlotCount);\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > @@ -781,7 +777,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> > \t\treturn ret;\n> > \t/* Allocate buffers for internal pipeline usage. */\n> > -\tret = allocateBuffers(camera);\n> > +\tret = allocateBuffers(camera, kIPU3BufferSlotCount);\n> > \tif (ret)\n> > \t\treturn ret;\n> > @@ -795,7 +791,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> > \t * Start the ImgU video devices, buffers will be queued to the\n> > \t * ImgU output and viewfinder when requests will be queued.\n> > \t */\n> > -\tret = cio2->start();\n> > +\tret = cio2->start(kIPU3BufferSlotCount);\n> > \tif (ret)\n> > \t\tgoto error;\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 21420BD78E\n\tfor ;\n\tFri, 23 Dec 2022 22:28:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65297623B8;\n\tFri, 23 Dec 2022 23:28:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 028AE62398\n\tfor ;\n\tFri, 23 Dec 2022 23:28:17 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2604:2d80:ad8a:9000:1bf9:855b:22de:3645])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EDEA08B;\n\tFri, 23 Dec 2022 23:28:16 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671834499;\n\tbh=PwN6bBvpTnHKFaeLvEJreyajLWCnKs8XypUReWpqM3w=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=1HLOoLPQQxWKySmiuwUrmKsTQI9Worfhhvr3LreNhuXsgrxdpUDi2mO2nc1R8owx8\n\tsQSf9CapyALW0x5ewRnRc9LdOGaXbPpJsVhFiNjQKb1jwUGJS0sPjn2GGYrd2h03Nv\n\t4OOj0u4FeVwtl07cV3WfskeQD0uh8vmQ11qTh6Y/PtSWkzMF0+ZfmYTU2rtAt2dl2I\n\tA00W/75NQy23X0OkiEpCGX9Y78nONKtuwb+YX/voSHj8Djxf0oDkk4DFQFaMa6X6EW\n\tyk6ALOhMn2Zrf5WhC8rMLqPIi4Xb5keNL21pl8IOrcBd8C+paB3i6yfAkfZqenxLcx\n\t9yrkXxrEyxdIg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671834497;\n\tbh=PwN6bBvpTnHKFaeLvEJreyajLWCnKs8XypUReWpqM3w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z7e9z3LRqWpOG2BzsfBKtUmRQ7nxuAYRuqk/k6MmEwp8mcHgiRGuZXBp7OiwC9Jh7\n\tM5BZl9KVUbq3izxvGjG8B43JSnTJKE1UiGBDz/oP/DoEqWtvIZdTCMd3JKOYxZ/BlU\n\tGqiJVhgTSZE0dsYKrtpHCvbiwtdy3yHLpGMc+zAs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Z7e9z3LR\"; dkim-atps=neutral","Date":"Fri, 23 Dec 2022 16:28:11 -0600","To":"Umang Jain ","Message-ID":"","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-6-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3:\n\tDon't rely on bufferCount","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","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]