[libcamera-devel] [PATCH v3 11/38] libcamera: Add IPAIPC

Jacopo Mondi jacopo at jmondi.org
Thu Oct 29 13:34:20 CET 2020 

Hi again,

On Thu, Oct 29, 2020 at 07:28:44PM +0900, paul.elder at ideasonboard.com wrote:
> 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

Also, for both these methods, the return code reflects the message
sending operation result, not the result of the operation itself. I
think it would help documenting it.

> > > + */
> > > +
> > > +/**
> > > + * \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