[libcamera-devel,15/38] libcamera: Add IPAIPC implementation based on unix socket

9719 diff mbox series

Add an implementation of IPAIPC using unix socket.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- specify in doxygen to skip generating documentation for
  IPAIPCUnixSocket
---
 Documentation/Doxyfile.in                     |   2 +
 .../libcamera/internal/ipa_ipc_unixsocket.h   | 115 ++++++++++++
 src/libcamera/ipa_ipc_unixsocket.cpp          | 175 ++++++++++++++++++
 src/libcamera/meson.build                     |   1 +
 4 files changed, 293 insertions(+)
 create mode 100644 include/libcamera/internal/ipa_ipc_unixsocket.h
 create mode 100644 src/libcamera/ipa_ipc_unixsocket.cpp

On Tue, Sep 22, 2020 at 10:35:14PM +0900, Paul Elder wrote:
> Add an implementation of IPAIPC using unix socket.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v2:
> - specify in doxygen to skip generating documentation for
>   IPAIPCUnixSocket
> ---
>  Documentation/Doxyfile.in                     |   2 +
>  .../libcamera/internal/ipa_ipc_unixsocket.h   | 115 ++++++++++++
>  src/libcamera/ipa_ipc_unixsocket.cpp          | 175 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  4 files changed, 293 insertions(+)
>  create mode 100644 include/libcamera/internal/ipa_ipc_unixsocket.h
>  create mode 100644 src/libcamera/ipa_ipc_unixsocket.cpp
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 598eca9b..71509ff7 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -837,8 +837,10 @@ RECURSIVE              = YES
>  EXCLUDE                = @TOP_SRCDIR@/include/libcamera/span.h \
>  			 @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
>  			 @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> +			 @TOP_SRCDIR@/include/libcamera/internal/ipa_ipc_unixsocket.h \
>  			 @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
>  			 @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> +			 @TOP_SRCDIR@/src/libcamera/ipa_ipc_unixsocket.cpp \
>  			 @TOP_SRCDIR@/src/libcamera/pipeline/ \
>  			 @TOP_SRCDIR@/src/libcamera/proxy/ \
>  			 @TOP_BUILDDIR@/include/libcamera/ipa/ \
> diff --git a/include/libcamera/internal/ipa_ipc_unixsocket.h b/include/libcamera/internal/ipa_ipc_unixsocket.h
> new file mode 100644
> index 00000000..de8af35b
> --- /dev/null
> +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h
> @@ -0,0 +1,115 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc_unixsocket.h - Image Processing Algorithm IPC module using unix socket
> + */
> +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__
> +#define __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__
> +
> +#include <vector>
> +
> +#include <libcamera/span.h>
> +
> +#include "libcamera/internal/ipa_ipc.h"
> +#include "libcamera/internal/ipa_module.h"
> +#include "libcamera/internal/ipc_unixsocket.h"
> +
> +namespace libcamera {
> +
> +class Process;
> +
> +inline void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq)
> +{
> +	uint8_t cmd_arr[] = {static_cast<uint8_t>(cmd & 0xff),
> +			     static_cast<uint8_t>((cmd >> 8 ) & 0xff),
> +			     static_cast<uint8_t>((cmd >> 16) & 0xff),
> +			     static_cast<uint8_t>((cmd >> 24) & 0xff)};
> +	uint8_t seq_arr[] = {static_cast<uint8_t>(seq & 0xff),
> +			     static_cast<uint8_t>((seq >> 8 ) & 0xff),
> +			     static_cast<uint8_t>((seq >> 16) & 0xff),
> +			     static_cast<uint8_t>((seq >> 24) & 0xff)};

I think you can omit the 0xff as the cast to uint8_t implies it

> +	payload.data.insert(payload.data.begin(), cmd_arr, cmd_arr+4);
> +	payload.data.insert(payload.data.begin() + 4, seq_arr, seq_arr+4);
> +}
> +
> +inline std::tuple<uint32_t, uint32_t> readHeader(IPCUnixSocket::Payload &payload)
> +{
> +	uint32_t cmd = (payload.data[0] & 0xff) |
> +		       ((payload.data[1] & 0xff) << 8) |
> +		       ((payload.data[2] & 0xff) << 16) |
> +		       ((payload.data[3] & 0xff) << 24);
> +	uint32_t seq = (payload.data[4] & 0xff) |
> +		       ((payload.data[5] & 0xff) << 8) |
> +		       ((payload.data[6] & 0xff) << 16) |
> +		       ((payload.data[7] & 0xff) << 24);
> +
> +	return {cmd, seq};
> +}
> +
> +inline void eraseHeader(IPCUnixSocket::Payload &payload)
> +{
> +	payload.data.erase(payload.data.begin(), payload.data.begin() + 8);
> +}

I would not place these in the header file but rather as static
functions in an anonymous namespace in the cpp file. Or are they used
outside from this module ? It seems so:

src/libcamera/ipa_ipc_unixsocket.cpp:   std::tie(cmd, seq) = readHeader(message);
test/ipc/unixsocket_ipc.cpp:            std::tie(cmd, seq) = readHeader(message);
utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl:      std::tie(_cmd, _seq) = readHeader(_message);

So why are they defined in this header which implements the IPAIPC
interface ? What if another IPC mechanism is used ?

(I -think- also this causes the function to be defined in every module
that includes this header, wasting a bit of space, but that's a
secondary issue).

