[libcamera-devel] [PATCH] utils: ipc, raspberrypi: Make first output parameter direct return if int32
Naushir Patuck
naush at raspberrypi.com
Fri Mar 5 12:30:18 CET 2021
Hi Paul,
Thank you for your work.
On Fri, 5 Mar 2021 at 09:31, Paul Elder <paul.elder at ideasonboard.com> wrote:
> To make it more convenient for synchronous IPA calls to return a status,
> convert the first output into a direct return if it is an int32.
>
> Convert the raspberrypi IPA interface and usage appropriately.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> This is still an RFC until we decide what we want to do.
>
> This patch depends on:
> - utils: ipc: Support custom parameters to init()
> - DEMO: raspberrypi: Use custom parameters to init()
>
> I'm expecting to squash with the first patch. If we drop the second
> patch then we can remove the raspberrypi components from this patch.
>
> I still need to update the IPA guide appropriately, but I decided to get
> approval on this RFC first.
>
> Basically the only question (besides standard review) I want to ask is,
> should we squash this with "utils: ipc: Support custom parameters to
> init()"?
>
> Naush, does this (and "utils: ipc: Support custom parameters to init()")
> fit what you want?
>
Yes, it seems to. However, I have not had a chance to actually apply these
with my proposed changes yet.
Acked-by: Naushir Patuck <naush at raspberrypi.com>
>
> ---
> include/libcamera/ipa/raspberrypi.mojom | 2 +-
> src/ipa/raspberrypi/raspberrypi.cpp | 44 +++++++++---------
> .../pipeline/raspberrypi/raspberrypi.cpp | 7 ++-
> .../module_ipa_proxy.cpp.tmpl | 7 ++-
> .../module_ipa_proxy_worker.cpp.tmpl | 8 ++--
> .../libcamera_templates/proxy_functions.tmpl | 4 +-
> .../generators/mojom_libcamera_generator.py | 45 ++++++++++++-------
> 7 files changed, 64 insertions(+), 53 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> index b8944227..99a62c02 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -78,7 +78,7 @@ interface IPARPiInterface {
> map<uint32, IPAStream> streamConfig,
> map<uint32, ControlInfoMap> entityControls,
> ConfigInput ipaConfig)
> - => (ConfigOutput results, int32 ret);
> + => (int32 ret, ConfigOutput results);
>
> /**
> * \fn mapBuffers()
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 6a9aba6f..ac18dcbd 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -79,17 +79,17 @@ public:
> munmap(lsTable_, ipa::RPi::MaxLsGridSize);
> }
>
> - void init(const IPASettings &settings, const std::string
> &sensorName,
> - int *ret, bool *metadataSupport) override;
> + int init(const IPASettings &settings, const std::string
> &sensorName,
> + bool *metadataSupport) override;
> void start(const ipa::RPi::StartControls &data,
> ipa::RPi::StartControls *result) override;
> void stop() override {}
>
> - void configure(const CameraSensorInfo &sensorInfo,
> - const std::map<unsigned int, IPAStream>
> &streamConfig,
> - const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> - const ipa::RPi::ConfigInput &data,
> - ipa::RPi::ConfigOutput *response, int32_t *ret)
> override;
> + int configure(const CameraSensorInfo &sensorInfo,
> + const std::map<unsigned int, IPAStream>
> &streamConfig,
> + const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> + const ipa::RPi::ConfigInput &data,
> + ipa::RPi::ConfigOutput *response) override;
> void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> void unmapBuffers(const std::vector<unsigned int> &ids) override;
> void signalStatReady(const uint32_t bufferId) override;
> @@ -165,15 +165,15 @@ private:
> double maxFrameDuration_;
> };
>
> -void IPARPi::init(const IPASettings &settings, const std::string
> &sensorName,
> - int *ret, bool *metadataSupport)
> +int IPARPi::init(const IPASettings &settings, const std::string
> &sensorName,
> + bool *metadataSupport)
> {
> LOG(IPARPI, Debug) << "sensor name is " << sensorName;
>
> tuningFile_ = settings.configurationFile;
>
> *metadataSupport = true;
> - *ret = 0;
> + return 0;
> }
>
> void IPARPi::start(const ipa::RPi::StartControls &data,
> @@ -296,16 +296,15 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
> mode_.max_frame_length = sensorInfo.maxFrameLength;
> }
>
> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> - [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> - const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> - const ipa::RPi::ConfigInput &ipaConfig,
> - ipa::RPi::ConfigOutput *result, int32_t *ret)
> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> + [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> + const std::map<unsigned int, ControlInfoMap>
> &entityControls,
> + const ipa::RPi::ConfigInput &ipaConfig,
> + ipa::RPi::ConfigOutput *result)
> {
> if (entityControls.size() != 2) {
> LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> - *ret = -1;
> - return;
> + return -1;
> }
>
> result->params = 0;
> @@ -315,14 +314,12 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
>
> if (!validateSensorControls()) {
> LOG(IPARPI, Error) << "Sensor control validation failed.";
> - *ret = -1;
> - return;
> + return -1;
> }
>
> if (!validateIspControls()) {
> LOG(IPARPI, Error) << "ISP control validation failed.";
> - *ret = -1;
> - return;
> + return -1;
> }
>
> /* Setup a metadata ControlList to output metadata. */
> @@ -340,8 +337,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> if (!helper_) {
> LOG(IPARPI, Error) << "Could not create camera
> helper for "
> << cameraName;
> - *ret = -1;
> - return;
> + return -1;
> }
>
> /*
> @@ -409,7 +405,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
>
> lastMode_ = mode_;
>
> - *ret = 0;
> + return 0;
> }
>
> void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index a1c90028..106318ed 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1194,9 +1194,8 @@ int RPiCameraData::loadIPA()
>
> IPASettings settings(ipa_->configurationFile(sensor_->model() +
> ".json"));
>
> - int ret;
> bool metadataSupport;
> - ipa_->init(settings, "sensor name", &ret, &metadataSupport);
> + int ret = ipa_->init(settings, "sensor name", &metadataSupport);
> LOG(RPI, Debug) << "metadata support " << (metadataSupport ? "yes"
> : "no");
> return ret;
> }
> @@ -1254,8 +1253,8 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> /* Ready the IPA - it must know about the sensor resolution. */
> ipa::RPi::ConfigOutput result;
>
> - ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> - &result, &ret);
> + ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> + &result);
>
> if (ret < 0) {
> LOG(RPI, Error) << "IPA configuration failed!";
> diff --git
> a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index ff667ce0..167aaabd 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -212,7 +212,12 @@ void {{proxy_name}}::recvMessage(const IPCMessage
> &data)
> {%- endif %}
> }
> {% if method|method_return_value != "void" %}
> - return
> IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf.data(),
> 0);
> + {{method|method_return_value}} _finalRet =
> IPADataSerializer<{{method|method_return_value}}>::deserialize(_ipcOutputBuf.data(),
> 0);
> +
> +{{proxy_funcs.deserialize_call(method|method_param_outputs,
> '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()', init_offset =
> method|method_return_value|byte_width|int)}}
> +
> + return _finalRet;
> +
> {% elif method|method_param_outputs|length > 0 %}
> {{proxy_funcs.deserialize_call(method|method_param_outputs,
> '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()')}}
> {% endif -%}
> 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
> index d08af76d..c4206162 100644
> ---
> a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> +++
> b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> @@ -85,15 +85,14 @@ public:
> {{param|name}} {{param.mojom_name}};
> {% endfor %}
> {%- if method|method_return_value != "void" %}
> - {{method|method_return_value}} _callRet =
> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> -{%- else %}
> + {{method|method_return_value}} _callRet =
> +{%- endif -%}
>
> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}
> {{- ", " if method|method_param_outputs|params_comma_sep -}}
> {%- for param in method|method_param_outputs -%}
> &{{param.mojom_name}}{{", " if not loop.last}}
> {%- endfor -%}
> );
> -{%- endif %}
> {% if not method|is_async %}
> IPCMessage::Header header = {
> _ipcMessage.header().cmd, _ipcMessage.header().cookie };
> IPCMessage _response(header);
> @@ -102,9 +101,8 @@ public:
> std::tie(_callRetBuf, std::ignore) =
>
> IPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);
> _response.data().insert(_response.data().end(),
> _callRetBuf.cbegin(), _callRetBuf.cend());
> -{%- else %}
> - {{proxy_funcs.serialize_call(method|method_param_outputs,
> "_response.data()", "_response.fds()")|indent(16, true)}}
> {%- endif %}
> + {{proxy_funcs.serialize_call(method|method_param_outputs,
> "_response.data()", "_response.fds()")|indent(16, true)}}
> int _ret = socket_.send(_response.payload());
> if (_ret < 0) {
> LOG({{proxy_worker_name}}, Error)
> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> index f2d86b67..c2ac42fc 100644
> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -135,8 +135,8 @@
> #
> # \todo Avoid intermediate vectors
> #}
> -{%- macro deserialize_call(params, buf, fds, pointer = true, declare =
> false, iter = false, data_size = '') -%}
> -{% set ns = namespace(size_offset = 0) %}
> +{%- macro deserialize_call(params, buf, fds, pointer = true, declare =
> false, iter = false, data_size = '', init_offset = 0) -%}
> +{% set ns = namespace(size_offset = init_offset) %}
> {%- if params|length > 1 %}
> {%- for param in params %}
> [[maybe_unused]] const size_t {{param.mojom_name}}BufSize =
> readPOD<uint32_t>({{buf}}, {{ns.size_offset}}
> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py
> b/utils/ipc/generators/mojom_libcamera_generator.py
> index fa0c21a2..e72a05ff 100644
> --- a/utils/ipc/generators/mojom_libcamera_generator.py
> +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> @@ -149,10 +149,16 @@ def MethodParamInputs(method):
> return method.parameters
>
> def MethodParamOutputs(method):
> - if (MethodReturnValue(method) != 'void' or
> - method.response_parameters is None):
> + if method.response_parameters is None:
> + return []
> +
> + if MethodReturnValue(method) == 'void':
> + return method.response_parameters
> +
> + if len(method.response_parameters) <= 1:
> return []
> - return method.response_parameters
> +
> + return method.response_parameters[1:]
>
> def MethodParamsHaveFd(parameters):
> return len([x for x in parameters if HasFd(x)]) > 0
> @@ -167,11 +173,8 @@ def MethodParamNames(method):
> params = []
> for param in method.parameters:
> params.append(param.mojom_name)
> - if MethodReturnValue(method) == 'void':
> - if method.response_parameters is None:
> - return params
> - for param in method.response_parameters:
> - params.append(param.mojom_name)
> + for param in MethodParamOutputs(method):
> + params.append(param.mojom_name)
> return params
>
> def MethodParameters(method):
> @@ -180,18 +183,17 @@ def MethodParameters(method):
> params.append('const %s %s%s' % (GetNameForElement(param),
> '&' if not IsPod(param) else '',
> param.mojom_name))
> - if MethodReturnValue(method) == 'void':
> - if method.response_parameters is None:
> - return params
> - for param in method.response_parameters:
> - params.append(f'{GetNameForElement(param)}
> *{param.mojom_name}')
> + for param in MethodParamOutputs(method):
> + params.append(f'{GetNameForElement(param)} *{param.mojom_name}')
> return params
>
> def MethodReturnValue(method):
> - if method.response_parameters is None:
> + if method.response_parameters is None or
> len(method.response_parameters) == 0:
> return 'void'
> - if len(method.response_parameters) == 1 and
> IsPod(method.response_parameters[0]):
> - return GetNameForElement(method.response_parameters[0])
> + first_output = method.response_parameters[0]
> + if ((len(method.response_parameters) == 1 and IsPod(first_output)) or
> + first_output.kind == mojom.INT32):
> + return GetNameForElement(first_output)
> return 'void'
>
> def IsAsync(method):
> @@ -237,6 +239,16 @@ def BitWidth(element):
> return '32'
> return ''
>
> +def ByteWidthFromCppType(t):
> + key = None
> + for mojo_type, cpp_type in _kind_to_cpp_type.items():
> + if t == cpp_type:
> + key = mojo_type
> + if key is None:
> + raise Exception('invalid type')
> + return str(int(int(_bit_widths[key]) / 8))
> +
> +
> # Get the type name for a given element
> def GetNameForElement(element):
> # structs
> @@ -373,6 +385,7 @@ class Generator(generator.Generator):
> libcamera_filters = {
> 'all_types': GetAllTypes,
> 'bit_width': BitWidth,
> + 'byte_width' : ByteWidthFromCppType,
> 'cap': Capitalize,
> 'choose': Choose,
> 'comma_sep': CommaSep,
> --
> 2.27.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210305/329edc7d/attachment-0001.htm>
More information about the libcamera-devel
mailing list