[libcamera-devel] [RFC 1/2] libcamera: ipc: unix: Add a IPC mechanism based on Unix sockets
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Jun 23 16:53:03 CEST 2019
Hi Laurent,
Thanks for your feedback.
On 2019-06-23 00:46:28 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Fri, Jun 21, 2019 at 06:15:18AM +0200, Niklas Söderlund wrote:
> > To be able to isolate an IPA component in a separate process and IPC
>
> s/and/an/
>
> > mechanism is needed to communicate with it. Add a IPC mechanism based on
>
> s/a IPC/an IPC/
>
> > Unix sockets which allows users to pass both data and file descriptors
> > to to and from the IPA process.
>
> s/to to/to/
>
> >
> > The implementation allows users to send both data and file descriptors
> > in the same message. This allows users to more easily implement
> > serialization and deserialization of objects as all elements belonging
> > to an object can be sent in one message,
>
> s/,/./
>
> >
> > The implementation guarantees that a whole object is transmitted and
> > received over IPC before it's handed of. This allows IPC users to not
>
> What do you mean by handed of here ?
What I tried to express is that messages are sent and received as a
whole unit and that users of the IPC class do not need to care about
assembling partly transmitted messages.
>
> > have to deal with buffering and may depend on that it only needs to deal
> > with serialization/deserialization of complete object blobs.
> >
> > The implementation also provides a priv field in the IPC message header
> > which is a 32 bit integer that can be used by IPA implementations that
> > do not require a complex protocol header to describe what type of
> > message is transmitted.
>
> I'm not sure I would do that, wouldn't it make more sense to completely
> separate the IPC transport from the protocol ? Otherwise, where will we
> draw the line ?
I don't feel strongly about this, and you might be right that providing
this one filed it becomes hard to draw the line. Will drop this field
for next version.
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/include/ipc_unixsocket.h | 58 +++++
> > src/libcamera/ipc_unixsocket.cpp | 330 +++++++++++++++++++++++++
> > src/libcamera/meson.build | 2 +
> > 3 files changed, 390 insertions(+)
> > create mode 100644 src/libcamera/include/ipc_unixsocket.h
> > create mode 100644 src/libcamera/ipc_unixsocket.cpp
> >
> > diff --git a/src/libcamera/include/ipc_unixsocket.h b/src/libcamera/include/ipc_unixsocket.h
> > new file mode 100644
> > index 0000000000000000..864fa93b1f190fb7
> > --- /dev/null
> > +++ b/src/libcamera/include/ipc_unixsocket.h
> > @@ -0,0 +1,58 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * ipc_unixsocket.h - IPC mechanism based on Unix sockets
> > + */
> > +
> > +#ifndef __LIBCAMERA_IPC_UNIXSOCKET_H__
> > +#define __LIBCAMERA_IPC_UNIXSOCKET_H__
> > +
> > +#include <cstdint>
> > +#include <sys/types.h>
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +class IPCUnixSocket
> > +{
> > +public:
> > + struct Payload {
> > + uint32_t priv;
> > + std::vector<uint8_t> data;
> > + std::vector<int32_t> fds;
> > + };
> > +
> > + IPCUnixSocket();
> > + IPCUnixSocket(int fd);
> > +
> > + int create();
> > + int connect();
> > + void close();
> > +
> > + int send(const Payload &payload);
> > + int recv(Payload *payload, int timeout);
> > + int call(const Payload &payload, Payload *response, int timeout);
> > +
> > +private:
> > + struct Header {
> > + uint32_t priv;
> > + uint32_t data;
> > + uint8_t fds;
> > + };
> > +
> > + int poll(int timeout);
> > +
> > + int sendData(const void *buffer, ssize_t length);
> > + int recvData(void *buffer, ssize_t length, int timeout);
>
> Does length ever need to be negative ? If not, I would make it a size_t.
>
> > +
> > + int sendFds(const int32_t *fds, unsigned int num);
> > + int recvFds(int32_t *fds, unsigned int num, int timeout);
> > +
> > + int fd_;
> > + bool master_;
> > +};
>
> I think it could make sense to create an abstract IPC class, with
> IPCUnixSocket being a particular implementation.
I wondered about this when designing the class and my view is that this
is not needed. I don't think designing IPA proxies that could work with
any IPC mechanism is a good idea. Is there any other users of IPC then
IPA proxies you can think of that could benefit from a baseclass
implementation?
>
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_IPC_UNIXSOCKET_H__ */
> > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp
> > new file mode 100644
> > index 0000000000000000..b34fa0317a18b37c
> > --- /dev/null
> > +++ b/src/libcamera/ipc_unixsocket.cpp
> > @@ -0,0 +1,330 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * ipc_unixsocket.cpp - IPC mechanism based on Unix sockets
> > + */
> > +
> > +#include "ipc_unixsocket.h"
> > +
> > +#include <errno.h>
> > +#include <poll.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <sys/socket.h>
> > +#include <unistd.h>
> > +
> > +#include "log.h"
> > +
> > +/**
> > + * \file ipc_unixsocket.h
> > + * \brief IPC mechanism based on Unix sockets
> > + */
> > +
> > +/*
> > + * Markers to use in IPC protocol, there is no specific meaning to the values,
> > + * but they should be unique.
> > + */
> > +#define CMD_PING 0x1f
> > +#define CMD_PONG 0xf1
> > +#define CMD_FD 0x77
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(IPCUnixSocket)
> > +
> > +IPCUnixSocket::IPCUnixSocket()
> > + : fd_(-1), master_(false)
> > +{
> > +}
> > +
> > +IPCUnixSocket::IPCUnixSocket(int fd)
> > + : fd_(fd), master_(false)
> > +{
> > +}
> > +
> > +int IPCUnixSocket::create()
> > +{
> > + int sockets[2];
> > + int ret;
> > +
> > + ret = socketpair(AF_UNIX, SOCK_STREAM, 0, sockets);
> > + if (ret) {
> > + ret = -errno;
> > + LOG(IPCUnixSocket, Error)
> > + << "Failed to create socket pair: " << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + fd_ = sockets[0];
> > + master_ = true;
> > +
> > + return sockets[1];
> > +}
> > +
> > +int IPCUnixSocket::connect()
> > +{
> > + Payload payload = {};
> > + Payload response = {};
> > +
> > + if (master_) {
> > + payload.data.push_back(CMD_PING);
> > +
> > + if (call(payload, &response, 500))
> > + return -1;
> > +
> > + if (response.data[0] != CMD_PONG)
> > + return -1;
> > + } else {
> > + if (recv(&payload, 500))
> > + return -1;
> > +
> > + if (payload.data[0] != CMD_PING)
> > + return -1;
> > +
> > + response.data.push_back(CMD_PONG);
> > +
> > + if (send(response))
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void IPCUnixSocket::close()
> > +{
> > + if (fd_ == -1)
> > + return;
> > +
> > + ::close(fd_);
> > +
> > + fd_ = -1;
> > +}
> > +
> > +int IPCUnixSocket::send(const Payload &payload)
> > +{
> > + Header hdr;
> > + int ret;
> > +
> > + if (fd_ < 0)
> > + return -ENOTCONN;
> > +
> > + hdr.priv = payload.priv;
> > + hdr.data = payload.data.size();
> > + hdr.fds = payload.fds.size();
> > +
> > + ret = sendData(&hdr, sizeof(hdr));
> > + if (ret)
> > + return ret;
> > +
> > + if (hdr.data) {
> > + ret = sendData(payload.data.data(), hdr.data);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (hdr.fds) {
> > + ret = sendFds(payload.fds.data(), hdr.fds);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int IPCUnixSocket::recv(Payload *payload, int timeout)
> > +{
> > + Header hdr;
> > + int ret;
> > +
> > + if (fd_ < 0)
> > + return -ENOTCONN;
> > +
> > + ret = recvData(&hdr, sizeof(hdr), timeout);
> > + if (ret)
> > + return ret;
> > +
> > + payload->priv = hdr.priv;
> > + payload->data.resize(hdr.data);
> > + payload->fds.resize(hdr.fds);
> > +
> > + if (hdr.data) {
> > + ret = recvData(payload->data.data(), hdr.data, timeout);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (hdr.fds) {
> > + ret = recvFds(payload->fds.data(), hdr.fds, timeout);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int IPCUnixSocket::call(const Payload &payload, Payload *response, int timeout)
> > +{
> > + int ret = send(payload);
> > + if (ret)
> > + return ret;
> > +
> > + return recv(response, timeout);
> > +}
> > +
> > +int IPCUnixSocket::poll(int timeout)
> > +{
> > + struct pollfd pollfd = { fd_, POLLIN, 0 };
> > +
> > + int ret = ::poll(&pollfd, 1, timeout);
> > + if (ret < 0) {
> > + ret = -errno;
> > + LOG(IPCUnixSocket, Error)
> > + << "Failed to poll: " << strerror(-ret);
> > + return ret;
> > + } else if (ret == 0) {
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int IPCUnixSocket::sendData(const void *buffer, ssize_t length)
> > +{
> > + ssize_t len, sent;
> > + const uint8_t *pos;
> > +
> > + if (fd_ < 0)
> > + return -ENOTCONN;
> > +
>
> The caller already checks for this, so I think you can skip this check.
>
> > + pos = static_cast<const uint8_t *>(buffer);
> > + len = length;
> > +
> > + while (len) {
> > + sent = ::send(fd_, pos, len, 0);
> > + if (sent < 0) {
> > + sent = -errno;
> > + LOG(IPCUnixSocket, Error)
> > + << "Failed to send: " << strerror(-sent);
> > + return sent;
> > + }
> > +
> > + pos += sent;
> > + len -= sent;
> > + }
>
> Is the loop needed, can a message be fragmented over a UNIX socket ?
> According to the manpage of send(), "If the message is too long to pass
> atomically through the underlying protocol, the error EMSGSIZE is
> returned, and the message is not transmitted."
You are correct. I even checked this when writing the code but I must
have forgotten to remove the loop. Will drop form next version. Do you
think it's worth the effort to handle a EMSGSIZE error by breaking the
message apart or shall we make this error Fatal and handle it if we
endup trying to send very large messages?
>
> > +
> > + return 0;
> > +}
> > +
> > +int IPCUnixSocket::recvData(void *buffer, ssize_t length, int timeout)
> > +{
> > + ssize_t len, recived;
>
> s/recived/received/
>
> > + uint8_t *pos;
> > +
> > + if (fd_ < 0)
> > + return -ENOTCONN;
> > +
> > + pos = static_cast<uint8_t *>(buffer);
> > + len = length;
> > +
> > + while (len) {
> > + int ret = poll(timeout);
> > + if (ret < 0)
> > + return ret;
> > +
>
> Please don't make blocking calls. You should use an event notifier to be
> notified of incoming messages, and should then emit a signal with the
> received payload.
>
> This will make the call() method more difficult to implement, but I
> think we should be completely event-driven to avoid blocking event
> loops. The alternative would be threads, which we will likely end up
> using eventually, but that's compatible with an event-driven approach.
I toyed with event notifiers when designing this but once I started to
mock implement the real world use-cases as I see them they felt not that
useful. I'm happy to be convinced otherwise. The way I see it there are
only two use-cases for our IPC.
- Send statistics to IPA from pipeline handler
Once a buffer with statistics are completed in the pipeline handler
context it's event complete notifier should send() the data to the
IPA. This is a fire and forget operation from the pipelines point of
view and should not block.
- Ask IPA for controls to apply when applying a request to hardware.
Before a request is applied to hardware the IPA should be asked about
how to translate libcamera controls to device controls. In this case
the pipeline handler should call() the IPA to do this translation. In
my head it makes sens that this would be a blocking call as the
pipeline can't really do much before it gets the information from the
IPA.
Only thing that really could happen is that a new statistic buffers
completes while the call is blocking. But I reasoned that this is not
a problem as the IPA would need to have a lock to not update
statistics while in the middle of translating libcamera controls to
device controls.
That being said I'm not against making this event driven and as I write
this I could see there being some merit to it. But first lets hash out
the use-cases, maybe it will lead to the insight that moving to threads
directly is the best solution.
>
> > + recived = ::recv(fd_, pos, len, 0);
> > + if (recived < 0) {
> > + recived = -errno;
> > + LOG(IPCUnixSocket, Error)
> > + << "Failed to recv: " << strerror(-recived);
> > + return recived;
> > + }
> > +
> > + pos += recived;
> > + len -= recived;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int IPCUnixSocket::sendFds(const int32_t *fds, unsigned int num)
> > +{
> > + char cmd = CMD_FD;
> > + struct iovec iov[1];
> > + iov[0].iov_base = &cmd;
> > + iov[0].iov_len = sizeof(cmd);
> > +
> > + char buf[CMSG_SPACE(num * sizeof(uint32_t))];
> > + memset(buf, 0, sizeof(buf));
> > +
> > + struct cmsghdr *cmsg = (struct cmsghdr *)buf;
> > + cmsg->cmsg_len = CMSG_LEN(num * sizeof(uint32_t));
> > + cmsg->cmsg_level = SOL_SOCKET;
> > + cmsg->cmsg_type = SCM_RIGHTS;
> > +
> > + struct msghdr msg;
> > + msg.msg_name = nullptr;
> > + msg.msg_namelen = 0;
> > + msg.msg_iov = iov;
> > + msg.msg_iovlen = 1;
> > + msg.msg_control = cmsg;
> > + msg.msg_controllen = cmsg->cmsg_len;
> > + msg.msg_flags = 0;
> > + memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));
> > +
> > + if (sendmsg(fd_, &msg, 0) < 0) {
> > + int ret = -errno;
> > + LOG(IPCUnixSocket, Error)
> > + << "Failed to sendmsg: " << strerror(-ret);
> > + return ret;
> > + }
>
> Couldn't you use sendmsg() to send both the payload data and the file
> descriptors in a single message ? You would send a first datagram with
> the header containing the command, data size and number of file
> descriptors, and then a second datagram with the payload.
One could do that. The reason I did not do this is two fold.
- I have a notion that send()/recv() is faster then sendmsg()/recvmsg(),
but I have no prof of this.
- If we want to deal with messages that are too big to fit inside a
single message I think it would be easier to handle that with
send()/recv().
Depending on what we wish to do with the EMSGSIZE question above I will
act on this suggestion.
>
> > +
> > + return 0;
> > +}
> > +
> > +int IPCUnixSocket::recvFds(int32_t *fds, unsigned int num, int timeout)
> > +{
> > + char cmd;
> > + struct iovec iov[1];
> > + iov[0].iov_base = &cmd;
> > + iov[0].iov_len = sizeof(cmd);
> > +
> > + char buf[CMSG_SPACE(num * sizeof(uint32_t))];
> > + memset(buf, 0, sizeof(buf));
> > +
> > + struct cmsghdr *cmsg = (struct cmsghdr *)buf;
> > + cmsg->cmsg_len = CMSG_LEN(num * sizeof(uint32_t));
> > + cmsg->cmsg_level = SOL_SOCKET;
> > + cmsg->cmsg_type = SCM_RIGHTS;
> > +
> > + struct msghdr msg;
> > + msg.msg_name = nullptr;
> > + msg.msg_namelen = 0;
> > + msg.msg_iov = iov;
> > + msg.msg_iovlen = 1;
> > + msg.msg_control = cmsg;
> > + msg.msg_controllen = cmsg->cmsg_len;
> > + msg.msg_flags = 0;
> > +
> > + int ret = poll(timeout);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (recvmsg(fd_, &msg, 0) < 0) {
> > + int ret = -errno;
> > + LOG(IPCUnixSocket, Error)
> > + << "Failed to recvmsg: " << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + if (cmd != CMD_FD) {
> > + LOG(IPCUnixSocket, Error) << "FD marker wrong";
> > + return -EINVAL;
> > + }
> > +
> > + memcpy(fds, CMSG_DATA(cmsg), num * sizeof(uint32_t));
> > +
> > + return 0;
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index f26ad5b2dc57c014..1158825fa5b0702d 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -13,6 +13,7 @@ libcamera_sources = files([
> > 'ipa_interface.cpp',
> > 'ipa_manager.cpp',
> > 'ipa_module.cpp',
> > + 'ipc_unixsocket.cpp',
> > 'log.cpp',
> > 'media_device.cpp',
> > 'media_object.cpp',
> > @@ -37,6 +38,7 @@ libcamera_headers = files([
> > 'include/formats.h',
> > 'include/ipa_manager.h',
> > 'include/ipa_module.h',
> > + 'include/ipc_unixsocket.h',
> > 'include/log.h',
> > 'include/media_device.h',
> > 'include/media_object.h',
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list