| Message ID | 20210722232851.747614-1-nfraprado@collabora.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Nícolas, Thank you for the patch. 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 > > 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(-) > > -- > 2.32.0 >
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(-)