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

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Jan 18, 2023 at 08:59:44AM +0000, Naushir Patuck via libcamera-devel 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>
> 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();

I think loadPipelineConfiguration() would be more explicit. Would it be
too long though ?

>  
>  	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.
> +		 */
> +		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();
> +	if (ret) {
> +		LOG(RPI, Error) << "Unable to load pipeline handler configuration";

And to match the function name, s/handler //

This patch otherwise looks good to me.

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

-- 
Regards,

Laurent Pinchart