[{"id":13607,"web_url":"https://patchwork.libcamera.org/comment/13607/","msgid":"<7e2787ad-aa87-6024-8742-eb450799bfb3@ideasonboard.com>","date":"2020-11-05T11:23:16","subject":"Re: [libcamera-devel] [PATCH 04/11] libcamera: ipu3: imgu: Add\n\tparameters video device","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 05/11/2020 00:15, Niklas Söderlund wrote:\n> Add the parameters video device to the data structure of the ImgUDevice.\n> Even if the video device is configured, prepared to import buffers and\n> started no buffers are ever queued to it so it does not yet effect the\n> capture. Nor is does this change hinder the current capture mode to\n> function.\n> \n> This is done in preparation to attache an IPA to the IPU3 pipeline.\n> \n\ns/attache/attach/\n\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 38 ++++++++++++++++++++++++++--\n> src/libcamera/pipeline/ipu3/imgu.h | 3 ++-\n> 2 files changed, 38 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 0a3bf62020fd23fb..547a9e00325e7519 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -365,6 +365,11 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tparam_.reset(V4L2VideoDevice::fromEntityName(media, name_ + \" parameters\"));\n> +\tret = param_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tstat_.reset(V4L2VideoDevice::fromEntityName(media, name_ + \" 3a stat\"));\n> \tret = stat_->open();\n> \tif (ret)\n> @@ -477,6 +482,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF\n> \n> \tLOG(IPU3, Debug) << \"ImgU GDC format = \" << gdcFormat.toString();\n> \n> +\tStreamConfiguration paramCfg = {};\n> +\tparamCfg.size = inputFormat->size;\n> +\tV4L2DeviceFormat paramFormat;\n> +\tret = configureVideoDevice(param_.get(), PAD_PARAM, paramCfg, ¶mFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tStreamConfiguration statCfg = {};\n> \tstatCfg.size = inputFormat->size;\n> \tV4L2DeviceFormat statFormat;\n> @@ -507,8 +519,8 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\t/* No need to apply format to the stat node. */\n> -\tif (dev == stat_.get())\n> +\t/* No need to apply format to the param or stat video devices. */\n> +\tif (dev == param_.get() || dev == stat_.get())\n> \t\treturn 0;\n\nAyee, I'm not sure I like this 'intrinsic knowledge of what device it's\noperating on' in this function, but perhaps it's not better than\nduplicating code. (and it was here before this patch anyway).\n\n\n> \n> \t*outputFormat = {};\n> @@ -539,6 +551,12 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> \t\treturn ret;\n> \t}\n> \n> +\tret = param_->importBuffers(bufferCount);\n> +\tif (ret < 0) {\n> +\t\tLOG(IPU3, Error) << \"Failed to allocate ImgU param buffers\";\n> +\t\tgoto error;\n> +\t}\n\nWill you allocate parameter buffers on the parameter node?\n\n(Yes, I see that happen later... I wonder why it doesn't just happen\nhere, but it's fine for now).\n\n\n> +\n> \t/*\n> \t * The kernel fails to start if buffers are not either imported or\n> \t * allocated for the statistics video device. As statistics buffers are\n> @@ -588,6 +606,10 @@ void ImgUDevice::freeBuffers()\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to release ImgU output buffers\";\n> \n> +\tret = param_->releaseBuffers();\n> +\tif (ret)\n> +\t\tLOG(IPU3, Error) << \"Failed to release ImgU param buffers\";\n> +\n> \tret = stat_->releaseBuffers();\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to release ImgU stat buffers\";\n> @@ -618,6 +640,12 @@ int ImgUDevice::start()\n> \t\treturn ret;\n> \t}\n> \n> +\tret = param_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU param\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \tret = stat_->streamOn();\n> \tif (ret) {\n> \t\tLOG(IPU3, Error) << \"Failed to start ImgU stat\";\n> @@ -639,6 +667,7 @@ int ImgUDevice::stop()\n> \n> \tret = output_->streamOff();\n> \tret |= viewfinder_->streamOff();\n> +\tret |= param_->streamOff();\n> \tret |= stat_->streamOff();\n> \tret |= input_->streamOff();\n> \n> @@ -680,6 +709,7 @@ int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad,\n> int ImgUDevice::enableLinks(bool enable)\n> {\n> \tstd::string viewfinderName = name_ + \" viewfinder\";\n> +\tstd::string paramName = name_ + \" parameters\";\n> \tstd::string outputName = name_ + \" output\";\n> \tstd::string statName = name_ + \" 3a stat\";\n> \tstd::string inputName = name_ + \" input\";\n> @@ -697,6 +727,10 @@ int ImgUDevice::enableLinks(bool enable)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tret = linkSetup(paramName, 0, name_, PAD_PARAM, enable);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \treturn linkSetup(name_, PAD_STAT, statName, 0, enable);\n> }\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> index 37f5ae77c99ff8fe..1388d07a45b28590 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -73,11 +73,12 @@ public:\n> \tstd::unique_ptr input_;\n> \tstd::unique_ptr output_;\n> \tstd::unique_ptr viewfinder_;\n> +\tstd::unique_ptr param_;\n\nHow about keeping these in the same order as the pad values?\n(Put this between input/output)\n\nReviewed-by: Kieran Bingham \n\n> \tstd::unique_ptr stat_;\n> -\t/* \\todo Add param video device for 3A tuning */\n> \n> private:\n> \tstatic constexpr unsigned int PAD_INPUT = 0;\n> +\tstatic constexpr unsigned int PAD_PARAM = 1;\n> \tstatic constexpr unsigned int PAD_OUTPUT = 2;\n> \tstatic constexpr unsigned int PAD_VF = 3;\n> \tstatic constexpr unsigned int PAD_STAT = 4;\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 80389BE080\n\tfor ;\n\tThu, 5 Nov 2020 11:23:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D5F7A62C7D;\n\tThu, 5 Nov 2020 12:23: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 D457B60353\n\tfor ;\n\tThu, 5 Nov 2020 12:23:18 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D13C51D;\n\tThu, 5 Nov 2020 12:23:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"T9OEldJP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1604575398;\n\tbh=02grdUF0xiFJn9PDGSyGTiVRF5bo38eeytFzvSIho7w=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=T9OEldJPo9+7fQGA5Ul7eoMYMrmYFoqHOuszrW4S2CMJerChep6ZCm3NOST9EeR8n\n\tA1MXLh8HdiTIbtBdtqpaS3Dze21kTLT3b1Lat69a1YB2LvRRAsMi3ux26aX5aVZWFH\n\ty1Xj5YbNkPqxAguMiQ/REFxf3i3sVyFLcPpbR/Xs=","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20201105001546.1690179-1-niklas.soderlund@ragnatech.se>\n\t<20201105001546.1690179-5-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<7e2787ad-aa87-6024-8742-eb450799bfb3@ideasonboard.com>","Date":"Thu, 5 Nov 2020 11:23:16 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201105001546.1690179-5-niklas.soderlund@ragnatech.se>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 04/11] libcamera: ipu3: imgu: Add\n\tparameters video device","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13637,"web_url":"https://patchwork.libcamera.org/comment/13637/","msgid":"<20201109102243.fwbp5u3ju3rvufp6@uno.localdomain>","date":"2020-11-09T10:22:43","subject":"Re: [libcamera-devel] [PATCH 04/11] libcamera: ipu3: imgu: Add\n\tparameters video device","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Nov 05, 2020 at 01:15:39AM +0100, Niklas Söderlund wrote:\n> Add the parameters video device to the data structure of the ImgUDevice.\n> Even if the video device is configured, prepared to import buffers and\n> started no buffers are ever queued to it so it does not yet effect the\n> capture. Nor is does this change hinder the current capture mode to\n> function.\n>\n> This is done in preparation to attache an IPA to the IPU3 pipeline.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 38 ++++++++++++++++++++++++++--\n> src/libcamera/pipeline/ipu3/imgu.h | 3 ++-\n> 2 files changed, 38 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 0a3bf62020fd23fb..547a9e00325e7519 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -365,6 +365,11 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> \tif (ret)\n> \t\treturn ret;\n>\n> +\tparam_.reset(V4L2VideoDevice::fromEntityName(media, name_ + \" parameters\"));\n> +\tret = param_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tstat_.reset(V4L2VideoDevice::fromEntityName(media, name_ + \" 3a stat\"));\n> \tret = stat_->open();\n> \tif (ret)\n> @@ -477,6 +482,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF\n>\n> \tLOG(IPU3, Debug) << \"ImgU GDC format = \" << gdcFormat.toString();\n>\n> +\tStreamConfiguration paramCfg = {};\n> +\tparamCfg.size = inputFormat->size;\n> +\tV4L2DeviceFormat paramFormat;\n> +\tret = configureVideoDevice(param_.get(), PAD_PARAM, paramCfg, ¶mFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tStreamConfiguration statCfg = {};\n> \tstatCfg.size = inputFormat->size;\n> \tV4L2DeviceFormat statFormat;\n> @@ -507,8 +519,8 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\t/* No need to apply format to the stat node. */\n> -\tif (dev == stat_.get())\n> +\t/* No need to apply format to the param or stat video devices. */\n> +\tif (dev == param_.get() || dev == stat_.get())\n> \t\treturn 0;\n\nI guess this answers my previous question, so format and size are\nfixed on the stat and param nodes, and only the ImgU subdevice's source\npad needs configuration.\n\nLooks good (also for the previous patch):\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n>\n> \t*outputFormat = {};\n> @@ -539,6 +551,12 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> \t\treturn ret;\n> \t}\n>\n> +\tret = param_->importBuffers(bufferCount);\n> +\tif (ret < 0) {\n> +\t\tLOG(IPU3, Error) << \"Failed to allocate ImgU param buffers\";\n> +\t\tgoto error;\n> +\t}\n> +\n> \t/*\n> \t * The kernel fails to start if buffers are not either imported or\n> \t * allocated for the statistics video device. As statistics buffers are\n> @@ -588,6 +606,10 @@ void ImgUDevice::freeBuffers()\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to release ImgU output buffers\";\n>\n> +\tret = param_->releaseBuffers();\n> +\tif (ret)\n> +\t\tLOG(IPU3, Error) << \"Failed to release ImgU param buffers\";\n> +\n> \tret = stat_->releaseBuffers();\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to release ImgU stat buffers\";\n> @@ -618,6 +640,12 @@ int ImgUDevice::start()\n> \t\treturn ret;\n> \t}\n>\n> +\tret = param_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU param\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \tret = stat_->streamOn();\n> \tif (ret) {\n> \t\tLOG(IPU3, Error) << \"Failed to start ImgU stat\";\n> @@ -639,6 +667,7 @@ int ImgUDevice::stop()\n>\n> \tret = output_->streamOff();\n> \tret |= viewfinder_->streamOff();\n> +\tret |= param_->streamOff();\n> \tret |= stat_->streamOff();\n> \tret |= input_->streamOff();\n>\n> @@ -680,6 +709,7 @@ int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad,\n> int ImgUDevice::enableLinks(bool enable)\n> {\n> \tstd::string viewfinderName = name_ + \" viewfinder\";\n> +\tstd::string paramName = name_ + \" parameters\";\n> \tstd::string outputName = name_ + \" output\";\n> \tstd::string statName = name_ + \" 3a stat\";\n> \tstd::string inputName = name_ + \" input\";\n> @@ -697,6 +727,10 @@ int ImgUDevice::enableLinks(bool enable)\n> \tif (ret)\n> \t\treturn ret;\n>\n> +\tret = linkSetup(paramName, 0, name_, PAD_PARAM, enable);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \treturn linkSetup(name_, PAD_STAT, statName, 0, enable);\n> }\n>\n> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> index 37f5ae77c99ff8fe..1388d07a45b28590 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -73,11 +73,12 @@ public:\n> \tstd::unique_ptr input_;\n> \tstd::unique_ptr output_;\n> \tstd::unique_ptr viewfinder_;\n> +\tstd::unique_ptr param_;\n> \tstd::unique_ptr stat_;\n> -\t/* \\todo Add param video device for 3A tuning */\n>\n> private:\n> \tstatic constexpr unsigned int PAD_INPUT = 0;\n> +\tstatic constexpr unsigned int PAD_PARAM = 1;\n> \tstatic constexpr unsigned int PAD_OUTPUT = 2;\n> \tstatic constexpr unsigned int PAD_VF = 3;\n> \tstatic constexpr unsigned int PAD_STAT = 4;\n> --\n> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2180FBDB89\n\tfor ;\n\tMon, 9 Nov 2020 10:22:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3AAE6306B;\n\tMon, 9 Nov 2020 11:22:42 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 776D762D19\n\tfor ;\n\tMon, 9 Nov 2020 11:22:41 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id AE02E100027;\n\tMon, 9 Nov 2020 10:22:40 +0000 (UTC)"],"Date":"Mon, 9 Nov 2020 11:22:43 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201109102243.fwbp5u3ju3rvufp6@uno.localdomain>","References":"<20201105001546.1690179-1-niklas.soderlund@ragnatech.se>\n\t<20201105001546.1690179-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201105001546.1690179-5-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 04/11] libcamera: ipu3: imgu: Add\n\tparameters video device","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14113,"web_url":"https://patchwork.libcamera.org/comment/14113/","msgid":"","date":"2020-12-08T01:54:40","subject":"Re: [libcamera-devel] [PATCH 04/11] libcamera: ipu3: imgu: Add\n\tparameters video device","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, Nov 05, 2020 at 01:15:39AM +0100, Niklas Söderlund wrote:\n> Add the parameters video device to the data structure of the ImgUDevice.\n> Even if the video device is configured, prepared to import buffers and\n> started no buffers are ever queued to it so it does not yet effect the\n> capture. Nor is does this change hinder the current capture mode to\n> function.\n> \n> This is done in preparation to attache an IPA to the IPU3 pipeline.\n\ns/attache/attach/\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 38 ++++++++++++++++++++++++++--\n> src/libcamera/pipeline/ipu3/imgu.h | 3 ++-\n> 2 files changed, 38 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 0a3bf62020fd23fb..547a9e00325e7519 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -365,6 +365,11 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tparam_.reset(V4L2VideoDevice::fromEntityName(media, name_ + \" parameters\"));\n\nDepending on which series is merged first (see \"[PATCH 2/2] libcamera:\nv4l2_device: Return a unique pointer from fromEntityName()\"), you could\nwrite this\n\n\tparam_ = V4L2VideoDevice::fromEntityName(media, name_ + \" parameters\");\n\n> +\tret = param_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tstat_.reset(V4L2VideoDevice::fromEntityName(media, name_ + \" 3a stat\"));\n> \tret = stat_->open();\n> \tif (ret)\n> @@ -477,6 +482,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF\n> \n> \tLOG(IPU3, Debug) << \"ImgU GDC format = \" << gdcFormat.toString();\n> \n> +\tStreamConfiguration paramCfg = {};\n> +\tparamCfg.size = inputFormat->size;\n> +\tV4L2DeviceFormat paramFormat;\n> +\tret = configureVideoDevice(param_.get(), PAD_PARAM, paramCfg, ¶mFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \tStreamConfiguration statCfg = {};\n> \tstatCfg.size = inputFormat->size;\n> \tV4L2DeviceFormat statFormat;\n> @@ -507,8 +519,8 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\t/* No need to apply format to the stat node. */\n> -\tif (dev == stat_.get())\n> +\t/* No need to apply format to the param or stat video devices. */\n\nMaybe this is a better candidate than 03/11 to add a small explanation\nwhy this is the case. Could you check if we even need to set the format\non the subdev pad ? It seems ignored in the driver.\n\n> +\tif (dev == param_.get() || dev == stat_.get())\n> \t\treturn 0;\n> \n> \t*outputFormat = {};\n> @@ -539,6 +551,12 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n> \t\treturn ret;\n> \t}\n> \n> +\tret = param_->importBuffers(bufferCount);\n> +\tif (ret < 0) {\n> +\t\tLOG(IPU3, Error) << \"Failed to allocate ImgU param buffers\";\n> +\t\tgoto error;\n> +\t}\n> +\n> \t/*\n> \t * The kernel fails to start if buffers are not either imported or\n> \t * allocated for the statistics video device. As statistics buffers are\n> @@ -588,6 +606,10 @@ void ImgUDevice::freeBuffers()\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to release ImgU output buffers\";\n> \n> +\tret = param_->releaseBuffers();\n> +\tif (ret)\n> +\t\tLOG(IPU3, Error) << \"Failed to release ImgU param buffers\";\n> +\n> \tret = stat_->releaseBuffers();\n> \tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to release ImgU stat buffers\";\n> @@ -618,6 +640,12 @@ int ImgUDevice::start()\n> \t\treturn ret;\n> \t}\n> \n> +\tret = param_->streamOn();\n> +\tif (ret) {\n> +\t\tLOG(IPU3, Error) << \"Failed to start ImgU param\";\n> +\t\treturn ret;\n> +\t}\n> +\n> \tret = stat_->streamOn();\n> \tif (ret) {\n> \t\tLOG(IPU3, Error) << \"Failed to start ImgU stat\";\n> @@ -639,6 +667,7 @@ int ImgUDevice::stop()\n> \n> \tret = output_->streamOff();\n> \tret |= viewfinder_->streamOff();\n> +\tret |= param_->streamOff();\n> \tret |= stat_->streamOff();\n> \tret |= input_->streamOff();\n> \n> @@ -680,6 +709,7 @@ int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad,\n> int ImgUDevice::enableLinks(bool enable)\n> {\n> \tstd::string viewfinderName = name_ + \" viewfinder\";\n> +\tstd::string paramName = name_ + \" parameters\";\n> \tstd::string outputName = name_ + \" output\";\n> \tstd::string statName = name_ + \" 3a stat\";\n> \tstd::string inputName = name_ + \" input\";\n> @@ -697,6 +727,10 @@ int ImgUDevice::enableLinks(bool enable)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tret = linkSetup(paramName, 0, name_, PAD_PARAM, enable);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> \treturn linkSetup(name_, PAD_STAT, statName, 0, enable);\n> }\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\n> index 37f5ae77c99ff8fe..1388d07a45b28590 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.h\n> +++ b/src/libcamera/pipeline/ipu3/imgu.h\n> @@ -73,11 +73,12 @@ public:\n> \tstd::unique_ptr input_;\n> \tstd::unique_ptr output_;\n> \tstd::unique_ptr viewfinder_;\n> +\tstd::unique_ptr param_;\n\nI would also move this between input and output, but it's not big deal.\n\nReviewed-by: Laurent Pinchart \n\n> \tstd::unique_ptr stat_;\n> -\t/* \\todo Add param video device for 3A tuning */\n> \n> private:\n> \tstatic constexpr unsigned int PAD_INPUT = 0;\n> +\tstatic constexpr unsigned int PAD_PARAM = 1;\n> \tstatic constexpr unsigned int PAD_OUTPUT = 2;\n> \tstatic constexpr unsigned int PAD_VF = 3;\n> \tstatic constexpr unsigned int PAD_STAT = 4;","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 EC61FBDB1F\n\tfor ;\n\tTue, 8 Dec 2020 01:54:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5066D67E70;\n\tTue, 8 Dec 2020 02:54:45 +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 2E9DB60323\n\tfor ;\n\tTue, 8 Dec 2020 02:54:44 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 93AA4DD;\n\tTue, 8 Dec 2020 02:54:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"d7yxaBoX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607392483;\n\tbh=+aa7u/63lmOVg5dscwoNF3MyCyQh3RLPcZNpudYsDhI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d7yxaBoXBSqIqkYjCCWk5nu1ATHjYniJ73oa3vzXqzPe2QfzURQwahrJhsDutqUn7\n\tPipiu7Lup5k6WILxzdHPHFOnXCnj/O4T8boO1zx1DPNi1LtXqL6tUAOxuRQDgCzZ6b\n\tkPr51+6KPsU7DCZEhDtcIWGLswC4pLMZ8T7xEgqA=","Date":"Tue, 8 Dec 2020 03:54:40 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201105001546.1690179-1-niklas.soderlund@ragnatech.se>\n\t<20201105001546.1690179-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201105001546.1690179-5-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 04/11] libcamera: ipu3: imgu: Add\n\tparameters video device","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]