[libcamera-devel] [PATCH v6 01/10] libcamera: property: Add MinNumRequests property
Nícolas F. R. A. Prado
nfraprado at collabora.com
Thu Jul 15 16:52:50 CEST 2021
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*'
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
> >
> >
More information about the libcamera-devel
mailing list