Message ID | 20200616131244.70308-12-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Paul, Thank you for the patch. On Tue, Jun 16, 2020 at 10:12:40PM +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> > --- > src/v4l2/v4l2_camera.cpp | 32 ++++++++++++++++++++++++++++++-- > src/v4l2/v4l2_camera.h | 8 +++++++- > src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++-- > 3 files changed, 46 insertions(+), 5 deletions(-) > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index fdbf461..f0ec54b 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, > @@ -151,6 +153,7 @@ int V4L2Camera::allocBuffers(unsigned int count) > void V4L2Camera::freeBuffers() > { > Stream *stream = *camera_->streams().begin(); > + > bufferAllocator_->free(stream); > } > > @@ -168,6 +171,7 @@ FileDescriptor V4L2Camera::getBufferFd(unsigned int index) > > int V4L2Camera::streamOn() > { > + MutexLocker locker(isRunningMutex_); > if (isRunning_) > return 0; > > @@ -191,6 +195,7 @@ int V4L2Camera::streamOn() > > int V4L2Camera::streamOff() > { > + MutexLocker locker(isRunningMutex_); > /* \todo Restore buffers to reqbufs state? */ > if (!isRunning_) > return 0; > @@ -200,12 +205,15 @@ int V4L2Camera::streamOff() > return ret == -EACCES ? -EBUSY : ret; > > isRunning_ = false; > + bufferCV_.notify_all(); > > return 0; > } > > int V4L2Camera::qbuf(unsigned int index) > { > + MutexLocker locker(isRunningMutex_); > + > std::unique_ptr<Request> request = > std::unique_ptr<Request>(camera_->createRequest(index)); > if (!request) { > @@ -234,3 +242,23 @@ 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; > +} > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h > index 30114ed..81dcbe2 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(); > > private: > void requestComplete(Request *request); > @@ -65,6 +66,7 @@ private: > std::shared_ptr<Camera> camera_; > std::unique_ptr<CameraConfiguration> config_; > > + Mutex isRunningMutex_; Why do you use a separate mutex for this, especially given that V4L2Camera::waitForBufferAvailable() accesses isRunning_ using bufferMutex_, not isRunningMutex_ ? > bool isRunning_; > > std::mutex bufferLock_; > @@ -74,6 +76,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 45e4656..079961a 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -648,10 +648,17 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg) > return -EINVAL; > > if (!nonBlockingFds_[fd]) > - 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 (!streaming_) > + return -EINVAL; > + > updateBuffers(); > > struct v4l2_buffer &buf = buffers_[currentBuf_];
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index fdbf461..f0ec54b 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, @@ -151,6 +153,7 @@ int V4L2Camera::allocBuffers(unsigned int count) void V4L2Camera::freeBuffers() { Stream *stream = *camera_->streams().begin(); + bufferAllocator_->free(stream); } @@ -168,6 +171,7 @@ FileDescriptor V4L2Camera::getBufferFd(unsigned int index) int V4L2Camera::streamOn() { + MutexLocker locker(isRunningMutex_); if (isRunning_) return 0; @@ -191,6 +195,7 @@ int V4L2Camera::streamOn() int V4L2Camera::streamOff() { + MutexLocker locker(isRunningMutex_); /* \todo Restore buffers to reqbufs state? */ if (!isRunning_) return 0; @@ -200,12 +205,15 @@ int V4L2Camera::streamOff() return ret == -EACCES ? -EBUSY : ret; isRunning_ = false; + bufferCV_.notify_all(); return 0; } int V4L2Camera::qbuf(unsigned int index) { + MutexLocker locker(isRunningMutex_); + std::unique_ptr<Request> request = std::unique_ptr<Request>(camera_->createRequest(index)); if (!request) { @@ -234,3 +242,23 @@ 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; +} diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h index 30114ed..81dcbe2 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(); private: void requestComplete(Request *request); @@ -65,6 +66,7 @@ private: std::shared_ptr<Camera> camera_; std::unique_ptr<CameraConfiguration> config_; + Mutex isRunningMutex_; bool isRunning_; std::mutex bufferLock_; @@ -74,6 +76,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 45e4656..079961a 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -648,10 +648,17 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg) return -EINVAL; if (!nonBlockingFds_[fd]) - 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 (!streaming_) + 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> --- src/v4l2/v4l2_camera.cpp | 32 ++++++++++++++++++++++++++++++-- src/v4l2/v4l2_camera.h | 8 +++++++- src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++-- 3 files changed, 46 insertions(+), 5 deletions(-)