{"id":11502,"url":"https://patchwork.libcamera.org/api/patches/11502/?format=json","web_url":"https://patchwork.libcamera.org/patch/11502/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20210305093146.25943-1-paul.elder@ideasonboard.com>","date":"2021-03-05T09:31:46","name":"[libcamera-devel] utils: ipc, raspberrypi: Make first output parameter direct return if int32","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"7887e20e9db774918d44a2d0488f6a7dac7d6024","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/?format=json","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/11502/mbox/","series":[{"id":1763,"url":"https://patchwork.libcamera.org/api/series/1763/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=1763","date":"2021-03-05T09:31:46","name":"[libcamera-devel] utils: ipc, raspberrypi: Make first output parameter direct return if int32","version":1,"mbox":"https://patchwork.libcamera.org/series/1763/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/11502/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/11502/checks/","tags":{},"headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 63690BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  5 Mar 2021 09:31:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 203B268A7E;\n\tFri,  5 Mar 2021 10:31:57 +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 23008602E8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  5 Mar 2021 10:31:56 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 619D8CC;\n\tFri,  5 Mar 2021 10:31:54 +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=\"MxblqxYf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614936715;\n\tbh=vxy869LYnEuKK6UdOvjrVnqiHDf6Lc33l6y9liIF0sg=;\n\th=From:To:Cc:Subject:Date:From;\n\tb=MxblqxYfPqTqfaNdyqi1ZxAEIOIN2hiNuWJO469Ph/LYHVwvSvWYj7Q3fbt50y+Nj\n\tfkMTgjQBfYLEjhXlG3jY0e0XWzIDFwHbHuvpBzTSpvp3hOHPm343BJOd7x87ZLMRpe\n\tqb6x7QgV49til8HQUYYV4PucegVRd4z/jUkRz8zs=","From":"Paul Elder <paul.elder@ideasonboard.com>","To":"libcamera-devel@lists.libcamera.org","Date":"Fri,  5 Mar 2021 18:31:46 +0900","Message-Id":"<20210305093146.25943-1-paul.elder@ideasonboard.com>","X-Mailer":"git-send-email 2.27.0","MIME-Version":"1.0","Subject":"[libcamera-devel] [PATCH] utils: ipc,\n\traspberrypi: Make first output parameter direct return if int32","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"},"content":"To make it more convenient for synchronous IPA calls to return a status,\nconvert the first output into a direct return if it is an int32.\n\nConvert the raspberrypi IPA interface and usage appropriately.\n\nSigned-off-by: Paul Elder <paul.elder@ideasonboard.com>\n\n---\nThis is still an RFC until we decide what we want to do.\n\nThis patch depends on:\n- utils: ipc: Support custom parameters to init()\n- DEMO: raspberrypi: Use custom parameters to init()\n\nI'm expecting to squash with the first patch. If we drop the second\npatch then we can remove the raspberrypi components from this patch.\n\nI still need to update the IPA guide appropriately, but I decided to get\napproval on this RFC first.\n\nBasically the only question (besides standard review) I want to ask is,\nshould we squash this with \"utils: ipc: Support custom parameters to\ninit()\"?\n\nNaush, does this (and \"utils: ipc: Support custom parameters to init()\")\nfit what you want?\n\n---\n include/libcamera/ipa/raspberrypi.mojom       |  2 +-\n src/ipa/raspberrypi/raspberrypi.cpp           | 44 +++++++++---------\n .../pipeline/raspberrypi/raspberrypi.cpp      |  7 ++-\n .../module_ipa_proxy.cpp.tmpl                 |  7 ++-\n .../module_ipa_proxy_worker.cpp.tmpl          |  8 ++--\n .../libcamera_templates/proxy_functions.tmpl  |  4 +-\n .../generators/mojom_libcamera_generator.py   | 45 ++++++++++++-------\n 7 files changed, 64 insertions(+), 53 deletions(-)","diff":"diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\nindex b8944227..99a62c02 100644\n--- a/include/libcamera/ipa/raspberrypi.mojom\n+++ b/include/libcamera/ipa/raspberrypi.mojom\n@@ -78,7 +78,7 @@ interface IPARPiInterface {\n \t\t  map<uint32, IPAStream> streamConfig,\n \t\t  map<uint32, ControlInfoMap> entityControls,\n \t\t  ConfigInput ipaConfig)\n-\t\t=> (ConfigOutput results, int32 ret);\n+\t\t=> (int32 ret, ConfigOutput results);\n \n \t/**\n \t * \\fn mapBuffers()\ndiff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\nindex 6a9aba6f..ac18dcbd 100644\n--- a/src/ipa/raspberrypi/raspberrypi.cpp\n+++ b/src/ipa/raspberrypi/raspberrypi.cpp\n@@ -79,17 +79,17 @@ public:\n \t\t\tmunmap(lsTable_, ipa::RPi::MaxLsGridSize);\n \t}\n \n-\tvoid init(const IPASettings &settings, const std::string &sensorName,\n-\t\t  int *ret, bool *metadataSupport) override;\n+\tint init(const IPASettings &settings, const std::string &sensorName,\n+\t\t bool *metadataSupport) override;\n \tvoid start(const ipa::RPi::StartControls &data,\n \t\t   ipa::RPi::StartControls *result) override;\n \tvoid stop() override {}\n \n-\tvoid configure(const CameraSensorInfo &sensorInfo,\n-\t\t       const std::map<unsigned int, IPAStream> &streamConfig,\n-\t\t       const std::map<unsigned int, ControlInfoMap> &entityControls,\n-\t\t       const ipa::RPi::ConfigInput &data,\n-\t\t       ipa::RPi::ConfigOutput *response, int32_t *ret) override;\n+\tint configure(const CameraSensorInfo &sensorInfo,\n+\t\t      const std::map<unsigned int, IPAStream> &streamConfig,\n+\t\t      const std::map<unsigned int, ControlInfoMap> &entityControls,\n+\t\t      const ipa::RPi::ConfigInput &data,\n+\t\t      ipa::RPi::ConfigOutput *response) override;\n \tvoid mapBuffers(const std::vector<IPABuffer> &buffers) override;\n \tvoid unmapBuffers(const std::vector<unsigned int> &ids) override;\n \tvoid signalStatReady(const uint32_t bufferId) override;\n@@ -165,15 +165,15 @@ private:\n \tdouble maxFrameDuration_;\n };\n \n-void IPARPi::init(const IPASettings &settings, const std::string &sensorName,\n-\t\t  int *ret, bool *metadataSupport)\n+int IPARPi::init(const IPASettings &settings, const std::string &sensorName,\n+\t\t bool *metadataSupport)\n {\n \tLOG(IPARPI, Debug) << \"sensor name is \" << sensorName;\n \n \ttuningFile_ = settings.configurationFile;\n \n \t*metadataSupport = true;\n-\t*ret = 0;\n+\treturn 0;\n }\n \n void IPARPi::start(const ipa::RPi::StartControls &data,\n@@ -296,16 +296,15 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n \tmode_.max_frame_length = sensorInfo.maxFrameLength;\n }\n \n-void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n-\t\t       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,\n-\t\t       const std::map<unsigned int, ControlInfoMap> &entityControls,\n-\t\t       const ipa::RPi::ConfigInput &ipaConfig,\n-\t\t       ipa::RPi::ConfigOutput *result, int32_t *ret)\n+int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n+\t\t      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,\n+\t\t      const std::map<unsigned int, ControlInfoMap> &entityControls,\n+\t\t      const ipa::RPi::ConfigInput &ipaConfig,\n+\t\t      ipa::RPi::ConfigOutput *result)\n {\n \tif (entityControls.size() != 2) {\n \t\tLOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n-\t\t*ret = -1;\n-\t\treturn;\n+\t\treturn -1;\n \t}\n \n \tresult->params = 0;\n@@ -315,14 +314,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n \n \tif (!validateSensorControls()) {\n \t\tLOG(IPARPI, Error) << \"Sensor control validation failed.\";\n-\t\t*ret = -1;\n-\t\treturn;\n+\t\treturn -1;\n \t}\n \n \tif (!validateIspControls()) {\n \t\tLOG(IPARPI, Error) << \"ISP control validation failed.\";\n-\t\t*ret = -1;\n-\t\treturn;\n+\t\treturn -1;\n \t}\n \n \t/* Setup a metadata ControlList to output metadata. */\n@@ -340,8 +337,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n \t\tif (!helper_) {\n \t\t\tLOG(IPARPI, Error) << \"Could not create camera helper for \"\n \t\t\t\t\t   << cameraName;\n-\t\t\t*ret = -1;\n-\t\t\treturn;\n+\t\t\treturn -1;\n \t\t}\n \n \t\t/*\n@@ -409,7 +405,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n \n \tlastMode_ = mode_;\n \n-\t*ret = 0;\n+\treturn 0;\n }\n \n void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)\ndiff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nindex a1c90028..106318ed 100644\n--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n@@ -1194,9 +1194,8 @@ int RPiCameraData::loadIPA()\n \n \tIPASettings settings(ipa_->configurationFile(sensor_->model() + \".json\"));\n \n-\tint ret;\n \tbool metadataSupport;\n-\tipa_->init(settings, \"sensor name\", &ret, &metadataSupport);\n+\tint ret = ipa_->init(settings, \"sensor name\", &metadataSupport);\n \tLOG(RPI, Debug) << \"metadata support \" << (metadataSupport ? \"yes\" : \"no\");\n \treturn ret;\n }\n@@ -1254,8 +1253,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n \t/* Ready the IPA - it must know about the sensor resolution. */\n \tipa::RPi::ConfigOutput result;\n \n-\tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n-\t\t\t&result, &ret);\n+\tret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n+\t\t\t      &result);\n \n \tif (ret < 0) {\n \t\tLOG(RPI, Error) << \"IPA configuration failed!\";\ndiff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\nindex ff667ce0..167aaabd 100644\n--- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n+++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n@@ -212,7 +212,12 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n {%- endif %}\n \t}\n {% if method|method_return_value != \"void\" %}\n-\treturn IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf.data(), 0);\n+\t{{method|method_return_value}} _finalRet = IPADataSerializer<{{method|method_return_value}}>::deserialize(_ipcOutputBuf.data(), 0);\n+\n+{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()', init_offset = method|method_return_value|byte_width|int)}}\n+\n+\treturn _finalRet;\n+\n {% elif method|method_param_outputs|length > 0 %}\n {{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()')}}\n {% endif -%}\ndiff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\nindex d08af76d..c4206162 100644\n--- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n+++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n@@ -85,15 +85,14 @@ public:\n \t\t\t{{param|name}} {{param.mojom_name}};\n {% endfor %}\n {%- if method|method_return_value != \"void\" %}\n-\t\t\t{{method|method_return_value}} _callRet = ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n-{%- else %}\n+\t\t\t{{method|method_return_value}} _callRet =\n+{%- endif -%}\n \t\t\tipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}\n {{- \", \" if method|method_param_outputs|params_comma_sep -}}\n {%- for param in method|method_param_outputs -%}\n &{{param.mojom_name}}{{\", \" if not loop.last}}\n {%- endfor -%}\n );\n-{%- endif %}\n {% if not method|is_async %}\n \t\t\tIPCMessage::Header header = { _ipcMessage.header().cmd, _ipcMessage.header().cookie };\n \t\t\tIPCMessage _response(header);\n@@ -102,9 +101,8 @@ public:\n \t\t\tstd::tie(_callRetBuf, std::ignore) =\n \t\t\t\tIPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);\n \t\t\t_response.data().insert(_response.data().end(), _callRetBuf.cbegin(), _callRetBuf.cend());\n-{%- else %}\n-\t\t{{proxy_funcs.serialize_call(method|method_param_outputs, \"_response.data()\", \"_response.fds()\")|indent(16, true)}}\n {%- endif %}\n+\t\t{{proxy_funcs.serialize_call(method|method_param_outputs, \"_response.data()\", \"_response.fds()\")|indent(16, true)}}\n \t\t\tint _ret = socket_.send(_response.payload());\n \t\t\tif (_ret < 0) {\n \t\t\t\tLOG({{proxy_worker_name}}, Error)\ndiff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\nindex f2d86b67..c2ac42fc 100644\n--- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n+++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n@@ -135,8 +135,8 @@\n  #\n  # \\todo Avoid intermediate vectors\n  #}\n-{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false, iter = false, data_size = '') -%}\n-{% set ns = namespace(size_offset = 0) %}\n+{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false, iter = false, data_size = '', init_offset = 0) -%}\n+{% set ns = namespace(size_offset = init_offset) %}\n {%- if params|length > 1 %}\n {%- for param in params %}\n \t[[maybe_unused]] const size_t {{param.mojom_name}}BufSize = readPOD<uint32_t>({{buf}}, {{ns.size_offset}}\ndiff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py\nindex fa0c21a2..e72a05ff 100644\n--- a/utils/ipc/generators/mojom_libcamera_generator.py\n+++ b/utils/ipc/generators/mojom_libcamera_generator.py\n@@ -149,10 +149,16 @@ def MethodParamInputs(method):\n     return method.parameters\n \n def MethodParamOutputs(method):\n-    if (MethodReturnValue(method) != 'void' or\n-        method.response_parameters is None):\n+    if method.response_parameters is None:\n+        return []\n+\n+    if MethodReturnValue(method) == 'void':\n+        return method.response_parameters\n+\n+    if len(method.response_parameters) <= 1:\n         return []\n-    return method.response_parameters\n+\n+    return method.response_parameters[1:]\n \n def MethodParamsHaveFd(parameters):\n     return len([x for x in parameters if HasFd(x)]) > 0\n@@ -167,11 +173,8 @@ def MethodParamNames(method):\n     params = []\n     for param in method.parameters:\n         params.append(param.mojom_name)\n-    if MethodReturnValue(method) == 'void':\n-        if method.response_parameters is None:\n-            return params\n-        for param in method.response_parameters:\n-            params.append(param.mojom_name)\n+    for param in MethodParamOutputs(method):\n+        params.append(param.mojom_name)\n     return params\n \n def MethodParameters(method):\n@@ -180,18 +183,17 @@ def MethodParameters(method):\n         params.append('const %s %s%s' % (GetNameForElement(param),\n                                          '&' if not IsPod(param) else '',\n                                          param.mojom_name))\n-    if MethodReturnValue(method) == 'void':\n-        if method.response_parameters is None:\n-            return params\n-        for param in method.response_parameters:\n-            params.append(f'{GetNameForElement(param)} *{param.mojom_name}')\n+    for param in MethodParamOutputs(method):\n+        params.append(f'{GetNameForElement(param)} *{param.mojom_name}')\n     return params\n \n def MethodReturnValue(method):\n-    if method.response_parameters is None:\n+    if method.response_parameters is None or len(method.response_parameters) == 0:\n         return 'void'\n-    if len(method.response_parameters) == 1 and IsPod(method.response_parameters[0]):\n-        return GetNameForElement(method.response_parameters[0])\n+    first_output = method.response_parameters[0]\n+    if ((len(method.response_parameters) == 1 and IsPod(first_output)) or\n+        first_output.kind == mojom.INT32):\n+        return GetNameForElement(first_output)\n     return 'void'\n \n def IsAsync(method):\n@@ -237,6 +239,16 @@ def BitWidth(element):\n         return '32'\n     return ''\n \n+def ByteWidthFromCppType(t):\n+    key = None\n+    for mojo_type, cpp_type in _kind_to_cpp_type.items():\n+        if t == cpp_type:\n+            key = mojo_type\n+    if key is None:\n+        raise Exception('invalid type')\n+    return str(int(int(_bit_widths[key]) / 8))\n+\n+\n # Get the type name for a given element\n def GetNameForElement(element):\n     # structs\n@@ -373,6 +385,7 @@ class Generator(generator.Generator):\n         libcamera_filters = {\n             'all_types': GetAllTypes,\n             'bit_width': BitWidth,\n+            'byte_width' : ByteWidthFromCppType,\n             'cap': Capitalize,\n             'choose': Choose,\n             'comma_sep': CommaSep,\n","prefixes":["libcamera-devel"]}