[{"id":1276,"web_url":"https://patchwork.libcamera.org/comment/1276/","msgid":"<20190405113000.GY23466@bigcity.dyn.berto.se>","date":"2019-04-05T11:30:00","subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-04-03 17:07:31 +0200, Jacopo Mondi wrote:\n> Perform allocation and setup of memory sharing betweent the CIO2 output\n> and the ImgU input and allocate memory for each active stream.\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 78 ++++++++++++++++++++++------\n> 1 file changed, 62 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index a12e3f9a496d..f7e75fac1dfe 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -91,6 +91,7 @@ public:\n> \n> \tBufferPool vfPool_;\n> \tBufferPool statPool_;\n> +\tBufferPool outPool_;\n> };\n> \n> class CIO2Device\n> @@ -428,13 +429,21 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and\n> + * started even if not in use. As of now, if not properly configured and\n> + * enabled, the ImgU processing pipeline stalls.\n> + *\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> \t\t\t\t\t const std::set &streams)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tStream *stream = *streams.begin();\n> \tCIO2Device *cio2 = &data->cio2_;\n> \tImgUDevice *imgu = data->imgu_;\n> +\tunsigned int bufferCount;\n> \tint ret;\n> \n> \t/* Share buffers between CIO2 output and ImgU input. */\n> @@ -446,28 +455,64 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\t/* Export ImgU output buffers to the stream's pool. */\n> -\tret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> \t/*\n> -\t * Reserve memory in viewfinder and stat output devices. Use the\n> -\t * same number of buffers as the ones requested for the output\n> -\t * stream.\n> +\t * Use for internal pools the same buffer number as the input\n> +\t * one: it should match the pipeline lenght.\n> \t */\n> -\tunsigned int bufferCount = stream->bufferPool().count();\n> -\n> -\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> -\tret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> +\tbufferCount = pool->count();\n> \timgu->stat_.pool->createBuffers(bufferCount);\n> \tret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tfor (Stream *stream : streams) {\n> +\t\tif (isOutput(data, stream)) {\n> +\t\t\t/* Export output buffers to the stream's pool. */\n> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> +\t\t\t\t\t\t &stream->bufferPool());\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tif (isViewfinderActive(data))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Reserve memory in viewfinder device if it is not\n> +\t\t\t * part of the active streams list. Use the same\n> +\t\t\t * number of buffers as the ones requested for the\n> +\t\t\t * output stream.\n> +\t\t\t */\n> +\t\t\tbufferCount = stream->bufferPool().count();\n> +\t\t\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> +\t\t\t\t\t\t imgu->viewfinder_.pool);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t} else if (isViewfinder(data, stream)) {\n> +\t\t\t/* Export viewfinder buffers to the stream's pool. */\n> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> +\t\t\t\t\t\t &stream->bufferPool());\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tif (isOutputActive(data))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Reserve memory in output device if it is not part\n> +\t\t\t * of the active streams list. Use the same number\n> +\t\t\t * of buffers as the ones requested for the viewfinder\n> +\t\t\t * stream.\n> +\t\t\t */\n> +\t\t\tbufferCount = stream->bufferPool().count();\n> +\t\t\timgu->output_.pool->createBuffers(bufferCount);\n> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> +\t\t\t\t\t\t imgu->output_.pool);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n\nThis change looks ok to me but I wonder about the error path here. What \nhappens if exportBuffers fails for the second stream? I'm only curious \n:-)\n\n> \treturn 0;\n> }\n> \n> @@ -819,6 +864,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> \n> \toutput_.pad = PAD_OUTPUT;\n> \toutput_.name = \"output\";\n> +\toutput_.pool = &outPool_;\n> \n> \tviewfinder_.dev = V4L2Device::fromEntityName(media,\n> \t\t\t\t\t\t name_ + \" viewfinder\");\n> -- \n> 2.21.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A11FF60DB3\n\tfor ;\n\tFri, 5 Apr 2019 13:30:32 +0200 (CEST)","by mail-lj1-x22a.google.com with SMTP id v13so4933276ljk.4\n\tfor ;\n\tFri, 05 Apr 2019 04:30:32 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tf18sm4613330lja.91.2019.04.05.04.30.00\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 05 Apr 2019 04:30:00 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=ctmsINLvEqkPszD9mnb6t2fbFbdJZmtHMiEEZiqxBoA=;\n\tb=oSkIGNBcZJFuin3e03PySBUlvzz3Z6lPw1LP4NTgYCzw4XhV9NwfCUHgX/gULxIjuL\n\tepywWd1oMpalrIkVdcaAzQyRRaMp6DLmHLqMQ21hQhfjZmnwQYQPNBUSbPjbO+LYcDq8\n\tIQJroPRHEn2DJbGw34aOsRPip4EVkRaUDyQu6k7P9XNNp94U194w1scSZlsl70Axiu3I\n\tyFrQ5Qh/Qe1nGxvaPXczhYlUEZuQyTpl6J/4yI9G8YmkanI+0MS312tcQLwjffoAxGUg\n\tWBTnyIX3Wd0QqksoVqJPpfdE/JmKYfw1AjaiQfZrwcE5joDQY4u1Pk2rdVcdFiEL1jKX\n\t8GaQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=ctmsINLvEqkPszD9mnb6t2fbFbdJZmtHMiEEZiqxBoA=;\n\tb=SDfguqvyugeOSnCnyVa6FE2N6ykTCtoz1QOmXbu4r2nisJLgwydkYJ0n0EcDWGVV0k\n\tbpuFF0Tzko23NV9Ar3o6jLzUSD/ihT/BLqob6tAF2612NpTdW3fKJd8zfyUmEgRsub1N\n\tqE2bGu9aIdNVCbQO4oiMw/OgmcFZTl+kMEhr4fWuB7i1U3KGmB5mqEXpFHmc/N7HO36c\n\tLtqBEJEs3B41ZXYSKKcmww37OF8ZJoN3AUF2PVsTzbGP+IWABQg3MpfeSJlRhlksyUb0\n\ttD0lonCUXwcpPp5Iv1F92IJT+vKvU5vriT+3bZF2jBPt7JjRCnbaVhHX+YY+KpiGDJUp\n\tGufg==","X-Gm-Message-State":"APjAAAUp+q2jNlC5KU4hDoaBfZ9yi75VI+c9SaYdFKgDWg/TijpBFiDM\n\tLeuEjOcX9pGPDvp2YwuMetLZ8Q==","X-Google-Smtp-Source":"APXvYqxU86YfcRiiNNMRIKuEbRxPazZneYRHf8bZQThWlsvFwc9tqSxso8h9sX9t8TYDb0eCSiYv1Q==","X-Received":"by 2002:a2e:8693:: with SMTP id l19mr6581960lji.47.1554463801046;\n\tFri, 05 Apr 2019 04:30:01 -0700 (PDT)","Date":"Fri, 5 Apr 2019 13:30:00 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405113000.GY23466@bigcity.dyn.berto.se>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190403150735.27580-5-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 05 Apr 2019 11:30:32 -0000"}},{"id":1286,"web_url":"https://patchwork.libcamera.org/comment/1286/","msgid":"<20190405154501.GC4636@pendragon.ideasonboard.com>","date":"2019-04-05T15:45:01","subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Apr 03, 2019 at 05:07:31PM +0200, Jacopo Mondi wrote:\n> Perform allocation and setup of memory sharing betweent the CIO2 output\n\ns/betweent/between/\n\n> and the ImgU input and allocate memory for each active stream.\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 78 ++++++++++++++++++++++------\n> 1 file changed, 62 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index a12e3f9a496d..f7e75fac1dfe 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -91,6 +91,7 @@ public:\n> \n> \tBufferPool vfPool_;\n> \tBufferPool statPool_;\n> +\tBufferPool outPool_;\n> };\n> \n> class CIO2Device\n> @@ -428,13 +429,21 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and\n> + * started even if not in use. As of now, if not properly configured and\n> + * enabled, the ImgU processing pipeline stalls.\n> + *\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> \t\t\t\t\t const std::set &streams)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tStream *stream = *streams.begin();\n> \tCIO2Device *cio2 = &data->cio2_;\n> \tImgUDevice *imgu = data->imgu_;\n> +\tunsigned int bufferCount;\n> \tint ret;\n> \n> \t/* Share buffers between CIO2 output and ImgU input. */\n> @@ -446,28 +455,64 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\t/* Export ImgU output buffers to the stream's pool. */\n> -\tret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> \t/*\n> -\t * Reserve memory in viewfinder and stat output devices. Use the\n> -\t * same number of buffers as the ones requested for the output\n> -\t * stream.\n> +\t * Use for internal pools the same buffer number as the input\n\ns/buffer number/number of buffers/\ns/as the input one/as for the input pool/\n\n> +\t * one: it should match the pipeline lenght.\n\ns/lenght/length/\n\nAnd what do you mean by pipeline length here ?\n\n> \t */\n> -\tunsigned int bufferCount = stream->bufferPool().count();\n> -\n> -\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> -\tret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> +\tbufferCount = pool->count();\n\nI think you can keep the variable declaration here, there's no need to\nmove it to the beginning of the function.\n\n> \timgu->stat_.pool->createBuffers(bufferCount);\n> \tret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tfor (Stream *stream : streams) {\n> +\t\tif (isOutput(data, stream)) {\n> +\t\t\t/* Export output buffers to the stream's pool. */\n> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> +\t\t\t\t\t\t &stream->bufferPool());\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tif (isViewfinderActive(data))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Reserve memory in viewfinder device if it is not\n> +\t\t\t * part of the active streams list. Use the same\n> +\t\t\t * number of buffers as the ones requested for the\n> +\t\t\t * output stream.\n> +\t\t\t */\n> +\t\t\tbufferCount = stream->bufferPool().count();\n> +\t\t\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> +\t\t\t\t\t\t imgu->viewfinder_.pool);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t} else if (isViewfinder(data, stream)) {\n> +\t\t\t/* Export viewfinder buffers to the stream's pool. */\n> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> +\t\t\t\t\t\t &stream->bufferPool());\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\tif (isOutputActive(data))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Reserve memory in output device if it is not part\n> +\t\t\t * of the active streams list. Use the same number\n> +\t\t\t * of buffers as the ones requested for the viewfinder\n> +\t\t\t * stream.\n> +\t\t\t */\n> +\t\t\tbufferCount = stream->bufferPool().count();\n> +\t\t\timgu->output_.pool->createBuffers(bufferCount);\n> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> +\t\t\t\t\t\t imgu->output_.pool);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> +\t}\n\nAs for the previous patch, I think you should allocate memory for the\nstreams present in the set in a loop without caring about the stream\ntype, and then for the other devices outside of the loop. It should also\nease error handling, which is missing here.\n\n> +\n> \treturn 0;\n> }\n> \n> @@ -819,6 +864,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> \n> \toutput_.pad = PAD_OUTPUT;\n> \toutput_.name = \"output\";\n> +\toutput_.pool = &outPool_;\n> \n> \tviewfinder_.dev = V4L2Device::fromEntityName(media,\n> \t\t\t\t\t\t name_ + \" viewfinder\");","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3C4260DB3\n\tfor ;\n\tFri, 5 Apr 2019 17:45:12 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.140.214.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B7EEE2;\n\tFri, 5 Apr 2019 17:45:12 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554479112;\n\tbh=kzJaMNHn2Qxf+QdoY5OQhfg5+3OEoEIVCGxViSTQ1Mc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JMfvzDRn26j3dKFzOLxvvzDPcoxPqTJa3T8gibAyAvnApk56TyAT4mBDHQMNl1Bix\n\t1FY4QISrd2M+Dio+zGwiPphe1ZBQVwaCqufpYwNE0LAyX8o67qKKQJaip3ardSLkCp\n\t/cdEAcYLqTP+qQlGs2DNSXj3gmzrLGBpUTI1qxM8=","Date":"Fri, 5 Apr 2019 18:45:01 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405154501.GC4636@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190403150735.27580-5-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 05 Apr 2019 15:45:13 -0000"}},{"id":1319,"web_url":"https://patchwork.libcamera.org/comment/1319/","msgid":"<20190408155028.5d2eobqlrakljju4@uno.localdomain>","date":"2019-04-08T15:50:28","subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Niklas,\n replying to a comment you both had.\n\nOn Fri, Apr 05, 2019 at 06:45:01PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 03, 2019 at 05:07:31PM +0200, Jacopo Mondi wrote:\n> > Perform allocation and setup of memory sharing betweent the CIO2 output\n>\n> s/betweent/between/\n>\n> > and the ImgU input and allocate memory for each active stream.\n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 78 ++++++++++++++++++++++------\n> > 1 file changed, 62 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index a12e3f9a496d..f7e75fac1dfe 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -91,6 +91,7 @@ public:\n> >\n> > \tBufferPool vfPool_;\n> > \tBufferPool statPool_;\n> > +\tBufferPool outPool_;\n> > };\n> >\n> > class CIO2Device\n> > @@ -428,13 +429,21 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > \treturn 0;\n> > }\n> >\n> > +/**\n> > + * \\todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and\n> > + * started even if not in use. As of now, if not properly configured and\n> > + * enabled, the ImgU processing pipeline stalls.\n> > + *\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> > \t\t\t\t\t const std::set &streams)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tStream *stream = *streams.begin();\n> > \tCIO2Device *cio2 = &data->cio2_;\n> > \tImgUDevice *imgu = data->imgu_;\n> > +\tunsigned int bufferCount;\n> > \tint ret;\n> >\n> > \t/* Share buffers between CIO2 output and ImgU input. */\n> > @@ -446,28 +455,64 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > -\t/* Export ImgU output buffers to the stream's pool. */\n> > -\tret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > \t/*\n> > -\t * Reserve memory in viewfinder and stat output devices. Use the\n> > -\t * same number of buffers as the ones requested for the output\n> > -\t * stream.\n> > +\t * Use for internal pools the same buffer number as the input\n>\n> s/buffer number/number of buffers/\n> s/as the input one/as for the input pool/\n>\n> > +\t * one: it should match the pipeline lenght.\n>\n> s/lenght/length/\n>\n> And what do you mean by pipeline length here ?\n>\n> > \t */\n> > -\tunsigned int bufferCount = stream->bufferPool().count();\n> > -\n> > -\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> > -\tret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > +\tbufferCount = pool->count();\n>\n> I think you can keep the variable declaration here, there's no need to\n> move it to the beginning of the function.\n>\n> > \timgu->stat_.pool->createBuffers(bufferCount);\n> > \tret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > +\tfor (Stream *stream : streams) {\n> > +\t\tif (isOutput(data, stream)) {\n> > +\t\t\t/* Export output buffers to the stream's pool. */\n> > +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> > +\t\t\t\t\t\t &stream->bufferPool());\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tif (isViewfinderActive(data))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Reserve memory in viewfinder device if it is not\n> > +\t\t\t * part of the active streams list. Use the same\n> > +\t\t\t * number of buffers as the ones requested for the\n> > +\t\t\t * output stream.\n> > +\t\t\t */\n> > +\t\t\tbufferCount = stream->bufferPool().count();\n> > +\t\t\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> > +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> > +\t\t\t\t\t\t imgu->viewfinder_.pool);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t} else if (isViewfinder(data, stream)) {\n> > +\t\t\t/* Export viewfinder buffers to the stream's pool. */\n> > +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> > +\t\t\t\t\t\t &stream->bufferPool());\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\n> > +\t\t\tif (isOutputActive(data))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Reserve memory in output device if it is not part\n> > +\t\t\t * of the active streams list. Use the same number\n> > +\t\t\t * of buffers as the ones requested for the viewfinder\n> > +\t\t\t * stream.\n> > +\t\t\t */\n> > +\t\t\tbufferCount = stream->bufferPool().count();\n> > +\t\t\timgu->output_.pool->createBuffers(bufferCount);\n> > +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> > +\t\t\t\t\t\t imgu->output_.pool);\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n>\n> As for the previous patch, I think you should allocate memory for the\n> streams present in the set in a loop without caring about the stream\n> type, and then for the other devices outside of the loop. It should also\n> ease error handling, which is missing here.\n\nBoth you and Niklas reported that errors are not handled by this\nfunction. Actually, if allocateBuffers fail, the Camera class calls\nfreeBuffers, which will free the allocated one and will simply call\nreqBuffers(0) on the non allocated one. In my opinion this seems safe,\nhave I mis-interpreted something?\n\nThanks\n j\n\n>\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -819,6 +864,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> >\n> > \toutput_.pad = PAD_OUTPUT;\n> > \toutput_.name = \"output\";\n> > +\toutput_.pool = &outPool_;\n> >\n> > \tviewfinder_.dev = V4L2Device::fromEntityName(media,\n> > \t\t\t\t\t\t name_ + \" viewfinder\");\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B74760B2E\n\tfor ;\n\tMon, 8 Apr 2019 17:49:41 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 780F41BF20A;\n\tMon, 8 Apr 2019 15:49:40 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 8 Apr 2019 17:50:28 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190408155028.5d2eobqlrakljju4@uno.localdomain>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-5-jacopo@jmondi.org>\n\t<20190405154501.GC4636@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"e2zezhbblsd42pm4\"","Content-Disposition":"inline","In-Reply-To":"<20190405154501.GC4636@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 08 Apr 2019 15:49:41 -0000"}},{"id":1322,"web_url":"https://patchwork.libcamera.org/comment/1322/","msgid":"<20190409140639.GB4578@pendragon.ideasonboard.com>","date":"2019-04-09T14:06:39","subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 08, 2019 at 05:50:28PM +0200, Jacopo Mondi wrote:\n> On Fri, Apr 05, 2019 at 06:45:01PM +0300, Laurent Pinchart wrote:\n> > On Wed, Apr 03, 2019 at 05:07:31PM +0200, Jacopo Mondi wrote:\n> >> Perform allocation and setup of memory sharing betweent the CIO2 output\n> >\n> > s/betweent/between/\n> >\n> >> and the ImgU input and allocate memory for each active stream.\n> >> ---\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 78 ++++++++++++++++++++++------\n> >> 1 file changed, 62 insertions(+), 16 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index a12e3f9a496d..f7e75fac1dfe 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -91,6 +91,7 @@ public:\n> >>\n> >> \tBufferPool vfPool_;\n> >> \tBufferPool statPool_;\n> >> +\tBufferPool outPool_;\n> >> };\n> >>\n> >> class CIO2Device\n> >> @@ -428,13 +429,21 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >> \treturn 0;\n> >> }\n> >>\n> >> +/**\n> >> + * \\todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and\n> >> + * started even if not in use. As of now, if not properly configured and\n> >> + * enabled, the ImgU processing pipeline stalls.\n> >> + *\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> >> \t\t\t\t\t const std::set &streams)\n> >> {\n> >> \tIPU3CameraData *data = cameraData(camera);\n> >> -\tStream *stream = *streams.begin();\n> >> \tCIO2Device *cio2 = &data->cio2_;\n> >> \tImgUDevice *imgu = data->imgu_;\n> >> +\tunsigned int bufferCount;\n> >> \tint ret;\n> >>\n> >> \t/* Share buffers between CIO2 output and ImgU input. */\n> >> @@ -446,28 +455,64 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> >> \tif (ret)\n> >> \t\treturn ret;\n> >>\n> >> -\t/* Export ImgU output buffers to the stream's pool. */\n> >> -\tret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> -\n> >> \t/*\n> >> -\t * Reserve memory in viewfinder and stat output devices. Use the\n> >> -\t * same number of buffers as the ones requested for the output\n> >> -\t * stream.\n> >> +\t * Use for internal pools the same buffer number as the input\n> >\n> > s/buffer number/number of buffers/\n> > s/as the input one/as for the input pool/\n> >\n> >> +\t * one: it should match the pipeline lenght.\n> >\n> > s/lenght/length/\n> >\n> > And what do you mean by pipeline length here ?\n> >\n> >> \t */\n> >> -\tunsigned int bufferCount = stream->bufferPool().count();\n> >> -\n> >> -\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> >> -\tret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> -\n> >> +\tbufferCount = pool->count();\n> >\n> > I think you can keep the variable declaration here, there's no need to\n> > move it to the beginning of the function.\n> >\n> >> \timgu->stat_.pool->createBuffers(bufferCount);\n> >> \tret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);\n> >> \tif (ret)\n> >> \t\treturn ret;\n> >>\n> >> +\tfor (Stream *stream : streams) {\n> >> +\t\tif (isOutput(data, stream)) {\n> >> +\t\t\t/* Export output buffers to the stream's pool. */\n> >> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> >> +\t\t\t\t\t\t &stream->bufferPool());\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\n> >> +\t\t\tif (isViewfinderActive(data))\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\t/*\n> >> +\t\t\t * Reserve memory in viewfinder device if it is not\n> >> +\t\t\t * part of the active streams list. Use the same\n> >> +\t\t\t * number of buffers as the ones requested for the\n> >> +\t\t\t * output stream.\n> >> +\t\t\t */\n> >> +\t\t\tbufferCount = stream->bufferPool().count();\n> >> +\t\t\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> >> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> >> +\t\t\t\t\t\t imgu->viewfinder_.pool);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\t\t} else if (isViewfinder(data, stream)) {\n> >> +\t\t\t/* Export viewfinder buffers to the stream's pool. */\n> >> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> >> +\t\t\t\t\t\t &stream->bufferPool());\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\n> >> +\t\t\tif (isOutputActive(data))\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\t/*\n> >> +\t\t\t * Reserve memory in output device if it is not part\n> >> +\t\t\t * of the active streams list. Use the same number\n> >> +\t\t\t * of buffers as the ones requested for the viewfinder\n> >> +\t\t\t * stream.\n> >> +\t\t\t */\n> >> +\t\t\tbufferCount = stream->bufferPool().count();\n> >> +\t\t\timgu->output_.pool->createBuffers(bufferCount);\n> >> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> >> +\t\t\t\t\t\t imgu->output_.pool);\n> >> +\t\t\tif (ret)\n> >> +\t\t\t\treturn ret;\n> >> +\t\t}\n> >> +\t}\n> >\n> > As for the previous patch, I think you should allocate memory for the\n> > streams present in the set in a loop without caring about the stream\n> > type, and then for the other devices outside of the loop. It should also\n> > ease error handling, which is missing here.\n> \n> Both you and Niklas reported that errors are not handled by this\n> function. Actually, if allocateBuffers fail, the Camera class calls\n> freeBuffers, which will free the allocated one and will simply call\n> reqBuffers(0) on the non allocated one. In my opinion this seems safe,\n> have I mis-interpreted something?\n\nDo you think it's safe to call the freeBuffers() function if\nallocateBuffers() failed ? That would require handling partial\nallocation in freeBuffers(). I think it would be best to clean up on\nfailure in allocateBuffers(), and not call freeBuffers() in that case.\nThe code to handle failure after partial allocation will then be close\nto the location of the failure, I think that would be less error-prone.\n\n> >> +\n> >> \treturn 0;\n> >> }\n> >>\n> >> @@ -819,6 +864,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> >>\n> >> \toutput_.pad = PAD_OUTPUT;\n> >> \toutput_.name = \"output\";\n> >> +\toutput_.pool = &outPool_;\n> >>\n> >> \tviewfinder_.dev = V4L2Device::fromEntityName(media,\n> >> \t\t\t\t\t\t name_ + \" viewfinder\");","headers":{"Return-Path":"","Received":["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 40C1560DB4\n\tfor ;\n\tTue, 9 Apr 2019 16:06:50 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:2788:66a:3eb:e14:8605:25ce:b577])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A9FCC33A;\n\tTue, 9 Apr 2019 16:06:49 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554818809;\n\tbh=rB1gwqhEiMFg6aU/B2OodP8eE55F4M8ovXz0/c2FHLE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p/gJ8DwBaSZnlDJBfSVfwNZPhByUpdyyaWSxYoXFf4/Q5IrPrgZ4uQsZjCVNqoxw0\n\tgsZgj/K0DxKW5E+vlAqeQwUVpSlnrz65s/jDGGKvIorV4aTwxvUlPhl4lPtY1BnDSx\n\tqk+MkuAt5XMllsAbFUEgqAGr0aZGmWHmmhzAxa+k=","Date":"Tue, 9 Apr 2019 17:06:39 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190409140639.GB4578@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-5-jacopo@jmondi.org>\n\t<20190405154501.GC4636@pendragon.ideasonboard.com>\n\t<20190408155028.5d2eobqlrakljju4@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190408155028.5d2eobqlrakljju4@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 09 Apr 2019 14:06:50 -0000"}},{"id":1328,"web_url":"https://patchwork.libcamera.org/comment/1328/","msgid":"<20190409182142.tmgkm2tmg5qukzct@uno.localdomain>","date":"2019-04-09T18:21:42","subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 09, 2019 at 05:06:39PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Apr 08, 2019 at 05:50:28PM +0200, Jacopo Mondi wrote:\n> > On Fri, Apr 05, 2019 at 06:45:01PM +0300, Laurent Pinchart wrote:\n> > > On Wed, Apr 03, 2019 at 05:07:31PM +0200, Jacopo Mondi wrote:\n> > >> Perform allocation and setup of memory sharing betweent the CIO2 output\n> > >\n> > > s/betweent/between/\n> > >\n> > >> and the ImgU input and allocate memory for each active stream.\n> > >> ---\n> > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 78 ++++++++++++++++++++++------\n> > >> 1 file changed, 62 insertions(+), 16 deletions(-)\n> > >>\n> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> index a12e3f9a496d..f7e75fac1dfe 100644\n> > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> @@ -91,6 +91,7 @@ public:\n> > >>\n> > >> \tBufferPool vfPool_;\n> > >> \tBufferPool statPool_;\n> > >> +\tBufferPool outPool_;\n> > >> };\n> > >>\n> > >> class CIO2Device\n> > >> @@ -428,13 +429,21 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > >> \treturn 0;\n> > >> }\n> > >>\n> > >> +/**\n> > >> + * \\todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and\n> > >> + * started even if not in use. As of now, if not properly configured and\n> > >> + * enabled, the ImgU processing pipeline stalls.\n> > >> + *\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> > >> \t\t\t\t\t const std::set &streams)\n> > >> {\n> > >> \tIPU3CameraData *data = cameraData(camera);\n> > >> -\tStream *stream = *streams.begin();\n> > >> \tCIO2Device *cio2 = &data->cio2_;\n> > >> \tImgUDevice *imgu = data->imgu_;\n> > >> +\tunsigned int bufferCount;\n> > >> \tint ret;\n> > >>\n> > >> \t/* Share buffers between CIO2 output and ImgU input. */\n> > >> @@ -446,28 +455,64 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> > >> \tif (ret)\n> > >> \t\treturn ret;\n> > >>\n> > >> -\t/* Export ImgU output buffers to the stream's pool. */\n> > >> -\tret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> -\n> > >> \t/*\n> > >> -\t * Reserve memory in viewfinder and stat output devices. Use the\n> > >> -\t * same number of buffers as the ones requested for the output\n> > >> -\t * stream.\n> > >> +\t * Use for internal pools the same buffer number as the input\n> > >\n> > > s/buffer number/number of buffers/\n> > > s/as the input one/as for the input pool/\n> > >\n> > >> +\t * one: it should match the pipeline lenght.\n> > >\n> > > s/lenght/length/\n> > >\n> > > And what do you mean by pipeline length here ?\n> > >\n> > >> \t */\n> > >> -\tunsigned int bufferCount = stream->bufferPool().count();\n> > >> -\n> > >> -\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> > >> -\tret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> -\n> > >> +\tbufferCount = pool->count();\n> > >\n> > > I think you can keep the variable declaration here, there's no need to\n> > > move it to the beginning of the function.\n> > >\n> > >> \timgu->stat_.pool->createBuffers(bufferCount);\n> > >> \tret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);\n> > >> \tif (ret)\n> > >> \t\treturn ret;\n> > >>\n> > >> +\tfor (Stream *stream : streams) {\n> > >> +\t\tif (isOutput(data, stream)) {\n> > >> +\t\t\t/* Export output buffers to the stream's pool. */\n> > >> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> > >> +\t\t\t\t\t\t &stream->bufferPool());\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\n> > >> +\t\t\tif (isViewfinderActive(data))\n> > >> +\t\t\t\tcontinue;\n> > >> +\n> > >> +\t\t\t/*\n> > >> +\t\t\t * Reserve memory in viewfinder device if it is not\n> > >> +\t\t\t * part of the active streams list. Use the same\n> > >> +\t\t\t * number of buffers as the ones requested for the\n> > >> +\t\t\t * output stream.\n> > >> +\t\t\t */\n> > >> +\t\t\tbufferCount = stream->bufferPool().count();\n> > >> +\t\t\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> > >> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> > >> +\t\t\t\t\t\t imgu->viewfinder_.pool);\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\t\t} else if (isViewfinder(data, stream)) {\n> > >> +\t\t\t/* Export viewfinder buffers to the stream's pool. */\n> > >> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> > >> +\t\t\t\t\t\t &stream->bufferPool());\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\n> > >> +\t\t\tif (isOutputActive(data))\n> > >> +\t\t\t\tcontinue;\n> > >> +\n> > >> +\t\t\t/*\n> > >> +\t\t\t * Reserve memory in output device if it is not part\n> > >> +\t\t\t * of the active streams list. Use the same number\n> > >> +\t\t\t * of buffers as the ones requested for the viewfinder\n> > >> +\t\t\t * stream.\n> > >> +\t\t\t */\n> > >> +\t\t\tbufferCount = stream->bufferPool().count();\n> > >> +\t\t\timgu->output_.pool->createBuffers(bufferCount);\n> > >> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> > >> +\t\t\t\t\t\t imgu->output_.pool);\n> > >> +\t\t\tif (ret)\n> > >> +\t\t\t\treturn ret;\n> > >> +\t\t}\n> > >> +\t}\n> > >\n> > > As for the previous patch, I think you should allocate memory for the\n> > > streams present in the set in a loop without caring about the stream\n> > > type, and then for the other devices outside of the loop. It should also\n> > > ease error handling, which is missing here.\n> >\n> > Both you and Niklas reported that errors are not handled by this\n> > function. Actually, if allocateBuffers fail, the Camera class calls\n> > freeBuffers, which will free the allocated one and will simply call\n> > reqBuffers(0) on the non allocated one. In my opinion this seems safe,\n> > have I mis-interpreted something?\n>\n> Do you think it's safe to call the freeBuffers() function if\n> allocateBuffers() failed ? That would require handling partial\n> allocation in freeBuffers(). I think it would be best to clean up on\n> failure in allocateBuffers(), and not call freeBuffers() in that case.\n> The code to handle failure after partial allocation will then be close\n> to the location of the failure, I think that would be less error-prone.\n>\n\nIs this the part that concerns you?\nfrom: Documentation/output/media/uapi/v4l/vidioc-reqbufs.html\n Note that if any buffers are still mapped or exported via DMABUF,\n then ioctl VIDIOC_REQBUFS can only succeed if the\n V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS capability is set. Otherwise\n ioctl VIDIOC_REQBUFS will return the EBUSY error code.\n\nAs I read this, and if that's you concern, we should close and unmap\nany exported buffers before calling reqbufs(0).\n\nI think that if allocation fails (in particular if exportBuffers() fails)\nthen the V4L2Device destroy all the buffer planes, closing any open fd\nand unmapping them. Then we can freeBuffers in the driver memory\nthrough freeBuffers. For non-yet-allocated buffers, they would receive\na reqbufs(0) ioctl, before any fd has been mapped or exported. It\nseems ok to me, have I missed something?\n\nThanks\n j\n\n> > >> +\n> > >> \treturn 0;\n> > >> }\n> > >>\n> > >> @@ -819,6 +864,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> > >>\n> > >> \toutput_.pad = PAD_OUTPUT;\n> > >> \toutput_.name = \"output\";\n> > >> +\toutput_.pool = &outPool_;\n> > >>\n> > >> \tviewfinder_.dev = V4L2Device::fromEntityName(media,\n> > >> \t\t\t\t\t\t name_ + \" viewfinder\");\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D1DA60004\n\tfor ;\n\tTue, 9 Apr 2019 20:20:54 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 04091C0004;\n\tTue, 9 Apr 2019 18:20:53 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 9 Apr 2019 20:21:42 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190409182142.tmgkm2tmg5qukzct@uno.localdomain>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-5-jacopo@jmondi.org>\n\t<20190405154501.GC4636@pendragon.ideasonboard.com>\n\t<20190408155028.5d2eobqlrakljju4@uno.localdomain>\n\t<20190409140639.GB4578@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"wv5577i5v3rmxlxl\"","Content-Disposition":"inline","In-Reply-To":"<20190409140639.GB4578@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 09 Apr 2019 18:20:54 -0000"}},{"id":1340,"web_url":"https://patchwork.libcamera.org/comment/1340/","msgid":"<20190415075611.GA4809@pendragon.ideasonboard.com>","date":"2019-04-15T07:56:11","subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Apr 09, 2019 at 08:21:42PM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 09, 2019 at 05:06:39PM +0300, Laurent Pinchart wrote:\n> > On Mon, Apr 08, 2019 at 05:50:28PM +0200, Jacopo Mondi wrote:\n> > > On Fri, Apr 05, 2019 at 06:45:01PM +0300, Laurent Pinchart wrote:\n> > > > On Wed, Apr 03, 2019 at 05:07:31PM +0200, Jacopo Mondi wrote:\n> > > >> Perform allocation and setup of memory sharing betweent the CIO2 output\n> > > >\n> > > > s/betweent/between/\n> > > >\n> > > >> and the ImgU input and allocate memory for each active stream.\n> > > >> ---\n> > > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 78 ++++++++++++++++++++++------\n> > > >> 1 file changed, 62 insertions(+), 16 deletions(-)\n> > > >>\n> > > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > >> index a12e3f9a496d..f7e75fac1dfe 100644\n> > > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > >> @@ -91,6 +91,7 @@ public:\n> > > >>\n> > > >> \tBufferPool vfPool_;\n> > > >> \tBufferPool statPool_;\n> > > >> +\tBufferPool outPool_;\n> > > >> };\n> > > >>\n> > > >> class CIO2Device\n> > > >> @@ -428,13 +429,21 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > > >> \treturn 0;\n> > > >> }\n> > > >>\n> > > >> +/**\n> > > >> + * \\todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and\n> > > >> + * started even if not in use. As of now, if not properly configured and\n> > > >> + * enabled, the ImgU processing pipeline stalls.\n> > > >> + *\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> > > >> \t\t\t\t\t const std::set &streams)\n> > > >> {\n> > > >> \tIPU3CameraData *data = cameraData(camera);\n> > > >> -\tStream *stream = *streams.begin();\n> > > >> \tCIO2Device *cio2 = &data->cio2_;\n> > > >> \tImgUDevice *imgu = data->imgu_;\n> > > >> +\tunsigned int bufferCount;\n> > > >> \tint ret;\n> > > >>\n> > > >> \t/* Share buffers between CIO2 output and ImgU input. */\n> > > >> @@ -446,28 +455,64 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> > > >> \tif (ret)\n> > > >> \t\treturn ret;\n> > > >>\n> > > >> -\t/* Export ImgU output buffers to the stream's pool. */\n> > > >> -\tret = imgu->exportBuffers(&imgu->output_, &stream->bufferPool());\n> > > >> -\tif (ret)\n> > > >> -\t\treturn ret;\n> > > >> -\n> > > >> \t/*\n> > > >> -\t * Reserve memory in viewfinder and stat output devices. Use the\n> > > >> -\t * same number of buffers as the ones requested for the output\n> > > >> -\t * stream.\n> > > >> +\t * Use for internal pools the same buffer number as the input\n> > > >\n> > > > s/buffer number/number of buffers/\n> > > > s/as the input one/as for the input pool/\n> > > >\n> > > >> +\t * one: it should match the pipeline lenght.\n> > > >\n> > > > s/lenght/length/\n> > > >\n> > > > And what do you mean by pipeline length here ?\n> > > >\n> > > >> \t */\n> > > >> -\tunsigned int bufferCount = stream->bufferPool().count();\n> > > >> -\n> > > >> -\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> > > >> -\tret = imgu->exportBuffers(&imgu->viewfinder_, imgu->viewfinder_.pool);\n> > > >> -\tif (ret)\n> > > >> -\t\treturn ret;\n> > > >> -\n> > > >> +\tbufferCount = pool->count();\n> > > >\n> > > > I think you can keep the variable declaration here, there's no need to\n> > > > move it to the beginning of the function.\n> > > >\n> > > >> \timgu->stat_.pool->createBuffers(bufferCount);\n> > > >> \tret = imgu->exportBuffers(&imgu->stat_, imgu->stat_.pool);\n> > > >> \tif (ret)\n> > > >> \t\treturn ret;\n> > > >>\n> > > >> +\tfor (Stream *stream : streams) {\n> > > >> +\t\tif (isOutput(data, stream)) {\n> > > >> +\t\t\t/* Export output buffers to the stream's pool. */\n> > > >> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> > > >> +\t\t\t\t\t\t &stream->bufferPool());\n> > > >> +\t\t\tif (ret)\n> > > >> +\t\t\t\treturn ret;\n> > > >> +\n> > > >> +\t\t\tif (isViewfinderActive(data))\n> > > >> +\t\t\t\tcontinue;\n> > > >> +\n> > > >> +\t\t\t/*\n> > > >> +\t\t\t * Reserve memory in viewfinder device if it is not\n> > > >> +\t\t\t * part of the active streams list. Use the same\n> > > >> +\t\t\t * number of buffers as the ones requested for the\n> > > >> +\t\t\t * output stream.\n> > > >> +\t\t\t */\n> > > >> +\t\t\tbufferCount = stream->bufferPool().count();\n> > > >> +\t\t\timgu->viewfinder_.pool->createBuffers(bufferCount);\n> > > >> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> > > >> +\t\t\t\t\t\t imgu->viewfinder_.pool);\n> > > >> +\t\t\tif (ret)\n> > > >> +\t\t\t\treturn ret;\n> > > >> +\t\t} else if (isViewfinder(data, stream)) {\n> > > >> +\t\t\t/* Export viewfinder buffers to the stream's pool. */\n> > > >> +\t\t\tret = imgu->exportBuffers(&imgu->viewfinder_,\n> > > >> +\t\t\t\t\t\t &stream->bufferPool());\n> > > >> +\t\t\tif (ret)\n> > > >> +\t\t\t\treturn ret;\n> > > >> +\n> > > >> +\t\t\tif (isOutputActive(data))\n> > > >> +\t\t\t\tcontinue;\n> > > >> +\n> > > >> +\t\t\t/*\n> > > >> +\t\t\t * Reserve memory in output device if it is not part\n> > > >> +\t\t\t * of the active streams list. Use the same number\n> > > >> +\t\t\t * of buffers as the ones requested for the viewfinder\n> > > >> +\t\t\t * stream.\n> > > >> +\t\t\t */\n> > > >> +\t\t\tbufferCount = stream->bufferPool().count();\n> > > >> +\t\t\timgu->output_.pool->createBuffers(bufferCount);\n> > > >> +\t\t\tret = imgu->exportBuffers(&imgu->output_,\n> > > >> +\t\t\t\t\t\t imgu->output_.pool);\n> > > >> +\t\t\tif (ret)\n> > > >> +\t\t\t\treturn ret;\n> > > >> +\t\t}\n> > > >> +\t}\n> > > >\n> > > > As for the previous patch, I think you should allocate memory for the\n> > > > streams present in the set in a loop without caring about the stream\n> > > > type, and then for the other devices outside of the loop. It should also\n> > > > ease error handling, which is missing here.\n> > >\n> > > Both you and Niklas reported that errors are not handled by this\n> > > function. Actually, if allocateBuffers fail, the Camera class calls\n> > > freeBuffers, which will free the allocated one and will simply call\n> > > reqBuffers(0) on the non allocated one. In my opinion this seems safe,\n> > > have I mis-interpreted something?\n> >\n> > Do you think it's safe to call the freeBuffers() function if\n> > allocateBuffers() failed ? That would require handling partial\n> > allocation in freeBuffers(). I think it would be best to clean up on\n> > failure in allocateBuffers(), and not call freeBuffers() in that case.\n> > The code to handle failure after partial allocation will then be close\n> > to the location of the failure, I think that would be less error-prone.\n> >\n> \n> Is this the part that concerns you?\n> from: Documentation/output/media/uapi/v4l/vidioc-reqbufs.html\n> Note that if any buffers are still mapped or exported via DMABUF,\n> then ioctl VIDIOC_REQBUFS can only succeed if the\n> V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS capability is set. Otherwise\n> ioctl VIDIOC_REQBUFS will return the EBUSY error code.\n> \n> As I read this, and if that's you concern, we should close and unmap\n> any exported buffers before calling reqbufs(0).\n\nNo, what concerns me is internal to libcamera. It's an API matter, how\nto deal with cleanups after partial failure. There are two models,\nrequiring functions to cleanup internally when they return a failure, or\ncalling the cleanup function on failure. In the first case the cleanup\nfunction only has to deal with fully initialised states, while in the\nsecond case the cleanup function has to deal with partially initialised\nstates if a partial failure occurs. I think I prefer the former as it\ngroups the partial failure handling with the code where partial failure\noccurs.\n\n> I think that if allocation fails (in particular if exportBuffers() fails)\n> then the V4L2Device destroy all the buffer planes, closing any open fd\n> and unmapping them. Then we can freeBuffers in the driver memory\n> through freeBuffers. For non-yet-allocated buffers, they would receive\n> a reqbufs(0) ioctl, before any fd has been mapped or exported. It\n> seems ok to me, have I missed something?\n> \n> > > >> +\n> > > >> \treturn 0;\n> > > >> }\n> > > >>\n> > > >> @@ -819,6 +864,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> > > >>\n> > > >> \toutput_.pad = PAD_OUTPUT;\n> > > >> \toutput_.name = \"output\";\n> > > >> +\toutput_.pool = &outPool_;\n> > > >>\n> > > >> \tviewfinder_.dev = V4L2Device::fromEntityName(media,\n> > > >> \t\t\t\t\t\t name_ + \" viewfinder\");","headers":{"Return-Path":"","Received":["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 1FF1E60B2E\n\tfor ;\n\tMon, 15 Apr 2019 09:56:21 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C11A301;\n\tMon, 15 Apr 2019 09:56:20 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555314980;\n\tbh=LEAHw/6hM59vWLjzsNc132dCmaJKGN3v8ZF+9hUOv5Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mGYMa3H/KMIGsrBttZzvrVsjYdkvCBuh5IqEINAsfqeQoGytcYHhUNnhlDYE+WjCY\n\tT830LPWBOJ7L5T0zSrRF/ZnwAM+Su0o5Sk434IdpiBzlWmltvkans2HbENSogw2onx\n\tGDfGPUrUzA+EFjQrMEoVIKa0JXn8kQxPy0K13uhc=","Date":"Mon, 15 Apr 2019 10:56:11 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190415075611.GA4809@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-5-jacopo@jmondi.org>\n\t<20190405154501.GC4636@pendragon.ideasonboard.com>\n\t<20190408155028.5d2eobqlrakljju4@uno.localdomain>\n\t<20190409140639.GB4578@pendragon.ideasonboard.com>\n\t<20190409182142.tmgkm2tmg5qukzct@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190409182142.tmgkm2tmg5qukzct@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 4/8] libcamera: ipu3: Add multiple\n\tstream memory management","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 15 Apr 2019 07:56:21 -0000"}}]