[PATCH 1/4] libcamera: Add signal disconnected for IPC
Cheng-Hao Yang
chenghaoyang at chromium.org
Mon Oct 21 14:11:16 CEST 2024
Hi Kieran and Laurent,
I'll upload a new version when we have a consensus on the signal.
On Mon, Oct 21, 2024 at 12:40 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Sun, Oct 20, 2024 at 11:30:45AM +0100, Kieran Bingham wrote:
> > Quoting Kieran Bingham (2024-10-19 23:44:07)
> > > Quoting Harvey Yang (2024-10-18 08:57:34)
> > > > This CL also adds an API in Camera::Private to trigger
> > > > Camera::disconnected signal.
> > >
> > > CL?
>
> Change List, the gerrit term for a patch.
Right, sorry. Updated.
>
> > > Please read: https://cbea.ms/git-commit/#why-not-how
>
> +1
>
> > >
> > > But adding a signal on this sounds like a good idea to me.
> > >
> > > How does the disconnected signal work. Is it reported when the IPA is
> > > closed? (or crashes?)
Yes, the signal is added exactly for this reason.
> >
> > Sorry - somehow I dropped a sentence here:
> >
> > > or is there some reason for the IP
> >
> > ... or is there some other reason for the IPC to be disconnected ?
> >
> > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > Co-developed-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > > ---
> > > > include/libcamera/internal/camera.h | 2 ++
> > > > .../libcamera/internal/ipc_pipe_unixsocket.h | 2 ++
> > > > include/libcamera/internal/ipc_unixsocket.h | 2 ++
> > > > src/libcamera/camera.cpp | 10 ++++++++
> > > > src/libcamera/ipc_pipe_unixsocket.cpp | 8 +++++++
> > > > src/libcamera/ipc_unixsocket.cpp | 24 +++++++++++++++++--
> > > > .../module_ipa_proxy.cpp.tmpl | 8 +++++++
> > > > .../module_ipa_proxy.h.tmpl | 2 ++
> > > > 8 files changed, 56 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> > > > index 0add0428b..0bef0980e 100644
> > > > --- a/include/libcamera/internal/camera.h
> > > > +++ b/include/libcamera/internal/camera.h
> > > > @@ -33,6 +33,8 @@ public:
> > > >
> > > > PipelineHandler *pipe() { return pipe_.get(); }
> > > >
> > > > + void notifyDisconnection();
> > > > +
> > > > std::list<Request *> queuedRequests_;
> > > > ControlInfoMap controlInfo_;
> > > > ControlList properties_;
> > > > diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h
> > > > index 8c972613f..09f65b102 100644
> > > > --- a/include/libcamera/internal/ipc_pipe_unixsocket.h
> > > > +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h
> > > > @@ -28,6 +28,8 @@ public:
> > > >
> > > > int sendAsync(const IPCMessage &data) override;
> > > >
> > > > + Signal<> *disconnected();
> > > > +
> > > > private:
> > > > struct CallData {
> > > > IPCUnixSocket::Payload *response;
> > > > diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h
> > > > index 48bb7a942..a1e326b6b 100644
> > > > --- a/include/libcamera/internal/ipc_unixsocket.h
> > > > +++ b/include/libcamera/internal/ipc_unixsocket.h
> > > > @@ -39,6 +39,8 @@ public:
> > > >
> > > > Signal<> readyRead;
> > > >
> > > > + Signal<> disconnected;
> > > > +
> > > > private:
> > > > struct Header {
> > > > uint32_t data;
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index a86f552a4..ef5a6725f 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -603,6 +603,16 @@ Camera::Private::~Private()
> > > > * \return The pipeline handler that created this camera
> > > > */
> > > >
> > > > +/**
> > > > + * \brief Notify the application that camera is disconnected with signal
> > > > + * Camera::disconnected
>
> That seems to be a big shortcut. IPC disconnection doesn't mean the
> camera has been disconnected. What will happen if the camera then really
> becomes disconnected ? Won't the disconnect signal be emitted a second
> time, confusing applications ? And how is the pipeline handler expected
> to gracefully shut things down ? I think there's still some design
> homework to be done here.
You're right. I think this is how CrOS camera service thinks when the IPA
process is terminated. There's a test that expects to receive
CAMERA3_MSG_ERROR_REQUEST when the IPA process crashes [1].
I understand that this is not how libcamera takes an IPA disconnection.
Do you think we need another signal for this case?
We might want to discuss how an IPA proxy is bound to a camera as well,
as some pipeline handlers might use a single IPA proxy to process all
cameras.
[1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5467266
>
> I would also like to understand under which circumstances you've seen
> this happening.
>
> > > > + */
> > > > +void Camera::Private::notifyDisconnection()
> > > > +{
> > > > + Camera *o = LIBCAMERA_O_PTR();
> > > > + o->disconnected.emit();
> > > > +}
> > > > +
> > > > /**
> > > > * \fn Camera::Private::validator()
> > > > * \brief Retrieve the control validator related to this camera
> > > > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp
> > > > index 668ec73b9..51fd3b1fb 100644
> > > > --- a/src/libcamera/ipc_pipe_unixsocket.cpp
> > > > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp
> > > > @@ -84,6 +84,14 @@ int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)
> > > > return 0;
> > > > }
> > > >
> > > > +Signal<> *IPCPipeUnixSocket::disconnected()
> > > > +{
> > > > + if (socket_)
> > > > + return &socket_->disconnected;
> > > > +
> > > > + return nullptr;
> > > > +}
> > > > +
> > > > void IPCPipeUnixSocket::readyRead()
> > > > {
> > > > IPCUnixSocket::Payload payload;
> > > > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp
> > > > index 002053e35..3d0248857 100644
> > > > --- a/src/libcamera/ipc_unixsocket.cpp
> > > > +++ b/src/libcamera/ipc_unixsocket.cpp
> > > > @@ -186,11 +186,16 @@ int IPCUnixSocket::send(const Payload &payload)
> > > > if (!hdr.data && !hdr.fds)
> > > > return -EINVAL;
> > > >
> > > > - ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);
> > > > + ret = ::send(fd_.get(), &hdr, sizeof(hdr), MSG_NOSIGNAL);
> > >
> > > What's this flag change for? Can you explain in the commit message or as
> > > a comment please?
>
> The MSG_NOSIGNAL documentation states
>
> Don’t generate a SIGPIPE signal if the peer on a stream-oriented
> socket has closed the connection. The EPIPE error is still
> returned. This provides similar behavior to using sigaction(2) to
> ignore SIGPIPE, but, whereas MSG_NOSIGNAL is a per-call feature,
> ignoring SIGPIPE sets a process attribute that affects all threads
> in the process.
>
> but here you're checking ECONNRESET. The documentation of ECONNRESET and
> EPIPE for this function respectively state
>
> ECONNRESET
> Connection reset by peer.
>
> EPIPE
> The local end has been shut down on a connection oriented
> socket. In this case, the process will also receive a SIGPIPE
> unless MSG_NOSIGNAL is set.
>
> Do we need MSG_NOSIGNAL to properly handle ECONNRESET ? Do we need to
> handle EPIPE ? In any case, all this needs to be explained in the commit
> message.
Right, we probably don't need MSG_NOSIGNAL. Removed.
I'll also detect both ECONNRESET and EPIPE as a disconnection in the
next version. Updated the commit message.
In my trials though, it's always ECONNRESET. I'm not sure how the current
code can trigger an EPIPE.
>
> > >
> > > > if (ret < 0) {
> > > > ret = -errno;
> > > > LOG(IPCUnixSocket, Error)
> > > > << "Failed to send: " << strerror(-ret);
> > > > + if (errno == ECONNRESET) {
>
> errno may have been modified by the LOG statement. You need to use ret
> here. Same below.
Got it. Updated.
>
> > > > + disconnected.emit();
> > > > + fd_.reset();
>
> Why fd_.reset() here and below ? The commit message really misses lots
> of information.
Yeah we probably don't need to prevent the following messages.
Tested on mtkisp7 and it works.
>
> > >
> > > So this is really about if the IPA connection gets severed indeed?
Yes, it is.
BR,
Harvey
> > >
> > > > + }
> > > > +
> > > > return ret;
> > > > }
> > > >
> > > > @@ -241,6 +246,11 @@ int IPCUnixSocket::receive(Payload *payload)
> > > > * \brief A Signal emitted when a message is ready to be read
> > > > */
> > > >
> > > > +/**
> > > > + * \var IPCUnixSocket::disconnected
> > > > + * \brief A Signal emitted when the Unix socket IPC is disconnected
> > > > + */
> > > > +
> > > > int IPCUnixSocket::sendData(const void *buffer, size_t length,
> > > > const int32_t *fds, unsigned int num)
> > > > {
> > > > @@ -266,10 +276,15 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,
> > > > if (fds)
> > > > memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));
> > > >
> > > > - if (sendmsg(fd_.get(), &msg, 0) < 0) {
> > > > + if (sendmsg(fd_.get(), &msg, MSG_NOSIGNAL) < 0) {
> > > > int ret = -errno;
> > > > LOG(IPCUnixSocket, Error)
> > > > << "Failed to sendmsg: " << strerror(-ret);
> > > > + if (errno == ECONNRESET) {
> > > > + disconnected.emit();
> > > > + fd_.reset();
> > > > + }
> > > > +
> > > > return ret;
> > > > }
> > > >
> > > > @@ -324,6 +339,11 @@ void IPCUnixSocket::dataNotifier()
> > > > ret = -errno;
> > > > LOG(IPCUnixSocket, Error)
> > > > << "Failed to receive header: " << strerror(-ret);
> > > > + if (errno == ECONNRESET) {
> > > > + disconnected.emit();
> > > > + fd_.reset();
> > > > + }
> > > > +
> > > > return;
> > > > }
> > > >
> > > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > > index ce3cc5ab6..27f03417a 100644
> > > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > > @@ -104,6 +104,14 @@ namespace {{ns}} {
> > > > }
> > > > }
> > > >
> > > > +Signal<> *{{proxy_name}}::disconnected()
> > > > +{
> > > > + if (ipc_)
> > > > + return ipc_->disconnected();
> > > > +
> > > > + return nullptr;
> > > > +}
> > > > +
> > > > {% if interface_event.methods|length > 0 %}
> > > > void {{proxy_name}}::recvMessage(const IPCMessage &data)
> > > > {
> > > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > > index e213b18a0..2b7ba872e 100644
> > > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > > @@ -53,6 +53,8 @@ public:
> > > > > {{method.mojom_name}};
> > > > {% endfor %}
> > > >
> > > > + Signal<> *disconnected();
> > > > +
> > > > private:
> > > > void recvMessage(const IPCMessage &data);
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list