[PATCH 0/4] Helpers to manage starting / stopping multi-driver media pipelines

[PATCH 0/4] Helpers to manage starting / stopping multi-driver media pipelines

[Daniel Scally]

Hello all

This series is adds some helper functions that are intended to
simplify driver code that starts a pipeline to which multiple
drivers contribute video devices. Oone of the patches from my
earlier media jobs series [1] is included, and will now be dropped
from there. The extra changes mostly stem from review comments
to that patch and the IVC series [2].

The media pipeline entity iterator is embedded into
struct media_pipeline and inited during media_pipeline_start() with
updates to functions interacting with the iterator to accomodate
that. The commit adding the new entity operations was updated to
account for that change, and a further new patch was introduced to
create a helper for drivers to call the new media_pipeline_started()
and media_pipeline_stopped() based on the state of all the video
device type entities in the pipeline

Thanks
Dan

[1] https://lore.kernel.org/linux-media/20250624-media-jobs-v2-0-8e649b069a96@ideasonboard.com/
[2] https://lore.kernel.org/linux-media/20250624-ivc-v2-0-e4ecdddb0a96@ideasonboard.com/

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Daniel Scally (4):
      media: mc: entity: Move media pipeline entity iteration functions
      media: mc: entity: Embed entity iterator in pipeline
      media: mc: entity: Add pipeline_started/stopped ops
      media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()

 drivers/media/mc/mc-entity.c                  | 93 ++++++++++++++++++---------
 drivers/media/platform/ti/omap3isp/ispvideo.c |  9 +--
 drivers/media/v4l2-core/v4l2-dev.c            | 49 ++++++++++++++
 include/media/media-entity.h                  | 91 +++++++++++++-------------
 include/media/v4l2-dev.h                      | 46 +++++++++++++
 5 files changed, 204 insertions(+), 84 deletions(-)
---
base-commit: c0b1da281d84d33281fc49289f0c7f8aada450ff
change-id: 20250702-pipelines-6af819efbed1

Best regards,
-- 
Daniel Scally <dan.scally@ideasonboard.com>

[PATCH 1/4] media: mc: entity: Move media pipeline entity iteration functions

[Daniel Scally]

Move the media pipeline entity iteration functions higher in the
file so that they can be used by more of the functions within it.
No functional change is intended with this commit.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

---

Although there's no functional change here I thought it would make it
easier to review if the re-ordering and the actual changes to these
functions were committed seperately.
---
 drivers/media/mc/mc-entity.c | 74 ++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 045590905582054c46656e20463271b1f93fa6b4..eef2fd4b73a11eba143243c964852cd494422204 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -704,6 +704,43 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
 	return 0;
 }
 
+int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
+				    struct media_pipeline_entity_iter *iter)
+{
+	return media_entity_enum_init(&iter->ent_enum, pipe->mdev);
+}
+EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_init);
+
+void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter)
+{
+	media_entity_enum_cleanup(&iter->ent_enum);
+}
+EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_cleanup);
+
+struct media_entity *
+__media_pipeline_entity_iter_next(struct media_pipeline *pipe,
+				  struct media_pipeline_entity_iter *iter,
+				  struct media_entity *entity)
+{
+	if (!entity)
+		iter->cursor = pipe->pads.next;
+
+	while (iter->cursor != &pipe->pads) {
+		struct media_pipeline_pad *ppad;
+		struct media_entity *entity;
+
+		ppad = list_entry(iter->cursor, struct media_pipeline_pad, list);
+		entity = ppad->pad->entity;
+		iter->cursor = iter->cursor->next;
+
+		if (!media_entity_enum_test_and_set(&iter->ent_enum, entity))
+			return entity;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
+
 static void media_pipeline_cleanup(struct media_pipeline *pipe)
 {
 	while (!list_empty(&pipe->pads)) {
@@ -1016,43 +1053,6 @@ __media_pipeline_pad_iter_next(struct media_pipeline *pipe,
 }
 EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
 
-int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
-				    struct media_pipeline_entity_iter *iter)
-{
-	return media_entity_enum_init(&iter->ent_enum, pipe->mdev);
-}
-EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_init);
-
-void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter)
-{
-	media_entity_enum_cleanup(&iter->ent_enum);
-}
-EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_cleanup);
-
-struct media_entity *
-__media_pipeline_entity_iter_next(struct media_pipeline *pipe,
-				  struct media_pipeline_entity_iter *iter,
-				  struct media_entity *entity)
-{
-	if (!entity)
-		iter->cursor = pipe->pads.next;
-
-	while (iter->cursor != &pipe->pads) {
-		struct media_pipeline_pad *ppad;
-		struct media_entity *entity;
-
-		ppad = list_entry(iter->cursor, struct media_pipeline_pad, list);
-		entity = ppad->pad->entity;
-		iter->cursor = iter->cursor->next;
-
-		if (!media_entity_enum_test_and_set(&iter->ent_enum, entity))
-			return entity;
-	}
-
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
-
 /* -----------------------------------------------------------------------------
  * Links management
  */

-- 
2.34.1

[PATCH 2/4] media: mc: entity: Embed entity iterator in pipeline

[Daniel Scally]

Embed a struct media_pipeline_entity_iter into struct media_pipeline.
Init the iterator in _media_pipeline_start() and clean it up during
media_pipeline_cleanup(). Update call-sites for the related functions
to not require a pointer to a struct media_pipeline_entity_iter.

With this change the following functions no longer need to be
exported; make them static.

media_pipeline_entity_iter_init()
media_pipeline_entity_iter_cleanup()

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c                  | 24 ++++++----
 drivers/media/platform/ti/omap3isp/ispvideo.c |  9 +---
 include/media/media-entity.h                  | 64 +++++++--------------------
 3 files changed, 33 insertions(+), 64 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index eef2fd4b73a11eba143243c964852cd494422204..30d15a180ad4525e9438083216ac328a4b76653a 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -704,24 +704,22 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
 	return 0;
 }
 
-int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
-				    struct media_pipeline_entity_iter *iter)
+static int media_pipeline_entity_iter_init(struct media_pipeline *pipe)
 {
-	return media_entity_enum_init(&iter->ent_enum, pipe->mdev);
+	return media_entity_enum_init(&pipe->entity_iter.ent_enum, pipe->mdev);
 }
-EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_init);
 
-void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter)
+static void media_pipeline_entity_iter_cleanup(struct media_pipeline *pipe)
 {
-	media_entity_enum_cleanup(&iter->ent_enum);
+	media_entity_enum_cleanup(&pipe->entity_iter.ent_enum);
 }
-EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_cleanup);
 
 struct media_entity *
 __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
