[{"id":5363,"web_url":"https://patchwork.libcamera.org/comment/5363/","msgid":"<20200623225511.GM5870@pendragon.ideasonboard.com>","date":"2020-06-23T22:55:11","subject":"Re: [libcamera-devel] [PATCH v3 15/22] v4l2: v4l2_camera_proxy:\n\tReset buffer flags on reqbufs 0","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:29AM +0900, Paul Elder wrote:\n> Clear the queued and done buffer flags on VIDIOC_REQBUFS with count = 0\n> if the stream is not on. If the stream is on and reqbufs is called with\n> count = 0, return -EBUSY.\n> \n> Note that this is contrary to what the V4L2 docs say (reqbufs 0 when\n> streaming should also streamoff), but it is how the V4L2 implementation\n> works. v4l2-compliance doesn't seem to care either way, however, so we\n> cater to the implementation.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3:\n> - don't streamoff in reqbufs 0; return -EBUSY instead\n> \n> Changes in v2:\n> - call only the necessary components, instead of\n> V4L2CameraProxy::vidioc_streamoff\n> ---\n> src/v4l2/v4l2_camera_proxy.cpp | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 06fef21..fa58585 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -444,11 +444,6 @@ int V4L2CameraProxy::freeBuffers()\n> {\n> \tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> \n> -\tint ret = vcam_->streamOff();\n> -\tif (ret < 0) {\n> -\t\tLOG(V4L2Compat, Error) << \"Failed to stop stream\";\n> -\t\treturn ret;\n> -\t}\n> \tvcam_->freeBuffers();\n> \tbufferCount_ = 0;\n> \n> @@ -476,6 +471,12 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf\n> \tmemset(arg->reserved, 0, sizeof(arg->reserved));\n> \n> \tif (arg->count == 0) {\n> +\t\tif (vcam_->isRunning())\n> +\t\t\treturn -EBUSY;\n> +\n> +\t\tfor (struct v4l2_buffer &buf : buffers_)\n> +\t\t\tbuf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);\n> +\n\nIs this the correct fix ? Shouldn't you call buffers_.clear() instead ?\nI'd do so in V4L2CameraProxy::freeBuffers().\n\nWith this fixed,\n\nReviewed-by: Laurent Pinchart \n\n> \t\trelease(file);\n> \t\treturn freeBuffers();\n\nAdditionally, you probably want to avoid calling release() if\nfreeBuffers() failed. This should probably be done in the patch that\nintroduces the release() call. And in the error paths below in this\nfunction, you probably want to call release() too. To keep this simple,\nmaybe acquire() should be moved to the end, with just\n\n\tif (!hasOwnership() && owner_)\n\t\treturn -EBUSY;\n\ncheck at the top.\n\n> \t}","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 0EF05603B9\n\tfor ;\n\tWed, 24 Jun 2020 00:55:38 +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 7132B2A9;\n\tWed, 24 Jun 2020 00:55:37 +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=\"GDWsq7Ju\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592952937;\n\tbh=VIqJvaSOnZErK/yU7sUeNwplZZUaal402DVt7EuM+uA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GDWsq7JuN0fSypP+X3rPeywWZwBW9hWRrVv2oOQ+xCyLjsEewbg6FpxBfr+nJ3FAx\n\tB8z81inkF/adu5tsRJCHpCVPN9F/DycJrSb5lPGTAk53fg1l7ftPEWImwEVqdLAGPg\n\tPSAFEjHAVOjuUvA9NR512f7taYmD+Va3Veofmvus=","Date":"Wed, 24 Jun 2020 01:55:11 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200623225511.GM5870@pendragon.ideasonboard.com>","References":"<20200623190836.53446-1-paul.elder@ideasonboard.com>\n\t<20200623190836.53446-16-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200623190836.53446-16-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 15/22] v4l2: v4l2_camera_proxy:\n\tReset buffer flags on reqbufs 0","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 22:55:38 -0000"}},{"id":5370,"web_url":"https://patchwork.libcamera.org/comment/5370/","msgid":"<20200624055411.GB2285@jade.amanokami.net>","date":"2020-06-24T05:54:11","subject":"Re: [libcamera-devel] [PATCH v3 15/22] v4l2: v4l2_camera_proxy:\n\tReset buffer flags on reqbufs 0","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Wed, Jun 24, 2020 at 01:55:11AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Wed, Jun 24, 2020 at 04:08:29AM +0900, Paul Elder wrote:\n> > Clear the queued and done buffer flags on VIDIOC_REQBUFS with count = 0\n> > if the stream is not on. If the stream is on and reqbufs is called with\n> > count = 0, return -EBUSY.\n> > \n> > Note that this is contrary to what the V4L2 docs say (reqbufs 0 when\n> > streaming should also streamoff), but it is how the V4L2 implementation\n> > works. v4l2-compliance doesn't seem to care either way, however, so we\n> > cater to the implementation.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v3:\n> > - don't streamoff in reqbufs 0; return -EBUSY instead\n> > \n> > Changes in v2:\n> > - call only the necessary components, instead of\n> > V4L2CameraProxy::vidioc_streamoff\n> > ---\n> > src/v4l2/v4l2_camera_proxy.cpp | 11 ++++++-----\n> > 1 file changed, 6 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index 06fef21..fa58585 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -444,11 +444,6 @@ int V4L2CameraProxy::freeBuffers()\n> > {\n> > \tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> > \n> > -\tint ret = vcam_->streamOff();\n> > -\tif (ret < 0) {\n> > -\t\tLOG(V4L2Compat, Error) << \"Failed to stop stream\";\n> > -\t\treturn ret;\n> > -\t}\n> > \tvcam_->freeBuffers();\n> > \tbufferCount_ = 0;\n> > \n> > @@ -476,6 +471,12 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf\n> > \tmemset(arg->reserved, 0, sizeof(arg->reserved));\n> > \n> > \tif (arg->count == 0) {\n> > +\t\tif (vcam_->isRunning())\n> > +\t\t\treturn -EBUSY;\n> > +\n> > +\t\tfor (struct v4l2_buffer &buf : buffers_)\n> > +\t\t\tbuf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);\n> > +\n> \n> Is this the correct fix ? Shouldn't you call buffers_.clear() instead ?\n> I'd do so in V4L2CameraProxy::freeBuffers().\n\nAh yes, you're right.\n\n> With this fixed,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > \t\trelease(file);\n> > \t\treturn freeBuffers();\n> \n> Additionally, you probably want to avoid calling release() if\n> freeBuffers() failed. This should probably be done in the patch that\n\nWell, open -> reqbufs 0 is apparently expected to succeed, whereas\nFrameBufferAllocator->free() would fail, so I'm thinking freeBuffers()\ncould just return void because it looks like V4L2 doesn't expect reqbufs\n0 to ever fail.\n\nI think I'll call freeBuffers() before release() though, because that\nseems more correct (reset the status before relinquishing ownership).\n\n> introduces the release() call. And in the error paths below in this\n> function, you probably want to call release() too. To keep this simple,\n> maybe acquire() should be moved to the end, with just\n> \n> \tif (!hasOwnership() && owner_)\n> \t\treturn -EBUSY;\n> \n> check at the top.\n\nYeah.\n\n> > \t}\n> \n\n\nThanks,\n\nPaul","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 F2D8E603B6\n\tfor ;\n\tWed, 24 Jun 2020 07:54:19 +0200 (CEST)","from jade.amanokami.net (unknown\n\t[IPv6:2400:4051:61:600:8147:f2a2:a8c6:9087])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5BC962A8;\n\tWed, 24 Jun 2020 07:54:18 +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=\"UWacc9mj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592978059;\n\tbh=Y8gMGF7iJt9rfDjmLkH/9gfm0mHZ3jSZ69/b5l96huQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UWacc9mj/8u/Mc2gSvrlAYFYq3K1Ni1UcMUXLA7okqB52RfFcWL7bBkvBC5PboCNk\n\tqG2uPFbkD3GSHsuXETcg8G39cGtLJ1yVwhg6HVE+wETylVfBe7hN1fzxM7C+PaZ0jE\n\tijEpiwp/xCbnPBZ5xvvzGLr2xruHklrSj/b4i55c=","Date":"Wed, 24 Jun 2020 14:54:11 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200624055411.GB2285@jade.amanokami.net>","References":"<20200623190836.53446-1-paul.elder@ideasonboard.com>\n\t<20200623190836.53446-16-paul.elder@ideasonboard.com>\n\t<20200623225511.GM5870@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20200623225511.GM5870@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 15/22] v4l2: v4l2_camera_proxy:\n\tReset buffer flags on reqbufs 0","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 05:54:20 -0000"}}]