> +
> +inline void writeUInt32(IPCUnixSocket::Payload &payload, uint32_t val, uint32_t pos)
> +{
> +	if (pos + 4 > payload.data.size())
> +		return;
> +
> +	uint8_t arr[] = {static_cast<uint8_t>(val & 0xff),
> +			 static_cast<uint8_t>(val & (0xff << 8)),
> +			 static_cast<uint8_t>(val & (0xff << 16)),
> +			 static_cast<uint8_t>(val & (0xff << 24))};
> +	std::copy(arr, arr + 4, payload.data.begin() + pos);
> +}
> +
> +inline uint32_t readUInt32(IPCUnixSocket::Payload &payload, uint32_t pos)
> +{
> +	if (pos + 4 > payload.data.size())
> +		return 0;
> +
> +	return payload.data[pos] & (payload.data[pos + 1] << 8) &
> +	       (payload.data[pos + 2] << 16) & (payload.data[pos + 3] << 24);
> +}

I don't see these used.

Also, double empty line

> +
> +
> +class IPAIPCUnixSocket : public IPAIPC
> +{
> +public:
> +	IPAIPCUnixSocket(const char *ipa_module_path, const char *ipa_proxy_worker_path);
> +	~IPAIPCUnixSocket();
> +
> +	bool isValid() const { return valid_; }

that's defined in the base class, right ?

> +
> +	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) override;
> +
> +	int sendAsync(uint32_t cmd,
> +		      const std::vector<uint8_t> &data_in,
> +		      const std::vector<int32_t> &fds_in) override;
> +
> +private:
> +	struct CallData {
> +		IPCUnixSocket::Payload *response;
> +		bool done;
> +	};
> +
> +	void readyRead(IPCUnixSocket *socket);
> +	int call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq);
> +
> +	uint32_t seq_;
> +
> +	Process *proc_;
> +
> +	IPCUnixSocket *socket_;
> +
> +	std::map<uint32_t, struct CallData> callData_;
> +};
> +
> +} /* namespace libcamera */
> +

I'll for now skip the review of the implementation.


