[libcamera-devel] [PATCH v7 00/11] lc-compliance: Add test to queue more requests than hardware depth
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 2 00:23:20 CEST 2021
Hi Nícolas,
Another comment.
On Mon, Aug 02, 2021 at 12:23:10AM +0300, Laurent Pinchart wrote:
> On Thu, Jul 22, 2021 at 08:28:40PM -0300, Nícolas F. R. A. Prado wrote:
> > 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 amount of buffers to be allocated. But there's also the issue of
> > reporting the minimum amount of requests required by the pipeline handler, which
> > was solved by creating a new MinNumRequests property.
>
> It's now called MinimumRequests :-)
>
> > So patch 1 adds the new MinNumRequests property to report the minimum amount of
> > requests needed by the pipeline handler.
> >
> > Patch 2 adds a count argument to allocate() so that the amount of buffers to
> > allocate needs to be specified, as it is no longer assumed through bufferCount.
> >
> > Patches 3-6 does some refactoring in lc-compliance in order to reduce code
> > duplication.
> >
> > Patch 7 adds the test to lc-compliance.
> >
> > Patch 8 adds checks to ensure that requests which failed to be enqueued are
> > reported as test failure.
> >
> > Patch 9 replaces all usage of bufferCount for allocateBuffers() and
> > importBuffers() in pipeline handlers to constants, as bufferCount wasn't a good
> > fit and to allow its removal. This is intended as a temporary measure, and
> > pipeline handlers will probably want to improve the logic of those values.
> >
> > Patch 10 removes bufferCount from the StreamConfiguration as it is no longer
> > needed.
>
> Would it be possible to move patches 8 and 9 right after 2, to separate
> the libcamera changes from the lc-compliance changes ? It doesn't seem
> to cause any conflict.
>
> > Patch 11 adds a test to verify that the MinimumRequests property added is set
> > and valid in the pipeline handler.
> >
> > I've run 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.
> >
> > [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022508.html
Doesn't the simple pipeline handler need to be fixed in a similar way ?
> > The amount of buffers to allocate in the lc-compliance test (patch 7) was
> > hardcoded to 8 since more than that would cause errors when allocating.
> >
> > Changes in v7:
> > - Thanks to Kieran:
> > - Renamed property from MinNumRequests to MinimumRequests (patches 1, 2, 3, 10)
> > - Added patch 11 to test if MinimumRequests is valid
> > - Thanks to Jacopo:
> > - Changed description of MinimumRequests
> >
> > v6: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022356.html
> >
> > Changes in v6:
> > - Thanks to Naushir:
> > - Removed unneeded comment on MinNumRequests property of RPi
> > - Fixed style issues
> > - Changed static_cast to unsigned int when comparing buffer count in
> > lc-compliance
> > - Changed parameter order of queueRequests()
> > - Added comment on runCaptureSession()
> > - Added pipeline prefix to INTERNAL_BUFFER_COUNT and BUFFER_SLOT_COUNT constants
> > - Removed unused variable in the IPU3 pipeline
> >
> > v5: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022098.html
> >
> > 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
> >
> > v4: https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html
> >
> > Changes in v4:
> > - Thanks to Laurent:
> > - Renamed QueueDepth property to MinNumRequests and better documented it
> > - Changed patch 6 to also remove bufferCount from android
> > - Thanks to Niklas:
> > - Added patch 3 to factor common code in lc-compliance
> > - Added patch 5 to remove pipeline dependency on bufferCount
> >
> > v3: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019613.html
> >
> > 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
> >
> > v2: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019398.html
> >
> > 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
> >
> > v1: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019139.html
> >
> > Nícolas F. R. A. Prado (11):
> > libcamera: property: Add MinimumRequests property
> > libcamera: framebuffer_allocator: Make allocate() require count
> > 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
> > libcamera: pipeline: Don't rely on bufferCount
> > libcamera: stream: Remove bufferCount
> > lc-compliance: Add test to ensure MinimumRequests is valid
> >
> > include/libcamera/camera.h | 2 +-
> > include/libcamera/framebuffer_allocator.h | 2 +-
> > include/libcamera/internal/pipeline_handler.h | 2 +-
> > include/libcamera/stream.h | 2 -
> > src/android/camera_stream.cpp | 7 +-
> > src/cam/capture.cpp | 9 +-
> > src/gstreamer/gstlibcameraallocator.cpp | 4 +-
> > src/lc-compliance/capture_test.cpp | 138 +++++++++++++---
> > src/lc-compliance/simple_capture.cpp | 150 +++++++++++++-----
> > src/lc-compliance/simple_capture.h | 27 +++-
> > src/libcamera/camera.cpp | 4 +-
> > src/libcamera/camera_sensor.cpp | 1 +
> > src/libcamera/framebuffer_allocator.cpp | 5 +-
> > src/libcamera/pipeline/ipu3/cio2.cpp | 1 -
> > src/libcamera/pipeline/ipu3/imgu.cpp | 12 +-
> > src/libcamera/pipeline/ipu3/imgu.h | 5 +-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 21 +--
> > .../pipeline/raspberrypi/raspberrypi.cpp | 25 +--
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +-
> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +-
> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +
> > src/libcamera/pipeline/simple/converter.cpp | 7 +-
> > src/libcamera/pipeline/simple/converter.h | 6 +-
> > src/libcamera/pipeline/simple/simple.cpp | 17 +-
> > 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 | 10 ++
> > src/libcamera/stream.cpp | 9 +-
> > src/qcam/main_window.cpp | 11 +-
> > src/v4l2/v4l2_camera.cpp | 16 +-
> > src/v4l2/v4l2_camera.h | 5 +-
> > src/v4l2/v4l2_camera_proxy.cpp | 8 +-
> > test/camera/buffer_import.cpp | 10 +-
> > test/camera/capture.cpp | 4 +-
> > test/camera/statemachine.cpp | 4 +-
> > test/libtest/buffer_source.cpp | 4 +-
> > test/libtest/buffer_source.h | 2 +-
> > test/mapped-buffer.cpp | 4 +-
> > test/v4l2_videodevice/buffer_cache.cpp | 4 +-
> > 40 files changed, 389 insertions(+), 206 deletions(-)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list