[v1,1/3] pipeline: rpi: Fix for enumerating the media graphs
diff mbox series

Message ID 20250625084212.858487-2-naush@raspberrypi.com
State New
Headers show
Series
  • Raspberry Pi: Media graph enumeration updates
Related show

Commit Message

Naushir Patuck June 25, 2025, 8:41 a.m. UTC 
When there are multiple entities between the sensor and CFE device (e.g.
a seraliser and deserialiser or multiple mux devices), the media graph
enumeration would work incorrectly and report that the frontend entity
was not found. This is beause the found flag was stored locally in a
boolean and got lost in the recursion.

Fix this by explicitly tracking and returning the frontend found flag
through the return value of enumerateVideoDevices(). This ensures the
flag does not get lost through nested recursion.

This flag can also be used to fail a camera registration if the frontend
is not found.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/rpi/common/pipeline_base.cpp     | 22 +++++++++++--------
 .../pipeline/rpi/common/pipeline_base.h       |  2 +-
 2 files changed, 14 insertions(+), 10 deletions(-)

Comments

Umang Jain June 26, 2025, 6:52 a.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On 6/25/25 2:11 PM, Naushir Patuck wrote:
> When there are multiple entities between the sensor and CFE device (e.g.
> a seraliser and deserialiser or multiple mux devices), the media graph
> enumeration would work incorrectly and report that the frontend entity
> was not found. This is beause the found flag was stored locally in a
s/beause/because/
> boolean and got lost in the recursion.
>
> Fix this by explicitly tracking and returning the frontend found flag
> through the return value of enumerateVideoDevices(). This ensures the
> flag does not get lost through nested recursion.
>
> This flag can also be used to fail a camera registration if the frontend
> is not found.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>


Looks good to me,

Reviewed-by: Umang Jain <uajain@igalia.com>

> ---
>   .../pipeline/rpi/common/pipeline_base.cpp     | 22 +++++++++++--------
>   .../pipeline/rpi/common/pipeline_base.h       |  2 +-
>   2 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index e14d3b913aaa..eafe94427dbc 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -806,7 +806,8 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera
>   	 * chain. There may be a cascade of devices in this chain!
>   	 */
>   	MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];
> -	data->enumerateVideoDevices(link, frontendName);
> +	if (!data->enumerateVideoDevices(link, frontendName))
> +		return -EINVAL;
>   
>   	ipa::RPi::InitResult result;
>   	if (data->loadIPA(&result)) {
> @@ -1018,16 +1019,20 @@ void CameraData::freeBuffers()
>    *          | Sensor2 |   | Sensor3 |
>    *          +---------+   +---------+
>    */
> -void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &frontend)
> +bool CameraData::enumerateVideoDevices(MediaLink *link, const std::string &frontend)
>   {
>   	const MediaPad *sinkPad = link->sink();
>   	const MediaEntity *entity = sinkPad->entity();
>   	bool frontendFound = false;
>   
> +	/* Once we reach the Frontend entity, we are done. */
> +	if (link->sink()->entity()->name() == frontend)
> +		return true;
> +
>   	/* We only deal with Video Mux and Bridge devices in cascade. */
>   	if (entity->function() != MEDIA_ENT_F_VID_MUX &&
>   	    entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)
> -		return;
> +		return false;
>   
>   	/* Find the source pad for this Video Mux or Bridge device. */
>   	const MediaPad *sourcePad = nullptr;
> @@ -1039,7 +1044,7 @@ void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
>   			 * and this branch in the cascade.
>   			 */
>   			if (sourcePad)
> -				return;
> +				return false;
>   
>   			sourcePad = pad;
>   		}
> @@ -1056,12 +1061,9 @@ void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
>   	 * other Video Mux and Bridge devices.
>   	 */
>   	for (MediaLink *l : sourcePad->links()) {
> -		enumerateVideoDevices(l, frontend);
> -		/* Once we reach the Frontend entity, we are done. */
> -		if (l->sink()->entity()->name() == frontend) {
> -			frontendFound = true;
> +		frontendFound = enumerateVideoDevices(l, frontend);
> +		if (frontendFound)
>   			break;
> -		}
>   	}
>   
>   	/* This identifies the end of our entity enumeration recursion. */
> @@ -1076,6 +1078,8 @@ void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
>   			bridgeDevices_.clear();
>   		}
>   	}
> +
> +	return frontendFound;
>   }
>   
>   int CameraData::loadPipelineConfiguration()
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 4c5743e04f86..4bce4ec4f978 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -68,7 +68,7 @@ public:
>   	void freeBuffers();
>   	virtual void platformFreeBuffers() = 0;
>   
> -	void enumerateVideoDevices(MediaLink *link, const std::string &frontend);
> +	bool enumerateVideoDevices(MediaLink *link, const std::string &frontend);
>   
>   	int loadPipelineConfiguration();
>   	int loadIPA(ipa::RPi::InitResult *result);
Laurent Pinchart June 26, 2025, 12:53 p.m. UTC | #2 
Hi Naush,

