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

[David Plowman]

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