[libcamera-devel] [PATCH v7 07/10] libcamera: Add IPCPipe

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Feb 11, 2021 at 04:18:02PM +0900, Paul Elder wrote:
> Create a virtual IPCPipe class that models an IPC/RPC system. IPA proxies
> and proxy workers will call into the IPCPipe, rather than implementing
> the IPC themselves.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> ---
> Changes in v7:
> - add an IPCMessage construct that takes just a command code
> - remove 'c' from cheader, cdata, and cfds, and just overload them
> - implement new sendSync/sendAsync API, so that they only deal with
>   IPCMessages
>   - so the caller is responsible for populating the command code and the
>     sequence number
> - cosmetic changes, reword some docs, add some todos
> 
> No change in v6
> 
> Changes in v5:
> - fix style
> - rename ipa_ipc.[(cpp)h] to ipc_pipe.[(cpp),h]
> - rename IPAIPC to IPCPipe
> - add IPCMessage to use in the IPCPipe
> 
> 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/ipc_pipe.h |  69 ++++++++
>  src/libcamera/ipc_pipe.cpp            | 220 ++++++++++++++++++++++++++
>  src/libcamera/meson.build             |   1 +
>  3 files changed, 290 insertions(+)
>  create mode 100644 include/libcamera/internal/ipc_pipe.h
>  create mode 100644 src/libcamera/ipc_pipe.cpp
> 
> diff --git a/include/libcamera/internal/ipc_pipe.h b/include/libcamera/internal/ipc_pipe.h
> new file mode 100644
> index 00000000..c9a84b78
> --- /dev/null
> +++ b/include/libcamera/internal/ipc_pipe.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipc_pipe.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/internal/ipc_unixsocket.h"
> +
> +#include <libcamera/signal.h>
> +
> +namespace libcamera {
> +
> +class IPCMessage
> +{
> +public:
> +	struct Header {
> +		uint32_t cmd;
> +		uint32_t cookie;
> +	};
> +
> +	IPCMessage();
> +	IPCMessage(uint32_t cmd);
> +	IPCMessage(const Header &header);
> +	IPCMessage(const IPCUnixSocket::Payload &payload);
> +
> +	IPCUnixSocket::Payload payload() const;
> +
> +	Header &header() { return header_; }
> +	std::vector<uint8_t> &data() { return data_; }
> +	std::vector<int32_t> &fds() { return fds_; }
> +
> +	const Header &header() const { return header_; }
> +	const std::vector<uint8_t> &data() const { return data_; }
> +	const std::vector<int32_t> &fds() const { return fds_; }
> +
> +private:
> +	Header header_;
> +
> +	std::vector<uint8_t> data_;
> +	std::vector<int32_t> fds_;
> +};
> +
> +class IPCPipe
> +{
> +public:
> +	IPCPipe();
> +	virtual ~IPCPipe();
> +
> +	bool isConnected() const { return connected_; }
> +
> +	virtual int sendSync(const IPCMessage &in,
> +			     IPCMessage *out) = 0;
> +
> +	virtual int sendAsync(const IPCMessage &data) = 0;
> +
> +	Signal<const IPCMessage &> recv;
> +
> +protected:
> +	bool connected_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_H__ */
> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp
> new file mode 100644
> index 00000000..77b5ef2b
> --- /dev/null
> +++ b/src/libcamera/ipc_pipe.cpp
> @@ -0,0 +1,220 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipc_pipe.cpp - Image Processing Algorithm IPC module for IPA proxies
> + */
> +
> +#include "libcamera/internal/ipc_pipe.h"
> +
> +#include "libcamera/internal/log.h"
> +
> +/**
> + * \file ipc_pipe.h
> + * \brief IPC mechanism for IPA isolation
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPCPipe)
> +
> +/**
> + * \struct IPCMessage::Header
> + * \brief Container for an IPCMessage header
> + *
> + * Holds a cmd code for the IPC message, and a cookie.
> + */
> +
> +/**
> + * \var IPCMessage::Header::cmd
> + * \brief Type of IPCMessage
> + *
> + * Typically used to carry a command code for an RPC.
> + */
> +
> +/**
> + * \var IPCMessage::Header::cookie
> + * \brief Cookie to identify the message and a corresponding reply.
> + *
> + * Populated and used by IPCPipe implementations for matching calls with
> + * replies.
> + */
> +
> +/**
> + * \class IPCMessage
> + * \brief IPC message to be passed through IPC message pipe
> + */
> +
> +/**
> + * \brief Construct an empty IPCMessage instance
> + */
> +IPCMessage::IPCMessage()
> +	: header_(Header{ 0, 0 })
> +{
> +}
> +
> +/**
> + * \brief Construct an IPCMessage instance with a given command code
> + * \param[in] cmd The command code
> + */
> +IPCMessage::IPCMessage(uint32_t cmd)
> +	: header_(Header{ cmd, 0 })
> +{
> +}
> +
> +/**
> + * \brief Construct an IPCMessage instance with a given header
> + * \param[in] header The header that the constructed IPCMessage will contain
> + */
> +IPCMessage::IPCMessage(const Header &header)
> +	: header_(header)
> +{
> +}
> +
> +/**
> + * \brief Construct an IPCMessage instance from an IPC payload
> + * \param[in] payload The IPCUnixSocket payload to construct from
> + *
> + * This essentially converts an IPCUnixSocket payload into an IPCMessage.
> + * The header is extracted from the payload into the IPCMessage's header field.
> + */
> +IPCMessage::IPCMessage(const IPCUnixSocket::Payload &payload)
> +{
> +	memcpy(&header_, payload.data.data(), sizeof(header_));
> +	data_ = std::vector<uint8_t>(payload.data.begin() + sizeof(header_),
> +				     payload.data.end());
> +	fds_ = payload.fds;
> +}
> +
> +/**
> + * \brief Create an IPCUnixSocket payload from the IPCMessage
> + *
> + * This essentially converts the IPCMessage into an IPCUnixSocket payload.
> + *
> + * \todo Resolve the layering violation (add other converters later?)
> + */
> +IPCUnixSocket::Payload IPCMessage::payload() const
> +{
> +	IPCUnixSocket::Payload payload;
> +
> +	payload.data.resize(sizeof(Header) + data_.size());
> +	payload.fds.reserve(fds_.size());
> +
> +	memcpy(payload.data.data(), &header_, sizeof(Header));
> +
> +	/* \todo Make this work without copy */
> +	memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());
> +	payload.fds = fds_;
> +
> +	return payload;
> +}
> +
> +/**
> + * \fn IPCMessage::header()
> + * \brief Returns a reference to the header
> + */
> +
> +/**
> + * \fn IPCMessage::data()
> + * \brief Returns a reference to the byte vector containing data
> + */
> +
> +/**
> + * \fn IPCMessage::fds()
> + * \brief Returns a reference to the vector containing file descriptors
> + */
> +
> +/**
> + * \fn IPCMessage::header() const
> + * \brief Returns a const reference to the header
> + */
> +
> +/**
> + * \fn IPCMessage::data() const
> + * \brief Returns a const reference to the byte vector containing data
> + */
> +
> +/**
> + * \fn IPCMessage::fds() const
> + * \brief Returns a const reference to the vector containing file descriptors
> + */
> +
> +/**
> + * \class IPCPipe
> + * \brief IPC message pipe for IPA isolation
> + *
> + * Virtual class to model an IPC message pipe for use by IPA proxies for IPA
> + * isolation. sendSync() and sendAsync() must be implemented, and the recvMessage
> + * signal must be emitted whenever new data is available.
> + */
> +
> +/**
> + * \brief Construct an IPCPipe instance
> + */
> +IPCPipe::IPCPipe()
> +	: connected_(false)
> +{
> +}
> +
> +IPCPipe::~IPCPipe()
> +{
> +}
> +
> +/**
> + * \fn IPCPipe::isConnected()
> + * \brief Check if the IPCPipe instance is connected
> + *
> + * An IPCPipe instance is connected if IPC is successfully set up.
> + *
> + * \return True if the IPCPipe is connected, false otherwise
> + */
> +
> +/**
> + * \fn IPCPipe::sendSync()
> + * \brief Send a message over IPC synchronously
> + * \param[in] in Data to send, as an IPCMessage

You can drop ", as an IPCMessage", doxygen shows the type of parameters.
Same for the out parameter, and the other functions below.

> + * \param[in] out IPCMessage instance in which to receive data, 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
> + *
> + * \todo Determine if the event loop should limit the types of messages it
> + * processes, to avoid reintrancy in the caller, and carefully document what
> + * the caller needs to implement to make this safe.
> + */
> +
> +/**
> + * \fn IPCPipe::sendAsync()
> + * \brief Send a message over IPC asynchronously
> + * \param[in] data Data to send, as an IPCMessage
> + *
> + * This function will return immediately after sending the message.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \var IPCPipe::recv
> + * \brief Signal to be emitted when a message is received over IPC
> + *
> + * When data is received over IPC, this signal shall be emitted. Users must

s/data/a message/

> + * connect to this to receive messages.
> + *
> + * The parameters of the signal are an IPCMessage.

Doesn't doxygen also show this ?

> + */
> +
> +/**
> + * \var IPCPipe::connected_
> + * \brief Flag to indicate if the IPCPipe instance is connected
> + *
> + * An IPCPipe instance is connected if IPC is successfully set up.
> + *
> + * This flag can be read via IPCPipe::isConnected().
> + *
> + * Implementations of the IPCPipe class should set this flag upon successful
> + * connection.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 7d8ea00a..e736c41e 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -31,6 +31,7 @@ libcamera_sources = files([
>      'ipa_manager.cpp',
>      'ipa_module.cpp',
>      'ipa_proxy.cpp',
> +    'ipc_pipe.cpp',
>      'ipc_unixsocket.cpp',
>      'log.cpp',
>      'media_device.cpp',

-- 
Regards,

Laurent Pinchart