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

[Naushir Patuck]

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?


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

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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230120/a6fe0d70/attachment.htm>