[{"id":32087,"web_url":"https://patchwork.libcamera.org/comment/32087/","msgid":"<20241111080843.GH6002@pendragon.ideasonboard.com>","date":"2024-11-11T08:08:43","subject":"Re: [PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media\n\trequest API","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey, Han-Lin,\n\nThank you for the patch.\n\nOn Fri, Nov 08, 2024 at 07:38:27AM +0000, Harvey Yang wrote:\n> From: Han-Lin Chen \n> \n> Add request fd as an extra argument to V4L2 device queueBuffer.\n> Default to -1 for backward compatibility.\n> \n> It's used to bind multiple buffers into the same request.\n> \n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 2 +-\n> src/libcamera/v4l2_videodevice.cpp | 7 ++++++-\n> 2 files changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index f021c2a01..6e2b1e1f1 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -218,7 +218,7 @@ public:\n> \tint importBuffers(unsigned int count);\n> \tint releaseBuffers();\n> \n> -\tint queueBuffer(FrameBuffer *buffer);\n> +\tint queueBuffer(FrameBuffer *buffer, int requestFd = -1);\n> \tSignal bufferReady;\n> \n> \tint streamOn();\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 14eba0561..0c6ea086d 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1604,6 +1604,7 @@ int V4L2VideoDevice::releaseBuffers()\n> /**\n> * \\brief Queue a buffer to the video device\n> * \\param[in] buffer The buffer to be queued\n> + * \\param[in] requestFd The fd of the request to queue the buffer to\n\nWhy do you pass the request FD and not the request object pointer ?\n\n> *\n> * For capture video devices the \\a buffer will be filled with data by the\n> * device. For output video devices the \\a buffer shall contain valid data and\n> @@ -1618,7 +1619,7 @@ int V4L2VideoDevice::releaseBuffers()\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, int requestFd)\n> {\n> \tstruct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};\n> \tstruct v4l2_buffer buf = {};\n> @@ -1647,6 +1648,10 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> \tbuf.type = bufferType_;\n> \tbuf.memory = memoryType_;\n> \tbuf.field = V4L2_FIELD_NONE;\n> +\tif (requestFd >= 0) {\n> +\t\tbuf.flags |= V4L2_BUF_FLAG_REQUEST_FD;\n> +\t\tbuf.request_fd = requestFd;\n> +\t}\n\nWhat will happen if MEDIA_REQUEST_IOC_REINIT is called on the request ?\nAs far as I understand, on the kernel side, the buffer is queued to vb2\nonly when the request is queued, while here, the buffer will be\nconsidered queued as soon as queueBuffer() is called. If the request is\nthen reinitialized instead of being queued, the kernel and userspace\nstate will lose sync. How should this be addressed ?\n\n> \n> \tbool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> \tconst std::vector &planes = buffer->planes();","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 55ED8BF415\n\tfor ;\n\tMon, 11 Nov 2024 08:08:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A49D7657B9;\n\tMon, 11 Nov 2024 09:08:52 +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 F055F65486\n\tfor ;\n\tMon, 11 Nov 2024 09:08:50 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 13BB0788;\n\tMon, 11 Nov 2024 09:08:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rSjdufl9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731312519;\n\tbh=96tchza1wOpeT7T5cGBP1TovLg2w9cl4XmFDqBaZ89M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rSjdufl9AkM9JrcfB5toNSPHgb0ULuRz8YXnY3Eod/GRFXKq5gbnDEgiBRaJ8gGJ9\n\tNOuRuKF+oqXs8ruG7XCNsKPjEZdEhwIFZ34gK3BttAe4wnz3x6mpVHeSy5aVx4jp+M\n\tLcPGXgDawtKPUIAQKMg8CN93t8wWFdm3d2n3Pcy8=","Date":"Mon, 11 Nov 2024 10:08:43 +0200","From":"Laurent Pinchart ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media\n\trequest API","Message-ID":"<20241111080843.GH6002@pendragon.ideasonboard.com>","References":"<20241108075039.2116460-1-chenghaoyang@chromium.org>\n\t<20241108075039.2116460-2-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241108075039.2116460-2-chenghaoyang@chromium.org>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32091,"web_url":"https://patchwork.libcamera.org/comment/32091/","msgid":"","date":"2024-11-11T09:02:43","subject":"Re: [PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media\n\trequest API","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Nov 11, 2024 at 4:08 PM Laurent Pinchart\n wrote:\n>\n> Hi Harvey, Han-Lin,\n>\n> Thank you for the patch.\n>\n> On Fri, Nov 08, 2024 at 07:38:27AM +0000, Harvey Yang wrote:\n> > From: Han-Lin Chen \n> >\n> > Add request fd as an extra argument to V4L2 device queueBuffer.\n> > Default to -1 for backward compatibility.\n> >\n> > It's used to bind multiple buffers into the same request.\n> >\n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 2 +-\n> > src/libcamera/v4l2_videodevice.cpp | 7 ++++++-\n> > 2 files changed, 7 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index f021c2a01..6e2b1e1f1 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -218,7 +218,7 @@ public:\n> > int importBuffers(unsigned int count);\n> > int releaseBuffers();\n> >\n> > - int queueBuffer(FrameBuffer *buffer);\n> > + int queueBuffer(FrameBuffer *buffer, int requestFd = -1);\n> > Signal bufferReady;\n> >\n> > int streamOn();\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 14eba0561..0c6ea086d 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1604,6 +1604,7 @@ int V4L2VideoDevice::releaseBuffers()\n> > /**\n> > * \\brief Queue a buffer to the video device\n> > * \\param[in] buffer The buffer to be queued\n> > + * \\param[in] requestFd The fd of the request to queue the buffer to\n>\n> Why do you pass the request FD and not the request object pointer ?\n\nYeah that was a question I asked in the cover letter. I'm not sure if passing\nthe request object pointer is necessary.\n\nThe question below shows the necessity of passing the pointer though.\n\n>\n> > *\n> > * For capture video devices the \\a buffer will be filled with data by the\n> > * device. For output video devices the \\a buffer shall contain valid data and\n> > @@ -1618,7 +1619,7 @@ int V4L2VideoDevice::releaseBuffers()\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > -int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, int requestFd)\n> > {\n> > struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};\n> > struct v4l2_buffer buf = {};\n> > @@ -1647,6 +1648,10 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > buf.type = bufferType_;\n> > buf.memory = memoryType_;\n> > buf.field = V4L2_FIELD_NONE;\n> > + if (requestFd >= 0) {\n> > + buf.flags |= V4L2_BUF_FLAG_REQUEST_FD;\n> > + buf.request_fd = requestFd;\n> > + }\n>\n> What will happen if MEDIA_REQUEST_IOC_REINIT is called on the request ?\n> As far as I understand, on the kernel side, the buffer is queued to vb2\n> only when the request is queued, while here, the buffer will be\n> considered queued as soon as queueBuffer() is called. If the request is\n> then reinitialized instead of being queued, the kernel and userspace\n> state will lose sync. How should this be addressed ?\n\nThat's a good question. We assumed that the user of the request and buffers\nshould make sure the request is queued after being bound with buffer(s).\n\nMaybe we can set the state transition within the request object:\n{Idle, BoundToBuffers, Queued}\nIn \"V4L2VideoDevice::queueBuffer()\", we can pass the request object pointer\nto transit from Idle/BoundToBuffers to BoundToBuffers.\nIn \"Request::queueRequest()\", let the state transit from BoundToBuffers to\nQueued.\nIn \"Request::reinit()\", let the state transit from Queued to Idle.\n\nWDYT?\n\nBR,\nHarvey\n\n>\n> >\n> > bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > const std::vector &planes = buffer->planes();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 7B27FBE173\n\tfor ;\n\tMon, 11 Nov 2024 09:02:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 574B9657C6;\n\tMon, 11 Nov 2024 10:02:57 +0100 (CET)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA09A60355\n\tfor ;\n\tMon, 11 Nov 2024 10:02:54 +0100 (CET)","by mail-lf1-x136.google.com with SMTP id\n\t2adb3069b0e04-539f84907caso4677927e87.3\n\tfor ;\n\tMon, 11 Nov 2024 01:02:54 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"YpAV8QoQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1731315774; x=1731920574;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=KtOfqYGkXVdeJk8aE3RLzz+Id7sSpi/qYjk0Qz9lZgg=;\n\tb=YpAV8QoQozx23iKQhtkDufANfY03OaBDs/eRsQ6qP8lVQEL7nbYzWCZFsJDAuV2jzd\n\tvfRnzZFJNloEJKdpi4PrX/VAU7Gcz0TFzKC/wQHcQyzCgILoZEaESnZw/2QfXgIUTABb\n\tA6UotwIoAFeOWBjkmncHt5cdiOGV0wr+z/wUw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731315774; x=1731920574;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=KtOfqYGkXVdeJk8aE3RLzz+Id7sSpi/qYjk0Qz9lZgg=;\n\tb=GJKwjNDve/hFi719Ul56yoTjc7ywQYKLynxDwnYnNGWD+NWcLlVOqnz8fgJRbie7pH\n\tpNmE0QzNDtYSDz6PT/X7hKOhNUItgTy9k0Kgfy8G4ge50xYJmYZ9F7Uypxwx8KlSbSXY\n\tXISzinTAcebA9FzxYoKHJ/stdnbH/cKUp8HE7jI191oxvA+LYQtYJxV6rVAOseiP+pyh\n\tW5rhlxTvXpx6UdFfEb8htRruUKXacYHx3D2wD9S07vLnBAzdW5cgTdhmsEMumcIjGhtu\n\tFnDK+Zx46hPn2RQJMap01xHXQiemskNPdgWDQaDumZ7Osvu8sUrVeko1lBE2iZ9EVo96\n\tmdHw==","X-Gm-Message-State":"AOJu0YzDuiOf+be7IwrBvRGaAt3gOALZmvaaclexphc0Kn5EXK6IGZ4d\n\t3YkAbHTSeV/U4bYcM3ItQj+K45lt8iOjsJaHsuBq+ceGMmVbKomnTg4d5zgYukAZ8Mz2PomveP2\n\t4ECH7qCy8Kp+25I/aqFIgfAZpWeWhqprXzFM4","X-Google-Smtp-Source":"AGHT+IFtXQS7H1Na1evuLiODk0c/ww9ynOp2daxw8/wfvx8rG8B1g999+rgZ7M0xb4m/qOZOMsleeMEAUEX49ue5tdA=","X-Received":"by 2002:a2e:be24:0:b0:2fb:591d:3db8 with SMTP id\n\t38308e7fff4ca-2ff2028a90bmr48970521fa.35.1731315773890;\n\tMon, 11 Nov 2024 01:02:53 -0800 (PST)","MIME-Version":"1.0","References":"<20241108075039.2116460-1-chenghaoyang@chromium.org>\n\t<20241108075039.2116460-2-chenghaoyang@chromium.org>\n\t<20241111080843.GH6002@pendragon.ideasonboard.com>","In-Reply-To":"<20241111080843.GH6002@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Mon, 11 Nov 2024 17:02:43 +0800","Message-ID":"","Subject":"Re: [PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media\n\trequest API","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32112,"web_url":"https://patchwork.libcamera.org/comment/32112/","msgid":"<20241112065042.GD5877@pendragon.ideasonboard.com>","date":"2024-11-12T06:50:42","subject":"Re: [PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media\n\trequest API","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nOn Mon, Nov 11, 2024 at 05:02:43PM +0800, Cheng-Hao Yang wrote:\n> On Mon, Nov 11, 2024 at 4:08 PM Laurent Pinchart wrote:\n> > On Fri, Nov 08, 2024 at 07:38:27AM +0000, Harvey Yang wrote:\n> > > From: Han-Lin Chen \n> > >\n> > > Add request fd as an extra argument to V4L2 device queueBuffer.\n> > > Default to -1 for backward compatibility.\n> > >\n> > > It's used to bind multiple buffers into the same request.\n> > >\n> > > Signed-off-by: Han-Lin Chen \n> > > Co-developed-by: Harvey Yang \n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > include/libcamera/internal/v4l2_videodevice.h | 2 +-\n> > > src/libcamera/v4l2_videodevice.cpp | 7 ++++++-\n> > > 2 files changed, 7 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index f021c2a01..6e2b1e1f1 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -218,7 +218,7 @@ public:\n> > > int importBuffers(unsigned int count);\n> > > int releaseBuffers();\n> > >\n> > > - int queueBuffer(FrameBuffer *buffer);\n> > > + int queueBuffer(FrameBuffer *buffer, int requestFd = -1);\n> > > Signal bufferReady;\n> > >\n> > > int streamOn();\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 14eba0561..0c6ea086d 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1604,6 +1604,7 @@ int V4L2VideoDevice::releaseBuffers()\n> > > /**\n> > > * \\brief Queue a buffer to the video device\n> > > * \\param[in] buffer The buffer to be queued\n> > > + * \\param[in] requestFd The fd of the request to queue the buffer to\n> >\n> > Why do you pass the request FD and not the request object pointer ?\n> \n> Yeah that was a question I asked in the cover letter. I'm not sure if passing\n> the request object pointer is necessary.\n> \n> The question below shows the necessity of passing the pointer though.\n> \n> > > *\n> > > * For capture video devices the \\a buffer will be filled with data by the\n> > > * device. For output video devices the \\a buffer shall contain valid data and\n> > > @@ -1618,7 +1619,7 @@ int V4L2VideoDevice::releaseBuffers()\n> > > *\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > > -int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, int requestFd)\n> > > {\n> > > struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};\n> > > struct v4l2_buffer buf = {};\n> > > @@ -1647,6 +1648,10 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > buf.type = bufferType_;\n> > > buf.memory = memoryType_;\n> > > buf.field = V4L2_FIELD_NONE;\n> > > + if (requestFd >= 0) {\n> > > + buf.flags |= V4L2_BUF_FLAG_REQUEST_FD;\n> > > + buf.request_fd = requestFd;\n> > > + }\n> >\n> > What will happen if MEDIA_REQUEST_IOC_REINIT is called on the request ?\n> > As far as I understand, on the kernel side, the buffer is queued to vb2\n> > only when the request is queued, while here, the buffer will be\n> > considered queued as soon as queueBuffer() is called. If the request is\n> > then reinitialized instead of being queued, the kernel and userspace\n> > state will lose sync. How should this be addressed ?\n> \n> That's a good question. We assumed that the user of the request and buffers\n> should make sure the request is queued after being bound with buffer(s).\n\nCan that always be guaranteed, even in case of errors ? Graceful\nrecovery of errors is important.\n\n> Maybe we can set the state transition within the request object:\n> {Idle, BoundToBuffers, Queued}\n> In \"V4L2VideoDevice::queueBuffer()\", we can pass the request object pointer\n> to transit from Idle/BoundToBuffers to BoundToBuffers.\n> In \"Request::queueRequest()\", let the state transit from BoundToBuffers to\n> Queued.\n> In \"Request::reinit()\", let the state transit from Queued to Idle.\n> \n> WDYT?\n\nWe can certainly keep track of request states, which would help catching\nerrors such as trying to queue an already queued request, although I\nexpect the kernel to return an error anyway.\n\nI'm not sure how that will help with the issue at hand though, which is\nthat the internal state of the V4L2VideoDevice class can get out of sync\nwith the device state.\n\n> > >\n> > > bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > > const std::vector &planes = buffer->planes();","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 61792BE173\n\tfor ;\n\tTue, 12 Nov 2024 06:50:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6925D657E2;\n\tTue, 12 Nov 2024 07:50:53 +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 900E0657DD\n\tfor ;\n\tTue, 12 Nov 2024 07:50:52 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B78036AF;\n\tTue, 12 Nov 2024 07:50:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HdJCsowf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731394240;\n\tbh=/UUCMgwraAyEGTMeOWYJfRkU8bJwMjHwzyZcG5WgpDw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HdJCsowfiVkPy/9aCfYSSeY2PQnfk9qnGIuFz3H+VK8Keodf/5D/EK8Ke1VL29pDb\n\tEocbXG0bP8RcWXx6U6w1ytyJaOFk+NqD9GEGVo6vX0neqqpAhGVV0MqxhgUyrt8kNj\n\tWf+aplCQSqNZRHH0XqiVHHz3VVhQa/1kDkLcj7G8=","Date":"Tue, 12 Nov 2024 08:50:42 +0200","From":"Laurent Pinchart ","To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media\n\trequest API","Message-ID":"<20241112065042.GD5877@pendragon.ideasonboard.com>","References":"<20241108075039.2116460-1-chenghaoyang@chromium.org>\n\t<20241108075039.2116460-2-chenghaoyang@chromium.org>\n\t<20241111080843.GH6002@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32117,"web_url":"https://patchwork.libcamera.org/comment/32117/","msgid":"","date":"2024-11-12T08:26:33","subject":"Re: [PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media\n\trequest API","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Nov 12, 2024 at 2:50 PM Laurent Pinchart\n wrote:\n>\n> Hi Harvey,\n>\n> On Mon, Nov 11, 2024 at 05:02:43PM +0800, Cheng-Hao Yang wrote:\n> > On Mon, Nov 11, 2024 at 4:08 PM Laurent Pinchart wrote:\n> > > On Fri, Nov 08, 2024 at 07:38:27AM +0000, Harvey Yang wrote:\n> > > > From: Han-Lin Chen \n> > > >\n> > > > Add request fd as an extra argument to V4L2 device queueBuffer.\n> > > > Default to -1 for backward compatibility.\n> > > >\n> > > > It's used to bind multiple buffers into the same request.\n> > > >\n> > > > Signed-off-by: Han-Lin Chen \n> > > > Co-developed-by: Harvey Yang \n> > > > Signed-off-by: Harvey Yang \n> > > > ---\n> > > > include/libcamera/internal/v4l2_videodevice.h | 2 +-\n> > > > src/libcamera/v4l2_videodevice.cpp | 7 ++++++-\n> > > > 2 files changed, 7 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > > index f021c2a01..6e2b1e1f1 100644\n> > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > @@ -218,7 +218,7 @@ public:\n> > > > int importBuffers(unsigned int count);\n> > > > int releaseBuffers();\n> > > >\n> > > > - int queueBuffer(FrameBuffer *buffer);\n> > > > + int queueBuffer(FrameBuffer *buffer, int requestFd = -1);\n> > > > Signal bufferReady;\n> > > >\n> > > > int streamOn();\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index 14eba0561..0c6ea086d 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -1604,6 +1604,7 @@ int V4L2VideoDevice::releaseBuffers()\n> > > > /**\n> > > > * \\brief Queue a buffer to the video device\n> > > > * \\param[in] buffer The buffer to be queued\n> > > > + * \\param[in] requestFd The fd of the request to queue the buffer to\n> > >\n> > > Why do you pass the request FD and not the request object pointer ?\n> >\n> > Yeah that was a question I asked in the cover letter. I'm not sure if passing\n> > the request object pointer is necessary.\n> >\n> > The question below shows the necessity of passing the pointer though.\n> >\n> > > > *\n> > > > * For capture video devices the \\a buffer will be filled with data by the\n> > > > * device. For output video devices the \\a buffer shall contain valid data and\n> > > > @@ -1618,7 +1619,7 @@ int V4L2VideoDevice::releaseBuffers()\n> > > > *\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > > -int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > > +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, int requestFd)\n> > > > {\n> > > > struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};\n> > > > struct v4l2_buffer buf = {};\n> > > > @@ -1647,6 +1648,10 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > > buf.type = bufferType_;\n> > > > buf.memory = memoryType_;\n> > > > buf.field = V4L2_FIELD_NONE;\n> > > > + if (requestFd >= 0) {\n> > > > + buf.flags |= V4L2_BUF_FLAG_REQUEST_FD;\n> > > > + buf.request_fd = requestFd;\n> > > > + }\n> > >\n> > > What will happen if MEDIA_REQUEST_IOC_REINIT is called on the request ?\n> > > As far as I understand, on the kernel side, the buffer is queued to vb2\n> > > only when the request is queued, while here, the buffer will be\n> > > considered queued as soon as queueBuffer() is called. If the request is\n> > > then reinitialized instead of being queued, the kernel and userspace\n> > > state will lose sync. How should this be addressed ?\n> >\n> > That's a good question. We assumed that the user of the request and buffers\n> > should make sure the request is queued after being bound with buffer(s).\n>\n> Can that always be guaranteed, even in case of errors ? Graceful\n> recovery of errors is important.\n\nAh, now I understand your question: You're worrying that\n\"Request::queueRequest()\" is not called or returns an error after a\nbuffer is bound to a request. I was thinking that in my patch\nhttps://patchwork.libcamera.org/patch/21817/\n, `MEDIA_REQUEST_IOC_REINIT` is only called after\n\"Request::queueRequest()\" and notified by the kernel/hardware that\nthe request is done.\n\n>\n> > Maybe we can set the state transition within the request object:\n> > {Idle, BoundToBuffers, Queued}\n> > In \"V4L2VideoDevice::queueBuffer()\", we can pass the request object pointer\n> > to transit from Idle/BoundToBuffers to BoundToBuffers.\n> > In \"Request::queueRequest()\", let the state transit from BoundToBuffers to\n> > Queued.\n> > In \"Request::reinit()\", let the state transit from Queued to Idle.\n> >\n> > WDYT?\n>\n> We can certainly keep track of request states, which would help catching\n> errors such as trying to queue an already queued request, although I\n> expect the kernel to return an error anyway.\n>\n> I'm not sure how that will help with the issue at hand though, which is\n> that the internal state of the V4L2VideoDevice class can get out of sync\n> with the device state.\n\nI think we can register `V4L2VideoDevice`s into `MediaDevice::Request`,\nand trigger their `dequeueBuffer()` function when\n`MediaDevice::Request::queueRequest` fails.\n\nAlternatively, we can let `MediaDevice::Request` handle queuing buffers\nby registering the pairs of , and\ncalling `V4L2VideoDevice::queueBuffer()` right before\n`ioctl(fd, MEDIA_REQUEST_IOC_QUEUE)`.\n\nAs for the state \"buffer queued\" out of sync, I don't have a good answer\nnow. If there's a component that needs to check if the request that a buffer\nis bound to is queued or not, we can keep the request's pointer in\nV4L2VideoDevice and add an API to get access to the request's state.\n\nWDYT?\n\nBR,\nHarvey\n\n>\n> > > >\n> > > > bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> > > > const std::vector &planes = buffer->planes();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 1AD52C324C\n\tfor ;\n\tTue, 12 Nov 2024 08:26:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EF2F657E7;\n\tTue, 12 Nov 2024 09:26:47 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 065BF65474\n\tfor ;\n\tTue, 12 Nov 2024 09:26:46 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2fb57f97d75so49887841fa.2\n\tfor ;\n\tTue, 12 Nov 2024 00:26:45 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"cUF1NYWk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1731400005; x=1732004805;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=/NMqi80/k/L0YLMQ8NA6G9p2OXQH497h3GTXeZtvxNM=;\n\tb=cUF1NYWk4yjFbv6eTm2PrujR8Ov+LEjfAFovL5KPghHHmNOR0J6aZrIw61UfeM6EQG\n\tu61C4XswmE19F1kXvy3ItbxeiFjOr/vRrwsXetEb7rRRSiIkqDkLwyJDWu75YMEX0Qm5\n\tdiWOFDJg3WhBIGMIu0JTCU4ic47bOz47415vI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731400005; x=1732004805;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=/NMqi80/k/L0YLMQ8NA6G9p2OXQH497h3GTXeZtvxNM=;\n\tb=cjHi3TlcECktYOIqiiA1814X5KxPWNzGsLutftlQ5ybO8fuQ9MMacv9LAao98i38mf\n\tlnKN7f6ID1KN+tCxIn7IT6tzjhW6u6cfkPtYwwljzEREitN4jBh4x7KjDVYL6zQqbnl4\n\tmVy/fWD/jGZQHnvNfR/XClq9GHDOcnSCThUVvZ1ltBLQAKkUwym4sQMhgUmNlLa4oMZG\n\tNFWtz9JpZv6cGYTxfchgGZT40GBDn4Jhwseb+KZXYLUGIZBWMt/8ltiF+iuEmbHM3Cl0\n\t5NN3sU5/syDby+hP7wAvhHDKZ8fhDir7vVQyX48HDguglAki6WYoN7CZEjVDYJ5yMHmu\n\tWjyg==","X-Gm-Message-State":"AOJu0YzaPD//h7i7Z6AGmpMrwHIOA8YFVsTX6SCuQxQFU2N4Tw2D1NB8\n\twRpbYwqwISs5RpPQzhDzP4qk90X/MNiGzaia7ACHBC11O835408eCbCpgt3NHXVliuACRXJ9ric\n\tWYxWOyvo4knXjs7Mf2PSUuQheB7WhDdrOi9md","X-Google-Smtp-Source":"AGHT+IHdnm1+zZ57kJs24US4Rui0zXsrSzC0JYTLk8QyvCjc6zxTHhJuY1v8RzlO00ltboS26SK531FrLj8K5Gov9sY=","X-Received":"by 2002:a2e:a5c3:0:b0:2fb:4c08:be08 with SMTP id\n\t38308e7fff4ca-2ff2017287dmr80949311fa.11.1731400004975;\n\tTue, 12 Nov 2024 00:26:44 -0800 (PST)","MIME-Version":"1.0","References":"<20241108075039.2116460-1-chenghaoyang@chromium.org>\n\t<20241108075039.2116460-2-chenghaoyang@chromium.org>\n\t<20241111080843.GH6002@pendragon.ideasonboard.com>\n\t\n\t<20241112065042.GD5877@pendragon.ideasonboard.com>","In-Reply-To":"<20241112065042.GD5877@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Tue, 12 Nov 2024 16:26:33 +0800","Message-ID":"","Subject":"Re: [PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media\n\trequest API","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]