[libcamera-devel] [PATCH v1 02/10] pipeline: raspberrypi: Add a pipeline config structure

[Naushir Patuck]

Hi David,

Thank you for your feedback!

On Tue, 1 Nov 2022 at 11:50, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for the patch!
>
> On Fri, 14 Oct 2022 at 14:18, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > 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>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 43 ++++++++++++++++---
> >  1 file changed, 36 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index d366a8bec007..7d1e454cddcd 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -294,6 +294,13 @@ public:
> >         /* Have internal buffers been allocated? */
> >         bool buffersAllocated_;
> >
> > +       struct Config {
> > +               unsigned int minUnicamBuffers;
> > +               unsigned int minTotalUnicamBuffers;
>
> I wonder slightly whether it's worth saying here what these numbers
> represent? Or maybe the comments in the code below are sufficient.
>

Sure, I'll add comments to the struct - will probably copy out what is in
the
json file!

Regards,
Naush


> Either way:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > +       };
> > +
> > +       Config config_;
> > +
> >  private:
> >         void checkRequestCompleted();
> >         void fillRequestMetadata(const ControlList &bufferControls,
> > @@ -343,6 +350,7 @@ private:
> >         }
> >
> >         int registerCamera(MediaDevice *unicam, MediaDevice *isp,
> MediaEntity *sensorEntity);
> > +       int configurePipelineHandler(RPiCameraData *data);
> >         int queueAllBuffers(Camera *camera);
> >         int prepareBuffers(Camera *camera);
> >         void mapBuffers(Camera *camera, const RPi::BufferMap &buffers,
> unsigned int mask);
> > @@ -1368,6 +1376,12 @@ int
> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> >         streams.insert(&data->isp_[Isp::Output0]);
> >         streams.insert(&data->isp_[Isp::Output1]);
> >
> > +       int ret = configurePipelineHandler(data.get());
> > +       if (ret) {
> > +               LOG(RPI, Error) << "Unable to configure the pipeline
> handler!";
> > +               return ret;
> > +       }
> > +
> >         /* Create and register the camera. */
> >         const std::string &id = data->sensor_->id();
> >         std::shared_ptr<Camera> camera =
> > @@ -1380,6 +1394,18 @@ int
> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> >         return 0;
> >  }
> >
> > +int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)
> > +{
> > +       RPiCameraData::Config &config = data->config_;
> > +
> > +       config = {
> > +               .minUnicamBuffers = 2,
> > +               .minTotalUnicamBuffers = 4,
> > +       };
> > +
> > +       return 0;
> > +}
> > +
> >  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >  {
> >         RPiCameraData *data = cameraData(camera);
> > @@ -1430,25 +1456,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 sets 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]) {
> >                         /*
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221129/6f24f3e0/attachment.htm>