[v3,1/1] pipeline: simple: Fix matching with empty media graphs
diff mbox series

Message ID 20250513133751.1381724-2-paul.elder@ideasonboard.com
State New
Headers show
Series
  • pipeline: simple: Fix matching with an empty media graph
Related show

Commit Message

Paul Elder May 13, 2025, 1:37 p.m. UTC
The match() function currently reports that it is not possible to create
any cameras if it encounters an empty media graph.

Fix this by looping over all media graphs and only returning false when
all of them fail to create a camera.

It is worth noting that an issue does exist when on a partial match that
ends in an invalid match, any media devices that were acquired will stay
acquired. This is not a new issue though, as any acquired media devices
in general are not released until pipeline handler deconstruction. This
requires a rework of how we do matching and pipeline handler
construction, so it is captured in a comment.

In the meantime, this fix fixes a problem without increasing the net
number of problems.

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

---
Changes in v3:
- removed clearMediaDevices()
  - the reason is elaborated on in both the commit message and the todo
    comment

Changes in v2:
- added clearMediaDevices()
---
 src/libcamera/pipeline/simple/simple.cpp | 62 +++++++++++++++++-------
 1 file changed, 45 insertions(+), 17 deletions(-)

Comments

Kieran Bingham May 26, 2025, 8:58 a.m. UTC | #1
Quoting Paul Elder (2025-05-13 14:37:51)
> The match() function currently reports that it is not possible to create
> any cameras if it encounters an empty media graph.
> 
> Fix this by looping over all media graphs and only returning false when
> all of them fail to create a camera.
> 
> It is worth noting that an issue does exist when on a partial match that
> ends in an invalid match, any media devices that were acquired will stay
> acquired. This is not a new issue though, as any acquired media devices
> in general are not released until pipeline handler deconstruction. This
> requires a rework of how we do matching and pipeline handler
> construction, so it is captured in a comment.
> 
> In the meantime, this fix fixes a problem without increasing the net
> number of problems.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v3:
> - removed clearMediaDevices()
>   - the reason is elaborated on in both the commit message and the todo
>     comment

I think this is the best we can do until we have a rework of the
DeviceEnumerator...


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

