Message ID | 20200603141609.18584-6-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Paul, Thank you for the patch. On Wed, Jun 03, 2020 at 11:16:09PM +0900, Paul Elder wrote: > To support polling, we need to be able to signal when data is available > to be read (POLLIN), as well as events (POLLPRI). To allow signaling > POLLPRI specifically, change eventfd to a pair of sockets. > > After adding the socketpair, use the socketpair to signal POLLIN by > writing a byte to the socket, and read the byte away upon VIDIOC_DQBUF > to clear the handled POLLIN. > > Although POLLPRI is not actually implemented, since we don't support > events, the facilities are in place. The implementation for signaling s/events/V4L2 events/ > and clearing POLLPRI is the same as for POLLIN, except that we would > send and receive an out-of-band byte through the socket. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/v4l2/v4l2_camera.cpp | 13 +++++++++++++ > src/v4l2/v4l2_camera.h | 3 +++ > src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++++++ > src/v4l2/v4l2_camera_proxy.h | 4 ++++ > src/v4l2/v4l2_compat_manager.cpp | 22 ++++++++++++++++------ > 5 files changed, 50 insertions(+), 6 deletions(-) > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index 4da01a45..ecbc87ec 100644 > --- a/src/v4l2/v4l2_camera.cpp > +++ b/src/v4l2/v4l2_camera.cpp > @@ -8,6 +8,9 @@ > #include "v4l2_camera.h" > > #include <errno.h> > +#include <sys/socket.h> > +#include <sys/types.h> > +#include <unistd.h> > > #include "libcamera/internal/log.h" > > @@ -51,6 +54,13 @@ void V4L2Camera::close() > bufferAllocator_ = nullptr; > > camera_->release(); > + > + ::close(sockfd_); > +} > + > +void V4L2Camera::bind(int sockfd) > +{ > + sockfd_ = sockfd; > } > > void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig) > @@ -85,6 +95,9 @@ void V4L2Camera::requestComplete(Request *request) > bufferLock_.unlock(); > > bufferSema_.release(); > + > + char tmp = 0; Let's call this data, tmp should be outlawed as a variable name :-) > + ::send(sockfd_, &tmp, 1, 0); You should call send() before bufferSema_.release(), otherwise there will be a race condition in V4L2CameraProxy::vidioc_dqbuf() where the semaphore could be acquired, but recv() could fail. > } > > int V4L2Camera::configure(StreamConfiguration *streamConfigOut, > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h > index 303eda44..1ad040c1 100644 > --- a/src/v4l2/v4l2_camera.h > +++ b/src/v4l2/v4l2_camera.h > @@ -39,6 +39,7 @@ public: > > int open(); > void close(); > + void bind(int sockfd); > void getStreamConfig(StreamConfiguration *streamConfig); > std::vector<Buffer> completedBuffers(); > > @@ -70,6 +71,8 @@ private: > > std::deque<std::unique_ptr<Request>> pendingRequests_; > std::deque<std::unique_ptr<Buffer>> completedBuffers_; > + > + int sockfd_; We have a FileDescriptor class that could be used for this, removing the need to call ::close() > }; > > #endif /* __V4L2_CAMERA_H__ */ > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index f84982a2..c9d7945a 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -13,6 +13,9 @@ > #include <linux/videodev2.h> > #include <string.h> > #include <sys/mman.h> > +#include <sys/socket.h> > +#include <sys/types.h> > +#include <unistd.h> > > #include <libcamera/camera.h> > #include <libcamera/object.h> > @@ -451,6 +454,11 @@ int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg) > > currentBuf_ = (currentBuf_ + 1) % bufferCount_; > > + char tmp; Same here, s/tmp/data/ > + int ret = ::recv(sockfd_, &tmp, 1, 0); > + if (ret < 1) > + return -EBUSY; This should really not happen as we've already acquired the semaphore, which should guarantee the read will never fail, right ? In what conditions do you expect this to happen, and is EBUSY the best error code ? > + > return 0; > } > > @@ -529,6 +537,12 @@ int V4L2CameraProxy::ioctl(unsigned long request, void *arg) > return ret; > } > > +void V4L2CameraProxy::bind(int sockfds[2]) > +{ > + sockfd_ = sockfds[0]; > + vcam_->bind(sockfds[1]); > +} > + > struct PixelFormatPlaneInfo { > unsigned int bitsPerPixel; > unsigned int hSubSampling; > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h > index af9f9bbe..251b14a8 100644 > --- a/src/v4l2/v4l2_camera_proxy.h > +++ b/src/v4l2/v4l2_camera_proxy.h > @@ -33,6 +33,8 @@ public: > > int ioctl(unsigned long request, void *arg); > > + void bind(int sockfds[2]); > + > private: > bool validateBufferType(uint32_t type); > bool validateMemoryType(uint32_t memory); > @@ -77,6 +79,8 @@ private: > std::map<void *, unsigned int> mmaps_; > > std::unique_ptr<V4L2Camera> vcam_; > + > + int sockfd_; > }; > > #endif /* __V4L2_CAMERA_PROXY_H__ */ > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > index 2338a0ee..b6e84866 100644 > --- a/src/v4l2/v4l2_compat_manager.cpp > +++ b/src/v4l2/v4l2_compat_manager.cpp > @@ -12,8 +12,8 @@ > #include <map> > #include <stdarg.h> > #include <string.h> > -#include <sys/eventfd.h> > #include <sys/mman.h> > +#include <sys/socket.h> > #include <sys/stat.h> > #include <sys/sysmacros.h> > #include <sys/types.h> > @@ -155,15 +155,25 @@ int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mod > if (ret < 0) > return ret; > > - int efd = eventfd(0, oflag & (O_CLOEXEC | O_NONBLOCK)); > - if (efd < 0) { > + int sockets[2]; > + ret = socketpair(AF_UNIX, > + SOCK_STREAM > + | ((oflag & O_CLOEXEC) ? SOCK_CLOEXEC : 0) > + | ((oflag & O_NONBLOCK) ? SOCK_NONBLOCK : 0), > + 0, sockets); Have you checked if AF_UNIX SOCK_STREAM supports out-of-band data ? I suspect not, as it doesn't :-( I've only found out myself now. It seems there are very few options to receive a POLLPRI from a socket, the most straightforward is IPv4 TCP, but that requires a real client/server setup (socketpair() only supports AF_UNIX and AF_TIPC, and neither of them seem to support POLLPRI), and that doesn't seem like a good idea. I see multiple options here: - Adding POLLPRI support to an easy to use kernel facility. eventfd seems to be the best candidate, and this has been attempted before. See https://lore.kernel.org/lkml/1444873328-8466-2-git-send-email-dhobsong@igel.co.jp/ for instance. It is however not clear if such an approach would be accepted, and it would only be usable with future kernels. - Figuring out a way to create a pair of TCP sockets without having to bind to a local port, listen and connect. I don't think this is feasible, but I could be wrong. - Emulate select(), poll(), ppoll() and epoll() (and possibly other poll-related functions). That's a lot of work, especially for epoll(). - Not supporting POLLPRI, and thus not supporting the V4L2 events API. Maybe that's acceptable as a best-effort implementation ? I'm tempted to say we sohuld explore the first option, as it would be the simplest one. It would mean that support for V4L2 events wouldn't be available with kernels older than v5.9 at the earliest (if we can get this merged within one kernel release), but by the time use cases appear for V4L2 events with the libcamera V4L2 adaptation layer, maybe that will be acceptable ? Or maybe we should give up on V4L2 events directly ? > + if (ret) { > + int err = errno; > proxy->close(); > - return efd; > + errno = err; > + > + return ret; > } > > - devices_.emplace(efd, proxy); > + proxy->bind(sockets); > + > + devices_.emplace(sockets[0], proxy); > > - return efd; > + return sockets[0]; > } > > int V4L2CompatManager::dup(int oldfd)
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index 4da01a45..ecbc87ec 100644 --- a/src/v4l2/v4l2_camera.cpp +++ b/src/v4l2/v4l2_camera.cpp @@ -8,6 +8,9 @@ #include "v4l2_camera.h" #include <errno.h> +#include <sys/socket.h> +#include <sys/types.h> +#include <unistd.h> #include "libcamera/internal/log.h" @@ -51,6 +54,13 @@ void V4L2Camera::close() bufferAllocator_ = nullptr; camera_->release(); + + ::close(sockfd_); +} + +void V4L2Camera::bind(int sockfd) +{ + sockfd_ = sockfd; } void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig) @@ -85,6 +95,9 @@ void V4L2Camera::requestComplete(Request *request) bufferLock_.unlock(); bufferSema_.release(); + + char tmp = 0; + ::send(sockfd_, &tmp, 1, 0); } int V4L2Camera::configure(StreamConfiguration *streamConfigOut, diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h index 303eda44..1ad040c1 100644 --- a/src/v4l2/v4l2_camera.h +++ b/src/v4l2/v4l2_camera.h @@ -39,6 +39,7 @@ public: int open(); void close(); + void bind(int sockfd); void getStreamConfig(StreamConfiguration *streamConfig); std::vector<Buffer> completedBuffers(); @@ -70,6 +71,8 @@ private: std::deque<std::unique_ptr<Request>> pendingRequests_; std::deque<std::unique_ptr<Buffer>> completedBuffers_; + + int sockfd_; }; #endif /* __V4L2_CAMERA_H__ */ diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index f84982a2..c9d7945a 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -13,6 +13,9 @@ #include <linux/videodev2.h> #include <string.h> #include <sys/mman.h> +#include <sys/socket.h> +#include <sys/types.h> +#include <unistd.h> #include <libcamera/camera.h> #include <libcamera/object.h> @@ -451,6 +454,11 @@ int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg) currentBuf_ = (currentBuf_ + 1) % bufferCount_; + char tmp; + int ret = ::recv(sockfd_, &tmp, 1, 0); + if (ret < 1) + return -EBUSY; + return 0; } @@ -529,6 +537,12 @@ int V4L2CameraProxy::ioctl(unsigned long request, void *arg) return ret; } +void V4L2CameraProxy::bind(int sockfds[2]) +{ + sockfd_ = sockfds[0]; + vcam_->bind(sockfds[1]); +} + struct PixelFormatPlaneInfo { unsigned int bitsPerPixel; unsigned int hSubSampling; diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h index af9f9bbe..251b14a8 100644 --- a/src/v4l2/v4l2_camera_proxy.h +++ b/src/v4l2/v4l2_camera_proxy.h @@ -33,6 +33,8 @@ public: int ioctl(unsigned long request, void *arg); + void bind(int sockfds[2]); + private: bool validateBufferType(uint32_t type); bool validateMemoryType(uint32_t memory); @@ -77,6 +79,8 @@ private: std::map<void *, unsigned int> mmaps_; std::unique_ptr<V4L2Camera> vcam_; + + int sockfd_; }; #endif /* __V4L2_CAMERA_PROXY_H__ */ diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index 2338a0ee..b6e84866 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -12,8 +12,8 @@ #include <map> #include <stdarg.h> #include <string.h> -#include <sys/eventfd.h> #include <sys/mman.h> +#include <sys/socket.h> #include <sys/stat.h> #include <sys/sysmacros.h> #include <sys/types.h> @@ -155,15 +155,25 @@ int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mod if (ret < 0) return ret; - int efd = eventfd(0, oflag & (O_CLOEXEC | O_NONBLOCK)); - if (efd < 0) { + int sockets[2]; + ret = socketpair(AF_UNIX, + SOCK_STREAM + | ((oflag & O_CLOEXEC) ? SOCK_CLOEXEC : 0) + | ((oflag & O_NONBLOCK) ? SOCK_NONBLOCK : 0), + 0, sockets); + if (ret) { + int err = errno; proxy->close(); - return efd; + errno = err; + + return ret; } - devices_.emplace(efd, proxy); + proxy->bind(sockets); + + devices_.emplace(sockets[0], proxy); - return efd; + return sockets[0]; } int V4L2CompatManager::dup(int oldfd)
To support polling, we need to be able to signal when data is available to be read (POLLIN), as well as events (POLLPRI). To allow signaling POLLPRI specifically, change eventfd to a pair of sockets. After adding the socketpair, use the socketpair to signal POLLIN by writing a byte to the socket, and read the byte away upon VIDIOC_DQBUF to clear the handled POLLIN. Although POLLPRI is not actually implemented, since we don't support events, the facilities are in place. The implementation for signaling and clearing POLLPRI is the same as for POLLIN, except that we would send and receive an out-of-band byte through the socket. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/v4l2/v4l2_camera.cpp | 13 +++++++++++++ src/v4l2/v4l2_camera.h | 3 +++ src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++++++ src/v4l2/v4l2_camera_proxy.h | 4 ++++ src/v4l2/v4l2_compat_manager.cpp | 22 ++++++++++++++++------ 5 files changed, 50 insertions(+), 6 deletions(-)