[libcamera-devel] [PATCH v6 01/10] libcamera: property: Add MinNumRequests property
Nícolas F. R. A. Prado
nfraprado at collabora.com
Thu Jul 15 19:16:58 CEST 2021
Hi Kieran,
On Thu, Jul 15, 2021 at 05:45:58PM +0100, Kieran Bingham wrote:
> Hi Nicolas,
>
> On 15/07/2021 15:52, Nícolas F. R. A. Prado wrote:
> > Hi Kieran,
> >
> > On Thu, Jul 15, 2021 at 10:45:56AM +0100, Kieran Bingham wrote:
> >> Hi Nicolas,
> >>
> >> On 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:
> >>> The MinNumRequests property reports the bare minimum number of requests
> >>> needed in the camera pipeline for capture to be possible. It is equal to
> >>> the number of buffers required by the driver. At this quantity, there's
> >>> no guarantee that frames won't be dropped or that manual per-frame
> >>> controls will apply correctly. The quantity needed for those may be
> >>> added as separate properties in the future.
> >>>
> >>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> >>> ---
> >>>
> >>> Changes in v6:
> >>> - Thanks to Naushir:
> >>> - Removed comment from Raspberrypi MinNumRequests setting
> >>> - Removed an extra blank line
> >>>
> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++
> >>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++
> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++
> >>> src/libcamera/pipeline/simple/simple.cpp | 2 ++
> >>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
> >>> src/libcamera/pipeline/vimc/vimc.cpp | 2 ++
> >>> src/libcamera/property_ids.yaml | 8 ++++++++
> >>> 7 files changed, 24 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index 76c3bb3d8aa9..017018c845fa 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()
> >>> /* Initialize the camera properties. */
> >>> data->properties_ = cio2->sensor()->properties();
> >>>
> >>> + /*
> >>> + * \todo Make sure this is correct even after the stream is
> >>> + * configured to CIO2
> >>
> >> What does this comment mean?
> >>
> >> I see that you don't have a reference to IPU3 in your cover letter, so
> >> perhaps you're not able to test and validate this.
> >>
> >> What needs to be tested further?
> >>
> >> I'd rather see this tested and set correctly with the series than being
> >> incorrectly introduced, and having to fix up later, as IPU3 is one of
> >> our key targets.
> >
> > I'm just not sure that the IPU3 driver can work with a single request in all
> > StreamConfigurations (maybe different pipeline topologies require different
> > number of requests?).
> >
> > As you noted I don't have the hardware to test. So if you could, testing this
> > would be just a matter of running lc-compliance with the new Overflow test:
> >
> > lc-compliance -f '*Overflow*'
> >
>
> Using camera \_SB_.PCI0.I2C2.CAM0
> Note: Google Test filter = *Overflow*
> [==========] Running 4 tests from 1 test suite.
> [----------] Global test environment set-up.
> [----------] 4 tests from CaptureTests/RoleParametrizedTest
> [ RUN ] CaptureTests/RoleParametrizedTest.Overflow/Raw
> [2:17:21.161407453] [25870] INFO Camera camera.cpp:906 configuring
> streams: (0) 4224x3136-SGRBG10_IPU3
> [2:17:21.161682517] [25871] ERROR V4L2 v4l2_subdevice.cpp:286 'ov13858
> 8-0010': Unable to get rectangle 0 on pad 0: Inappropriate ioctl for device
> [2:17:21.161703674] [25871] WARN CameraSensor camera_sensor.cpp:737
> 'ov13858 8-0010': The analogue crop rectangle has been defaulted to the
> active area size
>
> <hangs>
>
> But I'm not sure if that's this overflow tests fault, as I'm not
> convinced it runs without your series yet anyway, as it looks like the
> tests are failing due to incomplete Raw stream support perhaps ...
Hmm. Well, you could try running one of the current raw tests, eg
lc-compliance -f '*Capture/Raw_5'
and also a test with a different role
lc-compliance -f '*Capture/StillCapture_5'
to find out if it's the pipeline or the Overflow test's fault.
You could also run the Overflow test for all roles but Raw with
lc-compliance -f '*Overflow*:-*Raw*'
although even if all pass, we might still be missing an issue with this test on
the raw streamrole...
Thanks,
Nícolas
>
>
>
>
> > If it passes in all StreamConfigurations then we're good and I can remove that
> > comment :).
> >
> >>
> >>
> >>
> >>> + */
> >>> + data->properties_.set(properties::MinNumRequests, 1);
> >>> +
> >>> ret = initControls(data.get());
> >>> if (ret)
> >>> continue;
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> index f821d8fe1b6c..98a97ffca15d 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >>> /* Initialize the camera properties. */
> >>> data->properties_ = data->sensor_->properties();
> >>>
> >>> + data->properties_.set(properties::MinNumRequests, 1);
> >>> +
> >>> /*
> >>> * Set a default value for the ScalerCropMaximum property to show
> >>> * that we support its use, however, initialise it to zero because
> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> index 42911a8fdfdb..c81ebf14c6bf 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> @@ -24,6 +24,7 @@
> >>> #include <libcamera/ipa/core_ipa_interface.h>
> >>> #include <libcamera/ipa/rkisp1_ipa_interface.h>
> >>> #include <libcamera/ipa/rkisp1_ipa_proxy.h>
> >>> +#include <libcamera/property_ids.h>
> >>> #include <libcamera/request.h>
> >>> #include <libcamera/stream.h>
> >>>
> >>> @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >>>
> >>> /* Initialize the camera properties. */
> >>> data->properties_ = data->sensor_->properties();
> >>> + data->properties_.set(properties::MinNumRequests, 3);
> >>>
> >>> /*
> >>> * \todo Read dealy values from the sensor itself or from a
> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >>> index b29fff9820e5..c4adea61519f 100644
> >>> --- a/src/libcamera/pipeline/simple/simple.cpp
> >>> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >>> @@ -25,6 +25,7 @@
> >>>
> >>> #include <libcamera/camera.h>
> >>> #include <libcamera/control_ids.h>
> >>> +#include <libcamera/property_ids.h>
> >>> #include <libcamera/request.h>
> >>> #include <libcamera/stream.h>
> >>>
> >>> @@ -436,6 +437,7 @@ int SimpleCameraData::init()
> >>> }
> >>>
> >>> properties_ = sensor_->properties();
> >>> + properties_.set(properties::MinNumRequests, 3);
> >>>
> >>> return 0;
> >>> }
> >>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>> index 0f634b8da609..0258111ad6cf 100644
> >>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>> @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)
> >>> properties_.set(properties::PixelArraySize, resolution);
> >>> properties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });
> >>>
> >>> + properties_.set(properties::MinNumRequests, 1);
> >>
> >> Is there any value in initialising this to one in the CameraSensor
> >> constructor? Or is it better to make sure every pipeline sets it.
> >
> > I think it would make sense to have this set to 1 by default in the CameraSensor
> > constructor, and any pipeline that has a different value can override it.
> >
> > One issue though is that the uvcvideo pipeline doesn't have a CameraSensor to
> > initialize its properties from. So we'd still need to manually set it in that
> > case.
> >
> >>
> >> Will lc-complicance have a test to ensure that all pipelines set this?
> >
> > I hadn't created such a test. If we don't go that route of setting it by default
> > I'll create it.
> >
> >>
> >>
> >>> +
> >>> /* Initialise the supported controls. */
> >>> ControlInfoMap::Map ctrls;
> >>>
> >>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> >>> index 12f7517fd0ae..60ec473db0ba 100644
> >>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> >>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> >>> @@ -21,6 +21,7 @@
> >>> #include <libcamera/control_ids.h>
> >>> #include <libcamera/controls.h>
> >>> #include <libcamera/formats.h>
> >>> +#include <libcamera/property_ids.h>
> >>> #include <libcamera/request.h>
> >>> #include <libcamera/stream.h>
> >>>
> >>> @@ -516,6 +517,7 @@ int VimcCameraData::init()
> >>>
> >>> /* Initialize the camera properties. */
> >>> properties_ = sensor_->properties();
> >>> + properties_.set(properties::MinNumRequests, 2);
> >>>
> >>> return 0;
> >>> }
> >>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> >>> index 12ecbce5eed4..0208f92074d3 100644
> >>> --- a/src/libcamera/property_ids.yaml
> >>> +++ b/src/libcamera/property_ids.yaml
> >>> @@ -678,6 +678,14 @@ controls:
> >>> \todo Turn this property into a "maximum control value" for the
> >>> ScalerCrop control once "dynamic" controls have been implemented.
> >>>
> >>> + - MinNumRequests:
> >>
> >> As it's v6, perhaps this has been discussed/(bikeshedded) already - but
> >> MinNumRequests grates on me a little
> >>
> >> We have property names such as
> >> - ScalerCropMaximum:
> >> - ColorFilterArrangement:
> >>
> >> so I see full words being used (Particularly looking at the 'Maximum').
> >>
> >> That would suggest to me:
> >>
> >> - MinimumNumberRequests
> >>
> >> which seems too verbose, so would then push me to:
> >>
> >> - MinimumRequests
> >
> > That sounds like a better name :)
> >
> > Thanks,
> > Nícolas
> >
> >>
> >>
> >>
> >>> + type: int32_t
> >>> + description: |
> >>> + The bare minimum number of requests needed in the camera pipeline for
> >>> + capture to be possible, as required by the driver. Note that this
> >>> + quantity does not guarantee that frames won't be dropped or that manual
> >>> + per-frame controls will be applied properly.
> >>> +
> >>> # ----------------------------------------------------------------------------
> >>> # Draft properties section
> >>>
> >>>
>
> --
> To unsubscribe, send mail to kernel-unsubscribe at lists.collabora.co.uk.
More information about the libcamera-devel
mailing list