| Message ID | 20200623190836.53446-17-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Wed, Jun 24, 2020 at 04:08:30AM +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 v3: > - don't notify all with the lock held > > Changes in v2: > - remove mutex for isRunning_ > - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in > the dqbuf ioctl handler > --- > src/v4l2/v4l2_camera.cpp | 30 ++++++++++++++++++++++++++++-- > src/v4l2/v4l2_camera.h | 9 +++++++-- > src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++-- > 3 files changed, 44 insertions(+), 6 deletions(-) > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index 177b1ea..bdf0036 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,11 @@ 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 +148,7 @@ int V4L2Camera::allocBuffers(unsigned int count) > void V4L2Camera::freeBuffers() > { > Stream *stream = *camera_->streams().begin(); > + This seems unrelated. > bufferAllocator_->free(stream); > } > > @@ -193,6 +198,7 @@ int V4L2Camera::streamOff() > return ret == -EACCES ? -EBUSY : ret; > > isRunning_ = false; > + bufferCV_.notify_all(); > > return 0; > } > @@ -228,6 +234,26 @@ int V4L2Camera::qbuf(unsigned int index) > return 0; > } > > +void V4L2Camera::waitForBufferAvailable() > +{ > + MutexLocker locker(bufferMutex_); > + bufferCV_.wait(locker, [&] { > + return bufferAvailableCount_ >= 1 || !isRunning_; > + }); The correct indentation would be bufferCV_.wait(locker, [&] { return bufferAvailableCount_ >= 1 || !isRunning_; }); (indenting lambdas is not very nice :-S) > + 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 ee53c2b..515e906 100644 > --- a/src/v4l2/v4l2_camera.h > +++ b/src/v4l2/v4l2_camera.h > @@ -57,9 +57,10 @@ public: > > int qbuf(unsigned int index); > > - bool isRunning(); > + void waitForBufferAvailable(); > + bool isBufferAvailable(); > > - Semaphore bufferSema_; > + bool isRunning(); > > private: > void requestComplete(Request *request); > @@ -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 fa58585..8420e23 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -594,10 +594,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg) > return -EINVAL; > > if (!(file->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 waitForBufferAvailable. s/waitForBufferAvailable/waitForBufferAvailable()/ > + */ > + if (!vcam_->isRunning()) > + return -EINVAL; I think there's a small race here in the non-blocking case, as isBufferAvailable() could decrement the buffer count and the !running case could ignore that. I'm not sure it would have consequences as we would be stopping streaming anyway in that case, but regardless, patch 22/22 will address that, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> as I'm not concerned about bisection breakages before this series lands. > + > updateBuffers(); > > struct v4l2_buffer &buf = buffers_[currentBuf_];
Hi again, On Wed, Jun 24, 2020 at 02:05:25AM +0300, Laurent Pinchart wrote: > On Wed, Jun 24, 2020 at 04:08:30AM +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 v3: > > - don't notify all with the lock held > > > > Changes in v2: > > - remove mutex for isRunning_ > > - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in > > the dqbuf ioctl handler > > --- > > src/v4l2/v4l2_camera.cpp | 30 ++++++++++++++++++++++++++++-- > > src/v4l2/v4l2_camera.h | 9 +++++++-- > > src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++-- > > 3 files changed, 44 insertions(+), 6 deletions(-) > > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > > index 177b1ea..bdf0036 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,11 @@ 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 +148,7 @@ int V4L2Camera::allocBuffers(unsigned int count) > > void V4L2Camera::freeBuffers() > > { > > Stream *stream = *camera_->streams().begin(); > > + > > This seems unrelated. > > > bufferAllocator_->free(stream); > > } > > > > @@ -193,6 +198,7 @@ int V4L2Camera::streamOff() > > return ret == -EACCES ? -EBUSY : ret; > > > > isRunning_ = false; > > + bufferCV_.notify_all(); > > > > return 0; > > } > > @@ -228,6 +234,26 @@ int V4L2Camera::qbuf(unsigned int index) > > return 0; > > } > > > > +void V4L2Camera::waitForBufferAvailable() > > +{ > > + MutexLocker locker(bufferMutex_); > > + bufferCV_.wait(locker, [&] { > > + return bufferAvailableCount_ >= 1 || !isRunning_; > > + }); > > The correct indentation would be > > bufferCV_.wait(locker, [&] { > return bufferAvailableCount_ >= 1 || !isRunning_; > }); > > (indenting lambdas is not very nice :-S) > > > + 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 ee53c2b..515e906 100644 > > --- a/src/v4l2/v4l2_camera.h > > +++ b/src/v4l2/v4l2_camera.h > > @@ -57,9 +57,10 @@ public: > > > > int qbuf(unsigned int index); > > > > - bool isRunning(); > > + void waitForBufferAvailable(); > > + bool isBufferAvailable(); > > > > - Semaphore bufferSema_; > > + bool isRunning(); > > > > private: > > void requestComplete(Request *request); > > @@ -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 fa58585..8420e23 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -594,10 +594,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg) > > return -EINVAL; > > > > if (!(file->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 waitForBufferAvailable. > > s/waitForBufferAvailable/waitForBufferAvailable()/ > > > + */ > > + if (!vcam_->isRunning()) > > + return -EINVAL; > > I think there's a small race here in the non-blocking case, as > isBufferAvailable() could decrement the buffer count and the !running > case could ignore that. I'm not sure it would have consequences as we > would be stopping streaming anyway in that case, but regardless, patch > 22/22 will address that, so > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > as I'm not concerned about bisection breakages before this series lands. I take this back, see explanation in my review of 22/22. > > + > > updateBuffers(); > > > > struct v4l2_buffer &buf = buffers_[currentBuf_];
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index 177b1ea..bdf0036 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,11 @@ 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 +148,7 @@ int V4L2Camera::allocBuffers(unsigned int count) void V4L2Camera::freeBuffers() { Stream *stream = *camera_->streams().begin(); + bufferAllocator_->free(stream); } @@ -193,6 +198,7 @@ int V4L2Camera::streamOff() return ret == -EACCES ? -EBUSY : ret; isRunning_ = false; + bufferCV_.notify_all(); return 0; } @@ -228,6 +234,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 ee53c2b..515e906 100644 --- a/src/v4l2/v4l2_camera.h +++ b/src/v4l2/v4l2_camera.h @@ -57,9 +57,10 @@ public: int qbuf(unsigned int index); - bool isRunning(); + void waitForBufferAvailable(); + bool isBufferAvailable(); - Semaphore bufferSema_; + bool isRunning(); private: void requestComplete(Request *request); @@ -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 fa58585..8420e23 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -594,10 +594,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg) return -EINVAL; if (!(file->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 waitForBufferAvailable. + */ + 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 v3: - don't notify all with the lock held Changes in v2: - remove mutex for isRunning_ - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in the dqbuf ioctl handler --- src/v4l2/v4l2_camera.cpp | 30 ++++++++++++++++++++++++++++-- src/v4l2/v4l2_camera.h | 9 +++++++-- src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++-- 3 files changed, 44 insertions(+), 6 deletions(-)