[libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Thu Oct 29 11:28:44 CET 2020
Hi Jacopo,
Thank you for the review.
On Thu, Oct 29, 2020 at 09:53:52AM +0100, Jacopo Mondi wrote:
> Hi Paul,
>
> On Fri, Oct 02, 2020 at 11:31:27PM +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>
> >
> > ---
> > 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 | 41 ++++++++++
> > src/libcamera/ipa_ipc.cpp | 114 +++++++++++++++++++++++++++
> > src/libcamera/meson.build | 1 +
> > 3 files changed, 156 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..09de36a5
> > --- /dev/null
> > +++ b/include/libcamera/internal/ipa_ipc.h
> > @@ -0,0 +1,41 @@
> > +/* 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>
> > +
> > +namespace libcamera {
> > +
> > +class IPAIPC
> > +{
> > +public:
> > + IPAIPC(const char *ipa_module_path [[maybe_unused]],
> > + const char *ipa_proxy_worker_path [[maybe_unused]]);
> > + virtual ~IPAIPC();
> > +
> > + bool isValid() const { return valid_; }
>
> We usually have getters with the same name as the variable they "get".
> I'm fine with 'isValid()' but I felt like pointing this out.
Oh, okay. IPAModule has this too though.
> > +
> > + virtual int sendSync(uint32_t cmd,
> > + const std::vector<uint8_t> &data_in,
> > + const std::vector<int32_t> &fds_in,
> > + std::vector<uint8_t> *data_out = nullptr,
> > + std::vector<int32_t> *fds_out = nullptr) = 0;
> > +
> > + virtual int sendAsync(uint32_t cmd,
> > + const std::vector<uint8_t> &data_in,
> > + const std::vector<int32_t> &fds_in) = 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..b963351f
> > --- /dev/null
> > +++ b/src/libcamera/ipa_ipc.cpp
> > @@ -0,0 +1,114 @@
> > +/* 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"
>
> The inclusion of the header corresponding to the cpp file is usually
> placed first, to ensure it is self-contained and does not depend on
> other include directives that might be placed before it.
Ah, okay.
> > +
> > +/**
> > + * \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
> > + * \param[in] ipa_module_path Path to the IPA module shared object
> > + * \param[in] ipa_proxy_worker_path Path to the IPA proxy worker shared object
> > + */
> > +IPAIPC::IPAIPC([[maybe_unused]] const char *ipa_module_path,
> > + [[maybe_unused]] const char *ipa_proxy_worker_path)
>
> nit: parameters alignment
This is aligned correctly. I guess patches mess up tabs?
> If these fields are not used by the base virtual class, do we need
> them in the constructor ?
I imagined that they would be required by all subclasses of IPAIPC, so I
wanted to put them here.
> > + : 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] data_in Data to send, as a byte vector
> > + * \param[in] fds_in Fds to send, in a vector
> > + * \param[in] data_out Byte vector in which to receive data, if applicable
> > + * \param[in] fds_out Fds vector in which to receive fds, if applicable
>
> Could you spell 'file descriptors' entirely ?
> * \param[in] fds_out Vector in which to receive file descriptors, if applicable
>
> Same for other locations.
Okay.
> I a bit wonder why fds should be kept separate and not serialized
> in the data_in vector. Does it depend on the fact that fds as
> serialized separately by the IPADataSerializer module ?
Because if you send the fds as data they won't get translated :)
> A final minor note on this: are _in and _out the best names we can have ?
> Should this be expressed as '_msg' and '_reply' ?
That'll work too. I just thought of it as input and output data.
> > + *
> > + * 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] data_in Data to send, as a byte vector
> > + * \param[in] fds_in 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.
>
> I assume this completes the sendAsync() method, right ? Can we state
> that explicitely, if that's the case ?
Not necessarily. One sendAsync() could have multiple responses.
> Mostly minor comments, please add
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thank you,
Paul
> > + */
> > +
> > +/**
> > + * \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 190d7490..e0a2ab47 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list