[{"id":26105,"web_url":"https://patchwork.libcamera.org/comment/26105/","msgid":"<20221216153225.ueqgg3losgyhod2t@uno.localdomain>","date":"2022-12-16T15:32:25","subject":"Re: [libcamera-devel] [PATCH v9 00/18] lc-compliance: Add test to\n\tqueue more requests than hardware depth","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul\n\n you have missed porting the newly introduced ISI pipeline handler\n\nOn Fri, Dec 16, 2022 at 09:29:21PM +0900, Paul Elder via libcamera-devel wrote:\n> The purpose of this series is to add a new test to lc-compliance that tests\n> queuing a lot of requests at once in order to ensure that pipeline handlers are\n> able to handle more requests than they have resources for (like internal buffers\n> and V4L2 buffer slots) [1].\n>\n> [1] https://bugs.libcamera.org/show_bug.cgi?id=24\n>\n> In order to achieve this, the FrameBufferAllocator had to be adapted to allow an\n> arbitrary number of buffers to be allocated. But there's also the issue of\n> reporting the minimum number of requests required by the pipeline handler, which\n> was solved by creating a new MinimumRequests property.\n>\n> Briefly, patches 2 through 10 rework the core and pipeline handlers to use\n> MinimumRequests and remove bufferCount, while patches 10 through 17 rework\n> lc-compliance to add the new test and an additional test for MinimumRequests.\n>\n> Patch 2 adds the new MinimumRequests property to report the minimum number of\n> requests needed by the pipeline handler.\n>\n> Patch 3 adds a count argument to allocate() so that the number of buffers to\n> allocate needs to be specified, as it is no longer assumed through bufferCount.\n>\n> Patches 4-6 decouple the number of internal buffers and V4L2 buffer slots from\n> bufferCount in each pipeline handler.\n>\n> Patch 9 reworks the V4L2 compatibility layer to not depend on bufferCount.\n>\n> Patch 10 removes bufferCount from the StreamConfiguration and everywhere it was\n> still used, as it is no longer needed.\n>\n> Patch 11 fixes a file ordering issue in lc-compliance's meson.build.\n>\n> Patches 12-13 does some refactoring in lc-compliance in order to reduce code\n> duplication.\n>\n> Patch 16 adds the test to lc-compliance.\n>\n> Patch 17 adds checks in lc-compliance to ensure that requests which failed to be\n> enqueued are reported as test failure.\n>\n> Patch 18 adds another, very short, test to lc-compliance to make sure that the\n> MinimumRequests property is set in the pipeline handler.\n>\n> Nícolas had run v8 of this new lc-compliance test on the raspberrypi,\n> rkisp1, uvcvideo and vimc pipelines. raspberrypi already handles it\n> well, while the other three run successfully after applying the series\n> in [2]. The ipu3 should run fine as well since the series in [2] was\n> based on the internal queue already present there. A patch for the\n> simple pipeline is still pending.\n>\n> [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022508.html\n>\n> The number of buffers to allocate in the lc-compliance test (patch 15) was\n> hardcoded to 8 since more than that would cause errors when allocating.\n>\n> v7: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022577.html\n> v6: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022356.html\n> v5: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022098.html\n> v4: https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html\n> v3: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019613.html\n> v2: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019398.html\n> v1: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019139.html\n>\n> Changes in v9:\n> - picked up and rebased by Paul\n>\n> Changes in v8:\n> (thanks to Laurent and Kieran)\n> - Changed internal buffer count constants for pipeline handlers to better values\n> - Reordered patches to group non-lc-compliance changes together\n> - Split buffer allocation changes into separate commits for each pipeline\n> handler (patches 3-7)\n> - Changed the MinimumRequests property meaning to require that frames aren't\n> dropped\n> - Set MinimumRequests on SimplePipeline depending on device and usage of\n> converter\n> - Undid definition of default MinimumRequests on CameraSensor constructor\n> - Updated application-developer and pipeline-handler guides with new allocate()\n> API and MinimumRequests property\n> - Added handling for when allocate() returns less buffers than needed in cam and\n> the capture unit test\n> - Reworked buffer allocation handling in the raspberrypi pipeline handler\n> - Moved V4L2 compatibility layer changes to separate commit\n> - Added patch 10 to fix wrong file order in lc-compliance's meson.build\n> - Added requests_ member to SimpleCapture to hold ownership of queued\n> requests during capture\n> - Moved CameraHolder to new test_base.{cpp,h} files\n> - Fixed issue in UnbalancedStop test where requests cancelled due to stop() call\n> were failing the test\n> - Moved RequiredProperties test to property_test.cpp\n> - Moved CameraTests to new test_base.{cpp,h} files\n>\n> Changes in v7:\n> (thanks to Kieran and Jacopo)\n> - Renamed property from MinNumRequests to MinimumRequests\n> - Changed MinimumRequests property's description\n> - Added patch 11 to test if MinimumRequests is valid\n>\n> Changes in v6:\n> (thanks to Naushir)\n> - Fixed style issues\n> - Changed static_cast to unsigned int when comparing buffer count in\n> lc-compliance\n> - Added pipeline prefix to INTERNAL_BUFFER_COUNT and BUFFER_SLOT_COUNT constants\n> - Removed comment from Raspberrypi MinNumRequests setting\n> - Switched queueRequests()'s 'buffers' and 'requests' parameters order, since\n> 'requests' is an output variable\n> - Added comment to runCaptureSession()\n>\n> Changes in v5:\n> - Rebased on master (now that lc-compliance was refactored to use Googletest)\n> - Added patches 3, 5, 6 and 8\n> - Fixed qcam to use at least two buffers\n> - Made sure that qcam allocates at least 2 buffers\n>\n> Changes in v4:\n> (thanks to Laurent and Niklas)\n> - Renamed QueueDepth property to MinNumRequests and better documented it\n> - Changed patch 6 to also remove bufferCount from android\n> - Added patch 3 to factor common code in lc-compliance\n> - Added patch 5 to remove pipeline dependency on bufferCount\n>\n> Changes in v3:\n> - Added patches 1 and 4 to add the QueueDepth property and remove bufferCount\n> - Made the count argument required in patch 2\n> - Added previously missing changes to the gstreamer and V4L2 compatibility\n> layers\n>\n> Changes in v2:\n> - Renamed and reworded commits and series\n> - Dropped patches 2 and 3, which were hacks to test, and added patch 1 to add\n> count to FrameBufferAllocator\n> - Thanks to Niklas:\n> - Created new standalone test instead of looping over the other tests\n>\n> Nícolas F. R. A. Prado (17):\n> libcamera: property: Add MinimumRequests property\n> libcamera: framebuffer_allocator: Make allocate() require count\n> libcamera: pipeline: raspberrypi: Don't rely on bufferCount\n> libcamera: pipeline: ipu3: Don't rely on bufferCount\n> libcamera: pipeline: simple: Don't rely on bufferCount\n> libcamera: pipeline: rkisp1: Don't rely on bufferCount\n> libcamera: pipeline: vimc, uvcvideo: Don't rely on bufferCount\n> v4l2: Allocate buffers based on requested count and MinimumRequests\n> libcamera: stream: Remove bufferCount\n> lc-compliance: Fix source file ordering in meson.build\n> lc-compliance: Move buffer allocation to separate function\n> lc-compliance: Factor common capture code into SimpleCapture\n> lc-compliance: Move camera setup to CameraHolder class\n> lc-compliance: Move role to string conversion to its own function\n> lc-compliance: Add test to queue more requests than hardware depth\n> lc-compliance: Check that requests complete successfully\n> lc-compliance: Add test to ensure MinimumRequests is valid\n>\n> Paul Elder (1):\n> pipeline: rkisp1: Reorder headers to appease the linter\n>\n> .../guides/application-developer.rst | 9 +-\n> Documentation/guides/pipeline-handler.rst | 40 +++--\n> include/libcamera/camera.h | 2 +-\n> include/libcamera/framebuffer_allocator.h | 2 +-\n> include/libcamera/internal/converter.h | 3 +-\n> .../internal/converter/converter_v4l2_m2m.h | 9 +-\n> include/libcamera/internal/pipeline_handler.h | 2 +-\n> include/libcamera/stream.h | 2 -\n> src/android/camera_stream.cpp | 2 +-\n> src/apps/cam/camera_session.cpp | 13 +-\n> src/apps/lc-compliance/capture_test.cpp | 92 ++++++++---\n> src/apps/lc-compliance/meson.build | 4 +-\n> src/apps/lc-compliance/property_test.cpp | 24 +++\n> src/apps/lc-compliance/simple_capture.cpp | 143 ++++++++++++------\n> src/apps/lc-compliance/simple_capture.h | 25 ++-\n> src/apps/lc-compliance/test_base.cpp | 38 +++++\n> src/apps/lc-compliance/test_base.h | 31 ++++\n> src/apps/qcam/main_window.cpp | 10 +-\n> src/gstreamer/gstlibcameraallocator.cpp | 5 +-\n> src/libcamera/camera.cpp | 4 +-\n> .../converter/converter_v4l2_m2m.cpp | 15 +-\n> src/libcamera/framebuffer_allocator.cpp | 9 +-\n> src/libcamera/pipeline/ipu3/cio2.cpp | 5 +-\n> src/libcamera/pipeline/ipu3/cio2.h | 16 +-\n> src/libcamera/pipeline/ipu3/imgu.cpp | 12 +-\n> src/libcamera/pipeline/ipu3/imgu.h | 15 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 34 ++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 95 ++++--------\n> .../pipeline/raspberrypi/rpi_stream.cpp | 39 ++---\n> .../pipeline/raspberrypi/rpi_stream.h | 24 ++-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 29 ++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 5 +-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 +-\n> src/libcamera/pipeline/simple/simple.cpp | 72 +++++++--\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 ++-\n> src/libcamera/pipeline/vimc/vimc.cpp | 17 ++-\n> src/libcamera/pipeline_handler.cpp | 1 +\n> src/libcamera/property_ids.yaml | 21 +++\n> src/libcamera/stream.cpp | 12 +-\n> src/v4l2/v4l2_camera.cpp | 22 ++-\n> src/v4l2/v4l2_camera.h | 5 +-\n> src/v4l2/v4l2_camera_proxy.cpp | 10 +-\n> test/camera/buffer_import.cpp | 11 +-\n> test/camera/camera_reconfigure.cpp | 5 +-\n> test/camera/capture.cpp | 11 +-\n> test/camera/statemachine.cpp | 5 +-\n> test/fence.cpp | 6 +-\n> test/libtest/buffer_source.cpp | 4 +-\n> test/libtest/buffer_source.h | 2 +-\n> test/mapped-buffer.cpp | 5 +-\n> test/v4l2_videodevice/buffer_cache.cpp | 3 +-\n> 51 files changed, 667 insertions(+), 324 deletions(-)\n> create mode 100644 src/apps/lc-compliance/property_test.cpp\n> create mode 100644 src/apps/lc-compliance/test_base.cpp\n> create mode 100644 src/apps/lc-compliance/test_base.h\n>\n> --\n> 2.35.1\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 4352CC3200\n\tfor ;\n\tFri, 16 Dec 2022 15:32:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 015EE6339A;\n\tFri, 16 Dec 2022 16:32:28 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::222])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A8E6603D0\n\tfor ;\n\tFri, 16 Dec 2022 16:32:26 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id E41064000C;\n\tFri, 16 Dec 2022 15:32:25 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671204748;\n\tbh=J0aw08LhTnDk/AYrg3Y3DuUQG1lcL/NrPUZ4/1cG/Sc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=DEAIlRb/Rka7t1rq+lDZA6xZNPKTq/rcSBnhcQBGFVZW+OYd24zRuGw+ef6pScuaS\n\t/nsVr0WKV7MOrEPsnVkBzNMfxPyqMdRTNlUVJQmousOLXwFx1u/o68y4KrGtiD57KL\n\t/u4qQlPpGDTbUbRxI23w6sRm8w0IhncYWv5YMD5MZOvRPVjEOPfN/TGlPr7Wkgi6T7\n\tjkeyhCa56WYXdzGdFIeKlqzGBqOO1KytqFnlkNcQxQFMy4k0RJqGOcd7MI7ewY1OW7\n\tHOKassLA6x7lg5eCpayUaFViEuq7gGHUL7wCKo3kfpfNnkYyf/hSQZ2UKBguPViNcj\n\t2iWRUMMySC9qw==","Date":"Fri, 16 Dec 2022 16:32:25 +0100","To":"Paul Elder ","Message-ID":"<20221216153225.ueqgg3losgyhod2t@uno.localdomain>","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221216122939.256534-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v9 00/18] lc-compliance: Add test to\n\tqueue more requests than hardware depth","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]