[libcamera-devel] [PATCH v5 03/12] pipeline: raspberrypi: Add a pipeline config structure

[Kieran Bingham]

Quoting Naushir Patuck (2023-01-20 11:37:13)
> Hi Kieran,
> 
> Thank you for your feedback!
> 
> On Fri, 20 Jan 2023 at 10:51, Kieran Bingham <
> kieran.bingham at ideasonboard.com> 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.




> > 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.

> 
> Regards,
> Naush
> 
> 
> >
> > 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
> > > --
> > > 2.25.1
> > >
> >