[libcamera-devel,v2,6/7] v4l2: v4l2_compat: Add eventfd signaling to support polling

Message ID 20200605090106.15424-7-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • Support qv4l2 with v4l2-compat
Related show

Commit Message

Paul Elder June 5, 2020, 9:01 a.m. UTC
To support polling, we need to be able to signal when data is
available to be read (POLLIN), as well as events (POLLPRI). Add the
necessary calls to eventfd to allow signaling POLLIN. We signal POLLIN
by writing writing to the eventfd, and clear it by reading from the
eventfd, upon VIDIOC_DQBUF.

Note that eventfd does not support signaling POLLPRI, so we don't yet
support V4L2 events.

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

---
Changes in v2: use eventfd instead of socket
---
 src/v4l2/v4l2_camera.cpp         |  9 +++++++++
 src/v4l2/v4l2_camera.h           |  3 +++
 src/v4l2/v4l2_camera_proxy.cpp   | 10 ++++++++++
 src/v4l2/v4l2_camera_proxy.h     |  4 ++++
 src/v4l2/v4l2_compat_manager.cpp |  7 ++++++-
 5 files changed, 32 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart June 5, 2020, 10:16 a.m. UTC | #1
Hi Paul,

Thank you for the patch.

On Fri, Jun 05, 2020 at 06:01:05PM +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). Add the
> necessary calls to eventfd to allow signaling POLLIN. We signal POLLIN
> by writing writing to the eventfd, and clear it by reading from the
> eventfd, upon VIDIOC_DQBUF.
> 
> Note that eventfd does not support signaling POLLPRI, so we don't yet
> support V4L2 events.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes in v2: use eventfd instead of socket
> ---
>  src/v4l2/v4l2_camera.cpp         |  9 +++++++++
>  src/v4l2/v4l2_camera.h           |  3 +++
>  src/v4l2/v4l2_camera_proxy.cpp   | 10 ++++++++++
>  src/v4l2/v4l2_camera_proxy.h     |  4 ++++
>  src/v4l2/v4l2_compat_manager.cpp |  7 ++++++-
>  5 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 4da01a45..3c369328 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -8,6 +8,7 @@
>  #include "v4l2_camera.h"
>  
>  #include <errno.h>
> +#include <unistd.h>
>  
>  #include "libcamera/internal/log.h"
>  
> @@ -53,6 +54,11 @@ void V4L2Camera::close()
>  	camera_->release();
>  }
>  
> +void V4L2Camera::bind(int efd)
> +{
> +	efd_ = efd;
> +}
> +
>  void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
>  {
>  	*streamConfig = config_->at(0);
> @@ -84,6 +90,9 @@ void V4L2Camera::requestComplete(Request *request)
>  	completedBuffers_.push_back(std::move(metadata));
>  	bufferLock_.unlock();
>  
> +	uint64_t data = 1;
> +	::write(efd_, &data, sizeof(data));
> +
>  	bufferSema_.release();
>  }
>  
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 303eda44..33f5eb02 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 efd);
>  	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 efd_;
>  };
>  
>  #endif /* __V4L2_CAMERA_H__ */
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index cbe9e026..7ee4c0cb 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -13,6 +13,7 @@
>  #include <linux/videodev2.h>
>  #include <string.h>
>  #include <sys/mman.h>
> +#include <unistd.h>
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/object.h>
> @@ -451,6 +452,9 @@ int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
>  
>  	currentBuf_ = (currentBuf_ + 1) % bufferCount_;
>  
> +	uint64_t data;
> +	::read(efd_, &data, sizeof(data));
> +
>  	return 0;
>  }
>  
> @@ -529,6 +533,12 @@ int V4L2CameraProxy::ioctl(unsigned long request, void *arg)
>  	return ret;
>  }
>  
> +void V4L2CameraProxy::bind(int fd)
> +{
> +	efd_ = fd;
> +	vcam_->bind(fd);
> +}
> +
>  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..7c65c886 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 fd);
> +
>  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 efd_;
>  };
>  
>  #endif /* __V4L2_CAMERA_PROXY_H__ */
> diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> index 2338a0ee..56533c4f 100644
> --- a/src/v4l2/v4l2_compat_manager.cpp
> +++ b/src/v4l2/v4l2_compat_manager.cpp
> @@ -155,12 +155,17 @@ 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));
> +	int efd = eventfd(0, EFD_SEMAPHORE |
> +			     ((oflag & O_CLOEXEC) ? EFD_CLOEXEC : 0) |
> +			     ((oflag & O_NONBLOCK) ? EFD_NONBLOCK : 0));
>  	if (efd < 0) {
> +		int err = errno;
>  		proxy->close();
> +		errno = err;
>  		return efd;
>  	}
>  
> +	proxy->bind(efd);
>  	devices_.emplace(efd, proxy);
>  
>  	return efd;
Kieran Bingham June 8, 2020, 7:06 p.m. UTC | #2
Hi Paul,