> 
> Changes in v2:
> - added clearMediaDevices()
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 62 +++++++++++++++++-------
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index efb07051b..4323472e1 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -427,6 +427,9 @@ private:
>                 return static_cast<SimpleCameraData *>(camera->_d());
>         }
>  
> +       bool matchDevice(MediaDevice *media, const SimplePipelineInfo &info,
> +                        DeviceEnumerator *enumerator);
> +
>         std::vector<MediaEntity *> locateSensors(MediaDevice *media);
>         static int resetRoutingTable(V4L2Subdevice *subdev);
>  
> @@ -1660,25 +1663,13 @@ int SimplePipelineHandler::resetRoutingTable(V4L2Subdevice *subdev)
>         return 0;
>  }
>  
> -bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> +bool SimplePipelineHandler::matchDevice(MediaDevice *media,
> +                                       const SimplePipelineInfo &info,
> +                                       DeviceEnumerator *enumerator)
>  {
> -       const SimplePipelineInfo *info = nullptr;
>         unsigned int numStreams = 1;
> -       MediaDevice *media;
>  
> -       for (const SimplePipelineInfo &inf : supportedDevices) {
> -               DeviceMatch dm(inf.driver);
> -               media = acquireMediaDevice(enumerator, dm);
> -               if (media) {
> -                       info = &inf;
> -                       break;
> -               }
> -       }
> -
> -       if (!media)
> -               return false;
> -
> -       for (const auto &[name, streams] : info->converters) {
> +       for (const auto &[name, streams] : info.converters) {
>                 DeviceMatch converterMatch(name);
>                 converter_ = acquireMediaDevice(enumerator, converterMatch);
>                 if (converter_) {
> @@ -1687,7 +1678,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>                 }
>         }
>  
> -       swIspEnabled_ = info->swIspEnabled;
> +       swIspEnabled_ = info.swIspEnabled;
>  
>         /* Locate the sensors. */
>         std::vector<MediaEntity *> sensors = locateSensors(media);
> @@ -1806,6 +1797,43 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>         return registered;
>  }
>  
> +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> +{
> +       MediaDevice *media;
> +
> +       for (const SimplePipelineInfo &inf : supportedDevices) {
> +               DeviceMatch dm(inf.driver);
> +               while ((media = acquireMediaDevice(enumerator, dm))) {
> +                       /*
> +                        * If match succeeds, return true to let match() be
> +                        * called again on a new instance of the pipeline
> +                        * handler. Otherwise keep looping until we do
> +                        * successfully match one (or run out).
> +                        */
> +                       if (matchDevice(media, inf, enumerator)) {
> +                               LOG(SimplePipeline, Debug)
> +                                       << "Matched on device: "
> +                                       << media->deviceNode();
> +                               return true;
> +                       }
> +
> +                       /*
> +                        * \todo We need to clear the list of media devices
> +                        * that we've already acquired in the event that we
> +                        * fail to create a camera. This requires a rework of
> +                        * DeviceEnumerator, or even how we create pipelines
> +                        * handlers. This is because at the moment acquired
> +                        * media devices are only released on pipeline handler
> +                        * deconstruction, and if we release them any earlier
> +                        * then DeviceEnumerator::search() will keep returning
> +                        * the same media devices.
> +                        */
> +               }
> +       }
> +
> +       return false;
> +}
> +
>  V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)
>  {
>         auto iter = entities_.find(entity);
> -- 
> 2.39.2
>
Fang Hui June 10, 2025, 8:10 a.m. UTC | #2
Tested-by:

Applied https://patchwork.libcamera.org/patch/23363/, tested ok on evk_8mq plugged 1 ov5640.

evk_8mq:/data/cam-test # ls /dev/media*
/dev/media0  /dev/media1
evk_8mq:/data/cam-test #
evk_8mq:/data/cam-test # cam -l
Available cameras:
1: 'ov5640' (/base/soc@0/bus@30800000/i2c@30a20000/ov5640_mipi2@3c)
evk_8mq:/data/cam-test #
evk_8mq:/data/cam-test # logcat
06-07 16:36:42.696  1637  1637 I LIBCAMERA: [0:01:43.676715045] [1637]  WARN IPAManager ipa_manager.cpp:148 No IPA found in '/vendor/lib64/ipa'
06-07 16:36:42.697  1637  1637 I LIBCAMERA: [0:01:43.677268965] [1637]  INFO Camera camera_manager.cpp:326 libcamera v0.5.0+349-b920c09f-dirty
06-07 16:36:42.700  1637  1638 I LIBCAMERA: [0:01:43.680738405] [1638]  INFO SimplePipeline simple.cpp:1686 No sensor found for /dev/media0
06-07 16:36:42.702  1637  1638 I LIBCAMERA: [0:01:43.682392365] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:354 'ov5640 0-003c': Recommended V4L2 control 0x009a0922 not supported
06-07 16:36:42.702  1637  1638 I LIBCAMERA: [0:01:43.682487405] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:426 'ov5640 0-003c': The sensor kernel driver needs to be fixed
06-07 16:36:42.702  1637  1638 I LIBCAMERA: [0:01:43.682568405] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:428 'ov5640 0-003c': See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information
06-07 16:36:42.703  1637  1638 I LIBCAMERA: [0:01:43.683572445] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:594 'ov5640 0-003c': Failed to retrieve the camera location
06-07 16:36:42.703  1637  1638 I LIBCAMERA: [0:01:43.683642885] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:616 'ov5640 0-003c': Rotation control not available, default to 0 degrees
06-07 16:36:42.703  1637  1638 I LIBCAMERA: [0:01:43.683768525] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:501 'ov5640 0-003c': No sensor delays found in static properties. Assuming unverified defaults.
	
BRs,
Fang Hui
Kieran Bingham June 10, 2025, 9:43 a.m. UTC | #3
Quoting Hui Fang (2025-06-10 09:10:41)
> Tested-by:

Tags need to be fully qualified, just like the Reviewed-by: and
Signed-off-by: by tags.

In this instance:

Tested-by: Hui Fang <hui.fang@nxp.com>

> 
> Applied https://patchwork.libcamera.org/patch/23363/, tested ok on evk_8mq plugged 1 ov5640.
> 
> evk_8mq:/data/cam-test # ls /dev/media*
> /dev/media0  /dev/media1
> evk_8mq:/data/cam-test #
> evk_8mq:/data/cam-test # cam -l
> Available cameras:
> 1: 'ov5640' (/base/soc@0/bus@30800000/i2c@30a20000/ov5640_mipi2@3c)
> evk_8mq:/data/cam-test #
> evk_8mq:/data/cam-test # logcat
> 06-07 16:36:42.696  1637  1637 I LIBCAMERA: [0:01:43.676715045] [1637]  WARN IPAManager ipa_manager.cpp:148 No IPA found in '/vendor/lib64/ipa'
> 06-07 16:36:42.697  1637  1637 I LIBCAMERA: [0:01:43.677268965] [1637]  INFO Camera camera_manager.cpp:326 libcamera v0.5.0+349-b920c09f-dirty
> 06-07 16:36:42.700  1637  1638 I LIBCAMERA: [0:01:43.680738405] [1638]  INFO SimplePipeline simple.cpp:1686 No sensor found for /dev/media0
> 06-07 16:36:42.702  1637  1638 I LIBCAMERA: [0:01:43.682392365] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:354 'ov5640 0-003c': Recommended V4L2 control 0x009a0922 not supported
> 06-07 16:36:42.702  1637  1638 I LIBCAMERA: [0:01:43.682487405] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:426 'ov5640 0-003c': The sensor kernel driver needs to be fixed
> 06-07 16:36:42.702  1637  1638 I LIBCAMERA: [0:01:43.682568405] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:428 'ov5640 0-003c': See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information
> 06-07 16:36:42.703  1637  1638 I LIBCAMERA: [0:01:43.683572445] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:594 'ov5640 0-003c': Failed to retrieve the camera location
> 06-07 16:36:42.703  1637  1638 I LIBCAMERA: [0:01:43.683642885] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:616 'ov5640 0-003c': Rotation control not available, default to 0 degrees
> 06-07 16:36:42.703  1637  1638 I LIBCAMERA: [0:01:43.683768525] [1638]  WARN CameraSensor camera_sensor_legacy.cpp:501 'ov5640 0-003c': No sensor delays found in static properties. Assuming unverified defaults.
>         
> BRs,
> Fang Hui
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index efb07051b..4323472e1 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -427,6 +427,9 @@  private:
 		return static_cast<SimpleCameraData *>(camera->_d());
 	}
 
