[libcamera-devel,v1,02/10] pipeline: raspberrypi: Add a pipeline config structure
diff mbox series

Message ID 20221014131846.27169-3-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Platform configuration and buffer allocation improvements
Related show

Commit Message

Naushir Patuck Oct. 14, 2022, 1:18 p.m. UTC 
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@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 43 ++++++++++++++++---
 1 file changed, 36 insertions(+), 7 deletions(-)

Comments

David Plowman Nov. 1, 2022, 11:49 a.m. UTC | #1 
Hi Naush

Thanks for the patch!

On Fri, 14 Oct 2022 at 14:18, Naushir Patuck via libcamera-devel
<libcamera-devel@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@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@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
>
Naushir Patuck Nov. 29, 2022, 10:23 a.m. UTC | #2 
Hi David,

Thank you for your feedback!

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

> Hi Naush
>
> Thanks for the patch!
>
> On Fri, 14 Oct 2022 at 14:18, Naushir Patuck via libcamera-devel
> <libcamera-devel@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@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@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
> >
>

Patch
diff mbox series 

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;
+	};
+
+	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]) {
 			/*