[libcamera-devel] [PATCH v5 03/12] pipeline: raspberrypi: Add a pipeline config structure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jan 22 20:36:40 CET 2023
On Fri, Jan 20, 2023 at 02:07:53PM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck (2023-01-20 11:37:13)
> > On Fri, 20 Jan 2023 at 10:51, Kieran Bingham wrote:
> > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44)
> > > > Add a configuration structure to store platform specific parameters used
> > > by
> > > > the pipeline handler. Currently, these only store Unicam buffer counts,
> > > > replacing the hardcoded static values in the source code.
> > > >
> > > > In subsequent commits, more parameters will be added to the configuration
> > > > structure, and parameters will be read in through a config file.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 49 ++++++++++++++++---
> > > > 1 file changed, 42 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index 8569df17976a..6bf9a669c679 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -199,6 +199,7 @@ public:
> > > >
> > > > int loadIPA(ipa::RPi::IPAInitResult *result);
> > > > int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);
> > > > + int configurePipeline();
> > > >
> > > > void enumerateVideoDevices(MediaLink *link);
> > > >
> > > > @@ -295,6 +296,21 @@ public:
> > > > /* Have internal buffers been allocated? */
> > > > bool buffersAllocated_;
> > > >
> > > > + struct Config {
> > > > + /*
> > > > + * The minimum number of internal buffers to be allocated for
> > > > + * the Unicam Image stream.
> > > > + */
> > > > + unsigned int minUnicamBuffers;
> > > > + /*
> > > > + * The minimum total (internal + external) buffer count used for
> > > > + * the Unicam Image stream.
> > >
> > > Are there any limits that should be conveyed by this documentation?
> > > Like should minTotalUnicamBuffers always be recommended to at least 2?
> > >
> >
> > minTotalUnicamBuffers must be >= 1 and minTotalUnicamBuffers >=
> > minUnicamBuffers
> >
> > This is tested when we read the config file
> > in RPiCameraData::configurePipeline(), but
> > perhaps a little comment in the config files might be needed?
>
> It needs to be whereever the option is documented such that the user
> (or system integrator?) will read that documentation. At the moment it
> seems we only have that in the config files, but we can't generate
> documentation from the config files... (or can we?)
>
> Essentially - I belive this information needs to find its way into:
> https://libcamera.org/api-html/
>
> I plan to split this into 'public' api documentation for libcamera, and
> internal documentation.
>
> But configuration files are essentially a public API.
I'm not sure about that, I think it will end up in a different
documentation. End-users are not expected to read the API reference
documentation, and neither are system integrator.
What is quite sure is that it should be documented somewhere, and having
the information in the comments above is a good idea. We can later move
it somewhere else when the time will be right.
> > > Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should
> > > this convey instead the quantity of buffers to allocate in addition to
> > > minUnicamBuffers instead?
> > >
> > > > + */
> > > > + unsigned int minTotalUnicamBuffers;
> > > > + };
> > > > +
> > > > + Config config_;
> > > > +
> > > > private:
> > > > void checkRequestCompleted();
> > > > void fillRequestMetadata(const ControlList &bufferControls,
> > > > @@ -1378,6 +1394,12 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> > > > streams.insert(&data->isp_[Isp::Output0]);
> > > > streams.insert(&data->isp_[Isp::Output1]);
> > > >
> > > > + int ret = data->configurePipeline();
> > >
> > > Glancing above - would the number of supported streams ever be
> > > configurable, requiring this to load earlier ?
> > >
> >
> > No, I think that would be fixed at compile time.
>
> Ok - that's fine.
>
> > > Of course such a change could load earlier as part of that so this is
> > > fine by me.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > > + if (ret) {
> > > > + LOG(RPI, Error) << "Unable to load pipeline handler configuration";
> > > > + return ret;
> > > > + }
> > > > +
> > > > /* Create and register the camera. */
> > > > const std::string &id = data->sensor_->id();
> > > > std::shared_ptr<Camera> camera =
> > > > @@ -1440,25 +1462,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > > > for (auto const stream : data->streams_) {
> > > > unsigned int numBuffers;
> > > > /*
> > > > - * For Unicam, allocate a minimum of 4 buffers as we want
> > > > - * to avoid any frame drops.
> > > > + * For Unicam, allocate a minimum number of buffers for internal
> > > > + * use as we want to avoid any frame drops.
> > > > */
> > > > - constexpr unsigned int minBuffers = 4;
> > > > + const unsigned int minBuffers = data->config_.minTotalUnicamBuffers;
> > > > if (stream == &data->unicam_[Unicam::Image]) {
> > > > /*
> > > > * If an application has configured a RAW stream, allocate
> > > > * additional buffers to make up the minimum, but ensure
> > > > - * we have at least 2 sets of internal buffers to use to
> > > > - * minimise frame drops.
> > > > + * we have at least minUnicamBuffers of internal buffers
> > > > + * to use to minimise frame drops.
> > > > */
> > > > - numBuffers = std::max<int>(2, minBuffers - numRawBuffers);
> > > > + numBuffers = std::max<int>(data->config_.minUnicamBuffers,
> > > > + minBuffers - numRawBuffers);
> > > > } else if (stream == &data->isp_[Isp::Input]) {
> > > > /*
> > > > * ISP input buffers are imported from Unicam, so follow
> > > > * similar logic as above to count all the RAW buffers
> > > > * available.
> > > > */
> > > > - numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers);
> > > > + numBuffers = numRawBuffers +
> > > > + std::max<int>(data->config_.minUnicamBuffers,
> > > > + minBuffers - numRawBuffers);
> > > >
> > > > } else if (stream == &data->unicam_[Unicam::Embedded]) {
> > > > /*
> > > > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> > > > return 0;
> > > > }
> > > >
> > > > +int RPiCameraData::configurePipeline()
> > > > +{
> > > > + config_ = {
> > > > + .minUnicamBuffers = 2,
> > > > + .minTotalUnicamBuffers = 4,
> > > > + };
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > /*
> > > > * enumerateVideoDevices() iterates over the Media Controller topology, starting
> > > > * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list