+	bool matchDevice(MediaDevice *media, const SimplePipelineInfo &info,
+			 DeviceEnumerator *enumerator);
+
 	std::vector<MediaEntity *> locateSensors(MediaDevice *media);
 	static int resetRoutingTable(V4L2Subdevice *subdev);
 
@@ -1660,25 +1663,13 @@  int SimplePipelineHandler::resetRoutingTable(V4L2Subdevice *subdev)
 	return 0;
 }
 
-bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
+bool SimplePipelineHandler::matchDevice(MediaDevice *media,
+					const SimplePipelineInfo &info,
+					DeviceEnumerator *enumerator)
 {
-	const SimplePipelineInfo *info = nullptr;
 	unsigned int numStreams = 1;
-	MediaDevice *media;
 
-	for (const SimplePipelineInfo &inf : supportedDevices) {
-		DeviceMatch dm(inf.driver);
-		media = acquireMediaDevice(enumerator, dm);
-		if (media) {
-			info = &inf;
-			break;
-		}
-	}
-
-	if (!media)
-		return false;
-
-	for (const auto &[name, streams] : info->converters) {
+	for (const auto &[name, streams] : info.converters) {
 		DeviceMatch converterMatch(name);
 		converter_ = acquireMediaDevice(enumerator, converterMatch);
 		if (converter_) {
@@ -1687,7 +1678,7 @@  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
 		}
 	}
 
-	swIspEnabled_ = info->swIspEnabled;
+	swIspEnabled_ = info.swIspEnabled;
 
 	/* Locate the sensors. */
 	std::vector<MediaEntity *> sensors = locateSensors(media);
@@ -1806,6 +1797,43 @@  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
 	return registered;
 }
 
+bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
+{
+	MediaDevice *media;
+
+	for (const SimplePipelineInfo &inf : supportedDevices) {
+		DeviceMatch dm(inf.driver);
+		while ((media = acquireMediaDevice(enumerator, dm))) {
+			/*
+			 * If match succeeds, return true to let match() be
+			 * called again on a new instance of the pipeline
+			 * handler. Otherwise keep looping until we do
+			 * successfully match one (or run out).
+			 */
+			if (matchDevice(media, inf, enumerator)) {
+				LOG(SimplePipeline, Debug)
+					<< "Matched on device: "
+					<< media->deviceNode();
+				return true;
+			}
+
+			/*
+			 * \todo We need to clear the list of media devices
+			 * that we've already acquired in the event that we
+			 * fail to create a camera. This requires a rework of
+			 * DeviceEnumerator, or even how we create pipelines
+			 * handlers. This is because at the moment acquired
+			 * media devices are only released on pipeline handler
+			 * deconstruction, and if we release them any earlier
+			 * then DeviceEnumerator::search() will keep returning
+			 * the same media devices.
+			 */
+		}
+	}
+
+	return false;
+}
+
 V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)
 {
 	auto iter = entities_.find(entity);