Thank you for the patch.

On Wed, Jun 25, 2025 at 09:41:16AM +0100, Naushir Patuck wrote:
> When there are multiple entities between the sensor and CFE device (e.g.
> a seraliser and deserialiser or multiple mux devices), the media graph

s/seraliser/serialiser/

> enumeration would work incorrectly and report that the frontend entity
> was not found. This is beause the found flag was stored locally in a

s/beause/because/

> boolean and got lost in the recursion.
> 
> Fix this by explicitly tracking and returning the frontend found flag
> through the return value of enumerateVideoDevices(). This ensures the
> flag does not get lost through nested recursion.
> 
> This flag can also be used to fail a camera registration if the frontend
> is not found.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 22 +++++++++++--------
>  .../pipeline/rpi/common/pipeline_base.h       |  2 +-
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index e14d3b913aaa..eafe94427dbc 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -806,7 +806,8 @@ int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera
>  	 * chain. There may be a cascade of devices in this chain!
>  	 */
>  	MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];
> -	data->enumerateVideoDevices(link, frontendName);
> +	if (!data->enumerateVideoDevices(link, frontendName))
> +		return -EINVAL;
>  
>  	ipa::RPi::InitResult result;
>  	if (data->loadIPA(&result)) {
> @@ -1018,16 +1019,20 @@ void CameraData::freeBuffers()
>   *          | Sensor2 |   | Sensor3 |
>   *          +---------+   +---------+
>   */
> -void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &frontend)
> +bool CameraData::enumerateVideoDevices(MediaLink *link, const std::string &frontend)
>  {
>  	const MediaPad *sinkPad = link->sink();
>  	const MediaEntity *entity = sinkPad->entity();
>  	bool frontendFound = false;
>  
> +	/* Once we reach the Frontend entity, we are done. */
> +	if (link->sink()->entity()->name() == frontend)
> +		return true;
> +
>  	/* We only deal with Video Mux and Bridge devices in cascade. */
>  	if (entity->function() != MEDIA_ENT_F_VID_MUX &&
>  	    entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)
> -		return;
> +		return false;
>  
>  	/* Find the source pad for this Video Mux or Bridge device. */
>  	const MediaPad *sourcePad = nullptr;
> @@ -1039,7 +1044,7 @@ void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
>  			 * and this branch in the cascade.
>  			 */
>  			if (sourcePad)
> -				return;
> +				return false;
>  
>  			sourcePad = pad;
>  		}
> @@ -1056,12 +1061,9 @@ void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
>  	 * other Video Mux and Bridge devices.
>  	 */
>  	for (MediaLink *l : sourcePad->links()) {
> -		enumerateVideoDevices(l, frontend);
> -		/* Once we reach the Frontend entity, we are done. */
> -		if (l->sink()->entity()->name() == frontend) {
> -			frontendFound = true;
> +		frontendFound = enumerateVideoDevices(l, frontend);
> +		if (frontendFound)
>  			break;
> -		}
>  	}
>  
>  	/* This identifies the end of our entity enumeration recursion. */
> @@ -1076,6 +1078,8 @@ void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
>  			bridgeDevices_.clear();
>  		}
>  	}
> +
> +	return frontendFound;
>  }
>  
>  int CameraData::loadPipelineConfiguration()
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 4c5743e04f86..4bce4ec4f978 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -68,7 +68,7 @@ public:
>  	void freeBuffers();
>  	virtual void platformFreeBuffers() = 0;
>  
> -	void enumerateVideoDevices(MediaLink *link, const std::string &frontend);
> +	bool enumerateVideoDevices(MediaLink *link, const std::string &frontend);
>  
>  	int loadPipelineConfiguration();
>  	int loadIPA(ipa::RPi::InitResult *result);

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index e14d3b913aaa..eafe94427dbc 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -806,7 +806,8 @@  int PipelineHandlerBase::registerCamera(std::unique_ptr<RPi::CameraData> &camera
 	 * chain. There may be a cascade of devices in this chain!
 	 */
 	MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];
