Message ID | 20200619054123.19052-13-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Paul, Thank you for the patch. On Fri, Jun 19, 2020 at 02:41:18PM +0900, Paul Elder wrote: > In V4L2, a blocked dqbuf should not not also block a streamoff. This > means that on streamoff, the blocked dqbuf must return (with error). We > cannot do this with the libcamera semaphore, so pull out the necessary > components of a semaphore, and put them into V4L2Camera, so that dqbuf > from V4L2CameraProxy can wait on a disjunct condition of the > availability of the semaphore or the stopping of the stream. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v2: > - remove mutex for isRunning_ > - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in > the dqbuf ioctl handler > --- > src/v4l2/v4l2_camera.cpp | 28 ++++++++++++++++++++++++++-- > src/v4l2/v4l2_camera.h | 7 ++++++- > src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++-- > 3 files changed, 41 insertions(+), 5 deletions(-) > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index 177b1ea..99d34b9 100644 > --- a/src/v4l2/v4l2_camera.cpp > +++ b/src/v4l2/v4l2_camera.cpp > @@ -18,7 +18,7 @@ LOG_DECLARE_CATEGORY(V4L2Compat); > > V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera) > : camera_(camera), isRunning_(false), bufferAllocator_(nullptr), > - efd_(-1) > + efd_(-1), bufferAvailableCount_(0) > { > camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete); > } > @@ -100,7 +100,9 @@ void V4L2Camera::requestComplete(Request *request) > if (ret != sizeof(data)) > LOG(V4L2Compat, Error) << "Failed to signal eventfd POLLIN"; > > - bufferSema_.release(); > + MutexLocker locker(bufferMutex_); > + bufferAvailableCount_++; > + bufferCV_.notify_all(); Calling notify_all() with the lock held will hinder performances. See https://en.cppreference.com/w/cpp/thread/condition_variable/notify_all. { MutexLocker locker(bufferMutex_); bufferAvailableCount_++; } bufferCV_.notify_all(); > } > > int V4L2Camera::configure(StreamConfiguration *streamConfigOut, > @@ -144,6 +146,7 @@ int V4L2Camera::allocBuffers(unsigned int count) > void V4L2Camera::freeBuffers() > { > Stream *stream = *camera_->streams().begin(); > + > bufferAllocator_->free(stream); > } > > @@ -193,6 +196,7 @@ int V4L2Camera::streamOff() > return ret == -EACCES ? -EBUSY : ret; > > isRunning_ = false; > + bufferCV_.notify_all(); Isn't this racy, can't the compiler (or the CPU) reorder these two lines ? I think you need to lock access to isRunning_ with bufferMutex_. On the other hand, if we want to support multi-threaded applications, we will have to add locking to the V4L2 compat layer. This will likely be done with a lock in V4L2CameraProxy, taken in V4L2CameraProxy::ioctl(). isRunning_ would thus be protected by that lock, so we could skip taking the bufferMutex_ here. I think I'd skip it for now, otherwise we would need to use bufferMutex_ to lock other accesses to isRunning_, and it would become messy. Now that I've written this, it seems being thread-safe would be quite simple with the V4L2CameraProxy lock. You would just need to be careful to unlock it before calling V4L2Camera::waitForBufferAvailable(), and relock it right after. Could you give it a try on top of this series ? > > return 0; > } > @@ -228,6 +232,26 @@ int V4L2Camera::qbuf(unsigned int index) > return 0; > } > > +void V4L2Camera::waitForBufferAvailable() > +{ > + MutexLocker locker(bufferMutex_); > + bufferCV_.wait(locker, [&] { > + return bufferAvailableCount_ >= 1 || !isRunning_; > + }); > + if (isRunning_) > + bufferAvailableCount_--; > +} > + > +bool V4L2Camera::isBufferAvailable() > +{ > + MutexLocker locker(bufferMutex_); > + if (bufferAvailableCount_ < 1) > + return false; > + > + bufferAvailableCount_--; > + return true; > +} > + > bool V4L2Camera::isRunning() > { > return isRunning_; > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h > index c157a80..515e906 100644 > --- a/src/v4l2/v4l2_camera.h > +++ b/src/v4l2/v4l2_camera.h > @@ -57,7 +57,8 @@ public: > > int qbuf(unsigned int index); > > - Semaphore bufferSema_; > + void waitForBufferAvailable(); > + bool isBufferAvailable(); > > bool isRunning(); > > @@ -76,6 +77,10 @@ private: > std::deque<std::unique_ptr<Buffer>> completedBuffers_; > > int efd_; > + > + Mutex bufferMutex_; > + std::condition_variable bufferCV_; > + unsigned int bufferAvailableCount_; > }; > > #endif /* __V4L2_CAMERA_H__ */ > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index ea9222e..2723450 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -595,10 +595,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg) > return -EINVAL; > > if (!(cf->nonBlocking())) > - vcam_->bufferSema_.acquire(); > - else if (!vcam_->bufferSema_.tryAcquire()) > + vcam_->waitForBufferAvailable(); > + else if (!vcam_->isBufferAvailable()) > return -EAGAIN; > > + /* > + * We need to check here again in case stream was turned off while we > + * were blocked on dqbuf. s/dqbuf/waitForBufferAvailable()/ ? I'm not sure this is checking "again". Well, technically it is, but it's really about checking which of the two conditions is true. > + */ > + if (!vcam_->isRunning()) > + return -EINVAL; > + > updateBuffers(); > > struct v4l2_buffer &buf = buffers_[currentBuf_];
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index 177b1ea..99d34b9 100644 --- a/src/v4l2/v4l2_camera.cpp +++ b/src/v4l2/v4l2_camera.cpp @@ -18,7 +18,7 @@ LOG_DECLARE_CATEGORY(V4L2Compat); V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera) : camera_(camera), isRunning_(false), bufferAllocator_(nullptr), - efd_(-1) + efd_(-1), bufferAvailableCount_(0) { camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete); } @@ -100,7 +100,9 @@ void V4L2Camera::requestComplete(Request *request) if (ret != sizeof(data)) LOG(V4L2Compat, Error) << "Failed to signal eventfd POLLIN"; - bufferSema_.release(); + MutexLocker locker(bufferMutex_); + bufferAvailableCount_++; + bufferCV_.notify_all(); } int V4L2Camera::configure(StreamConfiguration *streamConfigOut, @@ -144,6 +146,7 @@ int V4L2Camera::allocBuffers(unsigned int count) void V4L2Camera::freeBuffers() { Stream *stream = *camera_->streams().begin(); + bufferAllocator_->free(stream); } @@ -193,6 +196,7 @@ int V4L2Camera::streamOff() return ret == -EACCES ? -EBUSY : ret; isRunning_ = false; + bufferCV_.notify_all(); return 0; } @@ -228,6 +232,26 @@ int V4L2Camera::qbuf(unsigned int index) return 0; } +void V4L2Camera::waitForBufferAvailable() +{ + MutexLocker locker(bufferMutex_); + bufferCV_.wait(locker, [&] { + return bufferAvailableCount_ >= 1 || !isRunning_; + }); + if (isRunning_) + bufferAvailableCount_--; +} + +bool V4L2Camera::isBufferAvailable() +{ + MutexLocker locker(bufferMutex_); + if (bufferAvailableCount_ < 1) + return false; + + bufferAvailableCount_--; + return true; +} + bool V4L2Camera::isRunning() { return isRunning_; diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h index c157a80..515e906 100644 --- a/src/v4l2/v4l2_camera.h +++ b/src/v4l2/v4l2_camera.h @@ -57,7 +57,8 @@ public: int qbuf(unsigned int index); - Semaphore bufferSema_; + void waitForBufferAvailable(); + bool isBufferAvailable(); bool isRunning(); @@ -76,6 +77,10 @@ private: std::deque<std::unique_ptr<Buffer>> completedBuffers_; int efd_; + + Mutex bufferMutex_; + std::condition_variable bufferCV_; + unsigned int bufferAvailableCount_; }; #endif /* __V4L2_CAMERA_H__ */ diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index ea9222e..2723450 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -595,10 +595,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg) return -EINVAL; if (!(cf->nonBlocking())) - vcam_->bufferSema_.acquire(); - else if (!vcam_->bufferSema_.tryAcquire()) + vcam_->waitForBufferAvailable(); + else if (!vcam_->isBufferAvailable()) return -EAGAIN; + /* + * We need to check here again in case stream was turned off while we + * were blocked on dqbuf. + */ + if (!vcam_->isRunning()) + return -EINVAL; + updateBuffers(); struct v4l2_buffer &buf = buffers_[currentBuf_];
In V4L2, a blocked dqbuf should not not also block a streamoff. This means that on streamoff, the blocked dqbuf must return (with error). We cannot do this with the libcamera semaphore, so pull out the necessary components of a semaphore, and put them into V4L2Camera, so that dqbuf from V4L2CameraProxy can wait on a disjunct condition of the availability of the semaphore or the stopping of the stream. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v2: - remove mutex for isRunning_ - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in the dqbuf ioctl handler --- src/v4l2/v4l2_camera.cpp | 28 ++++++++++++++++++++++++++-- src/v4l2/v4l2_camera.h | 7 ++++++- src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++-- 3 files changed, 41 insertions(+), 5 deletions(-)