[libcamera-devel] [PATCH v9 02/18] libcamera: property: Add MinimumRequests property
Paul Elder
paul.elder at ideasonboard.com
Wed Dec 28 05:09:44 CET 2022
On Wed, Dec 21, 2022 at 03:15:43PM +0530, Umang Jain wrote:
> Hi Paul, Nicolas
>
> Thank you for the patch.
>
> On 12/16/22 5:59 PM, Paul Elder via libcamera-devel wrote:
> > From: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> >
> > The MinimumRequests property reports the minimum number of capture
> > requests that the camera pipeline requires to have queued in order to
> > sustain capture operations without frame drops. At this quantity,
> > there's no guarantee that manual per-frame controls will apply
> > correctly. The quantity needed for that may be added as a separate
> > property in the future.
> >
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Changes in v9:
> > - rebased
> >
> > Changes in v8:
> > - Changed the MinimumRequests property meaning to require that frames aren't
> > dropped
> > - Set MinimumRequests on SimplePipeline depending on device and usage of
> > converter
> > - Undid definition of default MinimumRequests on CameraSensor constructor
> > - Updated pipeline-handler guides with new MinimumRequests property
> >
> > Changes in v7:
> > - Renamed property from MinNumRequests to MinimumRequests
> > - Changed MinimumRequests property's description
> >
> > Changes in v6:
> > - Removed comment from Raspberrypi MinNumRequests setting
> > - Removed an extra blank line
> > ---
> > Documentation/guides/pipeline-handler.rst | 22 +++++++---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +
> > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +
> > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +
> > src/libcamera/pipeline/vimc/vimc.cpp | 2 +
> > src/libcamera/property_ids.yaml | 21 ++++++++++
> > 8 files changed, 84 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > index e1930fdf..a7356e4a 100644
> > --- a/Documentation/guides/pipeline-handler.rst
> > +++ b/Documentation/guides/pipeline-handler.rst
> > @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca
> > be used by applications to identify camera devices in the system. Properties can be
> > registered by inspecting the values of V4L2 controls from the video devices and
> > camera sensor (for example to retrieve the position and orientation of a camera)
> > -or to express other immutable characteristics. The example pipeline handler does
> > -not register any property, but examples are available in the libcamera code
> > -base.
> > +or to express other immutable characteristics.
> > -.. TODO: Add a property example to the pipeline handler. At least the model.
> > +A required property is ``MinimumRequests``, which indicates how many requests
> > +need to be queued in the pipeline for capture without frame drops to be
> > +possible.
> > +
> > +In our case, the vivid driver requires two buffers before it'll start streaming
> > +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in
> > +vivid's driver code). In order to not drop frames we should have one extra
> > +buffer queued to the driver so that it is used as soon as the previous buffer
> > +completes. Therefore we will set our ``MinimumRequests`` to three. Append the
> > +following line to ``init()``:
> > +
> > +.. code-block:: cpp
> > +
> > + properties_.set(properties::MinimumRequests, 3);
> > At this point you need to add the following includes to the top of the file for
> > -handling controls:
> > +handling controls and properties:
> > .. code-block:: cpp
> > #include <libcamera/controls.h>
> > #include <libcamera/control_ids.h>
> > + #include <libcamera/property_ids.h>
> > Generating a default configuration
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index e4d79ea4..98a4a3e5 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()
> > /* Initialize the camera properties. */
> > data->properties_ = cio2->sensor()->properties();
> > + data->properties_.set(properties::MinimumRequests, 3);
> > +
> > ret = initControls(data.get());
> > if (ret)
> > continue;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 8569df17..4a08d01e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > /* Enable SOF event generation. */
> > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> > + data->properties_.set(properties::MinimumRequests, 3);
> > +
>
> on my local kernel branch unicam reports min_buffers_needed = 1 but
> bcm2835-isp doesn't report any min_buffers_needed (dumb guess it'll be 1).
>
> Probably worth checking in with RPi devs regarding the min_buffers_needed
> for their ISP (adding a note in my todo list)
> > /*
> > * Reset the delayed controls with the gain and exposure values set by
> > * the IPA.
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index f658d75e..45b6beaf 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -23,6 +23,7 @@
> > #include <libcamera/control_ids.h>
> > #include <libcamera/formats.h>
> > #include <libcamera/framebuffer.h>
> > +#include <libcamera/property_ids.h>
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> > #include <libcamera/ipa/core_ipa_interface.h>
> > @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > /* Initialize the camera properties. */
> > data->properties_ = data->sensor_->properties();
> > + data->properties_.set(properties::MinimumRequests, 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 24ded4db..8ed983fe 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>
> > @@ -180,6 +181,10 @@ class SimplePipelineHandler;
> > struct SimplePipelineInfo {
> > const char *driver;
> > + /*
> > + * Minimum number of buffers required by the driver to start streaming.
> > + */
> > + unsigned int minimumBuffers;
> > /*
> > * Each converter in the list contains the name
> > * and the number of streams it supports.
> > @@ -190,10 +195,10 @@ struct SimplePipelineInfo {
> > namespace {
> > static const SimplePipelineInfo supportedDevices[] = {
> > - { "imx7-csi", { { "pxp", 1 } } },
> > - { "mxc-isi", {} },
> > - { "qcom-camss", {} },
> > - { "sun6i-csi", {} },
> > + { "imx7-csi", 2, { { "pxp", 1 } } },
> > + { "mxc-isi", 3, {} },
> > + { "qcom-camss", 1, {} },
> > + { "sun6i-csi", 3, {} },
> > };
> > } /* namespace */
> > @@ -271,6 +276,8 @@ public:
> > bool useConverter_;
> > std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_;
> > + const SimplePipelineInfo *deviceInfo;
> > +
> > private:
> > void tryPipeline(unsigned int code, const Size &size);
> > static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
> > @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > inputCfg.stride = captureFormat.planes[0].bpl;
> > inputCfg.bufferCount = kNumInternalBuffers;
> > + /* Set the MinimumRequests property. */
> > + unsigned int minimumRequests;
> > +
> > + if (data->useConverter_) {
> > + /*
> > + * The application will interact only with the capture node of
> > + * the converter. Require two buffers for a frame drop free
> > + * conversion, plus one extra to account for requeue delays.
> > + */
> > + minimumRequests = 3;
> > + } else {
> > + /*
> > + * The application will interact directly with the video capture
> > + * device. Require the minimum required by the driver, plus one
> > + * extra to account for requeue delays. Force at least three
> > + * buffers in order to not drop frames.
> > + */
> > + minimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,
> > + 3U);
> > + }
> > +
> > + data->properties_.set(properties::MinimumRequests, minimumRequests);
> > +
> > return data->converter_->configure(inputCfg, outputCfgs);
> > }
> > @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > bool registered = false;
> > for (std::unique_ptr<SimpleCameraData> &data : pipelines) {
> > + data->deviceInfo = info;
> > +
> > int ret = data->init();
> > if (ret < 0)
> > continue;
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 277465b7..7f580955 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)
> > properties_.set(properties::PixelArraySize, resolution);
> > properties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });
> > + properties_.set(properties::MinimumRequests, 3);
> > +
> > /* Initialise the supported controls. */
> > ControlInfoMap::Map ctrls;
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 204f5ad7..d2633be4 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>
> > @@ -571,6 +572,7 @@ int VimcCameraData::init()
> > /* Initialize the camera properties. */
> > properties_ = sensor_->properties();
> > + properties_.set(properties::MinimumRequests, 3);
> > return 0;
> > }
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index cb55e0ed..c82ac17e 100644
> > --- a/src/libcamera/property_ids.yaml
> > +++ b/src/libcamera/property_ids.yaml
> > @@ -690,6 +690,27 @@ controls:
> > that is twice that of the full resolution mode. This value will be valid
> > after the configure method has returned successfully.
> > + - MinimumRequests:
> > + type: int32_t
> > + description: |
> > + The minimum number of capture requests that the camera pipeline requires
> > + to have queued in order to sustain capture operations without frame
> > + drops. At this quantity, there's no guarantee that manual per-frame
> > + controls will apply correctly.
> > +
> > + This property is based on the minimum number of memory buffers
> > + needed to fill the capture pipeline composed of hardware processing
> > + blocks plus as many buffers as needed to take into account propagation
> > + delays and avoid dropping frames.
> > +
> > + \todo Should this be a per-stream property?
> > +
> > + \todo Extend this property to expose the different minimum buffer and
> > + request counts (the minimum number of buffers to be able to capture at
> > + all, the minimum number of buffers to sustain capture without frame
> > + drop, and the minimum number of requests to ensure proper operation of
> > + per-frame controls).
> > +
>
> So, I like the direction here.
>
> But given the questions mentioned in the \todo(s) and provided how we
> actually split Requests <> Buffers association going ahead - I think this
> will get impacted (probably MinimumRequests will become MinimumBuffers and
> what not...)
Yeah it definitely will. I think we can deal with that when we come to
it, though.
>
> However, this is still under discussion, so I don't want to start yet
> another parallel discussion here except that - in my opinion this property
> should be under "Draft properties section" for now. This will take a
Well we want this to be a required property, so I don't think it can go
in the draft properties.
> considerable set of iterations to get it right (provided how we define and
> declare other properties on this front to sustain capture, framedrops etc.
> etc.).
>
> Acked-by: Umang Jain <umang.jain at ideasonboard.com>
Thanks,
Paul
> > # ----------------------------------------------------------------------------
> > # Draft properties section
>
More information about the libcamera-devel
mailing list