[libcamera-devel] [PATCH v6 01/10] libcamera: property: Add MinNumRequests property
Nícolas F. R. A. Prado
nfraprado at collabora.com
Thu Jul 22 19:54:20 CEST 2021
Hi Kieran,
On Thu, Jul 15, 2021 at 02:16:58PM -0300, Nícolas F. R. A. Prado wrote:
> 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...
Actually, I think I'm just overthinking here :). I just looked at the ipu3
driver and it has:
vbq->min_buffers_needed = 1;
It's constant. Different stream roles won't matter. I'll go ahead and drop the
comment for v7.
Thanks,
Nícolas
>
> 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