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