[{"id":22059,"web_url":"https://patchwork.libcamera.org/comment/22059/","msgid":"","date":"2022-01-21T10:27:52","subject":"Re: [libcamera-devel] [PATCH v3] v4l2: V4L2CameraProxy: Add support\n\tfor PREPARE_BUF as one of the supported ioctl","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hello Paul,\nThanks for the review.\n\nOn Fri, Jan 21, 2022 at 3:44 PM wrote:\n>\n> Hi Vedant,\n>\n> On Wed, Jan 19, 2022 at 11:25:12AM +0530, Vedant Paranjape wrote:\n> > Add support for PREPARE_BUF as one of the ioctl. Since this is a compat\n> > layer, there doesn't seem to be an equivalent to the \"transfer ownership\n> > of the buffer to kernel driver\" in V4L2Camera class.\n>\n> + Thus, simply duplicate the checks done by vidioc_qbuf.\n>\n> , perhaps? You describe a (minor) issue but no solution.\n>\n> >\n> > To match the error checks done by kernel implementation, we'd have to\n> > check if dmabuf fd is valid and that the buffer size is large enough.\n> > Doing so will not add any particular value to the program as\n> > applications most likely don't depend on these conditions being\n> > handled correctly.\n>\n> Would we add these once we support importing buffers, just for\n> correctness?\nSure I will do that\n>\n> >\n> > Signed-off-by: Vedant Paranjape \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > src/v4l2/v4l2_camera_proxy.cpp | 35 +++++++++++++++++++++++++++++++++-\n> > src/v4l2/v4l2_camera_proxy.h | 1 +\n> > 2 files changed, 35 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index ec6daac605fb..f3470a6d312a 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -559,6 +559,38 @@ int V4L2CameraProxy::vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *a\n> > return 0;\n> > }\n> >\n> > +int V4L2CameraProxy::vidioc_prepare_buf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> > +{\n> > + LOG(V4L2Compat, Debug)\n> > + << \"[\" << file->description() << \"] \" << __func__\n> > + << \"(index=\" << arg->index << \")\";\n> > +\n> > + if (!hasOwnership(file))\n> > + return -EBUSY;\n> > +\n> > + if (arg->index >= bufferCount_)\n> > + return -EINVAL;\n> > +\n> > + if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD)\n> > + return -EINVAL;\n> > +\n> > + if (!validateBufferType(arg->type) ||\n> > + !validateMemoryType(arg->memory))\n> > + return -EINVAL;\n>\n> I would've said that these checks should be in the same order as in\n> qbuf, but I guess you've copied this order from the kernel. In which\n> case qbuf should be fixed (in a separate patch; this one's fine).\n>\n> > +\n> > + struct v4l2_buffer &buffer = buffers_[arg->index];\n> > +\n> > + if (buffer.flags & V4L2_BUF_FLAG_QUEUED ||\n> > + buffer.flags & V4L2_BUF_FLAG_PREPARED)\n> > + return -EINVAL;\n>\n> Huh, I didn't realize that preparing a buf that's already been prepared\n> would be an error. I thought you could just nop it. v4l2 docs haven't\n> kept up I suppose :/\n>\n> I would say that we could check the prepared flag in qbuf too, but these\n> checks are few and constant time so it's probably find as-is.\nI can address this in the qbuf patch you asked above.\n>\n>\n> Looks good.\n>\n> With or without the suggested changes,\n>\n> Reviewed-by: Paul Elder \n>\n> > +\n> > + buffer.flags |= V4L2_BUF_FLAG_PREPARED;\n> > +\n> > + arg->flags = buffer.flags;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> > {\n> > LOG(V4L2Compat, Debug)\n> > @@ -627,7 +659,7 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,\n> >\n> > struct v4l2_buffer &buf = buffers_[currentBuf_];\n> >\n> > - buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);\n> > + buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_PREPARED);\n> > buf.length = sizeimage_;\n> > *arg = buf;\n> >\n> > @@ -729,6 +761,7 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> > VIDIOC_S_INPUT,\n> > VIDIOC_REQBUFS,\n> > VIDIOC_QUERYBUF,\n> > + VIDIOC_PREPARE_BUF,\n> > VIDIOC_QBUF,\n> > VIDIOC_DQBUF,\n> > VIDIOC_EXPBUF,\n> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > index a38b28c744d3..76ca2d8a5558 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.h\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -58,6 +58,7 @@ private:\n> > int vidioc_s_input(V4L2CameraFile *file, int *arg);\n> > int vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuffers *arg);\n> > int vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> > + int vidioc_prepare_buf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> > int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> > int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,\n> > libcamera::Mutex *lock) LIBCAMERA_TSA_REQUIRES(*lock);\n> > --\n> > 2.25.1\n> >\n\nRegards,\nVedant Paranjape","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 69A4BBF415\n\tfor ;\n\tFri, 21 Jan 2022 10:28:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C6CD6095E;\n\tFri, 21 Jan 2022 11:28:07 +0100 (CET)","from mail-yb1-xb32.google.com (mail-yb1-xb32.google.com\n\t[IPv6:2607:f8b0:4864:20::b32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 76A9260211\n\tfor ;\n\tFri, 21 Jan 2022 11:28:05 +0100 (CET)","by mail-yb1-xb32.google.com with SMTP id c6so26271077ybk.3\n\tfor ;\n\tFri, 21 Jan 2022 02:28:05 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"aBtUY7+M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=dJ7ftCv8a6jTw3SM63TzBOqxGE2hw/OTzNyrG6BmDsM=;\n\tb=aBtUY7+MA0sd6X8lg8pLBwizgKMsBLG2QwbTH/UA+OIpPCj2hR7dNzVo7RgbWZKUL2\n\tM3ho/wOSqgiqwW0+MBUTpWH4X67TWynqGKlTF1hFODTxpVNJFvOflhRF5qyK7EYZFYTT\n\tNsS+B3SocM176lQgF8Fsy30q7qGPdzjpA8SxAbQo/chaMtX6CCV3gRPaAxdGwfj7qQ0V\n\tGj1WVvlO7zDMiFpmdGHGyFTxPZGy31X7mXIwddAdgPHYLyfp64UyWQnx18U33zXvKA6n\n\tig0/jYqxozDCymXk8gqKP8K5ZfG5iskXsN8dRNZQ+p4SXtFT1mkKGYoIWRmQkaLSTEH7\n\tej0Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=dJ7ftCv8a6jTw3SM63TzBOqxGE2hw/OTzNyrG6BmDsM=;\n\tb=8Gu4ieOdfuSY1Te3Zg72aSNbdt6MnXpXTQBVR3fGyQT9IffgSMQJbbX0JKJ2OnoVh5\n\t3jU45AU8+aV9T/38CEyQUYSsHTlUP1tI4cMJbwNWEaEGfYSfqbhEI2eAEsNEXMtyYImB\n\tfs/os6LK1CjGTs/1noGOePajX+hOOGmqbuF7ivAK96qRjNKXKW3I++QJ9SrvMH3gJ6+3\n\tIws8gDSQllwV5atZkYbGI+f3hEhimKOANls0ZQOODCpGwXCWo7iRT9Pr+7zTIOc2gzO0\n\tmK4oCz5KkvfEfNxr8BIGePThfy9t9iP9FRoERqx1IM6AYT/Elv0JxULP39jM8mZo92zC\n\t/Cyg==","X-Gm-Message-State":"AOAM533LsTaY8E72lxmWwqu7vy4nBnUjZ0V6JJfk82VYpLQe/bUVkN9o\n\tHwuirWvqsOt4yowLLqzWY+wT+kzH1ATmr5cam42oCCYhDZI=","X-Google-Smtp-Source":"ABdhPJyZ7STt1f+cywc/XTIRfYmXlYDnpoLsLDINGJf50KOuN6fN8n/6fljSQew4diBHtoRYRDKdEJYP71N2/OND5Cg=","X-Received":"by 2002:a25:15c3:: with SMTP id 186mr5428800ybv.71.1642760883930;\n\tFri, 21 Jan 2022 02:28:03 -0800 (PST)","MIME-Version":"1.0","References":"<20220119055512.31893-1-vedantparanjape160201@gmail.com>\n\t<20220121101356.GK4255@pyrite.rasen.tech>","In-Reply-To":"<20220121101356.GK4255@pyrite.rasen.tech>","From":"Vedant Paranjape ","Date":"Fri, 21 Jan 2022 15:57:52 +0530","Message-ID":"","To":"Paul Elder ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3] v4l2: V4L2CameraProxy: Add support\n\tfor PREPARE_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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22061,"web_url":"https://patchwork.libcamera.org/comment/22061/","msgid":"","date":"2022-01-21T14:28:37","subject":"Re: [libcamera-devel] [PATCH v3] v4l2: V4L2CameraProxy: Add support\n\tfor PREPARE_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":"On Fri, Jan 21, 2022 at 07:13:56PM +0900, paul.elder@ideasonboard.com wrote:\n> On Wed, Jan 19, 2022 at 11:25:12AM +0530, Vedant Paranjape wrote:\n> > Add support for PREPARE_BUF as one of the ioctl. Since this is a compat\n> > layer, there doesn't seem to be an equivalent to the \"transfer ownership\n> > of the buffer to kernel driver\" in V4L2Camera class.\n> \n> + Thus, simply duplicate the checks done by vidioc_qbuf.\n> \n> , perhaps? You describe a (minor) issue but no solution.\n> \n> > To match the error checks done by kernel implementation, we'd have to\n> > check if dmabuf fd is valid and that the buffer size is large enough.\n> > Doing so will not add any particular value to the program as\n> > applications most likely don't depend on these conditions being\n> > handled correctly.\n> \n> Would we add these once we support importing buffers, just for\n> correctness?\n> \n> > Signed-off-by: Vedant Paranjape \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > src/v4l2/v4l2_camera_proxy.cpp | 35 +++++++++++++++++++++++++++++++++-\n> > src/v4l2/v4l2_camera_proxy.h | 1 +\n> > 2 files changed, 35 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index ec6daac605fb..f3470a6d312a 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -559,6 +559,38 @@ 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)\n> > +\t\t<< \"[\" << file->description() << \"] \" << __func__\n> > +\t\t<< \"(index=\" << arg->index << \")\";\n> > +\n> > +\tif (!hasOwnership(file))\n> > +\t\treturn -EBUSY;\n> > +\n> > +\tif (arg->index >= bufferCount_)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tif (arg->flags & V4L2_BUF_FLAG_REQUEST_FD)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tif (!validateBufferType(arg->type) ||\n> > +\t !validateMemoryType(arg->memory))\n> > +\t\treturn -EINVAL;\n> \n> I would've said that these checks should be in the same order as in\n> qbuf, but I guess you've copied this order from the kernel. In which\n> case qbuf should be fixed (in a separate patch; this one's fine).\n> \n> > +\n> > +\tstruct v4l2_buffer &buffer = buffers_[arg->index];\n> > +\n> > +\tif (buffer.flags & V4L2_BUF_FLAG_QUEUED ||\n> > +\t buffer.flags & V4L2_BUF_FLAG_PREPARED)\n> > +\t\treturn -EINVAL;\n> \n> Huh, I didn't realize that preparing a buf that's already been prepared\n> would be an error. I thought you could just nop it. v4l2 docs haven't\n> kept up I suppose :/\n> \n> I would say that we could check the prepared flag in qbuf too, but these\n> checks are few and constant time so it's probably find as-is.\n\nYou can call QBUF on a buffer that is prepared, so that shouldn't be a\nproblem. What I now realize is missing, however, is setting the\nV4L2_BUF_FLAG_PREPARED flag in V4L2CameraProxy::vidioc_qbuf(). It can be\ndone in a separate patch, when fixing the order of checks in qbuf.\n\nReviewed-by: Laurent Pinchart \n\n> Looks good.\n> \n> With or without the suggested changes,\n> \n> Reviewed-by: Paul Elder \n> \n> > +\n> > +\tbuffer.flags |= V4L2_BUF_FLAG_PREPARED;\n> > +\n> > +\targ->flags = buffer.flags;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> > {\n> > \tLOG(V4L2Compat, Debug)\n> > @@ -627,7 +659,7 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,\n> > \n> > \tstruct v4l2_buffer &buf = buffers_[currentBuf_];\n> > \n> > -\tbuf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);\n> > +\tbuf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_PREPARED);\n> > \tbuf.length = sizeimage_;\n> > \t*arg = buf;\n> > \n> > @@ -729,6 +761,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 a38b28c744d3..76ca2d8a5558 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.h\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -58,6 +58,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 7DC93BF415\n\tfor ;\n\tFri, 21 Jan 2022 14:28:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED28660213;\n\tFri, 21 Jan 2022 15:28:55 +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 E794F6017A\n\tfor ;\n\tFri, 21 Jan 2022 15:28:53 +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 4A38225B;\n\tFri, 21 Jan 2022 15:28:53 +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=\"ioZI2Mz4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1642775333;\n\tbh=kGkVS9s+si3h+fKjDxtUhtkOVjIb0qot08mK1j6uRic=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ioZI2Mz4tnatdzLacRj1q7H7SBL7A1Cxi8wnnZmkwPpF0d0Mbg6T6ZZuq6SumtUkl\n\tz6b5pDyMZpMCbPHP9SfHgCYKhIXYezjFobx8GGfpfE4SNMQaga/dkKZ/CAxYWE2Nva\n\t9fnKRSb4GY3uv/AhYLLs22ZxI/iOEd9uokDRvCf4=","Date":"Fri, 21 Jan 2022 16:28:37 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20220119055512.31893-1-vedantparanjape160201@gmail.com>\n\t<20220121101356.GK4255@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220121101356.GK4255@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3] v4l2: V4L2CameraProxy: Add support\n\tfor PREPARE_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,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]