[{"id":15488,"web_url":"https://patchwork.libcamera.org/comment/15488/","msgid":"","date":"2021-03-05T11:30:18","subject":"Re: [libcamera-devel] [PATCH] utils: ipc,\n\traspberrypi: Make first output parameter direct return if int32","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\nThank you for your work.\n\nOn Fri, 5 Mar 2021 at 09:31, Paul Elder wrote:\n\n> To make it more convenient for synchronous IPA calls to return a status,\n> convert the first output into a direct return if it is an int32.\n>\n> Convert the raspberrypi IPA interface and usage appropriately.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> This is still an RFC until we decide what we want to do.\n>\n> This patch depends on:\n> - utils: ipc: Support custom parameters to init()\n> - DEMO: raspberrypi: Use custom parameters to init()\n>\n> I'm expecting to squash with the first patch. If we drop the second\n> patch then we can remove the raspberrypi components from this patch.\n>\n> I still need to update the IPA guide appropriately, but I decided to get\n> approval on this RFC first.\n>\n> Basically the only question (besides standard review) I want to ask is,\n> should we squash this with \"utils: ipc: Support custom parameters to\n> init()\"?\n>\n> Naush, does this (and \"utils: ipc: Support custom parameters to init()\")\n> fit what you want?\n>\n\nYes, it seems to. However, I have not had a chance to actually apply these\nwith my proposed changes yet.\n\nAcked-by: Naushir Patuck \n\n\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(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> index 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> map streamConfig,\n> map entityControls,\n> ConfigInput ipaConfig)\n> - => (ConfigOutput results, int32 ret);\n> + => (int32 ret, ConfigOutput results);\n>\n> /**\n> * \\fn mapBuffers()\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 6a9aba6f..ac18dcbd 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -79,17 +79,17 @@ public:\n> munmap(lsTable_, ipa::RPi::MaxLsGridSize);\n> }\n>\n> - void init(const IPASettings &settings, const std::string\n> &sensorName,\n> - int *ret, bool *metadataSupport) override;\n> + int init(const IPASettings &settings, const std::string\n> &sensorName,\n> + bool *metadataSupport) override;\n> void start(const ipa::RPi::StartControls &data,\n> ipa::RPi::StartControls *result) override;\n> void stop() override {}\n>\n> - void configure(const CameraSensorInfo &sensorInfo,\n> - const std::map\n> &streamConfig,\n> - const std::map\n> &entityControls,\n> - const ipa::RPi::ConfigInput &data,\n> - ipa::RPi::ConfigOutput *response, int32_t *ret)\n> override;\n> + int configure(const CameraSensorInfo &sensorInfo,\n> + const std::map\n> &streamConfig,\n> + const std::map\n> &entityControls,\n> + const ipa::RPi::ConfigInput &data,\n> + ipa::RPi::ConfigOutput *response) override;\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> void signalStatReady(const uint32_t bufferId) override;\n> @@ -165,15 +165,15 @@ private:\n> double maxFrameDuration_;\n> };\n>\n> -void IPARPi::init(const IPASettings &settings, const std::string\n> &sensorName,\n> - int *ret, bool *metadataSupport)\n> +int IPARPi::init(const IPASettings &settings, const std::string\n> &sensorName,\n> + bool *metadataSupport)\n> {\n> LOG(IPARPI, Debug) << \"sensor name is \" << sensorName;\n>\n> tuningFile_ = settings.configurationFile;\n>\n> *metadataSupport = true;\n> - *ret = 0;\n> + return 0;\n> }\n>\n> void IPARPi::start(const ipa::RPi::StartControls &data,\n> @@ -296,16 +296,15 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> mode_.max_frame_length = sensorInfo.maxFrameLength;\n> }\n>\n> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> - const std::map\n> &entityControls,\n> - const ipa::RPi::ConfigInput &ipaConfig,\n> - ipa::RPi::ConfigOutput *result, int32_t *ret)\n> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> + const std::map\n> &entityControls,\n> + const ipa::RPi::ConfigInput &ipaConfig,\n> + ipa::RPi::ConfigOutput *result)\n> {\n> if (entityControls.size() != 2) {\n> LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> - *ret = -1;\n> - return;\n> + return -1;\n> }\n>\n> result->params = 0;\n> @@ -315,14 +314,12 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n>\n> if (!validateSensorControls()) {\n> LOG(IPARPI, Error) << \"Sensor control validation failed.\";\n> - *ret = -1;\n> - return;\n> + return -1;\n> }\n>\n> if (!validateIspControls()) {\n> LOG(IPARPI, Error) << \"ISP control validation failed.\";\n> - *ret = -1;\n> - return;\n> + return -1;\n> }\n>\n> /* Setup a metadata ControlList to output metadata. */\n> @@ -340,8 +337,7 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> if (!helper_) {\n> LOG(IPARPI, Error) << \"Could not create camera\n> helper for \"\n> << cameraName;\n> - *ret = -1;\n> - return;\n> + return -1;\n> }\n>\n> /*\n> @@ -409,7 +405,7 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n>\n> lastMode_ = mode_;\n>\n> - *ret = 0;\n> + return 0;\n> }\n>\n> void IPARPi::mapBuffers(const std::vector &buffers)\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 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> IPASettings settings(ipa_->configurationFile(sensor_->model() +\n> \".json\"));\n>\n> - int ret;\n> bool metadataSupport;\n> - ipa_->init(settings, \"sensor name\", &ret, &metadataSupport);\n> + int ret = ipa_->init(settings, \"sensor name\", &metadataSupport);\n> LOG(RPI, Debug) << \"metadata support \" << (metadataSupport ? \"yes\"\n> : \"no\");\n> return ret;\n> }\n> @@ -1254,8 +1253,8 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> /* Ready the IPA - it must know about the sensor resolution. */\n> ipa::RPi::ConfigOutput result;\n>\n> - ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> ipaConfig,\n> - &result, &ret);\n> + ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> ipaConfig,\n> + &result);\n>\n> if (ret < 0) {\n> LOG(RPI, Error) << \"IPA configuration failed!\";\n> diff --git\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> index 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\n> &data)\n> {%- endif %}\n> }\n> {% if method|method_return_value != \"void\" %}\n> - return\n> IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf.data(),\n> 0);\n> + {{method|method_return_value}} _finalRet =\n> IPADataSerializer<{{method|method_return_value}}>::deserialize(_ipcOutputBuf.data(),\n> 0);\n> +\n> +{{proxy_funcs.deserialize_call(method|method_param_outputs,\n> '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()', init_offset =\n> method|method_return_value|byte_width|int)}}\n> +\n> + return _finalRet;\n> +\n> {% elif method|method_param_outputs|length > 0 %}\n> {{proxy_funcs.deserialize_call(method|method_param_outputs,\n> '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()')}}\n> {% endif -%}\n> diff --git\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> index d08af76d..c4206162 100644\n> ---\n> a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> +++\n> b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -85,15 +85,14 @@ public:\n> {{param|name}} {{param.mojom_name}};\n> {% endfor %}\n> {%- if method|method_return_value != \"void\" %}\n> - {{method|method_return_value}} _callRet =\n> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> -{%- else %}\n> + {{method|method_return_value}} _callRet =\n> +{%- endif -%}\n>\n> ipa_->{{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> IPCMessage::Header header = {\n> _ipcMessage.header().cmd, _ipcMessage.header().cookie };\n> IPCMessage _response(header);\n> @@ -102,9 +101,8 @@ public:\n> std::tie(_callRetBuf, std::ignore) =\n>\n> IPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);\n> _response.data().insert(_response.data().end(),\n> _callRetBuf.cbegin(), _callRetBuf.cend());\n> -{%- else %}\n> - {{proxy_funcs.serialize_call(method|method_param_outputs,\n> \"_response.data()\", \"_response.fds()\")|indent(16, true)}}\n> {%- endif %}\n> + {{proxy_funcs.serialize_call(method|method_param_outputs,\n> \"_response.data()\", \"_response.fds()\")|indent(16, true)}}\n> int _ret = socket_.send(_response.payload());\n> if (_ret < 0) {\n> LOG({{proxy_worker_name}}, Error)\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index 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 =\n> false, iter = false, data_size = '') -%}\n> -{% set ns = namespace(size_offset = 0) %}\n> +{%- macro deserialize_call(params, buf, fds, pointer = true, declare =\n> 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> [[maybe_unused]] const size_t {{param.mojom_name}}BufSize =\n> readPOD({{buf}}, {{ns.size_offset}}\n> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py\n> b/utils/ipc/generators/mojom_libcamera_generator.py\n> index 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)}\n> *{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\n> len(method.response_parameters) == 0:\n> return 'void'\n> - if len(method.response_parameters) == 1 and\n> 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> --\n> 2.27.0\n>\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 1F25EBD80C\n\tfor ;\n\tFri, 5 Mar 2021 11:30:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8847D68A92;\n\tFri, 5 Mar 2021 12:30:36 +0100 (CET)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41720602EC\n\tfor ;\n\tFri, 5 Mar 2021 12:30:35 +0100 (CET)","by mail-lf1-x133.google.com with SMTP id u4so3099990lfs.0\n\tfor ;\n\tFri, 05 Mar 2021 03:30:35 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"YmluYsdr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=zmbzBH4SOM+F/P2GSlcmMcNtdVNRWl8Eh5ohw8eoiss=;\n\tb=YmluYsdrwICqIXXHe3irbygSgocBOzGupv8d8obV6Gsjd51NfZG1/pjTVPN0YBff2O\n\tZsICbGEZrj5w8q6Tar6m0y028aeuUH/XZNt3Lj+SyuXSLvbY4MnfztXfX8F2cJ548sVP\n\tWI3nz/7Nx8vhmrvAuzlwUt3XWAEP8KlgtZ5tCOUC6cO2622ZMf00+FHmjcViEz69reES\n\tSn75dfXkZ1hubJ0mOBcyIDHKeu+nkhizYxYgvRdcEsuwdic4de8hPpt5uUnT4os7SO0D\n\t+1UqzZ7B8uCIo6e4qQwTy95nKZMFdBH4U8w4TTYQ57R2VDc1xBQo5xqWH8tEwKuceT6Z\n\t47fQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=zmbzBH4SOM+F/P2GSlcmMcNtdVNRWl8Eh5ohw8eoiss=;\n\tb=nycStEZrFT+qUlZk2Zq1Z9pw9A1ZvropsCF62IBe/9zZJfQkyHT+TsjIuEzkddsaWg\n\tlgLtWu5J0gSH1o0jpx3kaYQpcr+lD3YiY7btwjYu8WGQDIFg85cWDxIJ9uDH5HOBeztx\n\tJlMGU4vUSrD8WmXtpZ4sOPJJScFFJA83EraC3pei0gDVYshXkUeUfD1oiBl59ZgsjPxP\n\tygimQ76lYxbaE9W3a1Vk0nG4TbQQLNeBKus3BOcZCy+58di5faDAlojsbhEpZmNSxLLB\n\tEyZHNHhPdoiTucd1Cl7tlceGZkhoMtaFmnx4sRYobfJYZ2lryKAHHP5b2NdM3iottUPe\n\t6ZTQ==","X-Gm-Message-State":"AOAM5313jU7y0fLppMkDHnYAc94Br+GpGYeIFhuhxVsl3Ew6buc/Yo1i\n\t1DI4h3a0YOfWhaSku5ipb5zA7c9aHgK/tUyqv95uzFIGQnw6MA==","X-Google-Smtp-Source":"ABdhPJzcuRTls2lW27RJEt4RbJ8e29Ps96Z9w6UdpVnvtzjYgDgKzPicI1WYe7r/TM/GamUD/2xw87PzYrkTGwblvWs=","X-Received":"by 2002:a19:70b:: with SMTP id 11mr5246824lfh.375.1614943834610; \n\tFri, 05 Mar 2021 03:30:34 -0800 (PST)","MIME-Version":"1.0","References":"<20210305093146.25943-1-paul.elder@ideasonboard.com>","In-Reply-To":"<20210305093146.25943-1-paul.elder@ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 5 Mar 2021 11:30:18 +0000","Message-ID":"","To":"Paul Elder ","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============3363491977716729514==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15489,"web_url":"https://patchwork.libcamera.org/comment/15489/","msgid":"","date":"2021-03-05T15:16:04","subject":"Re: [libcamera-devel] [PATCH] utils: ipc,\n\traspberrypi: Make first output parameter direct return if int32","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Mar 05, 2021 at 06:31:46PM +0900, Paul Elder wrote:\n> To make it more convenient for synchronous IPA calls to return a status,\n> convert the first output into a direct return if it is an int32.\n> \n> Convert the raspberrypi IPA interface and usage appropriately.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> This is still an RFC until we decide what we want to do.\n> \n> This patch depends on:\n> - utils: ipc: Support custom parameters to init()\n> - DEMO: raspberrypi: Use custom parameters to init()\n> \n> I'm expecting to squash with the first patch. If we drop the second\n> patch then we can remove the raspberrypi components from this patch.\n> \n> I still need to update the IPA guide appropriately, but I decided to get\n> approval on this RFC first.\n> \n> Basically the only question (besides standard review) I want to ask is,\n> should we squash this with \"utils: ipc: Support custom parameters to\n> init()\"?\n\nI'd keep this patch separate from \"utils: ipc: Support custom parameters\nto init()\", and rebase the DEMO patch on top of the other two. This\npatch could actually go before \"utils: ipc: Support custom parameters to\ninit()\" if it can help.\n\nI think I'd split the RPi change out from this to a separate patch.\n\n> Naush, does this (and \"utils: ipc: Support custom parameters to init()\")\n> fit 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(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index 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 streamConfig,\n> \t\t map 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()\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 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 &streamConfig,\n> -\t\t const std::map &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 &streamConfig,\n> +\t\t const std::map &entityControls,\n> +\t\t const ipa::RPi::ConfigInput &data,\n> +\t\t ipa::RPi::ConfigOutput *response) override;\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &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 &streamConfig,\n> -\t\t const std::map &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 &streamConfig,\n> +\t\t const std::map &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 &buffers)\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 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\nYou can drop the blank line.\n\n> \tif (ret < 0) {\n> \t\tLOG(RPI, Error) << \"IPA configuration failed!\";\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index 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\nMaybe _retValue ?\n\n> +\n> +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()', init_offset = method|method_return_value|byte_width|int)}}\n\nPassing an offset feels a bit fragile to me. I think we could improve\nthe serializer API (possibly using span) in a way that would make it\nmore robust. That's out of scope for this patch though.\n\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 -%}\n> diff --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\n> index 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)\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index 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({{buf}}, {{ns.size_offset}}\n> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py\n> index 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\nIt's nice to see the flexibility in the generator :-)\n\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 return str(int(_bit_widths[key]) // 8)\n\nReviewed-by: Laurent Pinchart \n\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,","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 D6D55BD80C\n\tfor ;\n\tFri, 5 Mar 2021 15:16:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39F2468A7E;\n\tFri, 5 Mar 2021 16:16:37 +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 6A048602EC\n\tfor ;\n\tFri, 5 Mar 2021 16:16:35 +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 C7182CC;\n\tFri, 5 Mar 2021 16:16:34 +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=\"bMOY7goP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614957395;\n\tbh=//B21xHOLCopAFjH2xmy8vr0YcGpvxHcusFF50t4n+4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bMOY7goPiRtCSdAWGnX7ryWmwpJzdULEo0mhI5tN06UhbHwoYr9HTurjz4cgMuTI+\n\tUWf2GJC/Dc5w+Dk0sH5dO+XJNCVvYv/L/0ajWaFcvFO60rUh2Q96nu+OJNgUKFz1EO\n\thLOSTPC3qGyKB7+wRgeomqp+E/bzQgAk2uQJH3KA=","Date":"Fri, 5 Mar 2021 17:16:04 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210305093146.25943-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210305093146.25943-1-paul.elder@ideasonboard.com>","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]