From patchwork Wed Dec 28 22:29:44 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 18056 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id C7FA9C3220 for ; Wed, 28 Dec 2022 22:30:18 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 1DA27625D0; Wed, 28 Dec 2022 23:30:18 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1672266618; bh=pWuzyF10+nR4MJlHuQB8/4jXNajYIvSyTGTpno7HQtk=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=L2dT74D4G98b0feIPIXzGuTbeq/luAHlj1UcprfzB6gSaZ/FijzJAAM/tH52+iphY xdmVkbwhp3ZB1T6GjDU0cRe5RhYZT4wLv4lkNAfVx4xvPuq04H1bYWvqP91jXUtnOx kYUAStwXUkEPCt8samt0cWj5w4gtWAmXOlC9AUPeI5MOZr/3Rqt29JHSQazhfvHrXA Xtg7XAGDhwEeifjGlxdZEytqZ7Ka8AamO2A/Cq44QXQH7Siw5bV5f8EM9XbFeVwNZP i1H0wQYZ8BEjfMDPvHf45E1n/h9iuUlU5h3aCMlH4IxnXBKtBe3zbW6BXT+8sBbS0Y Ov2YGqrieissQ== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8386A61F13 for ; Wed, 28 Dec 2022 23:30:16 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="GUnXUUHU"; dkim-atps=neutral Received: from pyrite.mediacom.info (unknown [IPv6:2604:2d80:ad8a:9000:1bf9:855b:22de:3645]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EE6F109; Wed, 28 Dec 2022 23:30:15 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1672266616; bh=pWuzyF10+nR4MJlHuQB8/4jXNajYIvSyTGTpno7HQtk=; h=From:To:Cc:Subject:Date:From; b=GUnXUUHU/xtntQ00nyqNTL2aeyHta//LlOUOeyuIQKd+7JBQbDTHJZmV0F7VackjL 36eMf3LI2nqPH1VuLMT3yaxTz2hYaNNXhcB9nfaLyvhZFimBg6cb6C3/APAbkU+tii bZI5kxY6qD3x5Lru0+NZ5j69AaO6ZFPOnt/v/5Zw= To: libcamera-devel@lists.libcamera.org Date: Wed, 28 Dec 2022 16:29:44 -0600 Message-Id: <20221228223003.2265712-1-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.35.1 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v10 00/19] lc-compliance: Add test to queue more requests than hardware depth X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Paul Elder via libcamera-devel From: Paul Elder Reply-To: Paul Elder Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The purpose of this series is to add a new test to lc-compliance that tests queuing a lot of requests at once in order to ensure that pipeline handlers are able to handle more requests than they have resources for (like internal buffers and V4L2 buffer slots) [1]. [1] https://bugs.libcamera.org/show_bug.cgi?id=24 In order to achieve this, the FrameBufferAllocator had to be adapted to allow an arbitrary number of buffers to be allocated. But there's also the issue of reporting the minimum number of requests required by the pipeline handler, which was solved by creating a new MinimumRequests property. Briefly, patches 2 through 10 rework the core and pipeline handlers to use MinimumRequests and remove bufferCount, while patches 10 through 17 rework lc-compliance to add the new test and an additional test for MinimumRequests. Patch 2 adds the new MinimumRequests property to report the minimum number of requests needed by the pipeline handler. Patch 3 adds a count argument to allocate() so that the number of buffers to allocate needs to be specified, as it is no longer assumed through bufferCount. Patches 4-6 decouple the number of internal buffers and V4L2 buffer slots from bufferCount in each pipeline handler. Patch 9 reworks the V4L2 compatibility layer to not depend on bufferCount. Patch 10 removes bufferCount from the StreamConfiguration and everywhere it was still used, as it is no longer needed. Patch 11 fixes a file ordering issue in lc-compliance's meson.build. Patches 12-13 does some refactoring in lc-compliance in order to reduce code duplication. Patch 16 adds the test to lc-compliance. Patch 17 adds checks in lc-compliance to ensure that requests which failed to be enqueued are reported as test failure. Patch 18 adds another, very short, test to lc-compliance to make sure that the MinimumRequests property is set in the pipeline handler. Nícolas had run v8 of this new lc-compliance test on the raspberrypi, rkisp1, uvcvideo and vimc pipelines. raspberrypi already handles it well, while the other three run successfully after applying the series in [2]. The ipu3 should run fine as well since the series in [2] was based on the internal queue already present there. A patch for the simple pipeline is still pending. [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022508.html The number of buffers to allocate in the lc-compliance test (patch 15) was hardcoded to 8 since more than that would cause errors when allocating. v7: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022577.html v6: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022356.html v5: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022098.html v4: https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html v3: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019613.html v2: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019398.html v1: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019139.html Changes in v10: - add imx8-isi, pycamera, and android - update MinimumRequests description and pipeline hander guide - clean up raspberry pi and ipu3 (compared to the last version) - rename variables in simple Changes in v9: - picked up and rebased by Paul Changes in v8: (thanks to Laurent and Kieran) - Changed internal buffer count constants for pipeline handlers to better values - Reordered patches to group non-lc-compliance changes together - Split buffer allocation changes into separate commits for each pipeline handler (patches 3-7) - Changed the MinimumRequests property meaning to require that frames aren't dropped - Set MinimumRequests on SimplePipeline depending on device and usage of converter - Undid definition of default MinimumRequests on CameraSensor constructor - Updated application-developer and pipeline-handler guides with new allocate() API and MinimumRequests property - Added handling for when allocate() returns less buffers than needed in cam and the capture unit test - Reworked buffer allocation handling in the raspberrypi pipeline handler - Moved V4L2 compatibility layer changes to separate commit - Added patch 10 to fix wrong file order in lc-compliance's meson.build - Added requests_ member to SimpleCapture to hold ownership of queued requests during capture - Moved CameraHolder to new test_base.{cpp,h} files - Fixed issue in UnbalancedStop test where requests cancelled due to stop() call were failing the test - Moved RequiredProperties test to property_test.cpp - Moved CameraTests to new test_base.{cpp,h} files Changes in v7: (thanks to Kieran and Jacopo) - Renamed property from MinNumRequests to MinimumRequests - Changed MinimumRequests property's description - Added patch 11 to test if MinimumRequests is valid Changes in v6: (thanks to Naushir) - Fixed style issues - Changed static_cast to unsigned int when comparing buffer count in lc-compliance - Added pipeline prefix to INTERNAL_BUFFER_COUNT and BUFFER_SLOT_COUNT constants - Removed comment from Raspberrypi MinNumRequests setting - Switched queueRequests()'s 'buffers' and 'requests' parameters order, since 'requests' is an output variable - Added comment to runCaptureSession() Changes in v5: - Rebased on master (now that lc-compliance was refactored to use Googletest) - Added patches 3, 5, 6 and 8 - Fixed qcam to use at least two buffers - Made sure that qcam allocates at least 2 buffers Changes in v4: (thanks to Laurent and Niklas) - Renamed QueueDepth property to MinNumRequests and better documented it - Changed patch 6 to also remove bufferCount from android - Added patch 3 to factor common code in lc-compliance - Added patch 5 to remove pipeline dependency on bufferCount Changes in v3: - Added patches 1 and 4 to add the QueueDepth property and remove bufferCount - Made the count argument required in patch 2 - Added previously missing changes to the gstreamer and V4L2 compatibility layers Changes in v2: - Renamed and reworded commits and series - Dropped patches 2 and 3, which were hacks to test, and added patch 1 to add count to FrameBufferAllocator - Thanks to Niklas: - Created new standalone test instead of looping over the other tests Nícolas F. R. A. Prado (17): libcamera: property: Add MinimumRequests property libcamera: framebuffer_allocator: Make allocate() require count libcamera: pipeline: raspberrypi: Don't rely on bufferCount libcamera: pipeline: ipu3: Don't rely on bufferCount libcamera: pipeline: simple: Don't rely on bufferCount libcamera: pipeline: rkisp1: Don't rely on bufferCount libcamera: pipeline: vimc, uvcvideo: Don't rely on bufferCount v4l2: Allocate buffers based on requested count and MinimumRequests libcamera: stream: Remove bufferCount lc-compliance: Fix source file ordering in meson.build lc-compliance: Move buffer allocation to separate function lc-compliance: Factor common capture code into SimpleCapture lc-compliance: Move camera setup to CameraHolder class lc-compliance: Move role to string conversion to its own function lc-compliance: Add test to queue more requests than hardware depth lc-compliance: Check that requests complete successfully lc-compliance: Add test to ensure MinimumRequests is valid Paul Elder (2): pipeline: rkisp1: Reorder headers to appease the linter libcamera: pipeline: imx8-isi: Don't rely on bufferCount .../guides/application-developer.rst | 9 +- Documentation/guides/pipeline-handler.rst | 38 +++-- include/libcamera/camera.h | 2 +- include/libcamera/framebuffer_allocator.h | 2 +- include/libcamera/internal/converter.h | 2 +- .../internal/converter/converter_v4l2_m2m.h | 7 +- include/libcamera/internal/pipeline_handler.h | 2 +- include/libcamera/stream.h | 2 - src/android/camera_stream.cpp | 5 +- src/apps/cam/camera_session.cpp | 13 +- src/apps/lc-compliance/capture_test.cpp | 92 ++++++++--- src/apps/lc-compliance/meson.build | 4 +- src/apps/lc-compliance/property_test.cpp | 24 +++ src/apps/lc-compliance/simple_capture.cpp | 143 ++++++++++++------ src/apps/lc-compliance/simple_capture.h | 25 ++- src/apps/lc-compliance/test_base.cpp | 38 +++++ src/apps/lc-compliance/test_base.h | 31 ++++ src/apps/qcam/main_window.cpp | 10 +- src/gstreamer/gstlibcameraallocator.cpp | 5 +- src/libcamera/camera.cpp | 4 +- src/libcamera/converter.cpp | 2 + .../converter/converter_v4l2_m2m.cpp | 15 +- src/libcamera/framebuffer_allocator.cpp | 9 +- src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 13 +- src/libcamera/pipeline/ipu3/cio2.cpp | 8 +- src/libcamera/pipeline/ipu3/cio2.h | 4 +- src/libcamera/pipeline/ipu3/imgu.cpp | 13 +- src/libcamera/pipeline/ipu3/imgu.h | 3 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 59 +++++--- .../pipeline/raspberrypi/raspberrypi.cpp | 14 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 30 ++-- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 5 +- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 +- src/libcamera/pipeline/simple/simple.cpp | 71 +++++++-- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 ++- src/libcamera/pipeline/vimc/vimc.cpp | 17 ++- src/libcamera/pipeline_handler.cpp | 1 + src/libcamera/property_ids.yaml | 22 +++ src/libcamera/stream.cpp | 12 +- src/py/libcamera/py_main.cpp | 1 - src/v4l2/v4l2_camera.cpp | 22 ++- src/v4l2/v4l2_camera.h | 5 +- src/v4l2/v4l2_camera_proxy.cpp | 10 +- test/camera/buffer_import.cpp | 11 +- test/camera/camera_reconfigure.cpp | 5 +- test/camera/capture.cpp | 11 +- test/camera/statemachine.cpp | 5 +- test/fence.cpp | 6 +- test/libtest/buffer_source.cpp | 4 +- test/libtest/buffer_source.h | 2 +- test/mapped-buffer.cpp | 5 +- test/v4l2_videodevice/buffer_cache.cpp | 3 +- 52 files changed, 607 insertions(+), 260 deletions(-) create mode 100644 src/apps/lc-compliance/property_test.cpp create mode 100644 src/apps/lc-compliance/test_base.cpp create mode 100644 src/apps/lc-compliance/test_base.h