[{"id":21989,"web_url":"https://patchwork.libcamera.org/comment/21989/","msgid":"","date":"2022-01-10T00:33:51","subject":"Re: [libcamera-devel] [RFC PATCH v1] v4l2: Add support for\n\tPREPARE_BUF as one of the supported ioctl","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Vedant,\n\nThank you for the patch.\n\nOn Sun, Jan 09, 2022 at 01:03:30PM +0530, Vedant Paranjape wrote:\n> Add support for PREPARE_BUF as one of the ioctl. This needs adding the\n> prepare_buf function to the v4l2_camera class as this operation involves\n\nThe class is called V4L2Camera, not v4l2_camera.\n\n> passing on the ownership of buffers to the driver.\n> \n> Signed-off-by: Vedant Paranjape \n> ---\n> I have not cargo copied the error checking or the function, I actually\n> went to kernel source and checked what parameters are being checked to\n> give out a error and checked those in my definition as well.\n> \n> https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L729\n\nGood idea !\n\n> Please review the v4l2_camera_proxy part, if the function looks good\n> enough hypothetically assuming vcam_->prepare_buf() has a correct\n> implementation.\n> \n> As for the part in v4l2_camera, prepare_buf. Seeing the pattern of code,\n> any operation that needs exchanging buffers with the drivers needs to\n> have a function to be implemented inside v4l2_camera just like qbuf is.\n> Let me know if my assumption is wrong, and this change is absolutely not\n> needed. \n> \n> I am unable to figure out what is the equivalent of transfering ownership \n> of the buffer in the libcamera compat world. Just like queueing a buffer\n> has an equivalent, camera_->queueRequest(request) (not comparing apples\n> to apples, but to some extent).\n\nThat's probably because there's no equivalent :-) I think you can just\ndrop the V4L2Camera::prepare_buf() function, and just mark the buffer\nwith V4L2_BUF_FLAG_PREPARED in V4L2CameraProxy::vidioc_prepare_buf() as\nyou're doing already. A V4L2 application will have no way to figure out\nif the buffer has actually been prepared or not.\n\n> ---\n> src/v4l2/v4l2_camera.cpp | 10 ++++++++++\n> src/v4l2/v4l2_camera.h | 1 +\n> src/v4l2/v4l2_camera_proxy.cpp | 32 ++++++++++++++++++++++++++++++++\n> src/v4l2/v4l2_camera_proxy.h | 1 +\n> 4 files changed, 44 insertions(+)\n> \n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index e922b9e6aab2..2c155a63f836 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -275,6 +275,16 @@ int V4L2Camera::qbuf(unsigned int index)\n> \treturn 0;\n> }\n> \n> +int V4L2Camera::prepare_buf(unsigned int index, unsigned int memory)\n> +{\n> +\t// just a stub for now\n> +\t// referring to\n> +\t// https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L729\n> +\t// https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-core.c#L1341\n> +\n> +\treturn 0;\n> +}\n> +\n> void V4L2Camera::waitForBufferAvailable()\n> {\n> \tMutexLocker locker(bufferMutex_);\n> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> index 03e741180e8f..b4e4b65ed701 100644\n> --- a/src/v4l2/v4l2_camera.h\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -57,6 +57,7 @@ public:\n> \tint streamOff();\n> \n> \tint qbuf(unsigned int index);\n> +\tint prepare_buf(unsigned int index, unsigned int memory);\n> \n> \tvoid waitForBufferAvailable();\n> \tbool isBufferAvailable();\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 4d529bc29a4d..ae0b0691ca4e 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -544,6 +544,37 @@ int V4L2CameraProxy::vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *a\n> \treturn 0;\n> }\n> \n> +int V4L2CameraProxy::vidioc_prepare_buf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_prepare_buf, index = \"\n> +\t\t\t << arg->index << \" fd = \" << file->efd();\n> +\n> +\tif (arg->index >= bufferCount_)\n> +\t\treturn -EINVAL;\n> +\n> +\tif (arg->flags & V4L2_BUF_FLAG_REQUEST_FD ||\n> +\t arg->flags & V4L2_BUF_FLAG_PREPARED)\n> +\t\treturn -EINVAL;\n> +\n> +\tif (!hasOwnership(file))\n> +\t\treturn -EBUSY;\n\nThis check should go first, to mirror the order of\nvb2_ioctl_prepare_buf().\n\n> +\n> +\tif (!validateBufferType(arg->type) ||\n> +\t !validateMemoryType(arg->memory))\n> +\t\treturn -EINVAL;\n\nYou need to check the buffer state here (buffers_[arg->index].flags, not\narg->flags) to make sure it's currently dequeued, as it's invalid to\nprepare a buffer that has already been prepared (see the first checks in\nvb2_core_prepare_buf()).\n\nIf we wanted to fully match the error checks done by the kernel, we'd\nhave to check here that the buffer size is large enough, and that the\ndmabuf fd is valid. I don't think it's worth it (especially the dmabuf\ncheck), as those are error conditions that I can't imagine an\napplication would rely on being handled correctly. A comment to explain\nthis may be useful though.\n\n> +\n> +\t/* \\todo when DMABUF memory type is supported, add support here as well */\n> +\tint ret = vcam_->prepare_buf(arg->index, arg->memory);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tbuffers_[arg->index].flags |= V4L2_BUF_FLAG_PREPARED;\n> +\n> +\targ->flags = buffers_[arg->index].flags;\n> +\n> +\treturn ret;\n> +}\n> +\n> int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> {\n> \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_qbuf, index = \"\n> @@ -709,6 +740,7 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> \tVIDIOC_S_INPUT,\n> \tVIDIOC_REQBUFS,\n> \tVIDIOC_QUERYBUF,\n> +\tVIDIOC_PREPARE_BUF,\n> \tVIDIOC_QBUF,\n> \tVIDIOC_DQBUF,\n> \tVIDIOC_EXPBUF,\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index 14e027c3e7d1..6baba94262a9 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -57,6 +57,7 @@ private:\n> \tint vidioc_s_input(V4L2CameraFile *file, int *arg);\n> \tint vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuffers *arg);\n> \tint vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> +\tint vidioc_prepare_buf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> \tint vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> \tint vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,\n> \t\t\t libcamera::Mutex *lock) LIBCAMERA_TSA_REQUIRES(*lock);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C7151BE080\n\tfor ;\n\tMon, 10 Jan 2022 00:34:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E28F560921;\n\tMon, 10 Jan 2022 01:34:02 +0100 (CET)","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 E53C26017D\n\tfor ;\n\tMon, 10 Jan 2022 01:34:00 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 08316A50;\n\tMon, 10 Jan 2022 01:33:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sJ2BRnZH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641774840;\n\tbh=eYg9/9inPKaNqNOPFlby6Vq3yPj3FG3nTFzb35VfVM8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sJ2BRnZH4OEIe/cQk7w6NJSQ6D2A3P7rvI9hmE+/n+ljPnpqaeuf+xlInnNL+s0qm\n\tzuIi69XHSFVQFyRGhoRr4xwyFtHA6MHBweOcZ95aML5bk+b6cd7g3APbpfwBb8rRyC\n\tbXQgff2Dhyp9fLhR7qyjEw7i28dOsgLSKnIr8TB4=","Date":"Mon, 10 Jan 2022 02:33:51 +0200","From":"Laurent Pinchart ","To":"Vedant Paranjape ","Message-ID":"","References":"<20220109073330.923675-1-vedantparanjape160201@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220109073330.923675-1-vedantparanjape160201@gmail.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v1] v4l2: Add support for\n\tPREPARE_BUF as one of the supported ioctl","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]