-	data->enumerateVideoDevices(link, frontendName);
+	if (!data->enumerateVideoDevices(link, frontendName))
+		return -EINVAL;
 
 	ipa::RPi::InitResult result;
 	if (data->loadIPA(&result)) {
@@ -1018,16 +1019,20 @@  void CameraData::freeBuffers()
  *          | Sensor2 |   | Sensor3 |
  *          +---------+   +---------+
  */
-void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &frontend)
+bool CameraData::enumerateVideoDevices(MediaLink *link, const std::string &frontend)
 {
 	const MediaPad *sinkPad = link->sink();
 	const MediaEntity *entity = sinkPad->entity();
 	bool frontendFound = false;
 
+	/* Once we reach the Frontend entity, we are done. */
+	if (link->sink()->entity()->name() == frontend)
+		return true;
+
 	/* We only deal with Video Mux and Bridge devices in cascade. */
 	if (entity->function() != MEDIA_ENT_F_VID_MUX &&
 	    entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)
-		return;
+		return false;
 
 	/* Find the source pad for this Video Mux or Bridge device. */
 	const MediaPad *sourcePad = nullptr;
@@ -1039,7 +1044,7 @@  void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
 			 * and this branch in the cascade.
 			 */
 			if (sourcePad)
-				return;
+				return false;
 
 			sourcePad = pad;
 		}
@@ -1056,12 +1061,9 @@  void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
 	 * other Video Mux and Bridge devices.
 	 */
 	for (MediaLink *l : sourcePad->links()) {
-		enumerateVideoDevices(l, frontend);
-		/* Once we reach the Frontend entity, we are done. */
-		if (l->sink()->entity()->name() == frontend) {
-			frontendFound = true;
+		frontendFound = enumerateVideoDevices(l, frontend);
+		if (frontendFound)
 			break;
-		}
 	}
 
 	/* This identifies the end of our entity enumeration recursion. */
@@ -1076,6 +1078,8 @@  void CameraData::enumerateVideoDevices(MediaLink *link, const std::string &front
 			bridgeDevices_.clear();
 		}
 	}
+
+	return frontendFound;
 }
 
 int CameraData::loadPipelineConfiguration()
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
index 4c5743e04f86..4bce4ec4f978 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
@@ -68,7 +68,7 @@  public:
 	void freeBuffers();
 	virtual void platformFreeBuffers() = 0;
 
-	void enumerateVideoDevices(MediaLink *link, const std::string &frontend);
+	bool enumerateVideoDevices(MediaLink *link, const std::string &frontend);
 
 	int loadPipelineConfiguration();
 	int loadIPA(ipa::RPi::InitResult *result);