[{"id":5364,"web_url":"https://patchwork.libcamera.org/comment/5364/","msgid":"<20200623230525.GN5870@pendragon.ideasonboard.com>","date":"2020-06-23T23:05:25","subject":"Re: [libcamera-devel] [PATCH v3 16/22] v4l2: v4l2_camera: Don't use\n\tlibcamera::Semaphore for available buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jun 24, 2020 at 04:08:30AM +0900, Paul Elder wrote:\n> In V4L2, a blocked dqbuf should not not also block a streamoff. This\n> means that on streamoff, the blocked dqbuf must return (with error). We\n> cannot do this with the libcamera semaphore, so pull out the necessary\n> components of a semaphore, and put them into V4L2Camera, so that dqbuf\n> from V4L2CameraProxy can wait on a disjunct condition of the\n> availability of the semaphore or the stopping of the stream.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3:\n> - don't notify all with the lock held\n> \n> Changes in v2:\n> - remove mutex for isRunning_\n> - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in\n> the dqbuf ioctl handler\n> ---\n> src/v4l2/v4l2_camera.cpp | 30 ++++++++++++++++++++++++++++--\n> src/v4l2/v4l2_camera.h | 9 +++++++--\n> src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++--\n> 3 files changed, 44 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 177b1ea..bdf0036 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -18,7 +18,7 @@ LOG_DECLARE_CATEGORY(V4L2Compat);\n> \n> V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> \t: camera_(camera), isRunning_(false), bufferAllocator_(nullptr),\n> -\t efd_(-1)\n> +\t efd_(-1), bufferAvailableCount_(0)\n> {\n> \tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> }\n> @@ -100,7 +100,11 @@ void V4L2Camera::requestComplete(Request *request)\n> \tif (ret != sizeof(data))\n> \t\tLOG(V4L2Compat, Error) << \"Failed to signal eventfd POLLIN\";\n> \n> -\tbufferSema_.release();\n> +\t{\n> +\t\tMutexLocker locker(bufferMutex_);\n> +\t\tbufferAvailableCount_++;\n> +\t}\n> +\tbufferCV_.notify_all();\n> }\n> \n> int V4L2Camera::configure(StreamConfiguration *streamConfigOut,\n> @@ -144,6 +148,7 @@ int V4L2Camera::allocBuffers(unsigned int count)\n> void V4L2Camera::freeBuffers()\n> {\n> \tStream *stream = *camera_->streams().begin();\n> +\n\nThis seems unrelated.\n\n> \tbufferAllocator_->free(stream);\n> }\n> \n> @@ -193,6 +198,7 @@ int V4L2Camera::streamOff()\n> \t\treturn ret == -EACCES ? -EBUSY : ret;\n> \n> \tisRunning_ = false;\n> +\tbufferCV_.notify_all();\n> \n> \treturn 0;\n> }\n> @@ -228,6 +234,26 @@ int V4L2Camera::qbuf(unsigned int index)\n> \treturn 0;\n> }\n> \n> +void V4L2Camera::waitForBufferAvailable()\n> +{\n> +\tMutexLocker locker(bufferMutex_);\n> +\tbufferCV_.wait(locker, [&] {\n> +\t\t\treturn bufferAvailableCount_ >= 1 || !isRunning_;\n> +\t\t\t});\n\nThe correct indentation would be\n\n\tbufferCV_.wait(locker, [&] {\n\t\t\t return bufferAvailableCount_ >= 1 || !isRunning_;\n\t\t });\n\n(indenting lambdas is not very nice :-S)\n\n> +\tif (isRunning_)\n> +\t\tbufferAvailableCount_--;\n> +}\n> +\n> +bool V4L2Camera::isBufferAvailable()\n> +{\n> +\tMutexLocker locker(bufferMutex_);\n> +\tif (bufferAvailableCount_ < 1)\n> +\t\treturn false;\n> +\n> +\tbufferAvailableCount_--;\n> +\treturn true;\n> +}\n> +\n> bool V4L2Camera::isRunning()\n> {\n> \treturn isRunning_;\n> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> index ee53c2b..515e906 100644\n> --- a/src/v4l2/v4l2_camera.h\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -57,9 +57,10 @@ public:\n> \n> \tint qbuf(unsigned int index);\n> \n> -\tbool isRunning();\n> +\tvoid waitForBufferAvailable();\n> +\tbool isBufferAvailable();\n> \n> -\tSemaphore bufferSema_;\n> +\tbool isRunning();\n> \n> private:\n> \tvoid requestComplete(Request *request);\n> @@ -76,6 +77,10 @@ private:\n> \tstd::deque> completedBuffers_;\n> \n> \tint efd_;\n> +\n> +\tMutex bufferMutex_;\n> +\tstd::condition_variable bufferCV_;\n> +\tunsigned int bufferAvailableCount_;\n> };\n> \n> #endif /* __V4L2_CAMERA_H__ */\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index fa58585..8420e23 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -594,10 +594,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> \t\treturn -EINVAL;\n> \n> \tif (!(file->nonBlocking()))\n> -\t\tvcam_->bufferSema_.acquire();\n> -\telse if (!vcam_->bufferSema_.tryAcquire())\n> +\t\tvcam_->waitForBufferAvailable();\n> +\telse if (!vcam_->isBufferAvailable())\n> \t\treturn -EAGAIN;\n> \n> +\t/*\n> +\t * We need to check here again in case stream was turned off while we\n> +\t * were blocked on waitForBufferAvailable.\n\ns/waitForBufferAvailable/waitForBufferAvailable()/\n\n> +\t */\n> +\tif (!vcam_->isRunning())\n> +\t\treturn -EINVAL;\n\nI think there's a small race here in the non-blocking case, as\nisBufferAvailable() could decrement the buffer count and the !running\ncase could ignore that. I'm not sure it would have consequences as we\nwould be stopping streaming anyway in that case, but regardless, patch\n22/22 will address that, so\n\nReviewed-by: Laurent Pinchart \n\nas I'm not concerned about bisection breakages before this series lands.\n\n> +\n> \tupdateBuffers();\n> \n> \tstruct v4l2_buffer &buf = buffers_[currentBuf_];","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8B8F603B9\n\tfor ;\n\tWed, 24 Jun 2020 01:05:53 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C80432A9;\n\tWed, 24 Jun 2020 01:05:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"kD6vbkdD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592953553;\n\tbh=mw3Kkl64C4xRMhl1yqwcrX59Y+X0wbiYzn/4psfYcIw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kD6vbkdD50KgFBGPQpgdxNebp13GRFnCKSXsEZWn8nzkxNI/2xsuLQUHD03/2YvXl\n\tHvBMTeKHWz2zzETL+hT0eZAUPKonRJ2hSyF3X7ojUvQPbauAZBXCHsch6zIY2ixZwX\n\thjUSwz7r7OQEFAbpgUzaRdm688UcrYRzkKbLVGz8=","Date":"Wed, 24 Jun 2020 02:05:25 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200623230525.GN5870@pendragon.ideasonboard.com>","References":"<20200623190836.53446-1-paul.elder@ideasonboard.com>\n\t<20200623190836.53446-17-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200623190836.53446-17-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 16/22] v4l2: v4l2_camera: Don't use\n\tlibcamera::Semaphore for available buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 23 Jun 2020 23:05:54 -0000"}},{"id":5368,"web_url":"https://patchwork.libcamera.org/comment/5368/","msgid":"<20200624000501.GR5870@pendragon.ideasonboard.com>","date":"2020-06-24T00:05:01","subject":"Re: [libcamera-devel] [PATCH v3 16/22] v4l2: v4l2_camera: Don't use\n\tlibcamera::Semaphore for available buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi again,\n\nOn Wed, Jun 24, 2020 at 02:05:25AM +0300, Laurent Pinchart wrote:\n> On Wed, Jun 24, 2020 at 04:08:30AM +0900, Paul Elder wrote:\n> > In V4L2, a blocked dqbuf should not not also block a streamoff. This\n> > means that on streamoff, the blocked dqbuf must return (with error). We\n> > cannot do this with the libcamera semaphore, so pull out the necessary\n> > components of a semaphore, and put them into V4L2Camera, so that dqbuf\n> > from V4L2CameraProxy can wait on a disjunct condition of the\n> > availability of the semaphore or the stopping of the stream.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v3:\n> > - don't notify all with the lock held\n> > \n> > Changes in v2:\n> > - remove mutex for isRunning_\n> > - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in\n> > the dqbuf ioctl handler\n> > ---\n> > src/v4l2/v4l2_camera.cpp | 30 ++++++++++++++++++++++++++++--\n> > src/v4l2/v4l2_camera.h | 9 +++++++--\n> > src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++--\n> > 3 files changed, 44 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > index 177b1ea..bdf0036 100644\n> > --- a/src/v4l2/v4l2_camera.cpp\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -18,7 +18,7 @@ LOG_DECLARE_CATEGORY(V4L2Compat);\n> > \n> > V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> > \t: camera_(camera), isRunning_(false), bufferAllocator_(nullptr),\n> > -\t efd_(-1)\n> > +\t efd_(-1), bufferAvailableCount_(0)\n> > {\n> > \tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> > }\n> > @@ -100,7 +100,11 @@ void V4L2Camera::requestComplete(Request *request)\n> > \tif (ret != sizeof(data))\n> > \t\tLOG(V4L2Compat, Error) << \"Failed to signal eventfd POLLIN\";\n> > \n> > -\tbufferSema_.release();\n> > +\t{\n> > +\t\tMutexLocker locker(bufferMutex_);\n> > +\t\tbufferAvailableCount_++;\n> > +\t}\n> > +\tbufferCV_.notify_all();\n> > }\n> > \n> > int V4L2Camera::configure(StreamConfiguration *streamConfigOut,\n> > @@ -144,6 +148,7 @@ int V4L2Camera::allocBuffers(unsigned int count)\n> > void V4L2Camera::freeBuffers()\n> > {\n> > \tStream *stream = *camera_->streams().begin();\n> > +\n> \n> This seems unrelated.\n> \n> > \tbufferAllocator_->free(stream);\n> > }\n> > \n> > @@ -193,6 +198,7 @@ int V4L2Camera::streamOff()\n> > \t\treturn ret == -EACCES ? -EBUSY : ret;\n> > \n> > \tisRunning_ = false;\n> > +\tbufferCV_.notify_all();\n> > \n> > \treturn 0;\n> > }\n> > @@ -228,6 +234,26 @@ int V4L2Camera::qbuf(unsigned int index)\n> > \treturn 0;\n> > }\n> > \n> > +void V4L2Camera::waitForBufferAvailable()\n> > +{\n> > +\tMutexLocker locker(bufferMutex_);\n> > +\tbufferCV_.wait(locker, [&] {\n> > +\t\t\treturn bufferAvailableCount_ >= 1 || !isRunning_;\n> > +\t\t\t});\n> \n> The correct indentation would be\n> \n> \tbufferCV_.wait(locker, [&] {\n> \t\t\t return bufferAvailableCount_ >= 1 || !isRunning_;\n> \t\t });\n> \n> (indenting lambdas is not very nice :-S)\n> \n> > +\tif (isRunning_)\n> > +\t\tbufferAvailableCount_--;\n> > +}\n> > +\n> > +bool V4L2Camera::isBufferAvailable()\n> > +{\n> > +\tMutexLocker locker(bufferMutex_);\n> > +\tif (bufferAvailableCount_ < 1)\n> > +\t\treturn false;\n> > +\n> > +\tbufferAvailableCount_--;\n> > +\treturn true;\n> > +}\n> > +\n> > bool V4L2Camera::isRunning()\n> > {\n> > \treturn isRunning_;\n> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> > index ee53c2b..515e906 100644\n> > --- a/src/v4l2/v4l2_camera.h\n> > +++ b/src/v4l2/v4l2_camera.h\n> > @@ -57,9 +57,10 @@ public:\n> > \n> > \tint qbuf(unsigned int index);\n> > \n> > -\tbool isRunning();\n> > +\tvoid waitForBufferAvailable();\n> > +\tbool isBufferAvailable();\n> > \n> > -\tSemaphore bufferSema_;\n> > +\tbool isRunning();\n> > \n> > private:\n> > \tvoid requestComplete(Request *request);\n> > @@ -76,6 +77,10 @@ private:\n> > \tstd::deque> completedBuffers_;\n> > \n> > \tint efd_;\n> > +\n> > +\tMutex bufferMutex_;\n> > +\tstd::condition_variable bufferCV_;\n> > +\tunsigned int bufferAvailableCount_;\n> > };\n> > \n> > #endif /* __V4L2_CAMERA_H__ */\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index fa58585..8420e23 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -594,10 +594,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> > \t\treturn -EINVAL;\n> > \n> > \tif (!(file->nonBlocking()))\n> > -\t\tvcam_->bufferSema_.acquire();\n> > -\telse if (!vcam_->bufferSema_.tryAcquire())\n> > +\t\tvcam_->waitForBufferAvailable();\n> > +\telse if (!vcam_->isBufferAvailable())\n> > \t\treturn -EAGAIN;\n> > \n> > +\t/*\n> > +\t * We need to check here again in case stream was turned off while we\n> > +\t * were blocked on waitForBufferAvailable.\n> \n> s/waitForBufferAvailable/waitForBufferAvailable()/\n> \n> > +\t */\n> > +\tif (!vcam_->isRunning())\n> > +\t\treturn -EINVAL;\n> \n> I think there's a small race here in the non-blocking case, as\n> isBufferAvailable() could decrement the buffer count and the !running\n> case could ignore that. I'm not sure it would have consequences as we\n> would be stopping streaming anyway in that case, but regardless, patch\n> 22/22 will address that, so\n> \n> Reviewed-by: Laurent Pinchart \n> \n> as I'm not concerned about bisection breakages before this series lands.\n\nI take this back, see explanation in my review of 22/22.\n\n> > +\n> > \tupdateBuffers();\n> > \n> > \tstruct v4l2_buffer &buf = buffers_[currentBuf_];","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3460F603B9\n\tfor ;\n\tWed, 24 Jun 2020 02:05:28 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F1B02A9;\n\tWed, 24 Jun 2020 02:05:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nQhuw5pe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592957127;\n\tbh=ao8vqALaKwpC8iqmuQSPm+jASjDzFTD6nF97cFxzitg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nQhuw5peVCMl6sExtd0vKAuI0Pr1I+19FJVOjjFr4xjyThLLC4HPxp43ElltllP3O\n\tyi6KSQVtMerCTnfaX4DfLNX1DdyiXBes+h4YIx7cnjs74emC4clWiDR7/Lib0xXpcG\n\tmsYyhx1jiOsN6f1sHsOn6QEgkPu6VMQLVLH1VLHc=","Date":"Wed, 24 Jun 2020 03:05:01 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200624000501.GR5870@pendragon.ideasonboard.com>","References":"<20200623190836.53446-1-paul.elder@ideasonboard.com>\n\t<20200623190836.53446-17-paul.elder@ideasonboard.com>\n\t<20200623230525.GN5870@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200623230525.GN5870@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 16/22] v4l2: v4l2_camera: Don't use\n\tlibcamera::Semaphore for available buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 24 Jun 2020 00:05:28 -0000"}}]