[libcamera-devel] [PATCH 5/5] v4l2: v4l2_compat: Add sockets to support polling

[Laurent Pinchart]

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 at 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)

-- 
Regards,

Laurent Pinchart