[libcamera-devel,20/20] libcamera: pipeline: simple: Enable multiple streams for compatible devices
diff mbox series

Message ID 20210131224702.8838-21-laurent.pinchart@ideasonboard.com
State Accepted
Delegated to: Laurent Pinchart
Headers show
Series
  • [libcamera-devel,01/20] libcamera: pipeline: simple: Manage converter with std::unique_ptr<>
Related show

Commit Message

Laurent Pinchart Jan. 31, 2021, 10:47 p.m. UTC
Allow support for multiple streams on a per-device basis. The decision
should be made based on the ability of the converter to run multiple
times within the duration of one frame. Hardcode it in
SimplePipelineInfo for now.

We may later compute the number of supported streams dynamically based
on the requested configuration, using converter bandwidth information
instead of a hardcoded fixed value.

All platforms are currently limited to a single stream until they get
successfully tested with multiple streams.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 39 +++++++++++++-----------
 1 file changed, 21 insertions(+), 18 deletions(-)

Comments

Paul Elder March 2, 2021, 7:14 a.m. UTC | #1
Hi Laurent,

On Mon, Feb 01, 2021 at 12:47:02AM +0200, Laurent Pinchart wrote:
> Allow support for multiple streams on a per-device basis. The decision
> should be made based on the ability of the converter to run multiple
> times within the duration of one frame. Hardcode it in
> SimplePipelineInfo for now.
> 
> We may later compute the number of supported streams dynamically based
> on the requested configuration, using converter bandwidth information
> instead of a hardcoded fixed value.
> 
> All platforms are currently limited to a single stream until they get
> successfully tested with multiple streams.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks good to me!

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 39 +++++++++++++-----------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 55a5528611c8..4a8a7ed24960 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -126,14 +126,15 @@ class SimplePipelineHandler;
>  struct SimplePipelineInfo {
>  	const char *driver;
>  	const char *converter;
> +	unsigned int numStreams;
>  };
>  
>  namespace {
>  
>  static const SimplePipelineInfo supportedDevices[] = {
> -	{ "imx7-csi", "pxp" },
> -	{ "qcom-camss", nullptr },
> -	{ "sun6i-csi", nullptr },
> +	{ "imx7-csi", "pxp", 1 },
> +	{ "qcom-camss", nullptr, 1 },
> +	{ "sun6i-csi", nullptr, 1 },
>  };
>  
>  } /* namespace */
> @@ -141,7 +142,9 @@ static const SimplePipelineInfo supportedDevices[] = {
>  class SimpleCameraData : public CameraData
>  {
>  public:
> -	SimpleCameraData(SimplePipelineHandler *pipe, MediaEntity *sensor);
> +	SimpleCameraData(SimplePipelineHandler *pipe,
> +			 const SimplePipelineInfo *info,
> +			 MediaEntity *sensor);
>  
>  	bool isValid() const { return sensor_ != nullptr; }
>  
> @@ -254,13 +257,12 @@ private:
>   */
>  
>  SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> +				   const SimplePipelineInfo *info,
>  				   MediaEntity *sensor)
> -	: CameraData(pipe)
> +	: CameraData(pipe), streams_(info->numStreams)
>  {
>  	int ret;
>  
> -	streams_.resize(1);
> -
>  	/*
>  	 * Walk the pipeline towards the video node and store all entities
>  	 * along the way.
> @@ -864,25 +866,26 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>  
>  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  {
> +	const SimplePipelineInfo *info = nullptr;
>  	MediaDevice *converter = nullptr;
>  
> -	for (const SimplePipelineInfo &info : supportedDevices) {
> -		DeviceMatch dm(info.driver);
> +	for (const SimplePipelineInfo &inf : supportedDevices) {
> +		DeviceMatch dm(inf.driver);
>  		media_ = acquireMediaDevice(enumerator, dm);
> -		if (!media_)
> -			continue;
> -
> -		if (!info.converter)
> +		if (media_) {
> +			info = &inf;
>  			break;
> -
> -		DeviceMatch converterMatch(info.converter);
> -		converter = acquireMediaDevice(enumerator, converterMatch);
> -		break;
> +		}
>  	}
>  
>  	if (!media_)
>  		return false;
>  
> +	if (info->converter) {
> +		DeviceMatch converterMatch(info->converter);
> +		converter = acquireMediaDevice(enumerator, converterMatch);
> +	}
> +
>  	/* Locate the sensors. */
>  	std::vector<MediaEntity *> sensors;
>  
> @@ -926,7 +929,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  
>  	for (MediaEntity *sensor : sensors) {
>  		std::unique_ptr<SimpleCameraData> data =
> -			std::make_unique<SimpleCameraData>(this, sensor);
> +			std::make_unique<SimpleCameraData>(this, info, sensor);
>  		if (!data->isValid()) {
>  			LOG(SimplePipeline, Error)
>  				<< "No valid pipeline for sensor '"
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham March 2, 2021, 11:11 a.m. UTC | #2
On 31/01/2021 22:47, Laurent Pinchart wrote:
> Allow support for multiple streams on a per-device basis. The decision
> should be made based on the ability of the converter to run multiple
> times within the duration of one frame.

Well, that's not only hardware specific depending on how fast the
hardware can run, but also dependant upon the stream configurations - so
that's going to be complex to determine ;-)

Still we don't care here:

> Hardcode it in SimplePipelineInfo for now.

Because of that ;-)

> 
> We may later compute the number of supported streams dynamically based
> on the requested configuration, using converter bandwidth information
> instead of a hardcoded fixed value.
> 
> All platforms are currently limited to a single stream until they get
> successfully tested with multiple streams.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 39 +++++++++++++-----------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 55a5528611c8..4a8a7ed24960 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -126,14 +126,15 @@ class SimplePipelineHandler;
>  struct SimplePipelineInfo {
>  	const char *driver;
>  	const char *converter;
> +	unsigned int numStreams;
>  };
>  
>  namespace {
>  
>  static const SimplePipelineInfo supportedDevices[] = {
> -	{ "imx7-csi", "pxp" },
> -	{ "qcom-camss", nullptr },
> -	{ "sun6i-csi", nullptr },
> +	{ "imx7-csi", "pxp", 1 },
> +	{ "qcom-camss", nullptr, 1 },
> +	{ "sun6i-csi", nullptr, 1 },
>  };
>  
>  } /* namespace */
> @@ -141,7 +142,9 @@ static const SimplePipelineInfo supportedDevices[] = {
>  class SimpleCameraData : public CameraData
>  {
>  public:
> -	SimpleCameraData(SimplePipelineHandler *pipe, MediaEntity *sensor);
> +	SimpleCameraData(SimplePipelineHandler *pipe,
> +			 const SimplePipelineInfo *info,
> +			 MediaEntity *sensor);
>  
>  	bool isValid() const { return sensor_ != nullptr; }
>  
> @@ -254,13 +257,12 @@ private:
>   */
>  
>  SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> +				   const SimplePipelineInfo *info,
>  				   MediaEntity *sensor)
> -	: CameraData(pipe)
> +	: CameraData(pipe), streams_(info->numStreams)
>  {
>  	int ret;
>  
> -	streams_.resize(1);

I'm guessing that we already check at validate() or configure() that the
number of streams requested is less than streams_->size...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> -
>  	/*
>  	 * Walk the pipeline towards the video node and store all entities
>  	 * along the way.
> @@ -864,25 +866,26 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>  
>  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  {
> +	const SimplePipelineInfo *info = nullptr;
>  	MediaDevice *converter = nullptr;
>  
> -	for (const SimplePipelineInfo &info : supportedDevices) {
> -		DeviceMatch dm(info.driver);
> +	for (const SimplePipelineInfo &inf : supportedDevices) {
> +		DeviceMatch dm(inf.driver);
>  		media_ = acquireMediaDevice(enumerator, dm);
> -		if (!media_)
> -			continue;
> -
> -		if (!info.converter)
> +		if (media_) {
> +			info = &inf;
>  			break;
> -
> -		DeviceMatch converterMatch(info.converter);
> -		converter = acquireMediaDevice(enumerator, converterMatch);
> -		break;
> +		}
>  	}
>  
>  	if (!media_)
>  		return false;
>  
> +	if (info->converter) {
> +		DeviceMatch converterMatch(info->converter);
> +		converter = acquireMediaDevice(enumerator, converterMatch);
> +	}
> +
>  	/* Locate the sensors. */
>  	std::vector<MediaEntity *> sensors;
>  
> @@ -926,7 +929,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  
>  	for (MediaEntity *sensor : sensors) {
>  		std::unique_ptr<SimpleCameraData> data =
> -			std::make_unique<SimpleCameraData>(this, sensor);
> +			std::make_unique<SimpleCameraData>(this, info, sensor);
>  		if (!data->isValid()) {
>  			LOG(SimplePipeline, Error)
>  				<< "No valid pipeline for sensor '"
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 55a5528611c8..4a8a7ed24960 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -126,14 +126,15 @@  class SimplePipelineHandler;
 struct SimplePipelineInfo {
 	const char *driver;
 	const char *converter;
+	unsigned int numStreams;
 };
 
 namespace {
 
 static const SimplePipelineInfo supportedDevices[] = {
-	{ "imx7-csi", "pxp" },
-	{ "qcom-camss", nullptr },
-	{ "sun6i-csi", nullptr },
+	{ "imx7-csi", "pxp", 1 },
+	{ "qcom-camss", nullptr, 1 },
+	{ "sun6i-csi", nullptr, 1 },
 };
 
 } /* namespace */
@@ -141,7 +142,9 @@  static const SimplePipelineInfo supportedDevices[] = {
 class SimpleCameraData : public CameraData
 {
 public:
-	SimpleCameraData(SimplePipelineHandler *pipe, MediaEntity *sensor);
+	SimpleCameraData(SimplePipelineHandler *pipe,
+			 const SimplePipelineInfo *info,
+			 MediaEntity *sensor);
 
 	bool isValid() const { return sensor_ != nullptr; }
 
@@ -254,13 +257,12 @@  private:
  */
 
 SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
+				   const SimplePipelineInfo *info,
 				   MediaEntity *sensor)
-	: CameraData(pipe)
+	: CameraData(pipe), streams_(info->numStreams)
 {
 	int ret;
 
-	streams_.resize(1);
-
 	/*
 	 * Walk the pipeline towards the video node and store all entities
 	 * along the way.
@@ -864,25 +866,26 @@  int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
 
 bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
 {
+	const SimplePipelineInfo *info = nullptr;
 	MediaDevice *converter = nullptr;
 
-	for (const SimplePipelineInfo &info : supportedDevices) {
-		DeviceMatch dm(info.driver);
+	for (const SimplePipelineInfo &inf : supportedDevices) {
+		DeviceMatch dm(inf.driver);
 		media_ = acquireMediaDevice(enumerator, dm);
-		if (!media_)
-			continue;
-
-		if (!info.converter)
+		if (media_) {
+			info = &inf;
 			break;
-
-		DeviceMatch converterMatch(info.converter);
-		converter = acquireMediaDevice(enumerator, converterMatch);
-		break;
+		}
 	}
 
 	if (!media_)
 		return false;
 
+	if (info->converter) {
+		DeviceMatch converterMatch(info->converter);
+		converter = acquireMediaDevice(enumerator, converterMatch);
+	}
+
 	/* Locate the sensors. */
 	std::vector<MediaEntity *> sensors;
 
@@ -926,7 +929,7 @@  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
 
 	for (MediaEntity *sensor : sensors) {
 		std::unique_ptr<SimpleCameraData> data =
-			std::make_unique<SimpleCameraData>(this, sensor);
+			std::make_unique<SimpleCameraData>(this, info, sensor);
 		if (!data->isValid()) {
 			LOG(SimplePipeline, Error)
 				<< "No valid pipeline for sensor '"