[libcamera-devel] utils: ipc, raspberrypi: Make first output parameter direct return if int32
diff mbox series

Message ID 20210305093146.25943-1-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel] utils: ipc, raspberrypi: Make first output parameter direct return if int32
Related show

Commit Message

Paul Elder March 5, 2021, 9:31 a.m. UTC 
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@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?

---
 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(-)

Comments

Naushir Patuck March 5, 2021, 11:30 a.m. UTC | #1 
Hi Paul,

Thank you for your work.

On Fri, 5 Mar 2021 at 09:31, Paul Elder <paul.elder@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@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@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
>
>
Laurent Pinchart March 5, 2021, 3:16 p.m. UTC | #2 
Hi Paul,

Thank you for the patch.

On Fri, Mar 05, 2021 at 06:31:46PM +0900, Paul Elder 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@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()"?

I'd keep this patch separate from "utils: ipc: Support custom parameters
to init()", and rebase the DEMO patch on top of the other two. This
patch could actually go before "utils: ipc: Support custom parameters to
init()" if it can help.

I think I'd split the RPi change out from this to a separate patch.

> Naush, does this (and "utils: ipc: Support custom parameters to init()")
> fit what you want?
> 
> ---
>  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);
>  

You can drop the blank line.

>  	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);

Maybe _retValue ?

> +
> +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()', init_offset = method|method_return_value|byte_width|int)}}

Passing an offset feels a bit fragile to me. I think we could improve
the serializer API (possibly using span) in a way that would make it
more robust. That's out of scope for this patch though.

> +
> +	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'

It's nice to see the flexibility in the generator :-)

>  
>  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))

    return str(int(_bit_widths[key]) // 8)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +
>  # 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,

Patch
diff mbox series 

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,