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

Wed Nov 18 09:25:39 CET 2020

Hi Jacopo,

On Tue, Nov 17, 2020 at 03:59:29PM +0100, Jacopo Mondi wrote:
> 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 at ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo at 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

Yeah, checkstyle warned me about that :)

It's fixed in the next version.

Paul

> > +{
> > +}
> > +
> > +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
> >