-				  struct media_pipeline_entity_iter *iter,
 				  struct media_entity *entity)
 {
+	struct media_pipeline_entity_iter *iter = &pipe->entity_iter;
+
 	if (!entity)
 		iter->cursor = pipe->pads.next;
 
@@ -733,7 +731,7 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
 		entity = ppad->pad->entity;
 		iter->cursor = iter->cursor->next;
 
-		if (!media_entity_enum_test_and_set(&iter->ent_enum, entity))
+		if (!media_entity_enum_test(&iter->ent_enum, entity))
 			return entity;
 	}
 
@@ -743,6 +741,8 @@ EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
 
 static void media_pipeline_cleanup(struct media_pipeline *pipe)
 {
+	media_pipeline_entity_iter_cleanup(pipe);
+
 	while (!list_empty(&pipe->pads)) {
 		struct media_pipeline_pad *ppad;
 
@@ -928,6 +928,12 @@ __must_check int __media_pipeline_start(struct media_pad *origin,
 		pad->pipe = pipe;
 	}
 
+	ret = media_pipeline_entity_iter_init(pipe);
+	if (ret) {
+		dev_err(mdev->dev, "Failed to init pipeline iterator\n");
+		goto error;
+	}
+
 	pipe->start_count++;
 
 	return 0;
diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 78e30298c7ad155c70a2a369daa8c232b97e55b7..54ff16d4a2ef1cb6436de98487851caa6e380d7d 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -221,16 +221,11 @@ isp_video_remote_subdev(struct isp_video *video, u32 *pad)
 static int isp_video_get_graph_data(struct isp_video *video,
 				    struct isp_pipeline *pipe)
 {
-	struct media_pipeline_entity_iter iter;
 	struct media_entity *entity;
 	struct isp_video *far_end = NULL;
 	int ret;
 
-	ret = media_pipeline_entity_iter_init(&pipe->pipe, &iter);
-	if (ret)
-		return ret;
-
-	media_pipeline_for_each_entity(&pipe->pipe, &iter, entity) {
+	media_pipeline_for_each_entity(&pipe->pipe, entity) {
 		struct isp_video *__video;
 
 		media_entity_enum_set(&pipe->ent_enum, entity);
@@ -249,8 +244,6 @@ static int isp_video_get_graph_data(struct isp_video *video,
 			far_end = __video;
 	}
 
-	media_pipeline_entity_iter_cleanup(&iter);
-
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		pipe->input = far_end;
 		pipe->output = video;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 64cf590b11343f68a456c5870ca2f32917c122f9..0f3bad2b9c319b1792bd62fff336bf09c1a42c1b 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -97,6 +97,17 @@ struct media_graph {
 	int top;
 };
 
+/**
+ * struct media_pipeline_entity_iter - Iterator for media_pipeline_for_each_entity
+ *
+ * @ent_enum: The entity enumeration tracker
+ * @cursor: The current element
+ */
+struct media_pipeline_entity_iter {
+	struct media_entity_enum ent_enum;
+	struct list_head *cursor;
+};
+
 /**
  * struct media_pipeline - Media pipeline related information
  *
@@ -104,12 +115,14 @@ struct media_graph {
  * @mdev:		The media device the pipeline is part of
  * @pads:		List of media_pipeline_pad
  * @start_count:	Media pipeline start - stop count
+ * @entity_iter:	Iterator for media_pipeline_for_each_entity()
  */
 struct media_pipeline {
 	bool allocated;
 	struct media_device *mdev;
 	struct list_head pads;
 	int start_count;
+	struct media_pipeline_entity_iter entity_iter;
 };
 
 /**
@@ -139,17 +152,6 @@ struct media_pipeline_pad_iter {
 	struct list_head *cursor;
 };
 
-/**
- * struct media_pipeline_entity_iter - Iterator for media_pipeline_for_each_entity
- *
- * @ent_enum: The entity enumeration tracker
- * @cursor: The current element
- */
-struct media_pipeline_entity_iter {
-	struct media_entity_enum ent_enum;
-	struct list_head *cursor;
-};
-
 /**
  * struct media_link - A link object part of a media graph.
  *
@@ -1211,55 +1213,23 @@ __media_pipeline_pad_iter_next(struct media_pipeline *pipe,
 	     pad != NULL;						\
 	     pad = __media_pipeline_pad_iter_next((pipe), iter, pad))
 
-/**
- * media_pipeline_entity_iter_init - Initialize a pipeline entity iterator
- * @pipe: The pipeline
- * @iter: The iterator
- *
- * This function must be called to initialize the iterator before using it in a
- * media_pipeline_for_each_entity() loop. The iterator must be destroyed by a
- * call to media_pipeline_entity_iter_cleanup after the loop (including in code
- * paths that break from the loop).
- *
- * The same iterator can be used in multiple consecutive loops without being
- * destroyed and reinitialized.
- *
- * Return: 0 on success or a negative error code otherwise.
- */
-int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
-				    struct media_pipeline_entity_iter *iter);
-
-/**
- * media_pipeline_entity_iter_cleanup - Destroy a pipeline entity iterator
- * @iter: The iterator
- *
- * This function must be called to destroy iterators initialized with
- * media_pipeline_entity_iter_init().
- */
-void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter);
-
 struct media_entity *
 __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
-				  struct media_pipeline_entity_iter *iter,
 				  struct media_entity *entity);
 
 /**
  * media_pipeline_for_each_entity - Iterate on all entities in a media pipeline
  * @pipe: The pipeline
- * @iter: The iterator (struct media_pipeline_entity_iter)
  * @entity: The iterator entity
  *
  * Iterate on all entities in a media pipeline. This is only valid after the
  * pipeline has been built with media_pipeline_start() and before it gets
- * destroyed with media_pipeline_stop(). The iterator must be initialized with
- * media_pipeline_entity_iter_init() before iteration, and destroyed with
- * media_pipeline_entity_iter_cleanup() after (including in code paths that
- * break from the loop).
+ * destroyed with media_pipeline_stop().
  */
-#define media_pipeline_for_each_entity(pipe, iter, entity)			\
-	for (entity = __media_pipeline_entity_iter_next((pipe), iter, NULL);	\
+#define media_pipeline_for_each_entity(pipe, entity)			\
+	for (entity = __media_pipeline_entity_iter_next((pipe), NULL);	\
 	     entity != NULL;							\
-	     entity = __media_pipeline_entity_iter_next((pipe), iter, entity))
+	     entity = __media_pipeline_entity_iter_next((pipe), entity))
 
 /**
  * media_pipeline_alloc_start - Mark a pipeline as streaming

-- 
2.34.1

[PATCH 3/4] media: mc: entity: Add pipeline_started/stopped ops

[Daniel Scally]

Add two new members to struct media_entity_operations, along with new
functions in media-entity.c to traverse a media pipeline and call the
new operations. The new functions are intended to be used to signal
to a media pipeline that it has fully started, with the entity ops
allowing drivers to define some action to be taken when those
conditions are met.

The combination of the new functions and operations allows drivers
which are part of a multi-driver pipeline to delay actually starting
streaming until all of the conditions for streaming succcessfully are
met across all drivers.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
This commit was originally in the "Add media jobs framework" series
but will now be dropped from there.

Changes in this version:

	- Dropped the iter variable now that the pipeline entity
	  iterator functions don't need it.
	- Updated documentation to specify Optional and return
	  values

Changes in the previous version:

	- Refactored media_pipeline_started() such that the cleanup
	  function for media_pipeline_entity_iter is unconditionally
	  called
	- Avoided using media_entity_call() helper for operation that
	  has return type void to avoid compiler warnings
---
 drivers/media/mc/mc-entity.c | 29 +++++++++++++++++++++++++++++
 include/media/media-entity.h | 27 +++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 30d15a180ad4525e9438083216ac328a4b76653a..64bacb3197a8a6b4af312abfc1ea9e3dfacdc012 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -1059,6 +1059,35 @@ __media_pipeline_pad_iter_next(struct media_pipeline *pipe,
 }
 EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
 
+int media_pipeline_started(struct media_pipeline *pipe)
+{
+	struct media_entity *entity;
+	int ret;
+
+	media_pipeline_for_each_entity(pipe, entity) {
+		ret = media_entity_call(entity, pipeline_started);
+		if (ret && ret != -ENOIOCTLCMD)
+			break;
+	}
+
+	ret = ret == -ENOIOCTLCMD ? 0 : ret;
+	if (ret)
+		media_pipeline_stopped(pipe);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(media_pipeline_started);
+
+void media_pipeline_stopped(struct media_pipeline *pipe)
+{
+	struct media_entity *entity;
+
+	media_pipeline_for_each_entity(pipe, entity)
+		if (entity->ops && entity->ops->pipeline_stopped)
+			entity->ops->pipeline_stopped(entity);
+}
+EXPORT_SYMBOL_GPL(media_pipeline_stopped);
+
 /* -----------------------------------------------------------------------------
  * Links management
  */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 0f3bad2b9c319b1792bd62fff336bf09c1a42c1b..6e3a97183cc44a084581a941a1b5c02bef38b036 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -271,6 +271,10 @@ struct media_pad {
  *			media_entity_has_pad_interdep().
  *			Optional: If the operation isn't implemented all pads
  *			will be considered as interdependent.
+ * @pipeline_started:	Notify this entity that the pipeline it is a part of has
+ *			been started
+ * @pipeline_stopped:	Notify this entity that the pipeline it is a part of has
+ *			been stopped
  *
  * .. note::
  *
@@ -286,6 +290,8 @@ struct media_entity_operations {
 	int (*link_validate)(struct media_link *link);
 	bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0,
 				 unsigned int pad1);
+	int (*pipeline_started)(struct media_entity *entity);
+	void (*pipeline_stopped)(struct media_entity *entity);
 };
 
 /**
@@ -1231,6 +1237,27 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
 	     entity != NULL;							\
 	     entity = __media_pipeline_entity_iter_next((pipe), entity))
 
+/**
+ * media_pipeline_started - Inform entities in a pipeline that it has started
+ * @pipe:	The pipeline
+ *
+ * Iterate on all entities in a media pipeline and call their pipeline_started
+ * member of media_entity_operations. Optional.
+ *
+ * Return: zero on success, or a negative error code passed through from an
+ * entity's .pipeline_started() operation.
+ */
+int media_pipeline_started(struct media_pipeline *pipe);
+
+/**
+ * media_pipeline_stopped - Inform entities in a pipeline that it has stopped
+ * @pipe:	The pipeline
+ *
+ * Iterate on all entities in a media pipeline and call their pipeline_stopped
+ * member of media_entity_operations. Optional.
+ */
+void media_pipeline_stopped(struct media_pipeline *pipe);
+
 /**
  * media_pipeline_alloc_start - Mark a pipeline as streaming
  * @pad: Starting pad

-- 
2.34.1

[PATCH 4/4] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()

[Daniel Scally]

Add helpers to run the new media_pipeline_started() and
media_pipeline_stopped() functions. The helpers iterate over the
entities in the pipeline and count the number of video devices and
compare that to the pipeline's start_count() before acting. This
allows us to only run the media pipeline callbacks in the event that
the pipeline has had video_pipeline_start() called for each video
device.

Suggested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

---

We could take this further perhaps and include the equivalent routine
in video_device_pipeline[_alloc]_start()...if none of the entities
involved have .pipeline_started() or .pipeline_stopped() operations it
should be harmless. Dealing with the three possible return values
shouldn't be problematic I think but I'm a bit reluctant to force the
choice to run those operations on users.
---
 drivers/media/v4l2-core/v4l2-dev.c | 49 ++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-dev.h           | 46 +++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c369235113d98ae26c30a1aa386e7d60d541a66e..868be3b0ea40a6439aac53ccb54aa72a66a907f6 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1200,6 +1200,55 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
 }
 EXPORT_SYMBOL_GPL(video_device_pipeline);
 
+static int __video_device_pipeline_started(struct media_pipeline *pipe)
+{
+	unsigned int n_video_devices = 0;
+	struct media_entity *entity;
+
+	media_pipeline_for_each_entity(pipe, entity) {
+		if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
+			n_video_devices++;
+	}
+
+	return n_video_devices - pipe->start_count;
+}
+
+int video_device_pipeline_started(struct video_device *vdev)
+{
+	struct media_pipeline *pipe;
+	int ret;
+
+	pipe = video_device_pipeline(vdev);
+	if (!pipe)
+		return -ENODEV;
+
+	ret = __video_device_pipeline_started(pipe);
+	if (ret)
+		return ret;
+
+	return media_pipeline_started(pipe);
+}
+EXPORT_SYMBOL_GPL(video_device_pipeline_started);
+
+int video_device_pipeline_stopped(struct video_device *vdev)
+{
+	struct media_pipeline *pipe;
+	int ret;
+
+	pipe = video_device_pipeline(vdev);
+	if (!pipe)
+		return -ENODEV;
+
+	ret = __video_device_pipeline_started(pipe);
+	if (ret)
+		return ret;
+
+	media_pipeline_stopped(pipe);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 /*
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 1b6222fab24eda96cbe459b435431c01f7259366..cbd0636bc9f37c0286d0d6d1fcededfbd6182b55 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -654,6 +654,52 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
  */
 struct media_pipeline *video_device_pipeline(struct video_device *vdev);
 
+/**
+ * video_device_pipeline_started - Run the pipeline_started() entity operation
+ *				   for a fully-started media pipeline
+ * @vdev: A video device that's part of the pipeline
+ *
+ * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
+ * connected to a given video device through enabled links have been marked as
+ * streaming through the use of video_device_pipeline_start() or one of its
+ * equivalent functions. If so, media_pipeline_started() is called to inform
+ * entities in the pipeline of that fact. The intention is to provide drivers
+ * with a shortcut for checking whether their pipeline is fully ready to start
+ * processing data.
+ *
+ * Returns:
+ *
+ * The number of video devices in the pipeline remaining to be started, or a
+ * negative error number on failure.
+ *
+ * * %-ENODEV		- The given video device is not part of a pipeline
+ * * Other errors	- media_pipeline_started() may pass through other error
+ *			  codes.
+ */
+int video_device_pipeline_started(struct video_device *vdev);
+
+/**
+ * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
+ *				   for a fully-started media pipeline
+ * @vdev: A video device that's part of the pipeline
+ *
+ * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
+ * connected to a given video device through enabled links have been marked as
+ * streaming through the use of video_device_pipeline_start() or one of its
+ * equivalent functions. If so, media_pipeline_stopped() is called for each
+ * entity in the pipeline. The intention is to provide drivers with a shortcut
+ * for checking whether this video device is the first device in the pipeline
+ * to be stopped.
+ *
+ * Returns:
+ *
+ * The number of video devices in the pipeline remaining to be started, or a
+ * negative error number on failure.
+ *
+ * * %-ENODEV		- The given video device is not part of a pipeline
+ */
+int video_device_pipeline_stopped(struct video_device *vdev);
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 #endif /* _V4L2_DEV_H */

-- 
2.34.1

Re: [PATCH 2/4] media: mc: entity: Embed entity iterator in pipeline

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Wed, Jul 02, 2025 at 04:15:03PM +0100, Daniel Scally wrote:
> Embed a struct media_pipeline_entity_iter into struct media_pipeline.
> Init the iterator in _media_pipeline_start() and clean it up during
> media_pipeline_cleanup(). Update call-sites for the related functions
> to not require a pointer to a struct media_pipeline_entity_iter.
> 
> With this change the following functions no longer need to be
> exported; make them static.
> 
> media_pipeline_entity_iter_init()
> media_pipeline_entity_iter_cleanup()

The commit message doesn't explain *why* this is desired. That's the
most important information you need in every commit message.

What will happen if code tries to iterate over entities multiple times,
either in consecutive operations, or as nested operations ?

> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/media/mc/mc-entity.c                  | 24 ++++++----
>  drivers/media/platform/ti/omap3isp/ispvideo.c |  9 +---
>  include/media/media-entity.h                  | 64 +++++++--------------------
>  3 files changed, 33 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index eef2fd4b73a11eba143243c964852cd494422204..30d15a180ad4525e9438083216ac328a4b76653a 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -704,24 +704,22 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
>  	return 0;
>  }
>  
> -int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
> -				    struct media_pipeline_entity_iter *iter)
> +static int media_pipeline_entity_iter_init(struct media_pipeline *pipe)
>  {
> -	return media_entity_enum_init(&iter->ent_enum, pipe->mdev);
> +	return media_entity_enum_init(&pipe->entity_iter.ent_enum, pipe->mdev);
>  }
> -EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_init);
>  
> -void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter)
> +static void media_pipeline_entity_iter_cleanup(struct media_pipeline *pipe)
>  {
> -	media_entity_enum_cleanup(&iter->ent_enum);
> +	media_entity_enum_cleanup(&pipe->entity_iter.ent_enum);
>  }
> -EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_cleanup);
>  
>  struct media_entity *
>  __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
> -				  struct media_pipeline_entity_iter *iter,
>  				  struct media_entity *entity)
>  {
> +	struct media_pipeline_entity_iter *iter = &pipe->entity_iter;
> +
>  	if (!entity)
>  		iter->cursor = pipe->pads.next;
>  
> @@ -733,7 +731,7 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
>  		entity = ppad->pad->entity;
>  		iter->cursor = iter->cursor->next;
>  
> -		if (!media_entity_enum_test_and_set(&iter->ent_enum, entity))
> +		if (!media_entity_enum_test(&iter->ent_enum, entity))
>  			return entity;
>  	}
>  
> @@ -743,6 +741,8 @@ EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
>  
>  static void media_pipeline_cleanup(struct media_pipeline *pipe)
>  {
> +	media_pipeline_entity_iter_cleanup(pipe);
> +
>  	while (!list_empty(&pipe->pads)) {
>  		struct media_pipeline_pad *ppad;
>  
> @@ -928,6 +928,12 @@ __must_check int __media_pipeline_start(struct media_pad *origin,
>  		pad->pipe = pipe;
>  	}
>  
> +	ret = media_pipeline_entity_iter_init(pipe);
> +	if (ret) {
> +		dev_err(mdev->dev, "Failed to init pipeline iterator\n");
> +		goto error;
> +	}
> +
>  	pipe->start_count++;
>  
>  	return 0;
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index 78e30298c7ad155c70a2a369daa8c232b97e55b7..54ff16d4a2ef1cb6436de98487851caa6e380d7d 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -221,16 +221,11 @@ isp_video_remote_subdev(struct isp_video *video, u32 *pad)
>  static int isp_video_get_graph_data(struct isp_video *video,
>  				    struct isp_pipeline *pipe)
>  {
> -	struct media_pipeline_entity_iter iter;
>  	struct media_entity *entity;
>  	struct isp_video *far_end = NULL;
>  	int ret;
>  
> -	ret = media_pipeline_entity_iter_init(&pipe->pipe, &iter);
> -	if (ret)
> -		return ret;
> -
> -	media_pipeline_for_each_entity(&pipe->pipe, &iter, entity) {
> +	media_pipeline_for_each_entity(&pipe->pipe, entity) {
>  		struct isp_video *__video;
>  
>  		media_entity_enum_set(&pipe->ent_enum, entity);
> @@ -249,8 +244,6 @@ static int isp_video_get_graph_data(struct isp_video *video,
>  			far_end = __video;
>  	}
>  
> -	media_pipeline_entity_iter_cleanup(&iter);
> -
>  	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>  		pipe->input = far_end;
>  		pipe->output = video;
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 64cf590b11343f68a456c5870ca2f32917c122f9..0f3bad2b9c319b1792bd62fff336bf09c1a42c1b 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -97,6 +97,17 @@ struct media_graph {
>  	int top;
>  };
>  
> +/**
> + * struct media_pipeline_entity_iter - Iterator for media_pipeline_for_each_entity
> + *
> + * @ent_enum: The entity enumeration tracker
> + * @cursor: The current element
> + */
> +struct media_pipeline_entity_iter {
> +	struct media_entity_enum ent_enum;
> +	struct list_head *cursor;
> +};
> +
>  /**
>   * struct media_pipeline - Media pipeline related information
>   *
> @@ -104,12 +115,14 @@ struct media_graph {
>   * @mdev:		The media device the pipeline is part of
>   * @pads:		List of media_pipeline_pad
>   * @start_count:	Media pipeline start - stop count
> + * @entity_iter:	Iterator for media_pipeline_for_each_entity()
>   */
>  struct media_pipeline {
>  	bool allocated;
>  	struct media_device *mdev;
>  	struct list_head pads;
>  	int start_count;
> +	struct media_pipeline_entity_iter entity_iter;
>  };
>  
>  /**
> @@ -139,17 +152,6 @@ struct media_pipeline_pad_iter {
>  	struct list_head *cursor;
>  };
>  
> -/**
> - * struct media_pipeline_entity_iter - Iterator for media_pipeline_for_each_entity
> - *
> - * @ent_enum: The entity enumeration tracker
> - * @cursor: The current element
> - */
> -struct media_pipeline_entity_iter {
> -	struct media_entity_enum ent_enum;
> -	struct list_head *cursor;
> -};
> -
>  /**
>   * struct media_link - A link object part of a media graph.
>   *
> @@ -1211,55 +1213,23 @@ __media_pipeline_pad_iter_next(struct media_pipeline *pipe,
>  	     pad != NULL;						\
>  	     pad = __media_pipeline_pad_iter_next((pipe), iter, pad))
>  
> -/**
> - * media_pipeline_entity_iter_init - Initialize a pipeline entity iterator
> - * @pipe: The pipeline
> - * @iter: The iterator
> - *
> - * This function must be called to initialize the iterator before using it in a
> - * media_pipeline_for_each_entity() loop. The iterator must be destroyed by a
> - * call to media_pipeline_entity_iter_cleanup after the loop (including in code
> - * paths that break from the loop).
> - *
> - * The same iterator can be used in multiple consecutive loops without being
> - * destroyed and reinitialized.
> - *
> - * Return: 0 on success or a negative error code otherwise.
> - */
> -int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
> -				    struct media_pipeline_entity_iter *iter);
> -
> -/**
> - * media_pipeline_entity_iter_cleanup - Destroy a pipeline entity iterator
> - * @iter: The iterator
> - *
> - * This function must be called to destroy iterators initialized with
> - * media_pipeline_entity_iter_init().
> - */
> -void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter);
> -
>  struct media_entity *
>  __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
> -				  struct media_pipeline_entity_iter *iter,
>  				  struct media_entity *entity);
>  
>  /**
>   * media_pipeline_for_each_entity - Iterate on all entities in a media pipeline
>   * @pipe: The pipeline
> - * @iter: The iterator (struct media_pipeline_entity_iter)
>   * @entity: The iterator entity
>   *
>   * Iterate on all entities in a media pipeline. This is only valid after the
>   * pipeline has been built with media_pipeline_start() and before it gets
> - * destroyed with media_pipeline_stop(). The iterator must be initialized with
> - * media_pipeline_entity_iter_init() before iteration, and destroyed with
> - * media_pipeline_entity_iter_cleanup() after (including in code paths that
> - * break from the loop).
> + * destroyed with media_pipeline_stop().
>   */
> -#define media_pipeline_for_each_entity(pipe, iter, entity)			\
> -	for (entity = __media_pipeline_entity_iter_next((pipe), iter, NULL);	\
> +#define media_pipeline_for_each_entity(pipe, entity)			\
> +	for (entity = __media_pipeline_entity_iter_next((pipe), NULL);	\
>  	     entity != NULL;							\
> -	     entity = __media_pipeline_entity_iter_next((pipe), iter, entity))
> +	     entity = __media_pipeline_entity_iter_next((pipe), entity))
>  
>  /**
>   * media_pipeline_alloc_start - Mark a pipeline as streaming

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: mc: entity: Add pipeline_started/stopped ops

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Wed, Jul 02, 2025 at 04:15:04PM +0100, Daniel Scally wrote:
> Add two new members to struct media_entity_operations, along with new
> functions in media-entity.c to traverse a media pipeline and call the
> new operations. The new functions are intended to be used to signal
> to a media pipeline that it has fully started, with the entity ops
> allowing drivers to define some action to be taken when those
> conditions are met.
> 
> The combination of the new functions and operations allows drivers
> which are part of a multi-driver pipeline to delay actually starting
> streaming until all of the conditions for streaming succcessfully are
> met across all drivers.

I can't review this patch properly without looking at how it will be
used. I don't think this series should be split out of the media jobs
series.

> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> This commit was originally in the "Add media jobs framework" series
> but will now be dropped from there.
> 
> Changes in this version:
> 
> 	- Dropped the iter variable now that the pipeline entity
> 	  iterator functions don't need it.
> 	- Updated documentation to specify Optional and return
> 	  values
> 
> Changes in the previous version:
> 
> 	- Refactored media_pipeline_started() such that the cleanup
> 	  function for media_pipeline_entity_iter is unconditionally
> 	  called
> 	- Avoided using media_entity_call() helper for operation that
> 	  has return type void to avoid compiler warnings
> ---
>  drivers/media/mc/mc-entity.c | 29 +++++++++++++++++++++++++++++
>  include/media/media-entity.h | 27 +++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 30d15a180ad4525e9438083216ac328a4b76653a..64bacb3197a8a6b4af312abfc1ea9e3dfacdc012 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -1059,6 +1059,35 @@ __media_pipeline_pad_iter_next(struct media_pipeline *pipe,
>  }
>  EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
>  
> +int media_pipeline_started(struct media_pipeline *pipe)

The name "started" sounds like it's related to media_pipeline_start(),
but it doesn't seem to be the case. That's confusing, we need clearer
names.

> +{
> +	struct media_entity *entity;
> +	int ret;
> +
> +	media_pipeline_for_each_entity(pipe, entity) {
> +		ret = media_entity_call(entity, pipeline_started);
> +		if (ret && ret != -ENOIOCTLCMD)
> +			break;
> +	}
> +
> +	ret = ret == -ENOIOCTLCMD ? 0 : ret;
> +	if (ret)
> +		media_pipeline_stopped(pipe);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_started);
> +
> +void media_pipeline_stopped(struct media_pipeline *pipe)
> +{
> +	struct media_entity *entity;
> +
> +	media_pipeline_for_each_entity(pipe, entity)
> +		if (entity->ops && entity->ops->pipeline_stopped)
> +			entity->ops->pipeline_stopped(entity);
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_stopped);
> +
>  /* -----------------------------------------------------------------------------
>   * Links management
>   */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 0f3bad2b9c319b1792bd62fff336bf09c1a42c1b..6e3a97183cc44a084581a941a1b5c02bef38b036 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -271,6 +271,10 @@ struct media_pad {
>   *			media_entity_has_pad_interdep().
>   *			Optional: If the operation isn't implemented all pads
>   *			will be considered as interdependent.
> + * @pipeline_started:	Notify this entity that the pipeline it is a part of has
> + *			been started
> + * @pipeline_stopped:	Notify this entity that the pipeline it is a part of has
> + *			been stopped
>   *
>   * .. note::
>   *
> @@ -286,6 +290,8 @@ struct media_entity_operations {
>  	int (*link_validate)(struct media_link *link);
>  	bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0,
>  				 unsigned int pad1);
> +	int (*pipeline_started)(struct media_entity *entity);
> +	void (*pipeline_stopped)(struct media_entity *entity);
>  };
>  
>  /**
> @@ -1231,6 +1237,27 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
>  	     entity != NULL;							\
>  	     entity = __media_pipeline_entity_iter_next((pipe), entity))
>  
> +/**
> + * media_pipeline_started - Inform entities in a pipeline that it has started
> + * @pipe:	The pipeline
> + *
> + * Iterate on all entities in a media pipeline and call their pipeline_started
> + * member of media_entity_operations. Optional.
> + *
> + * Return: zero on success, or a negative error code passed through from an
> + * entity's .pipeline_started() operation.
> + */
> +int media_pipeline_started(struct media_pipeline *pipe);
> +
> +/**
> + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped
> + * @pipe:	The pipeline
> + *
> + * Iterate on all entities in a media pipeline and call their pipeline_stopped
> + * member of media_entity_operations. Optional.
> + */
> +void media_pipeline_stopped(struct media_pipeline *pipe);
> +
>  /**
>   * media_pipeline_alloc_start - Mark a pipeline as streaming
>   * @pad: Starting pad

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: mc: entity: Add pipeline_started/stopped ops

[Dan Scally]

Morning Laurent

On 02/07/2025 21:08, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Wed, Jul 02, 2025 at 04:15:04PM +0100, Daniel Scally wrote:
>> Add two new members to struct media_entity_operations, along with new
>> functions in media-entity.c to traverse a media pipeline and call the
>> new operations. The new functions are intended to be used to signal
>> to a media pipeline that it has fully started, with the entity ops
>> allowing drivers to define some action to be taken when those
>> conditions are met.
>>
>> The combination of the new functions and operations allows drivers
>> which are part of a multi-driver pipeline to delay actually starting
>> streaming until all of the conditions for streaming succcessfully are
>> met across all drivers.
> I can't review this patch properly without looking at how it will be
> used. I don't think this series should be split out of the media jobs
> series.

Well, it's first used in the IVC / ISP series; I could include these patches in one of those sets 
instead? They weren't used in the media jobs series either, I was bundling them together as 
precursors to the other two.

>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> This commit was originally in the "Add media jobs framework" series
>> but will now be dropped from there.
>>
>> Changes in this version:
>>
>> 	- Dropped the iter variable now that the pipeline entity
>> 	  iterator functions don't need it.
>> 	- Updated documentation to specify Optional and return
>> 	  values
>>
>> Changes in the previous version:
>>
>> 	- Refactored media_pipeline_started() such that the cleanup
>> 	  function for media_pipeline_entity_iter is unconditionally
>> 	  called
>> 	- Avoided using media_entity_call() helper for operation that
>> 	  has return type void to avoid compiler warnings
>> ---
>>   drivers/media/mc/mc-entity.c | 29 +++++++++++++++++++++++++++++
>>   include/media/media-entity.h | 27 +++++++++++++++++++++++++++
>>   2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index 30d15a180ad4525e9438083216ac328a4b76653a..64bacb3197a8a6b4af312abfc1ea9e3dfacdc012 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -1059,6 +1059,35 @@ __media_pipeline_pad_iter_next(struct media_pipeline *pipe,
>>   }
>>   EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
>>   
>> +int media_pipeline_started(struct media_pipeline *pipe)
> The name "started" sounds like it's related to media_pipeline_start(),
> but it doesn't seem to be the case. That's confusing, we need clearer
> names.
Yes, the naming of this area is a bit confusing. They're related in that the expectation would be 
for this media_pipeline_started() function to be called once media_pipeline_start() had been called 
for each of the video devices (through video_device_pipeline_start())....I think Jacopo proposed 
renaming the existing functions to "media_pipeline_prepare()" and that might work, with this 
becoming media_pipeline_start()?
>> +{
>> +	struct media_entity *entity;
>> +	int ret;
>> +
>> +	media_pipeline_for_each_entity(pipe, entity) {
>> +		ret = media_entity_call(entity, pipeline_started);
>> +		if (ret && ret != -ENOIOCTLCMD)
>> +			break;
>> +	}
>> +
>> +	ret = ret == -ENOIOCTLCMD ? 0 : ret;
>> +	if (ret)
>> +		media_pipeline_stopped(pipe);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(media_pipeline_started);
>> +
>> +void media_pipeline_stopped(struct media_pipeline *pipe)
>> +{
>> +	struct media_entity *entity;
>> +
>> +	media_pipeline_for_each_entity(pipe, entity)
>> +		if (entity->ops && entity->ops->pipeline_stopped)
>> +			entity->ops->pipeline_stopped(entity);
>> +}
>> +EXPORT_SYMBOL_GPL(media_pipeline_stopped);
>> +
>>   /* -----------------------------------------------------------------------------
>>    * Links management
>>    */
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index 0f3bad2b9c319b1792bd62fff336bf09c1a42c1b..6e3a97183cc44a084581a941a1b5c02bef38b036 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -271,6 +271,10 @@ struct media_pad {
>>    *			media_entity_has_pad_interdep().
>>    *			Optional: If the operation isn't implemented all pads
>>    *			will be considered as interdependent.
>> + * @pipeline_started:	Notify this entity that the pipeline it is a part of has
>> + *			been started
>> + * @pipeline_stopped:	Notify this entity that the pipeline it is a part of has
>> + *			been stopped
>>    *
>>    * .. note::
>>    *
>> @@ -286,6 +290,8 @@ struct media_entity_operations {
>>   	int (*link_validate)(struct media_link *link);
>>   	bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0,
>>   				 unsigned int pad1);
>> +	int (*pipeline_started)(struct media_entity *entity);
>> +	void (*pipeline_stopped)(struct media_entity *entity);
>>   };
>>   
>>   /**
>> @@ -1231,6 +1237,27 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
>>   	     entity != NULL;							\
>>   	     entity = __media_pipeline_entity_iter_next((pipe), entity))
>>   
>> +/**
>> + * media_pipeline_started - Inform entities in a pipeline that it has started
>> + * @pipe:	The pipeline
>> + *
>> + * Iterate on all entities in a media pipeline and call their pipeline_started
>> + * member of media_entity_operations. Optional.
>> + *
>> + * Return: zero on success, or a negative error code passed through from an
>> + * entity's .pipeline_started() operation.
>> + */
>> +int media_pipeline_started(struct media_pipeline *pipe);
>> +
>> +/**
>> + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped
>> + * @pipe:	The pipeline
>> + *
>> + * Iterate on all entities in a media pipeline and call their pipeline_stopped
>> + * member of media_entity_operations. Optional.
>> + */
>> +void media_pipeline_stopped(struct media_pipeline *pipe);
>> +
>>   /**
>>    * media_pipeline_alloc_start - Mark a pipeline as streaming
>>    * @pad: Starting pad

Re: [PATCH 2/4] media: mc: entity: Embed entity iterator in pipeline

[Dan Scally]

Morning Laurent

On 02/07/2025 21:05, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Wed, Jul 02, 2025 at 04:15:03PM +0100, Daniel Scally wrote:
>> Embed a struct media_pipeline_entity_iter into struct media_pipeline.
>> Init the iterator in _media_pipeline_start() and clean it up during
>> media_pipeline_cleanup(). Update call-sites for the related functions
>> to not require a pointer to a struct media_pipeline_entity_iter.
>>
>> With this change the following functions no longer need to be
>> exported; make them static.
>>
>> media_pipeline_entity_iter_init()
>> media_pipeline_entity_iter_cleanup()
> The commit message doesn't explain *why* this is desired. That's the
> most important information you need in every commit message.


Ah - my bad. It's a suggestion from Sakari in the C55 series review, which results in a nice 
reduction in the amount of duplicated code between the two drivers.

>
> What will happen if code tries to iterate over entities multiple times,
> either in consecutive operations, or as nested operations ?


Hm well I thought it was alright, but turns out it's just coincidentally working - the cleanup does 
need to happen after each iteration...I will just drop this and the previous patch.


Thanks

Dan

>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>>   drivers/media/mc/mc-entity.c                  | 24 ++++++----
>>   drivers/media/platform/ti/omap3isp/ispvideo.c |  9 +---
>>   include/media/media-entity.h                  | 64 +++++++--------------------
>>   3 files changed, 33 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index eef2fd4b73a11eba143243c964852cd494422204..30d15a180ad4525e9438083216ac328a4b76653a 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -704,24 +704,22 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
>>   	return 0;
>>   }
>>   
>> -int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
>> -				    struct media_pipeline_entity_iter *iter)
>> +static int media_pipeline_entity_iter_init(struct media_pipeline *pipe)
>>   {
>> -	return media_entity_enum_init(&iter->ent_enum, pipe->mdev);
>> +	return media_entity_enum_init(&pipe->entity_iter.ent_enum, pipe->mdev);
>>   }
>> -EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_init);
>>   
>> -void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter)
>> +static void media_pipeline_entity_iter_cleanup(struct media_pipeline *pipe)
>>   {
>> -	media_entity_enum_cleanup(&iter->ent_enum);
>> +	media_entity_enum_cleanup(&pipe->entity_iter.ent_enum);
>>   }
>> -EXPORT_SYMBOL_GPL(media_pipeline_entity_iter_cleanup);
>>   
>>   struct media_entity *
>>   __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
>> -				  struct media_pipeline_entity_iter *iter,
>>   				  struct media_entity *entity)
>>   {
>> +	struct media_pipeline_entity_iter *iter = &pipe->entity_iter;
>> +
>>   	if (!entity)
>>   		iter->cursor = pipe->pads.next;
>>   
>> @@ -733,7 +731,7 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
>>   		entity = ppad->pad->entity;
>>   		iter->cursor = iter->cursor->next;
>>   
>> -		if (!media_entity_enum_test_and_set(&iter->ent_enum, entity))
>> +		if (!media_entity_enum_test(&iter->ent_enum, entity))
>>   			return entity;
>>   	}
>>   
>> @@ -743,6 +741,8 @@ EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
>>   
>>   static void media_pipeline_cleanup(struct media_pipeline *pipe)
>>   {
>> +	media_pipeline_entity_iter_cleanup(pipe);
>> +
>>   	while (!list_empty(&pipe->pads)) {
>>   		struct media_pipeline_pad *ppad;
>>   
>> @@ -928,6 +928,12 @@ __must_check int __media_pipeline_start(struct media_pad *origin,
>>   		pad->pipe = pipe;
>>   	}
>>   
>> +	ret = media_pipeline_entity_iter_init(pipe);
>> +	if (ret) {
>> +		dev_err(mdev->dev, "Failed to init pipeline iterator\n");
>> +		goto error;
>> +	}
>> +
>>   	pipe->start_count++;
>>   
>>   	return 0;
>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> index 78e30298c7ad155c70a2a369daa8c232b97e55b7..54ff16d4a2ef1cb6436de98487851caa6e380d7d 100644
>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>> @@ -221,16 +221,11 @@ isp_video_remote_subdev(struct isp_video *video, u32 *pad)
>>   static int isp_video_get_graph_data(struct isp_video *video,
>>   				    struct isp_pipeline *pipe)
>>   {
>> -	struct media_pipeline_entity_iter iter;
>>   	struct media_entity *entity;
>>   	struct isp_video *far_end = NULL;
>>   	int ret;
>>   
>> -	ret = media_pipeline_entity_iter_init(&pipe->pipe, &iter);
>> -	if (ret)
>> -		return ret;
>> -
>> -	media_pipeline_for_each_entity(&pipe->pipe, &iter, entity) {
>> +	media_pipeline_for_each_entity(&pipe->pipe, entity) {
>>   		struct isp_video *__video;
>>   
>>   		media_entity_enum_set(&pipe->ent_enum, entity);
>> @@ -249,8 +244,6 @@ static int isp_video_get_graph_data(struct isp_video *video,
>>   			far_end = __video;
>>   	}
>>   
>> -	media_pipeline_entity_iter_cleanup(&iter);
>> -
>>   	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>>   		pipe->input = far_end;
>>   		pipe->output = video;
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index 64cf590b11343f68a456c5870ca2f32917c122f9..0f3bad2b9c319b1792bd62fff336bf09c1a42c1b 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -97,6 +97,17 @@ struct media_graph {
>>   	int top;
>>   };
>>   
>> +/**
>> + * struct media_pipeline_entity_iter - Iterator for media_pipeline_for_each_entity
>> + *
>> + * @ent_enum: The entity enumeration tracker
>> + * @cursor: The current element
>> + */
>> +struct media_pipeline_entity_iter {
>> +	struct media_entity_enum ent_enum;
>> +	struct list_head *cursor;
>> +};
>> +
>>   /**
>>    * struct media_pipeline - Media pipeline related information
>>    *
>> @@ -104,12 +115,14 @@ struct media_graph {
>>    * @mdev:		The media device the pipeline is part of
>>    * @pads:		List of media_pipeline_pad
>>    * @start_count:	Media pipeline start - stop count
>> + * @entity_iter:	Iterator for media_pipeline_for_each_entity()
>>    */
>>   struct media_pipeline {
>>   	bool allocated;
>>   	struct media_device *mdev;
>>   	struct list_head pads;
>>   	int start_count;
>> +	struct media_pipeline_entity_iter entity_iter;
>>   };
>>   
>>   /**
>> @@ -139,17 +152,6 @@ struct media_pipeline_pad_iter {
>>   	struct list_head *cursor;
>>   };
>>   
>> -/**
>> - * struct media_pipeline_entity_iter - Iterator for media_pipeline_for_each_entity
>> - *
>> - * @ent_enum: The entity enumeration tracker
>> - * @cursor: The current element
>> - */
>> -struct media_pipeline_entity_iter {
>> -	struct media_entity_enum ent_enum;
>> -	struct list_head *cursor;
>> -};
>> -
>>   /**
>>    * struct media_link - A link object part of a media graph.
>>    *
>> @@ -1211,55 +1213,23 @@ __media_pipeline_pad_iter_next(struct media_pipeline *pipe,
>>   	     pad != NULL;						\
>>   	     pad = __media_pipeline_pad_iter_next((pipe), iter, pad))
>>   
>> -/**
>> - * media_pipeline_entity_iter_init - Initialize a pipeline entity iterator
>> - * @pipe: The pipeline
>> - * @iter: The iterator
>> - *
>> - * This function must be called to initialize the iterator before using it in a
>> - * media_pipeline_for_each_entity() loop. The iterator must be destroyed by a
>> - * call to media_pipeline_entity_iter_cleanup after the loop (including in code
>> - * paths that break from the loop).
>> - *
>> - * The same iterator can be used in multiple consecutive loops without being
>> - * destroyed and reinitialized.
>> - *
>> - * Return: 0 on success or a negative error code otherwise.
>> - */
>> -int media_pipeline_entity_iter_init(struct media_pipeline *pipe,
>> -				    struct media_pipeline_entity_iter *iter);
>> -
>> -/**
>> - * media_pipeline_entity_iter_cleanup - Destroy a pipeline entity iterator
>> - * @iter: The iterator
>> - *
>> - * This function must be called to destroy iterators initialized with
>> - * media_pipeline_entity_iter_init().
>> - */
>> -void media_pipeline_entity_iter_cleanup(struct media_pipeline_entity_iter *iter);
>> -
>>   struct media_entity *
>>   __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
>> -				  struct media_pipeline_entity_iter *iter,
>>   				  struct media_entity *entity);
>>   
>>   /**
>>    * media_pipeline_for_each_entity - Iterate on all entities in a media pipeline
>>    * @pipe: The pipeline
>> - * @iter: The iterator (struct media_pipeline_entity_iter)
>>    * @entity: The iterator entity
>>    *
>>    * Iterate on all entities in a media pipeline. This is only valid after the
>>    * pipeline has been built with media_pipeline_start() and before it gets
>> - * destroyed with media_pipeline_stop(). The iterator must be initialized with
>> - * media_pipeline_entity_iter_init() before iteration, and destroyed with
>> - * media_pipeline_entity_iter_cleanup() after (including in code paths that
>> - * break from the loop).
>> + * destroyed with media_pipeline_stop().
>>    */
>> -#define media_pipeline_for_each_entity(pipe, iter, entity)			\
>> -	for (entity = __media_pipeline_entity_iter_next((pipe), iter, NULL);	\
>> +#define media_pipeline_for_each_entity(pipe, entity)			\
>> +	for (entity = __media_pipeline_entity_iter_next((pipe), NULL);	\
>>   	     entity != NULL;							\
>> -	     entity = __media_pipeline_entity_iter_next((pipe), iter, entity))
>> +	     entity = __media_pipeline_entity_iter_next((pipe), entity))
>>   
>>   /**
>>    * media_pipeline_alloc_start - Mark a pipeline as streaming

Re: [PATCH 2/4] media: mc: entity: Embed entity iterator in pipeline

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6295 bytes --]

Hi Daniel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on c0b1da281d84d33281fc49289f0c7f8aada450ff]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Scally/media-mc-entity-Move-media-pipeline-entity-iteration-functions/20250702-231706
base:   c0b1da281d84d33281fc49289f0c7f8aada450ff
patch link:    https://lore.kernel.org/r/20250702-pipelines-v1-2-34525973e773%40ideasonboard.com
patch subject: [PATCH 2/4] media: mc: entity: Embed entity iterator in pipeline
:::::: branch date: 6 days ago
:::::: commit date: 6 days ago
config: arc-allmodconfig (attached as .config)
compiler: arc-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (attached as reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507090728.WgTCOat0-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/media/platform/ti/omap3isp/ispvideo.c: In function 'isp_video_get_graph_data':
>> drivers/media/platform/ti/omap3isp/ispvideo.c:226:13: warning: unused variable 'ret' [-Wunused-variable]
     226 |         int ret;
         |             ^~~


vim +/ret +226 drivers/media/platform/ti/omap3isp/ispvideo.c

ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  219  
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  220  /* Return a pointer to the ISP video instance at the far end of the pipeline. */
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  221  static int isp_video_get_graph_data(struct isp_video *video,
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  222  				    struct isp_pipeline *pipe)
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  223  {
3e8537b4c15172 drivers/media/platform/ti/omap3isp/ispvideo.c Laurent Pinchart 2022-12-21  224  	struct media_entity *entity;
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  225  	struct isp_video *far_end = NULL;
28461451c0fc94 drivers/media/platform/omap3isp/ispvideo.c    Sakari Ailus     2015-12-16 @226  	int ret;
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  227  
4f5c32570d372e drivers/media/platform/ti/omap3isp/ispvideo.c Daniel Scally    2025-07-02  228  	media_pipeline_for_each_entity(&pipe->pipe, entity) {
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  229  		struct isp_video *__video;
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  230  
17d3d4058a6132 drivers/media/platform/omap3isp/ispvideo.c    Sakari Ailus     2015-12-16  231  		media_entity_enum_set(&pipe->ent_enum, entity);
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  232  
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  233  		if (far_end != NULL)
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  234  			continue;
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  235  
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  236  		if (entity == &video->video.entity)
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  237  			continue;
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  238  
45b46879a78567 drivers/media/platform/omap3isp/ispvideo.c    Laurent Pinchart 2016-02-29  239  		if (!is_media_entity_v4l2_video_device(entity))
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  240  			continue;
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  241  
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  242  		__video = to_isp_video(media_entity_to_video_device(entity));
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  243  		if (__video->type != video->type)
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  244  			far_end = __video;
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  245  	}
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  246  
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  247  	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  248  		pipe->input = far_end;
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  249  		pipe->output = video;
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  250  	} else {
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  251  		if (far_end == NULL)
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  252  			return -EPIPE;
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  253  
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  254  		pipe->input = video;
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  255  		pipe->output = far_end;
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  256  	}
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  257  
ae5df813314a58 drivers/media/video/omap3isp/ispvideo.c       Sakari Ailus     2012-03-05  258  	return 0;
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  259  }
ad614acb7eca42 drivers/media/video/omap3isp/ispvideo.c       Laurent Pinchart 2011-02-12  260  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 80792 bytes --]

[-- Attachment #3: reproduce --]
[-- Type: text/plain, Size: 609 bytes --]

reproduce (this is a W=1 build):
        git clone https://github.com/intel/lkp-tests.git ~/lkp-tests
        git checkout c0b1da281d84d33281fc49289f0c7f8aada450ff
        b4 shazam https://lore.kernel.org/r/20250702-pipelines-v1-2-34525973e773@ideasonboard.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-15.1.0 ~/lkp-tests/kbuild/make.cross W=1 O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-15.1.0 ~/lkp-tests/kbuild/make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash