[{"id":17968,"web_url":"https://patchwork.libcamera.org/comment/17968/","msgid":"<20210705080928.t6tunqop2wmkishz@uno.localdomain>","date":"2021-07-05T08:09:28","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: uvcvideo: Add\n\tinternal request queue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicolas,\n\nOn Thu, Jul 01, 2021 at 04:29:51PM -0300, Nícolas F. R. A. Prado wrote:\n> Add an internal queue that stores requests until there are v4l2 buffer\n> slots available. This avoids the need to cancel requests when there is a\n> shortage of said buffers.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n>\n> This is very similar to what 5a9d19210fad (\"libcamera: pipeline: ipu3: Try\n> queuing pending requests if a buffer is available\") and 89dae5844964\n> (\"libcamera: pipeline: ipu3: Store requests in the case a buffer shortage\") did\n> for IPU3.\n>\n> I tested this with a new lc-compliance test from [1] which is a rebased version\n> of the series in [2]. I'll resend this series after the googletest refactor\n> series [3] is merged. But this patch for uvcvideo doesn't depend on that.\n>\n> [1] https://gitlab.collabora.com/nfraprado/libcamera/-/tree/lcc-lots-requests-v5\n> [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html\n> [3] https://lists.libcamera.org/pipermail/libcamera-devel/2021-June/021891.html\n>\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 64 +++++++++++++++++---\n> 1 file changed, 54 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 0f634b8da609..a51131dce8e0 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -45,6 +46,11 @@ public:\n> \t\t\tControlInfoMap::Map *ctrls);\n> \tvoid bufferReady(FrameBuffer *buffer);\n>\n> +\tvoid queuePendingRequests();\n> +\tvoid cancelPendingRequests();\n\nAm I wrong or this is not called in this patch (or any other related\npatch I can find). Should you call it in stop() ?\n\nThanks\n j\n\n> +\n> +\tstd::queue pendingRequests_;\n> +\n> \tstd::unique_ptr video_;\n> \tStream stream_;\n> };\n> @@ -79,12 +85,13 @@ public:\n>\n> \tbool match(DeviceEnumerator *enumerator) override;\n>\n> +\tint processControls(UVCCameraData *data, Request *request);\n> +\n> private:\n> \tstd::string generateId(const UVCCameraData *data);\n>\n> \tint processControl(ControlList *controls, unsigned int id,\n> \t\t\t const ControlValue &value);\n> -\tint processControls(UVCCameraData *data, Request *request);\n>\n> \tUVCCameraData *cameraData(const Camera *camera)\n> \t{\n> @@ -363,24 +370,59 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> \treturn ret;\n> }\n>\n> +void UVCCameraData::cancelPendingRequests()\n> +{\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n> +\t\tFrameBuffer *buffer = request->findBuffer(&stream_);\n> +\n> +\t\tbuffer->cancel();\n> +\t\tpipe_->completeBuffer(request, buffer);\n> +\t\tpipe_->completeRequest(request);\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +}\n> +\n> +void UVCCameraData::queuePendingRequests()\n> +{\n> +\tPipelineHandlerUVC *pipe = static_cast(pipe_);\n> +\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n> +\t\tFrameBuffer *buffer = request->findBuffer(&stream_);\n> +\n> +\t\tint ret = pipe->processControls(this, request);\n> +\t\tif (ret < 0) {\n> +\t\t\tbuffer->cancel();\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> +\t\t\tpipe_->completeRequest(request);\n> +\t\t\tpendingRequests_.pop();\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* If we're missing v4l2 buffer slots, try again later */\n> +\t\tret = video_->queueBuffer(buffer);\n> +\t\tif (ret < 0)\n> +\t\t\tbreak;\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +}\n> +\n> int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> -\tFrameBuffer *buffer = request->findBuffer(&data->stream_);\n> -\tif (!buffer) {\n> +\n> +\tif (!request->findBuffer(&data->stream_)) {\n> \t\tLOG(UVC, Error)\n> \t\t\t<< \"Attempt to queue request with invalid stream\";\n>\n> \t\treturn -ENOENT;\n> \t}\n>\n> -\tint ret = processControls(data, request);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> -\tret = data->video_->queueBuffer(buffer);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> +\tdata->pendingRequests_.push(request);\n> +\tdata->queuePendingRequests();\n>\n> \treturn 0;\n> }\n> @@ -668,6 +710,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer)\n>\n> \tpipe_->completeBuffer(request, buffer);\n> \tpipe_->completeRequest(request);\n> +\n> +\tqueuePendingRequests();\n> }\n>\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)\n> --\n> 2.32.0\n>","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 1723DC0100\n\tfor ;\n\tMon, 5 Jul 2021 08:08:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A9C868501;\n\tMon, 5 Jul 2021 10:08:41 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1F7168500\n\tfor ;\n\tMon, 5 Jul 2021 10:08:39 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 1B86340012;\n\tMon, 5 Jul 2021 08:08:38 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 10:09:28 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210705080928.t6tunqop2wmkishz@uno.localdomain>","References":"<20210701192951.662584-1-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210701192951.662584-1-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: uvcvideo: Add\n\tinternal request queue","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17983,"web_url":"https://patchwork.libcamera.org/comment/17983/","msgid":"<20210705123813.i2ogchebm6z62tgo@notapiano>","date":"2021-07-05T12:38:13","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: uvcvideo: Add\n\tinternal request queue","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 05, 2021 at 10:09:28AM +0200, Jacopo Mondi wrote:\n> Hi Nicolas,\n> \n> On Thu, Jul 01, 2021 at 04:29:51PM -0300, Nícolas F. R. A. Prado wrote:\n> > Add an internal queue that stores requests until there are v4l2 buffer\n> > slots available. This avoids the need to cancel requests when there is a\n> > shortage of said buffers.\n> >\n> > Signed-off-by: Nícolas F. R. A. Prado \n> > ---\n> >\n> > This is very similar to what 5a9d19210fad (\"libcamera: pipeline: ipu3: Try\n> > queuing pending requests if a buffer is available\") and 89dae5844964\n> > (\"libcamera: pipeline: ipu3: Store requests in the case a buffer shortage\") did\n> > for IPU3.\n> >\n> > I tested this with a new lc-compliance test from [1] which is a rebased version\n> > of the series in [2]. I'll resend this series after the googletest refactor\n> > series [3] is merged. But this patch for uvcvideo doesn't depend on that.\n> >\n> > [1] https://gitlab.collabora.com/nfraprado/libcamera/-/tree/lcc-lots-requests-v5\n> > [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html\n> > [3] https://lists.libcamera.org/pipermail/libcamera-devel/2021-June/021891.html\n> >\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 64 +++++++++++++++++---\n> > 1 file changed, 54 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 0f634b8da609..a51131dce8e0 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -45,6 +46,11 @@ public:\n> > \t\t\tControlInfoMap::Map *ctrls);\n> > \tvoid bufferReady(FrameBuffer *buffer);\n> >\n> > +\tvoid queuePendingRequests();\n> > +\tvoid cancelPendingRequests();\n> \n> Am I wrong or this is not called in this patch (or any other related\n> patch I can find). Should you call it in stop() ?\n\nOops, indeed, totally forgot about that... I'll fix it for v2.\n\nThanks,\nNícolas\n\n> \n> Thanks\n> j\n> \n> > +\n> > +\tstd::queue pendingRequests_;\n> > +\n> > \tstd::unique_ptr video_;\n> > \tStream stream_;\n> > };\n> > @@ -79,12 +85,13 @@ public:\n> >\n> > \tbool match(DeviceEnumerator *enumerator) override;\n> >\n> > +\tint processControls(UVCCameraData *data, Request *request);\n> > +\n> > private:\n> > \tstd::string generateId(const UVCCameraData *data);\n> >\n> > \tint processControl(ControlList *controls, unsigned int id,\n> > \t\t\t const ControlValue &value);\n> > -\tint processControls(UVCCameraData *data, Request *request);\n> >\n> > \tUVCCameraData *cameraData(const Camera *camera)\n> > \t{\n> > @@ -363,24 +370,59 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > \treturn ret;\n> > }\n> >\n> > +void UVCCameraData::cancelPendingRequests()\n> > +{\n> > +\twhile (!pendingRequests_.empty()) {\n> > +\t\tRequest *request = pendingRequests_.front();\n> > +\t\tFrameBuffer *buffer = request->findBuffer(&stream_);\n> > +\n> > +\t\tbuffer->cancel();\n> > +\t\tpipe_->completeBuffer(request, buffer);\n> > +\t\tpipe_->completeRequest(request);\n> > +\n> > +\t\tpendingRequests_.pop();\n> > +\t}\n> > +}\n> > +\n> > +void UVCCameraData::queuePendingRequests()\n> > +{\n> > +\tPipelineHandlerUVC *pipe = static_cast(pipe_);\n> > +\n> > +\twhile (!pendingRequests_.empty()) {\n> > +\t\tRequest *request = pendingRequests_.front();\n> > +\t\tFrameBuffer *buffer = request->findBuffer(&stream_);\n> > +\n> > +\t\tint ret = pipe->processControls(this, request);\n> > +\t\tif (ret < 0) {\n> > +\t\t\tbuffer->cancel();\n> > +\t\t\tpipe_->completeBuffer(request, buffer);\n> > +\t\t\tpipe_->completeRequest(request);\n> > +\t\t\tpendingRequests_.pop();\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\t/* If we're missing v4l2 buffer slots, try again later */\n> > +\t\tret = video_->queueBuffer(buffer);\n> > +\t\tif (ret < 0)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tpendingRequests_.pop();\n> > +\t}\n> > +}\n> > +\n> > int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n> > {\n> > \tUVCCameraData *data = cameraData(camera);\n> > -\tFrameBuffer *buffer = request->findBuffer(&data->stream_);\n> > -\tif (!buffer) {\n> > +\n> > +\tif (!request->findBuffer(&data->stream_)) {\n> > \t\tLOG(UVC, Error)\n> > \t\t\t<< \"Attempt to queue request with invalid stream\";\n> >\n> > \t\treturn -ENOENT;\n> > \t}\n> >\n> > -\tint ret = processControls(data, request);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > -\n> > -\tret = data->video_->queueBuffer(buffer);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > +\tdata->pendingRequests_.push(request);\n> > +\tdata->queuePendingRequests();\n> >\n> > \treturn 0;\n> > }\n> > @@ -668,6 +710,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer)\n> >\n> > \tpipe_->completeBuffer(request, buffer);\n> > \tpipe_->completeRequest(request);\n> > +\n> > +\tqueuePendingRequests();\n> > }\n> >\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)\n> > --\n> > 2.32.0\n> >","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 D52E3BD794\n\tfor ;\n\tMon, 5 Jul 2021 12:38:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DA36684F9;\n\tMon, 5 Jul 2021 14:38:51 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E0746050A\n\tfor ;\n\tMon, 5 Jul 2021 14:38:50 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nfraprado) with ESMTPSA id BD6901F419A0"],"Date":"Mon, 5 Jul 2021 09:38:13 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Jacopo Mondi ","Message-ID":"<20210705123813.i2ogchebm6z62tgo@notapiano>","References":"<20210701192951.662584-1-nfraprado@collabora.com>\n\t<20210705080928.t6tunqop2wmkishz@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210705080928.t6tunqop2wmkishz@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: uvcvideo: Add\n\tinternal request queue","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]