[libcamera-devel] [PATCH v6 7/9] libcamera: Add IPCPipe
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 2 02:56:41 CET 2021
Hi Paul,
Thank you for the patch.
On Thu, Dec 24, 2020 at 05:15:32PM +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>
>
> ---
> 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
> ---
> .../libcamera/internal/ipa_data_serializer.h | 2 +-
> include/libcamera/internal/ipc_pipe.h | 70 ++++++
> src/libcamera/ipc_pipe.cpp | 211 ++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 4 files changed, 283 insertions(+), 1 deletion(-)
> create mode 100644 include/libcamera/internal/ipc_pipe.h
> create mode 100644 src/libcamera/ipc_pipe.cpp
>
> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h
> index 5468a0b7..bde5eaf7 100644
> --- a/include/libcamera/internal/ipa_data_serializer.h
> +++ b/include/libcamera/internal/ipa_data_serializer.h
> @@ -286,7 +286,7 @@ public:
> fdIter,
> fdIter + sizeofFds,
> cs);
> - ret.insert({key, value});
> + ret.insert({ key, value });
This belongs to a different patch.
>
> dataIter += sizeofData;
> fdIter += sizeofFds;
> diff --git a/include/libcamera/internal/ipc_pipe.h b/include/libcamera/internal/ipc_pipe.h
> new file mode 100644
> index 00000000..0232c3b5
> --- /dev/null
> +++ b/include/libcamera/internal/ipc_pipe.h
> @@ -0,0 +1,70 @@
> +/* 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(const Header &header);
> + IPCMessage(const IPCUnixSocket::Payload &payload);
> +
> + const IPCUnixSocket::Payload payload(const Header *header = nullptr) const;
s/const // (the function returns a value, not a reference)
> +
> + Header &header() { return header_; }
> + std::vector<uint8_t> &data() { return data_; }
> + std::vector<int32_t> &fds() { return fds_; }
> +
> + const Header &cheader() const { return header_; }
> + const std::vector<uint8_t> &cdata() const { return data_; }
> + const std::vector<int32_t> &cfds() const { return fds_; }
I think you don't have to name these functions with a 'c' prefix,
declaring them as overloads should be fine:
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(uint32_t cmd,
> + const IPCMessage &in,
> + IPCMessage *out) = 0;
> +
> + virtual int sendAsync(uint32_t cmd,
> + const IPCMessage &data) = 0;
> +
> + Signal<uint32_t, 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..eb439724
> --- /dev/null
> +++ b/src/libcamera/ipc_pipe.cpp
> @@ -0,0 +1,211 @@
> +/* 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 idetify the message and a corresponding reply.
s/idetify/identify/
> + *
> + * 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 IPCMessage instance
s/an/an empty/ ?
> + */
> +IPCMessage::IPCMessage()
> + : header_(Header{ 0, 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.begin(), sizeof(header_));
memcpy(&header_, payload.data.data(), sizeof(header_));
> + data_ = std::vector<uint8_t>(payload.data.begin() + sizeof(header_),
> + payload.data.end());
> + fds_ = std::vector<int32_t>(payload.fds.begin(), payload.fds.end());
fds_ = payload.fds;
> +}
> +
> +/**
> + * \brief Create an IPCUnixSocket payload from the IPCMessage
> + * \param[in] header Header to use when constructing the payload, if applicable
> + *
> + * This essentially converts the IPCMessage into an IPCUnixSocket payload. If
> + * \a header is not provided (ie. nullptr), then the header field that is
s/ie./i.e./
> + * internal to the IPCMessage will be used.
This is a bit of a layering violation, as it makes the IPCMessage
specific to IPCUnixSocket, while it's supposed to be generic. No big
deal, we can address this later.
> + */
> +const IPCUnixSocket::Payload IPCMessage::payload(const Header *header) const
> +{
> + IPCUnixSocket::Payload message;
s/message/payload/ ?
> +
> + message.data.resize(sizeof(Header) + data_.size());
> + message.fds.reserve(fds_.size());
> +
> + memcpy(&*message.data.begin(), header ? header : &header_, sizeof(Header));
memcpy(message.data.data(), header ? header : &header_, sizeof(Header));
> +
> + /* \todo Make this work without copy */
> + memcpy(&*(message.data.begin() + sizeof(Header)), data_.data(), data_.size());
memcpy(message.data.data() + sizeof(Header), data_.data(), data_.size());
> + message.fds = const_cast<std::vector<int32_t> &>(fds_);
Do you need the const_cast ?
> +
> + return message;
> +}
> +
> +/**
> + * \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::cheader()
> + * \brief Returns a const reference to the header
> + */
> +
> +/**
> + * \fn IPCMessage::cdata()
> + * \brief Returns a const reference to the byte vector containing data
> + */
> +
> +/**
> + * \fn IPCMessage::cfds()
> + * \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] cmd Command ID
> + * \param[in] in Data to send, as an IPCMessage
The API looks a bit weird. Couldn't you construct the IPCMessage in the
caller with the header, instead of passing the command here ? Same for
sendAsync(). I think it would also simplify the payload() function and
remove the need to pass it a header pointer. You may also add a
constructor to IPCMessage that takes a command only, not a full header,
for the cases where the caller constructs a message to be sent without a
sequence number.
> + * \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.
This is potentially quite dangerous, as it will create conditions in
which code could be reentered in the caller, without the caller being
explicitly aware. We need to be very careful here to limit what messages
will be delivered by the event loop when calling this function.
* \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.
> + *
> + * \return Zero on success, negative error code otherwise
> + */
> +
> +/**
> + * \fn IPCPipe::sendAsync()
> + * \brief Send a message over IPC asynchronously
> + * \param[in] cmd Command ID
> + * \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 data is received over IPC
s/data/a message/
> + *
> + * When data is received over IPC, this signal shall be emitted. Users must
s/data/a message/
> + * connect to this to receive new data.
s/data/messages/
> + *
> + * The parameters of the signal are a vector of bytes and a vector of file
> + * descriptors.
That's not correct anymore.
> + */
> +
> +/**
> + * \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
> + * construction.
s/construction/connection/ ?
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 1291f1b3..b7cfe0c2 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -30,6 +30,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
More information about the libcamera-devel
mailing list