This patch has introduced some warnings from coverity scan:

On 05/06/2020 10:01, 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). Add the
> necessary calls to eventfd to allow signaling POLLIN. We signal POLLIN
> by writing writing to the eventfd, and clear it by reading from the
> eventfd, upon VIDIOC_DQBUF.
> 
> Note that eventfd does not support signaling POLLPRI, so we don't yet
> support V4L2 events.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2: use eventfd instead of socket
> ---
>  src/v4l2/v4l2_camera.cpp         |  9 +++++++++
>  src/v4l2/v4l2_camera.h           |  3 +++
>  src/v4l2/v4l2_camera_proxy.cpp   | 10 ++++++++++
>  src/v4l2/v4l2_camera_proxy.h     |  4 ++++
>  src/v4l2/v4l2_compat_manager.cpp |  7 ++++++-
>  5 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 4da01a45..3c369328 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -8,6 +8,7 @@
>  #include "v4l2_camera.h"
>  
>  #include <errno.h>
> +#include <unistd.h>
>  
>  #include "libcamera/internal/log.h"
>  
> @@ -53,6 +54,11 @@ void V4L2Camera::close()
>  	camera_->release();
>  }
>  
> +void V4L2Camera::bind(int efd)
> +{
> +	efd_ = efd;
> +}
> +
>  void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
>  {
>  	*streamConfig = config_->at(0);
> @@ -84,6 +90,9 @@ void V4L2Camera::requestComplete(Request *request)
>  	completedBuffers_.push_back(std::move(metadata));
>  	bufferLock_.unlock();
>  
> +	uint64_t data = 1;
> +	::write(efd_, &data, sizeof(data));

** CID 290743:  Error handling issues  (CHECKED_RETURN)
/home/linuxembedded/iob/libcamera/libcamera-daily/src/v4l2/v4l2_camera.cpp:
94 in V4L2Camera::requestComplete(libcamera::Request *)()

Patch

diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
index 4da01a45..3c369328 100644
--- a/src/v4l2/v4l2_camera.cpp
+++ b/src/v4l2/v4l2_camera.cpp
@@ -8,6 +8,7 @@ 
 #include "v4l2_camera.h"
 
 #include <errno.h>
+#include <unistd.h>
 
 #include "libcamera/internal/log.h"
 
@@ -53,6 +54,11 @@  void V4L2Camera::close()
 	camera_->release();
 }
 
+void V4L2Camera::bind(int efd)
+{
+	efd_ = efd;
+}
+
 void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
 {
 	*streamConfig = config_->at(0);
@@ -84,6 +90,9 @@  void V4L2Camera::requestComplete(Request *request)
 	completedBuffers_.push_back(std::move(metadata));
 	bufferLock_.unlock();
 
+	uint64_t data = 1;
+	::write(efd_, &data, sizeof(data));
+
 	bufferSema_.release();
 }
 
diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
index 303eda44..33f5eb02 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 efd);
 	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 efd_;
 };
 
 #endif /* __V4L2_CAMERA_H__ */
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index cbe9e026..7ee4c0cb 100644
--- a/src/v4l2/v4l2_camera_proxy.cpp
+++ b/src/v4l2/v4l2_camera_proxy.cpp
@@ -13,6 +13,7 @@ 
 #include <linux/videodev2.h>
 #include <string.h>
 #include <sys/mman.h>
+#include <unistd.h>
 
 #include <libcamera/camera.h>
 #include <libcamera/object.h>
@@ -451,6 +452,9 @@  int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
 
 	currentBuf_ = (currentBuf_ + 1) % bufferCount_;
 
+	uint64_t data;
+	::read(efd_, &data, sizeof(data));
+
 	return 0;
 }
 
@@ -529,6 +533,12 @@  int V4L2CameraProxy::ioctl(unsigned long request, void *arg)
 	return ret;
 }
 
+void V4L2CameraProxy::bind(int fd)
+{
+	efd_ = fd;
+	vcam_->bind(fd);
+}
+
 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..7c65c886 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 fd);
+
 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 efd_;
 };
 
 #endif /* __V4L2_CAMERA_PROXY_H__ */
diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
index 2338a0ee..56533c4f 100644
--- a/src/v4l2/v4l2_compat_manager.cpp
+++ b/src/v4l2/v4l2_compat_manager.cpp
@@ -155,12 +155,17 @@  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));
+	int efd = eventfd(0, EFD_SEMAPHORE |
+			     ((oflag & O_CLOEXEC) ? EFD_CLOEXEC : 0) |
+			     ((oflag & O_NONBLOCK) ? EFD_NONBLOCK : 0));
 	if (efd < 0) {
+		int err = errno;
 		proxy->close();
+		errno = err;
 		return efd;
 	}
 
+	proxy->bind(efd);
 	devices_.emplace(efd, proxy);
 
 	return efd;