[libcamera-devel,v4,09/37] libcamera: Add IPAIPC

10357 diff mbox series

Create a virtual IPAIPC class that models an IPC/RPC system. IPA proxies
and proxy workers will call into the IPAIPC, rather than implementing
the IPC themselves.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

---
Change in v4:
- change snake_case to camelCase
- remove ipaModulePath and ipaProxyWorkerPath from parameters in the
  IPAIPC base contructor
- include signal.h

Changes in v3:
- expand documentation a bit
- move [[maybe_unused]] to after the parameters in the IPAIPC
  constructor, to appease gcc <= 9.1

Changes in v2:
- add documentation
---
 include/libcamera/internal/ipa_ipc.h |  42 ++++++++++
 src/libcamera/ipa_ipc.cpp            | 110 +++++++++++++++++++++++++++
 src/libcamera/meson.build            |   1 +
 3 files changed, 153 insertions(+)
 create mode 100644 include/libcamera/internal/ipa_ipc.h
 create mode 100644 src/libcamera/ipa_ipc.cpp

Hi Paul,

Thanks for your work.

On 2020-11-06 19:36:39 +0900, Paul Elder wrote:
> Create a virtual IPAIPC class that models an IPC/RPC system. IPA proxies
> and proxy workers will call into the IPAIPC, rather than implementing
> the IPC themselves.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> ---
> Change in v4:
> - change snake_case to camelCase
> - remove ipaModulePath and ipaProxyWorkerPath from parameters in the
>   IPAIPC base contructor
> - include signal.h
> 
> Changes in v3:
> - expand documentation a bit
> - move [[maybe_unused]] to after the parameters in the IPAIPC
>   constructor, to appease gcc <= 9.1
> 
> Changes in v2:
> - add documentation
> ---
>  include/libcamera/internal/ipa_ipc.h |  42 ++++++++++
>  src/libcamera/ipa_ipc.cpp            | 110 +++++++++++++++++++++++++++
>  src/libcamera/meson.build            |   1 +
>  3 files changed, 153 insertions(+)
>  create mode 100644 include/libcamera/internal/ipa_ipc.h
>  create mode 100644 src/libcamera/ipa_ipc.cpp
> 
> diff --git a/include/libcamera/internal/ipa_ipc.h b/include/libcamera/internal/ipa_ipc.h
> new file mode 100644
> index 00000000..f83a90a3
> --- /dev/null
> +++ b/include/libcamera/internal/ipa_ipc.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc.h - Image Processing Algorithm IPC module for IPA proxies
> + */
> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_H__
> +#define __LIBCAMERA_INTERNAL_IPA_IPC_H__
> +
> +#include <vector>
> +
> +#include <libcamera/signal.h>
> +
> +namespace libcamera {
> +
> +class IPAIPC
> +{
> +public:
> +	IPAIPC();
> +	virtual ~IPAIPC();
> +
> +	bool isValid() const { return valid_; }
> +
> +	virtual int sendSync(uint32_t cmd,
> +			     const std::vector<uint8_t> &dataIn,
> +			     const std::vector<int32_t> &fdsIn,
> +			     std::vector<uint8_t> *dataOut = nullptr,
> +			     std::vector<int32_t> *fdsOut = nullptr) = 0;
> +
> +	virtual int sendAsync(uint32_t cmd,
> +			      const std::vector<uint8_t> &dataIn,
> +			      const std::vector<int32_t> &fdsIn) = 0;
> +
> +	Signal<std::vector<uint8_t> &, std::vector<int32_t> &> recvIPC;
> +
> +protected:
> +	bool valid_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */
> diff --git a/src/libcamera/ipa_ipc.cpp b/src/libcamera/ipa_ipc.cpp
> new file mode 100644
> index 00000000..964d4ac1
> --- /dev/null
> +++ b/src/libcamera/ipa_ipc.cpp
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc.cpp - Image Processing Algorithm IPC module for IPA proxies
> + */
> +
> +#include <vector>
> +
> +#include "libcamera/internal/ipc_unixsocket.h"
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/process.h"
> +#include "libcamera/internal/thread.h"
> +
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include "libcamera/internal/ipa_ipc.h"
> +
> +/**
> + * \file ipa_ipc.h
> + * \brief IPC mechanism for IPA isolation
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPAIPC)
> +
> +/**
> + * \class IPAIPC
> + * \brief IPC mechanism for IPA isolation
> + *
> + * Virtual class to model an IPC mechanism for use by IPA proxies for IPA
> + * isolation. sendSync() and sendAsync() must be implemented, and the recvIPC
> + * signal must be emitted whenever new data is available.
> + */
> +
> +/**
> + * \brief Construct an IPAIPC instance
> + */
> +IPAIPC::IPAIPC() : valid_(false)
> +{
> +}
> +
> +IPAIPC::~IPAIPC()
> +{
> +}
> +
> +/**
> + * \fn IPAIPC::isValid()
> + * \brief Check if the IPAIPC instance is valid
> + *
> + * An IPAIPC instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * \return True if the IPAIPC is valid, false otherwise
> + */
> +
> +/**
> + * \fn IPAIPC::sendSync()
> + * \brief Send a message over IPC synchronously
> + * \param[in] cmd Command ID
> + * \param[in] dataIn Data to send, as a byte vector
> + * \param[in] fdsIn Fds to send, in a vector
> + * \param[in] dataOut Byte vector in which to receive data, if applicable
> + * \param[in] fdsOut Fds vector in which to receive fds, if applicable
> + *
> + * This function will not return until a response is received. The event loop
> + * will still continue to execute, however.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \fn IPAIPC::sendAsync()
> + * \brief Send a message over IPC asynchronously
> + * \param[in] cmd Command ID
> + * \param[in] dataIn Data to send, as a byte vector
> + * \param[in] fdsIn Fds to send, in a vector
> + *
> + * This function will return immediately after sending the message.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \var IPAIPC::recvIPC
> + * \brief Signal to be emitted when data is received over IPC
> + *
> + * When data is received over IPC, this signal shall be emitted. Users must
> + * connect to this to receive new data.
> + *
> + * The parameters of the signal are a vector of bytes and a vector of file
> + * descriptors.
> + */
> +
> +/**
> + * \var IPAIPC::valid_
> + * \brief Flag to indicate if the IPAIPC instance is valid
> + *
> + * An IPAIPC instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * This flag can be read via IPAIPC::isValid().
> + *
> + * Implementations of the IPAIPC class should set this flag upon successful
> + * construction.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 01bcffd4..85f3a202 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -25,6 +25,7 @@ libcamera_sources = files([
>      'ipa_context_wrapper.cpp',
>      'ipa_controls.cpp',
>      'ipa_data_serializer.cpp',
> +    'ipa_ipc.cpp',
>      'ipa_interface.cpp',
>      'ipa_manager.cpp',
>      'ipa_module.cpp',
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

On Fri, Nov 06, 2020 at 07:36:39PM +0900, Paul Elder wrote:
> Create a virtual IPAIPC class that models an IPC/RPC system. IPA proxies
> and proxy workers will call into the IPAIPC, rather than implementing
> the IPC themselves.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> ---
> Change in v4:
> - change snake_case to camelCase
> - remove ipaModulePath and ipaProxyWorkerPath from parameters in the
>   IPAIPC base contructor
> - include signal.h
>
> Changes in v3:
> - expand documentation a bit
> - move [[maybe_unused]] to after the parameters in the IPAIPC
>   constructor, to appease gcc <= 9.1
>
> Changes in v2:
> - add documentation
> ---
>  include/libcamera/internal/ipa_ipc.h |  42 ++++++++++
>  src/libcamera/ipa_ipc.cpp            | 110 +++++++++++++++++++++++++++
>  src/libcamera/meson.build            |   1 +
>  3 files changed, 153 insertions(+)
>  create mode 100644 include/libcamera/internal/ipa_ipc.h
>  create mode 100644 src/libcamera/ipa_ipc.cpp
>
> diff --git a/include/libcamera/internal/ipa_ipc.h b/include/libcamera/internal/ipa_ipc.h
> new file mode 100644
> index 00000000..f83a90a3
> --- /dev/null
> +++ b/include/libcamera/internal/ipa_ipc.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc.h - Image Processing Algorithm IPC module for IPA proxies
> + */
> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_H__
> +#define __LIBCAMERA_INTERNAL_IPA_IPC_H__
> +
> +#include <vector>
> +
> +#include <libcamera/signal.h>
> +
> +namespace libcamera {
> +
> +class IPAIPC
> +{
> +public:
> +	IPAIPC();
> +	virtual ~IPAIPC();
> +
> +	bool isValid() const { return valid_; }
> +
> +	virtual int sendSync(uint32_t cmd,
> +			     const std::vector<uint8_t> &dataIn,
> +			     const std::vector<int32_t> &fdsIn,
> +			     std::vector<uint8_t> *dataOut = nullptr,
> +			     std::vector<int32_t> *fdsOut = nullptr) = 0;
> +
> +	virtual int sendAsync(uint32_t cmd,
> +			      const std::vector<uint8_t> &dataIn,
> +			      const std::vector<int32_t> &fdsIn) = 0;
> +
> +	Signal<std::vector<uint8_t> &, std::vector<int32_t> &> recvIPC;
> +
> +protected:
> +	bool valid_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */
> diff --git a/src/libcamera/ipa_ipc.cpp b/src/libcamera/ipa_ipc.cpp
> new file mode 100644
> index 00000000..964d4ac1
> --- /dev/null
> +++ b/src/libcamera/ipa_ipc.cpp
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc.cpp - Image Processing Algorithm IPC module for IPA proxies
> + */
> +
> +#include <vector>
> +
> +#include "libcamera/internal/ipc_unixsocket.h"
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/process.h"
> +#include "libcamera/internal/thread.h"
> +
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include "libcamera/internal/ipa_ipc.h"
> +
> +/**
> + * \file ipa_ipc.h
> + * \brief IPC mechanism for IPA isolation
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPAIPC)
> +
> +/**
> + * \class IPAIPC
> + * \brief IPC mechanism for IPA isolation
> + *
> + * Virtual class to model an IPC mechanism for use by IPA proxies for IPA
> + * isolation. sendSync() and sendAsync() must be implemented, and the recvIPC
> + * signal must be emitted whenever new data is available.
> + */
> +
> +/**
> + * \brief Construct an IPAIPC instance
> + */
> +IPAIPC::IPAIPC() : valid_(false)

Nit: we usually break variable initialization to a new line

> +{
> +}
> +
> +IPAIPC::~IPAIPC()
> +{
> +}
> +
> +/**
> + * \fn IPAIPC::isValid()
> + * \brief Check if the IPAIPC instance is valid
> + *
> + * An IPAIPC instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * \return True if the IPAIPC is valid, false otherwise
> + */
> +
> +/**
> + * \fn IPAIPC::sendSync()
> + * \brief Send a message over IPC synchronously
> + * \param[in] cmd Command ID
> + * \param[in] dataIn Data to send, as a byte vector
> + * \param[in] fdsIn Fds to send, in a vector
> + * \param[in] dataOut Byte vector in which to receive data, if applicable
> + * \param[in] fdsOut Fds vector in which to receive fds, if applicable
> + *
> + * This function will not return until a response is received. The event loop
> + * will still continue to execute, however.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \fn IPAIPC::sendAsync()
> + * \brief Send a message over IPC asynchronously
> + * \param[in] cmd Command ID
> + * \param[in] dataIn Data to send, as a byte vector
> + * \param[in] fdsIn Fds to send, in a vector
> + *
> + * This function will return immediately after sending the message.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \var IPAIPC::recvIPC
> + * \brief Signal to be emitted when data is received over IPC
> + *
> + * When data is received over IPC, this signal shall be emitted. Users must
> + * connect to this to receive new data.
> + *
> + * The parameters of the signal are a vector of bytes and a vector of file
> + * descriptors.
> + */
> +
> +/**
> + * \var IPAIPC::valid_
> + * \brief Flag to indicate if the IPAIPC instance is valid
> + *
> + * An IPAIPC instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * This flag can be read via IPAIPC::isValid().
> + *
> + * Implementations of the IPAIPC class should set this flag upon successful
> + * construction.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 01bcffd4..85f3a202 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -25,6 +25,7 @@ libcamera_sources = files([
>      'ipa_context_wrapper.cpp',
>      'ipa_controls.cpp',
>      'ipa_data_serializer.cpp',
> +    'ipa_ipc.cpp',
>      'ipa_interface.cpp',
>      'ipa_manager.cpp',
>      'ipa_module.cpp',
> --
> 2.27.0
>

Hi Paul,

Thank you for the patch.

On Fri, Nov 06, 2020 at 07:36:39PM +0900, Paul Elder wrote:
> Create a virtual IPAIPC class that models an IPC/RPC system. IPA proxies
> and proxy workers will call into the IPAIPC, rather than implementing
> the IPC themselves.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> ---
> Change in v4:
> - change snake_case to camelCase
> - remove ipaModulePath and ipaProxyWorkerPath from parameters in the
>   IPAIPC base contructor
> - include signal.h
> 
> Changes in v3:
> - expand documentation a bit
> - move [[maybe_unused]] to after the parameters in the IPAIPC
>   constructor, to appease gcc <= 9.1
> 
> Changes in v2:
> - add documentation
> ---
>  include/libcamera/internal/ipa_ipc.h |  42 ++++++++++
>  src/libcamera/ipa_ipc.cpp            | 110 +++++++++++++++++++++++++++
>  src/libcamera/meson.build            |   1 +
>  3 files changed, 153 insertions(+)
>  create mode 100644 include/libcamera/internal/ipa_ipc.h
>  create mode 100644 src/libcamera/ipa_ipc.cpp
> 
> diff --git a/include/libcamera/internal/ipa_ipc.h b/include/libcamera/internal/ipa_ipc.h
> new file mode 100644
> index 00000000..f83a90a3
> --- /dev/null
> +++ b/include/libcamera/internal/ipa_ipc.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc.h - Image Processing Algorithm IPC module for IPA proxies
> + */
> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_H__
> +#define __LIBCAMERA_INTERNAL_IPA_IPC_H__
> +
> +#include <vector>
> +
> +#include <libcamera/signal.h>
> +
> +namespace libcamera {
> +
> +class IPAIPC
> +{
> +public:
> +	IPAIPC();
> +	virtual ~IPAIPC();
> +
> +	bool isValid() const { return valid_; }
> +
> +	virtual int sendSync(uint32_t cmd,
> +			     const std::vector<uint8_t> &dataIn,
> +			     const std::vector<int32_t> &fdsIn,
> +			     std::vector<uint8_t> *dataOut = nullptr,
> +			     std::vector<int32_t> *fdsOut = nullptr) = 0;

How about grouping the command, data and fds in an IPAIPCMessage class ?
This would become

	virtual int sendSync(const IPAIPCMessage &msg,
			     IPAIPCMessage *response = nullptr) = 0;

which should be easier to read. Same for sendAsync() and recvIPC.

I'm even tempted to merge sendSync() and sendAsync() into a single
function with an additional flag argument, with

class IPCMessage {
public:
	enum Flag {
		Sync = 1 << 0,
	};

	...
};

> +
> +	virtual int sendAsync(uint32_t cmd,
> +			      const std::vector<uint8_t> &dataIn,
> +			      const std::vector<int32_t> &fdsIn) = 0;
> +
> +	Signal<std::vector<uint8_t> &, std::vector<int32_t> &> recvIPC;

This can be called recv or receive.

> +
> +protected:
> +	bool valid_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */
> diff --git a/src/libcamera/ipa_ipc.cpp b/src/libcamera/ipa_ipc.cpp
> new file mode 100644
> index 00000000..964d4ac1
> --- /dev/null
> +++ b/src/libcamera/ipa_ipc.cpp
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc.cpp - Image Processing Algorithm IPC module for IPA proxies

I wonder if we should drop the IPA prefix in various places, as the IPC
mechanism could be used for other purposes too (although I don't have a
use case in mind at the moment). This class could be called IPCBase, or
IPCPipe, or IPCMessagePipe for instance (or ipc::MessagePipe, or...).

> + */
> +
> +#include <vector>
> +
> +#include "libcamera/internal/ipc_unixsocket.h"
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/process.h"
> +#include "libcamera/internal/thread.h"
> +
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +

You can drop all the headers above.

> +#include "libcamera/internal/ipa_ipc.h"
> +
> +/**
> + * \file ipa_ipc.h
> + * \brief IPC mechanism for IPA isolation
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPAIPC)
> +
> +/**
> + * \class IPAIPC
> + * \brief IPC mechanism for IPA isolation

Do you think "message pipe" would be a more accurate description than
"mechanism" ? The component modelled by this base class is meant to send
and receive messages over a pipe, so I think it's a good match.

> + *
> + * Virtual class to model an IPC mechanism for use by IPA proxies for IPA
> + * isolation. sendSync() and sendAsync() must be implemented, and the recvIPC
> + * signal must be emitted whenever new data is available.
> + */
> +
> +/**
> + * \brief Construct an IPAIPC instance
> + */
> +IPAIPC::IPAIPC() : valid_(false)
> +{
> +}
> +
> +IPAIPC::~IPAIPC()
> +{
> +}
> +
> +/**
> + * \fn IPAIPC::isValid()
> + * \brief Check if the IPAIPC instance is valid
> + *
> + * An IPAIPC instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * \return True if the IPAIPC is valid, false otherwise
> + */

If we're talking about a pipe, would isConnected() be a better name ?
I'd also avoid talking about isolation or IPA interface here, to avoid
coupling the class with IPA modules.

> +
> +/**
> + * \fn IPAIPC::sendSync()
> + * \brief Send a message over IPC synchronously

There we go, we're sending messages already :-)

> + * \param[in] cmd Command ID
> + * \param[in] dataIn Data to send, as a byte vector
> + * \param[in] fdsIn Fds to send, in a vector
> + * \param[in] dataOut Byte vector in which to receive data, if applicable
> + * \param[in] fdsOut Fds vector in which to receive fds, if applicable
> + *
> + * This function will not return until a response is received. The event loop
> + * will still continue to execute, however.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \fn IPAIPC::sendAsync()
> + * \brief Send a message over IPC asynchronously
> + * \param[in] cmd Command ID
> + * \param[in] dataIn Data to send, as a byte vector
> + * \param[in] fdsIn Fds to send, in a vector
> + *
> + * This function will return immediately after sending the message.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \var IPAIPC::recvIPC
> + * \brief Signal to be emitted when data is received over IPC
> + *
> + * When data is received over IPC, this signal shall be emitted. Users must
> + * connect to this to receive new data.
> + *
> + * The parameters of the signal are a vector of bytes and a vector of file
> + * descriptors.
> + */
> +
> +/**
> + * \var IPAIPC::valid_
> + * \brief Flag to indicate if the IPAIPC instance is valid
> + *
> + * An IPAIPC instance is valid if the IPA interface is successfully created in
> + * isolation, and IPC is successfully set up.
> + *
> + * This flag can be read via IPAIPC::isValid().
> + *
> + * Implementations of the IPAIPC class should set this flag upon successful
> + * construction.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 01bcffd4..85f3a202 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -25,6 +25,7 @@ libcamera_sources = files([
>      'ipa_context_wrapper.cpp',
>      'ipa_controls.cpp',
>      'ipa_data_serializer.cpp',
> +    'ipa_ipc.cpp',
>      'ipa_interface.cpp',

Another close call for alphabetical order :-)

>      'ipa_manager.cpp',
>      'ipa_module.cpp',