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

9920 diff mbox series

Add an implementation of IPAIPC using unix socket.

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

---
Changes in v3:
- remove unused writeUInt32() and readUInt32()
- remove redundant definition of IPAIPCUnixSocket::isValid()
- remove & 0xff in writeHeader()
- make readHeader, writeHeader, and eraseHeader static class functions
  of IPAIPCUnixSocket instead of globals

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

Hi Paul,

On Fri, Oct 02, 2020 at 11:31:28PM +0900, Paul Elder wrote:
> Add an implementation of IPAIPC using unix socket.

Spell UNIX with capital letter ?

>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v3:
> - remove unused writeUInt32() and readUInt32()
> - remove redundant definition of IPAIPCUnixSocket::isValid()
> - remove & 0xff in writeHeader()
> - make readHeader, writeHeader, and eraseHeader static class functions
>   of IPAIPCUnixSocket instead of globals
>
> Changes in v2:
> - specify in doxygen to skip generating documentation for
>   IPAIPCUnixSocket
> ---
>  Documentation/Doxyfile.in                     |   2 +
>  .../libcamera/internal/ipa_ipc_unixsocket.h   |  62 +++++
>  src/libcamera/ipa_ipc_unixsocket.cpp          | 211 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  4 files changed, 276 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 \

I wonder if we shouldn't make a src/libcamera/ipc/

>  			 @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..11f05b12
> --- /dev/null
> +++ b/include/libcamera/internal/ipa_ipc_unixsocket.h
> @@ -0,0 +1,62 @@
> +/* 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;
> +
> +class IPAIPCUnixSocket : public IPAIPC
> +{
> +public:
> +	IPAIPCUnixSocket(const char *ipa_module_path, const char *ipa_proxy_worker_path);
> +	~IPAIPCUnixSocket();
> +
> +	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;
> +
> +	static void writeHeader(IPCUnixSocket::Payload &payload, uint32_t cmd, uint32_t seq);
> +	static std::tuple<uint32_t, uint32_t> readHeader(IPCUnixSocket::Payload &payload);
> +	static void eraseHeader(IPCUnixSocket::Payload &payload);
> +
> +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 */
> +
> +#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..6dc92768
> --- /dev/null
> +++ b/src/libcamera/ipa_ipc_unixsocket.cpp
> @@ -0,0 +1,211 @@
> +/* 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"

This one should go first

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

I get this complaint from doxygen (on ipa_ipc.c actually)

/home/jmondi/project/libcamera/libcamera.git/src/libcamera/ipa_ipc.cpp:38: warning: argument 'ipa_module_path' of command @param is not found in the argument list of libcamera::IPAIPC::IPAIPC(const char *ipa_module_path][[maybe_unused], const char *ipa_proxy_worker_path][[maybe_unused])
/home/jmondi/project/libcamera/libcamera.git/src/libcamera/ipa_ipc.cpp:38: warning: argument 'ipa_proxy_worker_path' of command @param is not found in the argument list of libcamera::IPAIPC::IPAIPC(const char *ipa_module_path][[maybe_unused], const char *ipa_proxy_worker_path][[maybe_unused])

You don't ? (doxygen 1.8.20 here)

> +	: 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";

Fits on one line ?

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

What defines the proxy_worker interface ? ie that it wants 'fd' as
first argument ? (not that it is wrong but just to know where it is
defined)

> +	if (ret) {
> +		LOG(IPAIPC, Error)
> +			<< "Failed to start proxy worker process";
> +		return;
> +	}
> +
> +	valid_ = true;
> +}
> +
> +IPAIPCUnixSocket::~IPAIPCUnixSocket()
> +{
> +	delete proc_;
> +	delete socket_;

we can save this by making proc_ and socket_ unique_ptr<>

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

It would be easy to pre-reserve message.data
8 bytes for the header + data_in.size()

This would avoid at least two potential relocations

Same for fds, not possible for response unless we over-allocate.

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

Double empty line ?

> +void IPAIPCUnixSocket::writeHeader(IPCUnixSocket::Payload &payload,
> +				   uint32_t cmd, uint32_t seq)
> +{
> +	uint8_t cmd_arr[] = {static_cast<uint8_t>(cmd),
> +		static_cast<uint8_t>((cmd >> 8)),
> +		static_cast<uint8_t>((cmd >> 16)),
> +		static_cast<uint8_t>((cmd >> 24))};
> +	uint8_t seq_arr[] = {static_cast<uint8_t>(seq),
> +		static_cast<uint8_t>((seq >> 8)),
> +		static_cast<uint8_t>((seq >> 16)),
> +		static_cast<uint8_t>((seq >> 24))};

Do we care about endianesa between IPA and pipeline handlers ? Unless we run IPA
on a different CPU it should not matter, righ ?

Because otherwise you can take a uint8_t pointer to cmd and seq and just:

      	uint8_t *p = reinterpret_cast<uint8_t *>(&cmd);
	payload.data.insert(payload.data.begin(), p, p + 4);
      	p = reinterpret_cast<uint8_t *>(&seq);
	payload.data.insert(payload.data.begin(), p, p + 4);

and make this an inline function

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

same here by reintrpreting payload.data[0] and payload.data[4] as
uint32_t

> +}
> +
> +void IPAIPCUnixSocket::eraseHeader(IPCUnixSocket::Payload &payload)
> +{
> +	payload.data.erase(payload.data.begin(), payload.data.begin() + 8);
> +}
> +
> +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();

I'll more carefully review this last part later, but I would have a
question, possibly for Laurent: do we have or need to implement a
timed wait condition support in the thread library, taking a condition
variable and a timeout and implementing this part ? It should be
paired with a condition signaling couter-part. This would avoid having
library classes poking the eventDispatcher themselves.

Thanks
   j

> +	}
> +
> +	callData_.erase(seq);
> +
> +	return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index e0a2ab47..59417403 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -26,6 +26,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