> +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */
> diff --git a/src/libcamera/ipa_ipc_unixsocket.cpp b/src/libcamera/ipa_ipc_unixsocket.cpp
> new file mode 100644
> index 00000000..c1997e96
> --- /dev/null
> +++ b/src/libcamera/ipa_ipc_unixsocket.cpp
> @@ -0,0 +1,175 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_ipc_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket
> + */
> +
> +#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"
> +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAIPC)
> +
> +IPAIPCUnixSocket::IPAIPCUnixSocket(const char *ipa_module_path,
> +				   const char *ipa_proxy_worker_path)
> +	: IPAIPC(ipa_module_path, ipa_proxy_worker_path), seq_(0),
> +	  proc_(nullptr), socket_(nullptr)
> +{
> +	std::vector<int> fds;
> +	std::vector<std::string> args;
> +	args.push_back(ipa_module_path);
> +
> +	socket_ = new IPCUnixSocket();
> +	int fd = socket_->create();
> +	if (fd < 0) {
> +		LOG(IPAIPC, Error)
> +			<< "Failed to create socket";
> +		return;
> +	}
> +	socket_->readyRead.connect(this, &IPAIPCUnixSocket::readyRead);
> +	args.push_back(std::to_string(fd));
> +	fds.push_back(fd);
> +
> +	proc_ = new Process();
> +	int ret = proc_->start(ipa_proxy_worker_path, args, fds);
> +	if (ret) {
> +		LOG(IPAIPC, Error)
> +			<< "Failed to start proxy worker process";
> +		return;
> +	}
> +
> +	valid_ = true;
> +}
> +
> +IPAIPCUnixSocket::~IPAIPCUnixSocket()
> +{
> +	delete proc_;
> +	delete socket_;
> +}
> +
> +int IPAIPCUnixSocket::sendSync(uint32_t cmd,
> +			       const std::vector<uint8_t> &data_in,
> +			       const std::vector<int32_t> &fds_in,
> +			       std::vector<uint8_t> *data_out,
> +			       std::vector<int32_t> *fds_out)
> +{
> +	IPCUnixSocket::Payload message, response;
> +	int ret;
> +
> +	/* It's fine if seq_ overflows; that'll just be the new epoch. */
> +	seq_++;
> +	writeHeader(message, cmd, seq_);
> +	message.data.insert(message.data.end(), data_in.begin(), data_in.end());
> +
> +	message.fds = const_cast<std::vector<int32_t> &>(fds_in);
> +
> +	ret = call(message, &response, seq_);
> +	if (ret) {
> +		LOG(IPAIPC, Error) << "Failed to call sync";
> +		callData_.erase(seq_);
> +		return ret;
> +	}
> +
> +	if (data_out)
> +		data_out->insert(data_out->end(), response.data.begin(), response.data.end());
> +
> +	if (fds_out)
> +		fds_out->insert(fds_out->end(), response.fds.begin(), response.fds.end());
> +
> +	return 0;
> +}
> +
> +int IPAIPCUnixSocket::sendAsync(uint32_t cmd,
> +				const std::vector<uint8_t> &data_in,
> +				const std::vector<int32_t> &fds_in)
> +{
> +	IPCUnixSocket::Payload message;
> +	int ret;
> +
> +	writeHeader(message, cmd, 0);
> +	message.data.insert(message.data.end(), data_in.begin(), data_in.end());
> +
> +	message.fds = const_cast<std::vector<int32_t> &>(fds_in);
> +
> +	ret = socket_->send(message);
> +	if (ret) {
> +		LOG(IPAIPC, Error) << "Failed to call async";
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void IPAIPCUnixSocket::readyRead(IPCUnixSocket *socket)
> +{
> +	IPCUnixSocket::Payload message;
> +	int ret = socket->receive(&message);
> +	if (ret) {
> +		LOG(IPAIPC, Error) << "Receive message failed" << ret;
> +		return;
> +	}
> +
> +	uint32_t cmd, seq;
> +	std::tie(cmd, seq) = readHeader(message);
> +
> +	auto callData = callData_.find(seq);
> +	if (callData != callData_.end()) {
> +		eraseHeader(message);
> +		/* Is there any way to avoid this copy? */
> +		*callData->second.response = message;
> +		callData->second.done = true;
> +		return;
> +	}
> +
> +	/*
> +	 * Received unexpected data, this means it's a call from the IPA.
> +	 * We can't return anything to the IPA (gotta keep them under *our*
> +	 * control, plus returning would require blocking the caller, and we
> +	 * can't afford to do that). Let the proxy do switch-case on cmd.
> +	 */
> +	recvIPC.emit(message.data, message.fds);
> +
> +	return;
> +}
> +
> +int IPAIPCUnixSocket::call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq)
> +{
> +	Timer timeout;
> +	int ret;
> +
> +	callData_[seq].response = response;
> +	callData_[seq].done = false;
> +
> +	ret = socket_->send(message);
> +	if (ret)
> +		return ret;
> +
> +	timeout.start(200);
> +	while (!callData_[seq].done) {
> +		if (!timeout.isRunning()) {
> +			LOG(IPAIPC, Error) << "Call timeout!";
> +			callData_.erase(seq);
> +			return -ETIMEDOUT;
> +		}
> +
> +		Thread::current()->eventDispatcher()->processEvents();
> +	}
> +
> +	callData_.erase(seq);
> +
> +	return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index e3eade57..24f32815 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -25,6 +25,7 @@ libcamera_sources = files([
>      'ipa_controls.cpp',
>      'ipa_data_serializer.cpp',
>      'ipa_ipc.cpp',
> +    'ipa_ipc_unixsocket.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

On Thu, Sep 24, 2020 at 12:55:05PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 22, 2020 at 10:35:14PM +0900, Paul Elder wrote:
> > Add an implementation of IPAIPC using unix socket.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > Changes in v2:
> > - specify in doxygen to skip generating documentation for
> >   IPAIPCUnixSocket
> > ---
> >  Documentation/Doxyfile.in                     |   2 +
> >  .../libcamera/internal/ipa_ipc_unixsocket.h   | 115 ++++++++++++
> >  src/libcamera/ipa_ipc_unixsocket.cpp          | 175 ++++++++++++++++++
> >  src/libcamera/meson.build                     |   1 +
> >  4 files changed, 293 insertions(+)
> >  create mode 100644 include/libcamera/internal/ipa_ipc_unixsocket.h
> >  create mode 100644 src/libcamera/ipa_ipc_unixsocket.cpp
> >
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index 598eca9b..71509ff7 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -837,8 +837,10 @@ RECURSIVE              = YES
> >  EXCLUDE                = @TOP_SRCDIR@/include/libcamera/span.h \
> >  			 @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> >  			 @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> > +			 @TOP_SRCDIR@/include/libcamera/internal/ipa_ipc_unixsocket.h \
> >  			 @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> >  			 @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> > +			 @TOP_SRCDIR@/src/libcamera/ipa_ipc_unixsocket.cpp \
> >  			 @TOP_SRCDIR@/src/libcamera/pipeline/ \
> >  			 @TOP_SRCDIR@/src/libcamera/proxy/ \
> >  			 @TOP_BUILDDIR@/include/libcamera/ipa/ \
> > diff --git a/include/libcamera/internal/ipa_ipc_unixsocket.h b/include/libcamera/internal/ipa_ipc_unixsocket.h
> > new file mode 100644
> > index 00000000..de8af35b
> > --- /dev/null
> > +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h
> > @@ -0,0 +1,115 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * ipa_ipc_unixsocket.h - Image Processing Algorithm IPC module using unix socket
> > + */
> > +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__
> > +#define __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__
> > +
> > +#include <vector>
> > +
> > +#include <libcamera/span.h>
> > +
> > +#include "libcamera/internal/ipa_ipc.h"
> > +#include "libcamera/internal/ipa_module.h"
> > +#include "libcamera/internal/ipc_unixsocket.h"
> > +
> > +namespace libcamera {
> > +
> > +class Process;
> > +
> > +inline void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq)
> > +{
> > +	uint8_t cmd_arr[] = {static_cast<uint8_t>(cmd & 0xff),
> > +			     static_cast<uint8_t>((cmd >> 8 ) & 0xff),
> > +			     static_cast<uint8_t>((cmd >> 16) & 0xff),
> > +			     static_cast<uint8_t>((cmd >> 24) & 0xff)};
> > +	uint8_t seq_arr[] = {static_cast<uint8_t>(seq & 0xff),
> > +			     static_cast<uint8_t>((seq >> 8 ) & 0xff),
> > +			     static_cast<uint8_t>((seq >> 16) & 0xff),
> > +			     static_cast<uint8_t>((seq >> 24) & 0xff)};
> 
> I think you can omit the 0xff as the cast to uint8_t implies it

ack

> > +	payload.data.insert(payload.data.begin(), cmd_arr, cmd_arr+4);
> > +	payload.data.insert(payload.data.begin() + 4, seq_arr, seq_arr+4);
> > +}
> > +
> > +inline std::tuple<uint32_t, uint32_t> readHeader(IPCUnixSocket::Payload &payload)
> > +{
> > +	uint32_t cmd = (payload.data[0] & 0xff) |
> > +		       ((payload.data[1] & 0xff) << 8) |
> > +		       ((payload.data[2] & 0xff) << 16) |
> > +		       ((payload.data[3] & 0xff) << 24);
> > +	uint32_t seq = (payload.data[4] & 0xff) |
> > +		       ((payload.data[5] & 0xff) << 8) |
> > +		       ((payload.data[6] & 0xff) << 16) |
> > +		       ((payload.data[7] & 0xff) << 24);
> > +
> > +	return {cmd, seq};
> > +}
> > +
> > +inline void eraseHeader(IPCUnixSocket::Payload &payload)
> > +{
> > +	payload.data.erase(payload.data.begin(), payload.data.begin() + 8);
> > +}
> 
> I would not place these in the header file but rather as static
> functions in an anonymous namespace in the cpp file. Or are they used
> outside from this module ? It seems so:
> 
> src/libcamera/ipa_ipc_unixsocket.cpp:   std::tie(cmd, seq) = readHeader(message);
> test/ipc/unixsocket_ipc.cpp:            std::tie(cmd, seq) = readHeader(message);
> utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl:      std::tie(_cmd, _seq) = readHeader(_message);
> 
> So why are they defined in this header which implements the IPAIPC
> interface ? What if another IPC mechanism is used ?

This header is only for IPAIPCUnixSocket, not the base IPAIPC, so it
should be fine to keep them here. test/ipc/unixsocket_ipc.cpp tests
IPAIPCUnixSocket.

As for the proxy worker, as I mentioned before, technically it's a
layering violation and we should really have an IPAIPCWorker
(IPAIPCUnixSocketWorker) in between IPAIPC (IPAIPCUnixSocket)
and IPAProxy{pipeline_name}Worker. But we don't have any other IPC
mechanisms yet, so it should be fine for now. When we do, then the proxy
worker will no longer use these IPC-specific header manipulation
functions, and they will be used in the IPAIPCUnixSocketWorker.

> (I -think- also this causes the function to be defined in every module
> that includes this header, wasting a bit of space, but that's a
> secondary issue).

These are the only files that include this header:

src/libcamera/ipa_ipc_unixsocket.cpp
test/ipc/unixsocket_ipc.cpp
utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl

The first two are fine. The last one is fine for now. The other two
really only include this header for choosing the IPAIPC that they want
to use, which for now should be fine. When we add other IPC mechanisms
we can rename or namespace these.

> > +
> > +inline void writeUInt32(IPCUnixSocket::Payload &payload, uint32_t val, uint32_t pos)
> > +{
> > +	if (pos + 4 > payload.data.size())
> > +		return;
> > +
> > +	uint8_t arr[] = {static_cast<uint8_t>(val & 0xff),
> > +			 static_cast<uint8_t>(val & (0xff << 8)),
> > +			 static_cast<uint8_t>(val & (0xff << 16)),
> > +			 static_cast<uint8_t>(val & (0xff << 24))};
> > +	std::copy(arr, arr + 4, payload.data.begin() + pos);
> > +}
> > +
> > +inline uint32_t readUInt32(IPCUnixSocket::Payload &payload, uint32_t pos)
> > +{
> > +	if (pos + 4 > payload.data.size())
> > +		return 0;
> > +
> > +	return payload.data[pos] & (payload.data[pos + 1] << 8) &
> > +	       (payload.data[pos + 2] << 16) & (payload.data[pos + 3] << 24);
> > +}
> 
> I don't see these used.

Ah yes, these are leftovers from the de/serialization work...

> Also, double empty line
> 
> > +
> > +
> > +class IPAIPCUnixSocket : public IPAIPC
> > +{
> > +public:
> > +	IPAIPCUnixSocket(const char *ipa_module_path, const char *ipa_proxy_worker_path);
> > +	~IPAIPCUnixSocket();
> > +
> > +	bool isValid() const { return valid_; }
> 
> that's defined in the base class, right ?

Ah yeah it is.


Thanks,

Paul
> > +
> > +	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) override;
> > +
> > +	int sendAsync(uint32_t cmd,
> > +		      const std::vector<uint8_t> &data_in,
> > +		      const std::vector<int32_t> &fds_in) override;
> > +
> > +private:
> > +	struct CallData {
> > +		IPCUnixSocket::Payload *response;
> > +		bool done;
> > +	};
> > +
> > +	void readyRead(IPCUnixSocket *socket);
> > +	int call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq);
> > +
> > +	uint32_t seq_;
> > +
> > +	Process *proc_;
> > +
> > +	IPCUnixSocket *socket_;
> > +
> > +	std::map<uint32_t, struct CallData> callData_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> 
> I'll for now skip the review of the implementation.
> 
> 
> > +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */
> > diff --git a/src/libcamera/ipa_ipc_unixsocket.cpp b/src/libcamera/ipa_ipc_unixsocket.cpp
> > new file mode 100644
> > index 00000000..c1997e96
> > --- /dev/null
> > +++ b/src/libcamera/ipa_ipc_unixsocket.cpp
> > @@ -0,0 +1,175 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * ipa_ipc_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket
> > + */
> > +
> > +#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"
> > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(IPAIPC)
> > +
> > +IPAIPCUnixSocket::IPAIPCUnixSocket(const char *ipa_module_path,
> > +				   const char *ipa_proxy_worker_path)
> > +	: IPAIPC(ipa_module_path, ipa_proxy_worker_path), seq_(0),
> > +	  proc_(nullptr), socket_(nullptr)
> > +{
> > +	std::vector<int> fds;
> > +	std::vector<std::string> args;
> > +	args.push_back(ipa_module_path);
> > +
> > +	socket_ = new IPCUnixSocket();
> > +	int fd = socket_->create();
> > +	if (fd < 0) {
> > +		LOG(IPAIPC, Error)
> > +			<< "Failed to create socket";
> > +		return;
> > +	}
> > +	socket_->readyRead.connect(this, &IPAIPCUnixSocket::readyRead);
> > +	args.push_back(std::to_string(fd));
> > +	fds.push_back(fd);
> > +
> > +	proc_ = new Process();
> > +	int ret = proc_->start(ipa_proxy_worker_path, args, fds);
> > +	if (ret) {
> > +		LOG(IPAIPC, Error)
> > +			<< "Failed to start proxy worker process";
> > +		return;
> > +	}
> > +
> > +	valid_ = true;
> > +}
> > +
> > +IPAIPCUnixSocket::~IPAIPCUnixSocket()
> > +{
> > +	delete proc_;
> > +	delete socket_;
> > +}
> > +
> > +int IPAIPCUnixSocket::sendSync(uint32_t cmd,
> > +			       const std::vector<uint8_t> &data_in,
> > +			       const std::vector<int32_t> &fds_in,
> > +			       std::vector<uint8_t> *data_out,
> > +			       std::vector<int32_t> *fds_out)
> > +{
> > +	IPCUnixSocket::Payload message, response;
> > +	int ret;
> > +
> > +	/* It's fine if seq_ overflows; that'll just be the new epoch. */
> > +	seq_++;
> > +	writeHeader(message, cmd, seq_);
> > +	message.data.insert(message.data.end(), data_in.begin(), data_in.end());
> > +
> > +	message.fds = const_cast<std::vector<int32_t> &>(fds_in);
> > +
> > +	ret = call(message, &response, seq_);
> > +	if (ret) {
> > +		LOG(IPAIPC, Error) << "Failed to call sync";
> > +		callData_.erase(seq_);
> > +		return ret;
> > +	}
> > +
> > +	if (data_out)
> > +		data_out->insert(data_out->end(), response.data.begin(), response.data.end());
> > +
> > +	if (fds_out)
> > +		fds_out->insert(fds_out->end(), response.fds.begin(), response.fds.end());
> > +
> > +	return 0;
> > +}
> > +
> > +int IPAIPCUnixSocket::sendAsync(uint32_t cmd,
> > +				const std::vector<uint8_t> &data_in,
> > +				const std::vector<int32_t> &fds_in)
> > +{
> > +	IPCUnixSocket::Payload message;
> > +	int ret;
> > +
> > +	writeHeader(message, cmd, 0);
> > +	message.data.insert(message.data.end(), data_in.begin(), data_in.end());
> > +
> > +	message.fds = const_cast<std::vector<int32_t> &>(fds_in);
> > +
> > +	ret = socket_->send(message);
> > +	if (ret) {
> > +		LOG(IPAIPC, Error) << "Failed to call async";
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void IPAIPCUnixSocket::readyRead(IPCUnixSocket *socket)
> > +{
> > +	IPCUnixSocket::Payload message;
> > +	int ret = socket->receive(&message);
> > +	if (ret) {
> > +		LOG(IPAIPC, Error) << "Receive message failed" << ret;
> > +		return;
> > +	}
> > +
> > +	uint32_t cmd, seq;
> > +	std::tie(cmd, seq) = readHeader(message);
> > +
> > +	auto callData = callData_.find(seq);
> > +	if (callData != callData_.end()) {
> > +		eraseHeader(message);
> > +		/* Is there any way to avoid this copy? */
> > +		*callData->second.response = message;
> > +		callData->second.done = true;
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Received unexpected data, this means it's a call from the IPA.
> > +	 * We can't return anything to the IPA (gotta keep them under *our*
> > +	 * control, plus returning would require blocking the caller, and we
> > +	 * can't afford to do that). Let the proxy do switch-case on cmd.
> > +	 */
> > +	recvIPC.emit(message.data, message.fds);
> > +
> > +	return;
> > +}
> > +
> > +int IPAIPCUnixSocket::call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq)
> > +{
> > +	Timer timeout;
> > +	int ret;
> > +
> > +	callData_[seq].response = response;
> > +	callData_[seq].done = false;
> > +
> > +	ret = socket_->send(message);
> > +	if (ret)
> > +		return ret;
> > +
> > +	timeout.start(200);
> > +	while (!callData_[seq].done) {
> > +		if (!timeout.isRunning()) {
> > +			LOG(IPAIPC, Error) << "Call timeout!";
> > +			callData_.erase(seq);
> > +			return -ETIMEDOUT;
> > +		}
> > +
> > +		Thread::current()->eventDispatcher()->processEvents();
> > +	}
> > +
> > +	callData_.erase(seq);
> > +
> > +	return 0;
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index e3eade57..24f32815 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -25,6 +25,7 @@ libcamera_sources = files([
> >      'ipa_controls.cpp',
> >      'ipa_data_serializer.cpp',
> >      'ipa_ipc.cpp',
> > +    'ipa_ipc_unixsocket.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

Paul,

On Fri, Sep 25, 2020 at 11:57:27AM +0900, paul.elder@ideasonboard.com wrote:
> On Thu, Sep 24, 2020 at 12:55:05PM +0200, Jacopo Mondi wrote:
> > On Tue, Sep 22, 2020 at 10:35:14PM +0900, Paul Elder wrote:
> > > Add an implementation of IPAIPC using unix socket.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > ---
> > > Changes in v2:
> > > - specify in doxygen to skip generating documentation for
> > >   IPAIPCUnixSocket
> > > ---
> > >  Documentation/Doxyfile.in                     |   2 +
> > >  .../libcamera/internal/ipa_ipc_unixsocket.h   | 115 ++++++++++++
> > >  src/libcamera/ipa_ipc_unixsocket.cpp          | 175 ++++++++++++++++++
> > >  src/libcamera/meson.build                     |   1 +
> > >  4 files changed, 293 insertions(+)
> > >  create mode 100644 include/libcamera/internal/ipa_ipc_unixsocket.h
> > >  create mode 100644 src/libcamera/ipa_ipc_unixsocket.cpp
> > >
> > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > > index 598eca9b..71509ff7 100644
> > > --- a/Documentation/Doxyfile.in
> > > +++ b/Documentation/Doxyfile.in
> > > @@ -837,8 +837,10 @@ RECURSIVE              = YES
> > >  EXCLUDE                = @TOP_SRCDIR@/include/libcamera/span.h \
> > >  			 @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> > >  			 @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> > > +			 @TOP_SRCDIR@/include/libcamera/internal/ipa_ipc_unixsocket.h \
> > >  			 @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> > >  			 @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> > > +			 @TOP_SRCDIR@/src/libcamera/ipa_ipc_unixsocket.cpp \
> > >  			 @TOP_SRCDIR@/src/libcamera/pipeline/ \
> > >  			 @TOP_SRCDIR@/src/libcamera/proxy/ \
> > >  			 @TOP_BUILDDIR@/include/libcamera/ipa/ \
> > > diff --git a/include/libcamera/internal/ipa_ipc_unixsocket.h b/include/libcamera/internal/ipa_ipc_unixsocket.h
> > > new file mode 100644
> > > index 00000000..de8af35b
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h
> > > @@ -0,0 +1,115 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Google Inc.
> > > + *
> > > + * ipa_ipc_unixsocket.h - Image Processing Algorithm IPC module using unix socket
> > > + */
> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__
> > > +#define __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__
> > > +
> > > +#include <vector>
> > > +
> > > +#include <libcamera/span.h>
> > > +
> > > +#include "libcamera/internal/ipa_ipc.h"
> > > +#include "libcamera/internal/ipa_module.h"
> > > +#include "libcamera/internal/ipc_unixsocket.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class Process;
> > > +
> > > +inline void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq)
> > > +{
> > > +	uint8_t cmd_arr[] = {static_cast<uint8_t>(cmd & 0xff),
> > > +			     static_cast<uint8_t>((cmd >> 8 ) & 0xff),
> > > +			     static_cast<uint8_t>((cmd >> 16) & 0xff),
> > > +			     static_cast<uint8_t>((cmd >> 24) & 0xff)};
> > > +	uint8_t seq_arr[] = {static_cast<uint8_t>(seq & 0xff),
> > > +			     static_cast<uint8_t>((seq >> 8 ) & 0xff),
> > > +			     static_cast<uint8_t>((seq >> 16) & 0xff),
> > > +			     static_cast<uint8_t>((seq >> 24) & 0xff)};
> >
> > I think you can omit the 0xff as the cast to uint8_t implies it
>
> ack
>
> > > +	payload.data.insert(payload.data.begin(), cmd_arr, cmd_arr+4);
> > > +	payload.data.insert(payload.data.begin() + 4, seq_arr, seq_arr+4);
> > > +}
> > > +
> > > +inline std::tuple<uint32_t, uint32_t> readHeader(IPCUnixSocket::Payload &payload)
> > > +{
> > > +	uint32_t cmd = (payload.data[0] & 0xff) |
> > > +		       ((payload.data[1] & 0xff) << 8) |
> > > +		       ((payload.data[2] & 0xff) << 16) |
> > > +		       ((payload.data[3] & 0xff) << 24);
> > > +	uint32_t seq = (payload.data[4] & 0xff) |
> > > +		       ((payload.data[5] & 0xff) << 8) |
> > > +		       ((payload.data[6] & 0xff) << 16) |
> > > +		       ((payload.data[7] & 0xff) << 24);
> > > +

Also

payload.data is a vector of uint8_t, no need for the 0xff
You can save all ()


> > > +	return {cmd, seq};
> > > +}
> > > +
> > > +inline void eraseHeader(IPCUnixSocket::Payload &payload)
> > > +{
> > > +	payload.data.erase(payload.data.begin(), payload.data.begin() + 8);
> > > +}
> >
> > I would not place these in the header file but rather as static
> > functions in an anonymous namespace in the cpp file. Or are they used
> > outside from this module ? It seems so:
> >
> > src/libcamera/ipa_ipc_unixsocket.cpp:   std::tie(cmd, seq) = readHeader(message);
> > test/ipc/unixsocket_ipc.cpp:            std::tie(cmd, seq) = readHeader(message);
> > utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl:      std::tie(_cmd, _seq) = readHeader(_message);
> >
> > So why are they defined in this header which implements the IPAIPC
> > interface ? What if another IPC mechanism is used ?
>
> This header is only for IPAIPCUnixSocket, not the base IPAIPC, so it
> should be fine to keep them here. test/ipc/unixsocket_ipc.cpp tests
> IPAIPCUnixSocket.
>
> As for the proxy worker, as I mentioned before, technically it's a
> layering violation and we should really have an IPAIPCWorker
> (IPAIPCUnixSocketWorker) in between IPAIPC (IPAIPCUnixSocket)
> and IPAProxy{pipeline_name}Worker. But we don't have any other IPC
> mechanisms yet, so it should be fine for now. When we do, then the proxy
> worker will no longer use these IPC-specific header manipulation
> functions, and they will be used in the IPAIPCUnixSocketWorker.
>

So all the generated workers uses the UnixIPC mechanism
unconditionally, right ?

Anyway it feels really sloppy to pollute the global namespace with two
functions defined in the unix socket IPC. Do you expect other IPC
mechanism to have different read/writeHeader ? In this case you should
make these static class members (you can use inline in the function
definition in the .cpp file if you want to keep them inlined).
Will these methods be generic to all the IPC mechanisms instead? Move
them to the base virtual class. I don't see a real reason to keep them
globally visible.

I'm still not familiar on the Proxy/ProxyWorker architecture, so I
might be missing some parts.

> > (I -think- also this causes the function to be defined in every module
> > that includes this header, wasting a bit of space, but that's a
> > secondary issue).
>
> These are the only files that include this header:
>
> src/libcamera/ipa_ipc_unixsocket.cpp
> test/ipc/unixsocket_ipc.cpp
> utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
>
> The first two are fine. The last one is fine for now. The other two
> really only include this header for choosing the IPAIPC that they want
> to use, which for now should be fine. When we add other IPC mechanisms
> we can rename or namespace these.
>
> > > +
> > > +inline void writeUInt32(IPCUnixSocket::Payload &payload, uint32_t val, uint32_t pos)
> > > +{
> > > +	if (pos + 4 > payload.data.size())
> > > +		return;
> > > +
> > > +	uint8_t arr[] = {static_cast<uint8_t>(val & 0xff),
> > > +			 static_cast<uint8_t>(val & (0xff << 8)),
> > > +			 static_cast<uint8_t>(val & (0xff << 16)),
> > > +			 static_cast<uint8_t>(val & (0xff << 24))};
> > > +	std::copy(arr, arr + 4, payload.data.begin() + pos);
> > > +}
> > > +
> > > +inline uint32_t readUInt32(IPCUnixSocket::Payload &payload, uint32_t pos)
> > > +{
> > > +	if (pos + 4 > payload.data.size())
> > > +		return 0;
> > > +
> > > +	return payload.data[pos] & (payload.data[pos + 1] << 8) &
> > > +	       (payload.data[pos + 2] << 16) & (payload.data[pos + 3] << 24);
> > > +}
> >
> > I don't see these used.
>
> Ah yes, these are leftovers from the de/serialization work...
>
> > Also, double empty line
> >
> > > +
> > > +
> > > +class IPAIPCUnixSocket : public IPAIPC
> > > +{
> > > +public:
> > > +	IPAIPCUnixSocket(const char *ipa_module_path, const char *ipa_proxy_worker_path);
> > > +	~IPAIPCUnixSocket();
> > > +
> > > +	bool isValid() const { return valid_; }
> >
> > that's defined in the base class, right ?
>
> Ah yeah it is.
>
>
> Thanks,
>
> Paul
> > > +
> > > +	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) override;
> > > +
> > > +	int sendAsync(uint32_t cmd,
> > > +		      const std::vector<uint8_t> &data_in,
> > > +		      const std::vector<int32_t> &fds_in) override;
> > > +
> > > +private:
> > > +	struct CallData {
> > > +		IPCUnixSocket::Payload *response;
> > > +		bool done;
> > > +	};
> > > +
> > > +	void readyRead(IPCUnixSocket *socket);
> > > +	int call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq);
> > > +
> > > +	uint32_t seq_;
> > > +
> > > +	Process *proc_;
> > > +
> > > +	IPCUnixSocket *socket_;
> > > +
> > > +	std::map<uint32_t, struct CallData> callData_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> >
> > I'll for now skip the review of the implementation.
> >
> >
> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_IPC_UNIXSOCKET_H__ */
> > > diff --git a/src/libcamera/ipa_ipc_unixsocket.cpp b/src/libcamera/ipa_ipc_unixsocket.cpp
> > > new file mode 100644
> > > index 00000000..c1997e96
> > > --- /dev/null
> > > +++ b/src/libcamera/ipa_ipc_unixsocket.cpp
> > > @@ -0,0 +1,175 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Google Inc.
> > > + *
> > > + * ipa_ipc_unixsocket.cpp - Image Processing Algorithm IPC module using unix socket
> > > + */
> > > +
> > > +#include <vector>
> > > +
> > > +#include "libcamera/internal/ipc_unixsocket.h"
> > > +#include "libcamera/internal/log.h"
Anyway I see all the generated workers calling readHeader() and writeHeader(),
as a result of the ipa_proxy_worker template using them.

> > > +#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"
> > > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(IPAIPC)
> > > +
> > > +IPAIPCUnixSocket::IPAIPCUnixSocket(const char *ipa_module_path,
> > > +				   const char *ipa_proxy_worker_path)
> > > +	: IPAIPC(ipa_module_path, ipa_proxy_worker_path), seq_(0),
> > > +	  proc_(nullptr), socket_(nullptr)
> > > +{
> > > +	std::vector<int> fds;
> > > +	std::vector<std::string> args;
> > > +	args.push_back(ipa_module_path);
> > > +
> > > +	socket_ = new IPCUnixSocket();
> > > +	int fd = socket_->create();
> > > +	if (fd < 0) {
> > > +		LOG(IPAIPC, Error)
> > > +			<< "Failed to create socket";
> > > +		return;
> > > +	}
> > > +	socket_->readyRead.connect(this, &IPAIPCUnixSocket::readyRead);
> > > +	args.push_back(std::to_string(fd));
> > > +	fds.push_back(fd);
> > > +
> > > +	proc_ = new Process();
> > > +	int ret = proc_->start(ipa_proxy_worker_path, args, fds);
> > > +	if (ret) {
> > > +		LOG(IPAIPC, Error)
> > > +			<< "Failed to start proxy worker process";
> > > +		return;
> > > +	}
> > > +
> > > +	valid_ = true;
> > > +}
> > > +
> > > +IPAIPCUnixSocket::~IPAIPCUnixSocket()
> > > +{
> > > +	delete proc_;
> > > +	delete socket_;
> > > +}
> > > +
> > > +int IPAIPCUnixSocket::sendSync(uint32_t cmd,
> > > +			       const std::vector<uint8_t> &data_in,
> > > +			       const std::vector<int32_t> &fds_in,
> > > +			       std::vector<uint8_t> *data_out,
> > > +			       std::vector<int32_t> *fds_out)
> > > +{
> > > +	IPCUnixSocket::Payload message, response;
> > > +	int ret;
> > > +
> > > +	/* It's fine if seq_ overflows; that'll just be the new epoch. */
> > > +	seq_++;
> > > +	writeHeader(message, cmd, seq_);
> > > +	message.data.insert(message.data.end(), data_in.begin(), data_in.end());
> > > +
> > > +	message.fds = const_cast<std::vector<int32_t> &>(fds_in);
> > > +
> > > +	ret = call(message, &response, seq_);
> > > +	if (ret) {
> > > +		LOG(IPAIPC, Error) << "Failed to call sync";
> > > +		callData_.erase(seq_);
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (data_out)
> > > +		data_out->insert(data_out->end(), response.data.begin(), response.data.end());
> > > +
> > > +	if (fds_out)
> > > +		fds_out->insert(fds_out->end(), response.fds.begin(), response.fds.end());
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int IPAIPCUnixSocket::sendAsync(uint32_t cmd,
> > > +				const std::vector<uint8_t> &data_in,
> > > +				const std::vector<int32_t> &fds_in)
> > > +{
> > > +	IPCUnixSocket::Payload message;
> > > +	int ret;
> > > +
> > > +	writeHeader(message, cmd, 0);
> > > +	message.data.insert(message.data.end(), data_in.begin(), data_in.end());
> > > +
> > > +	message.fds = const_cast<std::vector<int32_t> &>(fds_in);
> > > +
> > > +	ret = socket_->send(message);
> > > +	if (ret) {
> > > +		LOG(IPAIPC, Error) << "Failed to call async";
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void IPAIPCUnixSocket::readyRead(IPCUnixSocket *socket)
> > > +{
> > > +	IPCUnixSocket::Payload message;
> > > +	int ret = socket->receive(&message);
> > > +	if (ret) {
> > > +		LOG(IPAIPC, Error) << "Receive message failed" << ret;
> > > +		return;
> > > +	}
> > > +
> > > +	uint32_t cmd, seq;
> > > +	std::tie(cmd, seq) = readHeader(message);
> > > +
> > > +	auto callData = callData_.find(seq);
> > > +	if (callData != callData_.end()) {
> > > +		eraseHeader(message);
> > > +		/* Is there any way to avoid this copy? */
> > > +		*callData->second.response = message;
> > > +		callData->second.done = true;
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Received unexpected data, this means it's a call from the IPA.
> > > +	 * We can't return anything to the IPA (gotta keep them under *our*
> > > +	 * control, plus returning would require blocking the caller, and we
> > > +	 * can't afford to do that). Let the proxy do switch-case on cmd.
> > > +	 */
> > > +	recvIPC.emit(message.data, message.fds);
> > > +
> > > +	return;
> > > +}
> > > +
> > > +int IPAIPCUnixSocket::call(const IPCUnixSocket::Payload &message, IPCUnixSocket::Payload *response, uint32_t seq)
> > > +{
> > > +	Timer timeout;
> > > +	int ret;
> > > +
> > > +	callData_[seq].response = response;
> > > +	callData_[seq].done = false;
> > > +
> > > +	ret = socket_->send(message);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	timeout.start(200);
> > > +	while (!callData_[seq].done) {
> > > +		if (!timeout.isRunning()) {
> > > +			LOG(IPAIPC, Error) << "Call timeout!";
> > > +			callData_.erase(seq);
> > > +			return -ETIMEDOUT;
> > > +		}
> > > +
> > > +		Thread::current()->eventDispatcher()->processEvents();
> > > +	}
> > > +
> > > +	callData_.erase(seq);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index e3eade57..24f32815 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -25,6 +25,7 @@ libcamera_sources = files([
> > >      'ipa_controls.cpp',
> > >      'ipa_data_serializer.cpp',
> > >      'ipa_ipc.cpp',
> > > +    'ipa_ipc_unixsocket.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