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

9919 diff mbox series

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@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

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@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.

> +
> +	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.

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

If these fields are not used by the base virtual class, do we need
them in the constructor ?

> +	: 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.

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 ?

A final minor note on this: are _in and _out the best names we can have ?
Should this be expressed as '_msg' and '_reply' ?

> + *
> + * 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 ?

Mostly minor comments, please add
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


> + */
> +
> +/**
> + * \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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

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

I would avoid that and only them in the dervied classes that need
them, or store them in the base class for derived to access them alternatively

> > > +	: 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 :)
>

This answer sound a bit like "because yes".

My question was more on the serialization part, where fds are always
treated as 'special' while in theory there is nothing preventing
placing them in the data buffer, as long as the serialization format
allows to identify them.

This would anyway require re-thinking how the whole serialization
process work, and I concur that's probably not worth it, but I still
fail to see the technical reason why they're kept separate. An fd is a
uin32_t like all other fields that could be part of a structure or class,
after all.

> > 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.
>

input output are fine too.

> > > + *
> > > + * 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.

What I meant was: "Is the recvIPA signal meant to be used to signal
a response to a asynchronous method call" or are there other intended
usages I am missing ?


>
> > Mostly minor comments, please add
> > Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel

Hi again,

On Thu, Oct 29, 2020 at 07:28:44PM +0900, paul.elder@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@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@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@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel