[PATCH v1 0/5] media: Replace media graph walk in several drivers

[PATCH v1 0/5] media: Replace media graph walk in several drivers

[Laurent Pinchart]

Hello,

This patch series replaces the media graph walk API usage in several
drivers with iteration over entities or pads stored in the
media_pipeline object. Iteration over the constructed pipeline is more
efficient, and will support the V4L2 stream API correctly.

Patches 1/5 and 2/5 start by adding two macros to iterate over pads and
entities in a pipeline. Patch 2/5 also marks the media graph walk API as
deprecated as a result. Patches 3/5 to 5/5 then use the new iterators in
three drivers (omap3isp, omap4iss and xilinx).

With this series applied, only two drivers still use the media graph
walk API: exynos4-is and vsp1. Those are more difficult to address. I
plan to work on the vsp1 driver, but not on exynos4-is as I miss
knowledge of the code base and have no hardware to test changes on.
Volunteers would be appreciated :-)

Laurent Pinchart (5):
  media: mc: entity: Add pad iterator for media_pipeline
  media: mc: entity: Add entity iterator for media_pipeline
  media: ti: omap3isp: Use media_pipeline_for_each_entity()
  media: ti: omap4iss: Use media_pipeline_for_each_entity()
  media: xilinx: dma: Use media_pipeline_for_each_pad()

 drivers/media/mc/mc-entity.c                  | 55 +++++++++++
 drivers/media/platform/ti/omap3isp/ispvideo.c | 21 +---
 drivers/media/platform/xilinx/xilinx-dma.c    | 28 ++----
 drivers/staging/media/omap4iss/iss_video.c    | 56 +++--------
 include/media/media-entity.h                  | 98 +++++++++++++++++++
 5 files changed, 177 insertions(+), 81 deletions(-)


base-commit: 3178804c64ef7c8c87a53cd5bba0b2942dd64fec
-- 
Regards,

Laurent Pinchart

[PATCH v1 1/5] media: mc: entity: Add pad iterator for media_pipeline

[Laurent Pinchart]

Add a media_pipeline_for_each_pad() macro to iterate over pads in a
pipeline. This should be used by driver as a replacement of the
media_graph_walk API, as iterating over the media_pipeline uses the
cached list of pads and is thus more efficient.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c | 18 ++++++++++++++++++
 include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index b8bcbc734eaf..70df2050951c 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -932,6 +932,24 @@ __must_check int media_pipeline_alloc_start(struct media_pad *pad)
 }
 EXPORT_SYMBOL_GPL(media_pipeline_alloc_start);
 
+struct media_pad *
+__media_pipeline_pad_iter_next(struct media_pipeline *pipe,
+			       struct media_pipeline_pad_iter *iter,
+			       struct media_pad *pad)
+{
+	if (!pad)
+		iter->cursor = pipe->pads.next;
+
+	if (iter->cursor == &pipe->pads)
+		return NULL;
+
+	pad = list_entry(iter->cursor, struct media_pipeline_pad, list)->pad;
+	iter->cursor = iter->cursor->next;
+
+	return pad;
+}
+EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
+
 /* -----------------------------------------------------------------------------
  * Links management
  */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 85ed08ddee9d..e881e483b550 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -130,6 +130,15 @@ struct media_pipeline_pad {
 	struct media_pad *pad;
 };
 
+/**
+ * struct media_pipeline_pad_iter - Iterator for media_pipeline_for_each_pad
+ *
+ * @cursor: The current element
+ */
+struct media_pipeline_pad_iter {
+	struct list_head *cursor;
+};
+
 /**
  * struct media_link - A link object part of a media graph.
  *
@@ -1163,6 +1172,26 @@ void media_pipeline_stop(struct media_pad *pad);
  */
 void __media_pipeline_stop(struct media_pad *pad);
 
+struct media_pad *
+__media_pipeline_pad_iter_next(struct media_pipeline *pipe,
+			       struct media_pipeline_pad_iter *iter,
+			       struct media_pad *pad);
+
+/**
+ * media_pipeline_for_each_pad - Iterate on all pads in a media pipeline
+ * @pipe: The pipeline
+ * @iter: The iterator (struct media_pipeline_pad_iter)
+ * @pad: The iterator pad
+ *
+ * Iterate on all pads 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().
+ */
+#define media_pipeline_for_each_pad(pipe, iter, pad)			\
+	for (pad = __media_pipeline_pad_iter_next((pipe), iter, NULL);	\
+	     pad != NULL;						\
+	     pad = __media_pipeline_pad_iter_next((pipe), iter, pad))
+
 /**
  * media_pipeline_alloc_start - Mark a pipeline as streaming
  * @pad: Starting pad
-- 
Regards,

Laurent Pinchart

[PATCH v1 2/5] media: mc: entity: Add entity iterator for media_pipeline

[Laurent Pinchart]

Add a media_pipeline_for_each_entity() macro to iterate over entities in
a pipeline. This should be used by driver as a replacement of the
media_graph_walk API, as iterating over the media_pipeline uses the
cached list of pads and is thus more efficient.

Deprecate the media_graph_walk API to indicate it shouldn't be used in
new drivers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c | 37 +++++++++++++++++++
 include/media/media-entity.h | 69 ++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 70df2050951c..f19bb98071b2 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -950,6 +950,43 @@ __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
  */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index e881e483b550..1b820cb6fed1 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -139,6 +139,17 @@ 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.
  *
@@ -1075,6 +1086,8 @@ int media_entity_get_fwnode_pad(struct media_entity *entity,
  * @graph: Media graph structure that will be used to walk the graph
  * @mdev: Pointer to the &media_device that contains the object
  *
+ * This function is deprecated, use media_pipeline_for_each_pad() instead.
+ *
  * The caller is required to hold the media_device graph_mutex during the graph
  * walk until the graph state is released.
  *
@@ -1087,6 +1100,8 @@ __must_check int media_graph_walk_init(
  * media_graph_walk_cleanup - Release resources used by graph walk.
  *
  * @graph: Media graph structure that will be used to walk the graph
+ *
+ * This function is deprecated, use media_pipeline_for_each_pad() instead.
  */
 void media_graph_walk_cleanup(struct media_graph *graph);
 
@@ -1097,6 +1112,8 @@ void media_graph_walk_cleanup(struct media_graph *graph);
  * @graph: Media graph structure that will be used to walk the graph
  * @entity: Starting entity
  *
+ * This function is deprecated, use media_pipeline_for_each_pad() instead.
+ *
  * Before using this function, media_graph_walk_init() must be
  * used to allocate resources used for walking the graph. This
  * function initializes the graph traversal structure to walk the
@@ -1112,6 +1129,8 @@ void media_graph_walk_start(struct media_graph *graph,
  * media_graph_walk_next - Get the next entity in the graph
  * @graph: Media graph structure
  *
+ * This function is deprecated, use media_pipeline_for_each_pad() instead.
+ *
  * Perform a depth-first traversal of the given media entities graph.
  *
  * The graph structure must have been previously initialized with a call to
@@ -1192,6 +1211,56 @@ __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).
+ */
+#define media_pipeline_for_each_entity(pipe, iter, entity)			\
+	for (entity = __media_pipeline_entity_iter_next((pipe), iter, NULL);	\
+	     entity != NULL;							\
+	     entity = __media_pipeline_entity_iter_next((pipe), iter, entity))
+
 /**
  * media_pipeline_alloc_start - Mark a pipeline as streaming
  * @pad: Starting pad
-- 
Regards,

Laurent Pinchart

[PATCH v1 3/5] media: ti: omap3isp: Use media_pipeline_for_each_entity()

[Laurent Pinchart]

Replace usage of the deprecated media graph walk API with the new
media_pipeline_for_each_entity() macro.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti/omap3isp/ispvideo.c | 21 +++----------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
index 3e5348c63773..aa81b5564b4f 100644
--- a/drivers/media/platform/ti/omap3isp/ispvideo.c
+++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
@@ -221,22 +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_graph graph;
-	struct media_entity *entity = &video->video.entity;
-	struct media_device *mdev = entity->graph_obj.mdev;
+	struct media_pipeline_entity_iter iter;
+	struct media_entity *entity;
 	struct isp_video *far_end = NULL;
-	int ret;
 
-	mutex_lock(&mdev->graph_mutex);
-	ret = media_graph_walk_init(&graph, mdev);
-	if (ret) {
-		mutex_unlock(&mdev->graph_mutex);
-		return ret;
-	}
-
-	media_graph_walk_start(&graph, entity);
-
-	while ((entity = media_graph_walk_next(&graph))) {
+	media_pipeline_for_each_entity(&pipe->pipe, &iter, entity) {
 		struct isp_video *__video;
 
 		media_entity_enum_set(&pipe->ent_enum, entity);
@@ -255,10 +244,6 @@ static int isp_video_get_graph_data(struct isp_video *video,
 			far_end = __video;
 	}
 
-	mutex_unlock(&mdev->graph_mutex);
-
-	media_graph_walk_cleanup(&graph);
-
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		pipe->input = far_end;
 		pipe->output = video;
-- 
Regards,

Laurent Pinchart

[PATCH v1 4/5] media: ti: omap4iss: Use media_pipeline_for_each_entity()

[Laurent Pinchart]

Replace usage of the deprecated media graph walk API with the new
media_pipeline_for_each_entity() macro.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/omap4iss/iss_video.c | 56 ++++++----------------
 1 file changed, 15 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
index 0ad70faa9ba0..be16abd7bf27 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -201,23 +201,14 @@ iss_video_remote_subdev(struct iss_video *video, u32 *pad)
 
 /* Return a pointer to the ISS video instance at the far end of the pipeline. */
 static struct iss_video *
-iss_video_far_end(struct iss_video *video)
+iss_video_far_end(struct iss_video *video, struct iss_pipeline *pipe)
 {
-	struct media_graph graph;
-	struct media_entity *entity = &video->video.entity;
-	struct media_device *mdev = entity->graph_obj.mdev;
-	struct iss_video *far_end = NULL;
+	struct media_pipeline_entity_iter iter;
+	struct media_entity *entity;
 
-	mutex_lock(&mdev->graph_mutex);
+	media_pipeline_for_each_entity(&pipe->pipe, &iter, entity) {
+		struct iss_video *far_end;
 
-	if (media_graph_walk_init(&graph, mdev)) {
-		mutex_unlock(&mdev->graph_mutex);
-		return NULL;
-	}
-
-	media_graph_walk_start(&graph, entity);
-
-	while ((entity = media_graph_walk_next(&graph))) {
 		if (entity == &video->video.entity)
 			continue;
 
@@ -226,16 +217,10 @@ iss_video_far_end(struct iss_video *video)
 
 		far_end = to_iss_video(media_entity_to_video_device(entity));
 		if (far_end->type != video->type)
-			break;
-
-		far_end = NULL;
+			return far_end;
 	}
 
-	mutex_unlock(&mdev->graph_mutex);
-
-	media_graph_walk_cleanup(&graph);
-
-	return far_end;
+	return NULL;
 }
 
 static int
@@ -850,9 +835,9 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 {
 	struct iss_video_fh *vfh = to_iss_video_fh(fh);
 	struct iss_video *video = video_drvdata(file);
-	struct media_graph graph;
-	struct media_entity *entity = &video->video.entity;
-	struct media_device *mdev = entity->graph_obj.mdev;
+	struct media_device *mdev = video->video.entity.graph_obj.mdev;
+	struct media_pipeline_entity_iter iter;
+	struct media_entity *entity;
 	enum iss_pipeline_state state;
 	struct iss_pipeline *pipe;
 	struct iss_video *far_end;
@@ -873,13 +858,9 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	pipe->external_rate = 0;
 	pipe->external_bpp = 0;
 
-	ret = media_entity_enum_init(&pipe->ent_enum, entity->graph_obj.mdev);
+	ret = media_entity_enum_init(&pipe->ent_enum, mdev);
 	if (ret)
-		goto err_graph_walk_init;
-
-	ret = media_graph_walk_init(&graph, entity->graph_obj.mdev);
-	if (ret)
-		goto err_graph_walk_init;
+		goto err_entity_enum_init;
 
 	if (video->iss->pdata->set_constraints)
 		video->iss->pdata->set_constraints(video->iss, true);
@@ -888,11 +869,8 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	if (ret < 0)
 		goto err_media_pipeline_start;
 
-	mutex_lock(&mdev->graph_mutex);
-	media_graph_walk_start(&graph, entity);
-	while ((entity = media_graph_walk_next(&graph)))
+	media_pipeline_for_each_entity(&pipe->pipe, &iter, entity)
 		media_entity_enum_set(&pipe->ent_enum, entity);
-	mutex_unlock(&mdev->graph_mutex);
 
 	/*
 	 * Verify that the currently configured format matches the output of
@@ -909,7 +887,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	 * Find the ISS video node connected at the far end of the pipeline and
 	 * update the pipeline.
 	 */
-	far_end = iss_video_far_end(video);
+	far_end = iss_video_far_end(video, pipe);
 
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		state = ISS_PIPELINE_STREAM_OUTPUT | ISS_PIPELINE_IDLE_OUTPUT;
@@ -966,8 +944,6 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 		spin_unlock_irqrestore(&video->qlock, flags);
 	}
 
-	media_graph_walk_cleanup(&graph);
-
 	mutex_unlock(&video->stream_lock);
 
 	return 0;
@@ -981,9 +957,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 		video->iss->pdata->set_constraints(video->iss, false);
 	video->queue = NULL;
 
-	media_graph_walk_cleanup(&graph);
-
-err_graph_walk_init:
+err_entity_enum_init:
 	media_entity_enum_cleanup(&pipe->ent_enum);
 
 	mutex_unlock(&video->stream_lock);
-- 
Regards,

Laurent Pinchart

[PATCH v1 5/5] media: xilinx: dma: Use media_pipeline_for_each_pad()

[Laurent Pinchart]

Replace usage of the deprecated media graph walk API with the new
media_pipeline_for_each_pad() macro.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++-----------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
index 0a7fd8642a65..fee02c8c85fd 100644
--- a/drivers/media/platform/xilinx/xilinx-dma.c
+++ b/drivers/media/platform/xilinx/xilinx-dma.c
@@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on)
 static int xvip_pipeline_validate(struct xvip_pipeline *pipe,
 				  struct xvip_dma *start)
 {
-	struct media_graph graph;
-	struct media_entity *entity = &start->video.entity;
-	struct media_device *mdev = entity->graph_obj.mdev;
+	struct media_pipeline_pad_iter iter;
 	unsigned int num_inputs = 0;
 	unsigned int num_outputs = 0;
-	int ret;
+	struct media_pad *pad;
 
-	mutex_lock(&mdev->graph_mutex);
-
-	/* Walk the graph to locate the video nodes. */
-	ret = media_graph_walk_init(&graph, mdev);
-	if (ret) {
-		mutex_unlock(&mdev->graph_mutex);
-		return ret;
-	}
-
-	media_graph_walk_start(&graph, entity);
-
-	while ((entity = media_graph_walk_next(&graph))) {
+	/* Locate the video nodes in the pipeline. */
+	media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) {
 		struct xvip_dma *dma;
 
-		if (entity->function != MEDIA_ENT_F_IO_V4L)
+		if (pad->entity->function != MEDIA_ENT_F_IO_V4L)
 			continue;
 
-		dma = to_xvip_dma(media_entity_to_video_device(entity));
+		dma = to_xvip_dma(media_entity_to_video_device(pad->entity));
 
 		if (dma->pad.flags & MEDIA_PAD_FL_SINK) {
 			pipe->output = dma;
@@ -207,10 +195,6 @@ static int xvip_pipeline_validate(struct xvip_pipeline *pipe,
 		}
 	}
 
-	mutex_unlock(&mdev->graph_mutex);
-
-	media_graph_walk_cleanup(&graph);
-
 	/* We need exactly one output and zero or one input. */
 	if (num_outputs != 1 || num_inputs > 1)
 		return -EPIPE;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/5] media: mc: entity: Add pad iterator for media_pipeline

[Tomi Valkeinen]

On 12/12/2022 16:16, Laurent Pinchart wrote:
> Add a media_pipeline_for_each_pad() macro to iterate over pads in a
> pipeline. This should be used by driver as a replacement of the
> media_graph_walk API, as iterating over the media_pipeline uses the
> cached list of pads and is thus more efficient.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/mc/mc-entity.c | 18 ++++++++++++++++++
>   include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
>   2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index b8bcbc734eaf..70df2050951c 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -932,6 +932,24 @@ __must_check int media_pipeline_alloc_start(struct media_pad *pad)
>   }
>   EXPORT_SYMBOL_GPL(media_pipeline_alloc_start);
>   
> +struct media_pad *
> +__media_pipeline_pad_iter_next(struct media_pipeline *pipe,
> +			       struct media_pipeline_pad_iter *iter,
> +			       struct media_pad *pad)
> +{
> +	if (!pad)
> +		iter->cursor = pipe->pads.next;
> +
> +	if (iter->cursor == &pipe->pads)
> +		return NULL;
> +
> +	pad = list_entry(iter->cursor, struct media_pipeline_pad, list)->pad;
> +	iter->cursor = iter->cursor->next;
> +
> +	return pad;
> +}
> +EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
> +
>   /* -----------------------------------------------------------------------------
>    * Links management
>    */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 85ed08ddee9d..e881e483b550 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -130,6 +130,15 @@ struct media_pipeline_pad {
>   	struct media_pad *pad;
>   };
>   
> +/**
> + * struct media_pipeline_pad_iter - Iterator for media_pipeline_for_each_pad
> + *
> + * @cursor: The current element
> + */
> +struct media_pipeline_pad_iter {
> +	struct list_head *cursor;
> +};
> +

Is there any reason to have this iter struct in a public header, vs. 
having it in mc-entity.c?

>   /**
>    * struct media_link - A link object part of a media graph.
>    *
> @@ -1163,6 +1172,26 @@ void media_pipeline_stop(struct media_pad *pad);
>    */
>   void __media_pipeline_stop(struct media_pad *pad);
>   
> +struct media_pad *
> +__media_pipeline_pad_iter_next(struct media_pipeline *pipe,
> +			       struct media_pipeline_pad_iter *iter,
> +			       struct media_pad *pad);
> +
> +/**
> + * media_pipeline_for_each_pad - Iterate on all pads in a media pipeline
> + * @pipe: The pipeline
> + * @iter: The iterator (struct media_pipeline_pad_iter)
> + * @pad: The iterator pad

If I understand this correctly, both iter and pad are just variables the 
macro will use. In other words, they are not used to pass any values.

Would it be better to declare those variables in the macro itself? Well, 
that has its downsides. But perhaps at least clarify in the doc that 
they are only variables used by the loop, and do not need to be initialized.

> + * Iterate on all pads 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().
> + */
> +#define media_pipeline_for_each_pad(pipe, iter, pad)			\
> +	for (pad = __media_pipeline_pad_iter_next((pipe), iter, NULL);	\
> +	     pad != NULL;						\
> +	     pad = __media_pipeline_pad_iter_next((pipe), iter, pad))
> +
>   /**
>    * media_pipeline_alloc_start - Mark a pipeline as streaming
>    * @pad: Starting pad

  Tomi

Re: [PATCH v1 2/5] media: mc: entity: Add entity iterator for media_pipeline

[Tomi Valkeinen]

On 12/12/2022 16:16, Laurent Pinchart wrote:
> Add a media_pipeline_for_each_entity() macro to iterate over entities in
> a pipeline. This should be used by driver as a replacement of the
> media_graph_walk API, as iterating over the media_pipeline uses the
> cached list of pads and is thus more efficient.
> 
> Deprecate the media_graph_walk API to indicate it shouldn't be used in
> new drivers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/mc/mc-entity.c | 37 +++++++++++++++++++
>   include/media/media-entity.h | 69 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 106 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 70df2050951c..f19bb98071b2 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -950,6 +950,43 @@ __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;

Shouldn't this be if (!media_entity...)? Doesn't 
media_entity_enum_test_and_set return true if the entity has already 
been seen, and thus we shouldn't return it?

> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
> +
>   /* -----------------------------------------------------------------------------
>    * Links management
>    */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index e881e483b550..1b820cb6fed1 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -139,6 +139,17 @@ 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;
> +};
> +

Same question here as for the previous one: can this be in the mc-entity.c?

  Tomi

Re: [PATCH v1 1/5] media: mc: entity: Add pad iterator for media_pipeline

[Laurent Pinchart]

Hi Tomi,

On Thu, Dec 15, 2022 at 02:33:43PM +0200, Tomi Valkeinen wrote:
> On 12/12/2022 16:16, Laurent Pinchart wrote:
> > Add a media_pipeline_for_each_pad() macro to iterate over pads in a
> > pipeline. This should be used by driver as a replacement of the
> > media_graph_walk API, as iterating over the media_pipeline uses the
> > cached list of pads and is thus more efficient.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/media/mc/mc-entity.c | 18 ++++++++++++++++++
> >   include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
> >   2 files changed, 47 insertions(+)
> > 
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index b8bcbc734eaf..70df2050951c 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -932,6 +932,24 @@ __must_check int media_pipeline_alloc_start(struct media_pad *pad)
> >   }
> >   EXPORT_SYMBOL_GPL(media_pipeline_alloc_start);
> >   
> > +struct media_pad *
> > +__media_pipeline_pad_iter_next(struct media_pipeline *pipe,
> > +			       struct media_pipeline_pad_iter *iter,
> > +			       struct media_pad *pad)
> > +{
> > +	if (!pad)
> > +		iter->cursor = pipe->pads.next;
> > +
> > +	if (iter->cursor == &pipe->pads)
> > +		return NULL;
> > +
> > +	pad = list_entry(iter->cursor, struct media_pipeline_pad, list)->pad;
> > +	iter->cursor = iter->cursor->next;
> > +
> > +	return pad;
> > +}
> > +EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
> > +
> >   /* -----------------------------------------------------------------------------
> >    * Links management
> >    */
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 85ed08ddee9d..e881e483b550 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -130,6 +130,15 @@ struct media_pipeline_pad {
> >   	struct media_pad *pad;
> >   };
> >   
> > +/**
> > + * struct media_pipeline_pad_iter - Iterator for media_pipeline_for_each_pad
> > + *
> > + * @cursor: The current element
> > + */
> > +struct media_pipeline_pad_iter {
> > +	struct list_head *cursor;
> > +};
> > +
> 
> Is there any reason to have this iter struct in a public header, vs. 
> having it in mc-entity.c?

It has to be instantiated on the stack by the user of the
media_pipeline_for_each_pad() macro. A typical usage is

	struct media_pipeline_pad_iter iter;
	struct media_pad *pad

	media_pipeline_for_each_pad(pipe, &iter, pad) {
		...
	};

Note how iter is not a pointer.

> >   /**
> >    * struct media_link - A link object part of a media graph.
> >    *
> > @@ -1163,6 +1172,26 @@ void media_pipeline_stop(struct media_pad *pad);
> >    */
> >   void __media_pipeline_stop(struct media_pad *pad);
> >   
> > +struct media_pad *
> > +__media_pipeline_pad_iter_next(struct media_pipeline *pipe,
> > +			       struct media_pipeline_pad_iter *iter,
> > +			       struct media_pad *pad);
> > +
> > +/**
> > + * media_pipeline_for_each_pad - Iterate on all pads in a media pipeline
> > + * @pipe: The pipeline
> > + * @iter: The iterator (struct media_pipeline_pad_iter)
> > + * @pad: The iterator pad
> 
> If I understand this correctly, both iter and pad are just variables the 
> macro will use. In other words, they are not used to pass any values.
> 
> Would it be better to declare those variables in the macro itself? Well, 
> that has its downsides. But perhaps at least clarify in the doc that 
> they are only variables used by the loop, and do not need to be initialized.

Now that the kernel uses C99, I suppose we could make the pad variable
locally declared within the loop:

#define media_pipeline_for_each_pad(pipe, pad)				\
	for (struct media_pipeline_pad *pad = __media_pipeline_pad_iter_next((pipe), iter, NULL);	\
	     pad != NULL;						\
	     pad = __media_pipeline_pad_iter_next((pipe), iter, pad))

Hiding the iter variable would be more difficult, as I don't think you
can declare multiple variables of different types.

I'm a bit concerned about backporting though, so I'd rather not do this
in this patch, but on top.

> > + * Iterate on all pads 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().
> > + */
> > +#define media_pipeline_for_each_pad(pipe, iter, pad)			\
> > +	for (pad = __media_pipeline_pad_iter_next((pipe), iter, NULL);	\
> > +	     pad != NULL;						\
> > +	     pad = __media_pipeline_pad_iter_next((pipe), iter, pad))
> > +
> >   /**
> >    * media_pipeline_alloc_start - Mark a pipeline as streaming
> >    * @pad: Starting pad

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/5] media: mc: entity: Add entity iterator for media_pipeline

[Laurent Pinchart]

Hi Tomi,

On Thu, Dec 15, 2022 at 02:48:43PM +0200, Tomi Valkeinen wrote:
> On 12/12/2022 16:16, Laurent Pinchart wrote:
> > Add a media_pipeline_for_each_entity() macro to iterate over entities in
> > a pipeline. This should be used by driver as a replacement of the
> > media_graph_walk API, as iterating over the media_pipeline uses the
> > cached list of pads and is thus more efficient.
> > 
> > Deprecate the media_graph_walk API to indicate it shouldn't be used in
> > new drivers.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/media/mc/mc-entity.c | 37 +++++++++++++++++++
> >   include/media/media-entity.h | 69 ++++++++++++++++++++++++++++++++++++
> >   2 files changed, 106 insertions(+)
> > 
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 70df2050951c..f19bb98071b2 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -950,6 +950,43 @@ __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;
> 
> Shouldn't this be if (!media_entity...)? Doesn't 
> media_entity_enum_test_and_set return true if the entity has already 
> been seen, and thus we shouldn't return it?

You're absolutely right. I'll fix that. I wonder how it escaped my
tests.

> > +	}
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
> > +
> >   /* -----------------------------------------------------------------------------
> >    * Links management
> >    */
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index e881e483b550..1b820cb6fed1 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -139,6 +139,17 @@ 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;
> > +};
> > +
> 
> Same question here as for the previous one: can this be in the mc-entity.c?

I'm afraid not, for the same reason as in the previous patch.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/5] media: mc: entity: Add pad iterator for media_pipeline

[Tomi Valkeinen]

On 15/12/2022 14:48, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Thu, Dec 15, 2022 at 02:33:43PM +0200, Tomi Valkeinen wrote:
>> On 12/12/2022 16:16, Laurent Pinchart wrote:
>>> Add a media_pipeline_for_each_pad() macro to iterate over pads in a
>>> pipeline. This should be used by driver as a replacement of the
>>> media_graph_walk API, as iterating over the media_pipeline uses the
>>> cached list of pads and is thus more efficient.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>    drivers/media/mc/mc-entity.c | 18 ++++++++++++++++++
>>>    include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
>>>    2 files changed, 47 insertions(+)
>>>
>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>>> index b8bcbc734eaf..70df2050951c 100644
>>> --- a/drivers/media/mc/mc-entity.c
>>> +++ b/drivers/media/mc/mc-entity.c
>>> @@ -932,6 +932,24 @@ __must_check int media_pipeline_alloc_start(struct media_pad *pad)
>>>    }
>>>    EXPORT_SYMBOL_GPL(media_pipeline_alloc_start);
>>>    
>>> +struct media_pad *
>>> +__media_pipeline_pad_iter_next(struct media_pipeline *pipe,
>>> +			       struct media_pipeline_pad_iter *iter,
>>> +			       struct media_pad *pad)
>>> +{
>>> +	if (!pad)
>>> +		iter->cursor = pipe->pads.next;
>>> +
>>> +	if (iter->cursor == &pipe->pads)
>>> +		return NULL;
>>> +
>>> +	pad = list_entry(iter->cursor, struct media_pipeline_pad, list)->pad;
>>> +	iter->cursor = iter->cursor->next;
>>> +
>>> +	return pad;
>>> +}
>>> +EXPORT_SYMBOL_GPL(__media_pipeline_pad_iter_next);
>>> +
>>>    /* -----------------------------------------------------------------------------
>>>     * Links management
>>>     */
>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>> index 85ed08ddee9d..e881e483b550 100644
>>> --- a/include/media/media-entity.h
>>> +++ b/include/media/media-entity.h
>>> @@ -130,6 +130,15 @@ struct media_pipeline_pad {
>>>    	struct media_pad *pad;
>>>    };
>>>    
>>> +/**
>>> + * struct media_pipeline_pad_iter - Iterator for media_pipeline_for_each_pad
>>> + *
>>> + * @cursor: The current element
>>> + */
>>> +struct media_pipeline_pad_iter {
>>> +	struct list_head *cursor;
>>> +};
>>> +
>>
>> Is there any reason to have this iter struct in a public header, vs.
>> having it in mc-entity.c?
> 
> It has to be instantiated on the stack by the user of the
> media_pipeline_for_each_pad() macro. A typical usage is
> 
> 	struct media_pipeline_pad_iter iter;
> 	struct media_pad *pad
> 
> 	media_pipeline_for_each_pad(pipe, &iter, pad) {
> 		...
> 	};
> 
> Note how iter is not a pointer.

Ah, right.

>>>    /**
>>>     * struct media_link - A link object part of a media graph.
>>>     *
>>> @@ -1163,6 +1172,26 @@ void media_pipeline_stop(struct media_pad *pad);
>>>     */
>>>    void __media_pipeline_stop(struct media_pad *pad);
>>>    
>>> +struct media_pad *
>>> +__media_pipeline_pad_iter_next(struct media_pipeline *pipe,
>>> +			       struct media_pipeline_pad_iter *iter,
>>> +			       struct media_pad *pad);
>>> +
>>> +/**
>>> + * media_pipeline_for_each_pad - Iterate on all pads in a media pipeline
>>> + * @pipe: The pipeline
>>> + * @iter: The iterator (struct media_pipeline_pad_iter)
>>> + * @pad: The iterator pad
>>
>> If I understand this correctly, both iter and pad are just variables the
>> macro will use. In other words, they are not used to pass any values.
>>
>> Would it be better to declare those variables in the macro itself? Well,
>> that has its downsides. But perhaps at least clarify in the doc that
>> they are only variables used by the loop, and do not need to be initialized.
> 
> Now that the kernel uses C99, I suppose we could make the pad variable
> locally declared within the loop:
> 
> #define media_pipeline_for_each_pad(pipe, pad)				\
> 	for (struct media_pipeline_pad *pad = __media_pipeline_pad_iter_next((pipe), iter, NULL);	\
> 	     pad != NULL;						\
> 	     pad = __media_pipeline_pad_iter_next((pipe), iter, pad))
> 
> Hiding the iter variable would be more difficult, as I don't think you
> can declare multiple variables of different types.
> 
> I'm a bit concerned about backporting though, so I'd rather not do this
> in this patch, but on top.

I was thinking about using a do {} while(0) around the for loop to 
declare the variables, but.. of course that can't be used here. One 
shouldn't do reviews when one has a cold =).

  Tomi

Re: [PATCH v1 3/5] media: ti: omap3isp: Use media_pipeline_for_each_entity()

[Tomi Valkeinen]

On 12/12/2022 16:16, Laurent Pinchart wrote:
> Replace usage of the deprecated media graph walk API with the new
> media_pipeline_for_each_entity() macro.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/platform/ti/omap3isp/ispvideo.c | 21 +++----------------
>   1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
> index 3e5348c63773..aa81b5564b4f 100644
> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
> @@ -221,22 +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_graph graph;
> -	struct media_entity *entity = &video->video.entity;
> -	struct media_device *mdev = entity->graph_obj.mdev;
> +	struct media_pipeline_entity_iter iter;
> +	struct media_entity *entity;
>   	struct isp_video *far_end = NULL;
> -	int ret;
>   
> -	mutex_lock(&mdev->graph_mutex);
> -	ret = media_graph_walk_init(&graph, mdev);
> -	if (ret) {
> -		mutex_unlock(&mdev->graph_mutex);
> -		return ret;
> -	}
> -
> -	media_graph_walk_start(&graph, entity);
> -
> -	while ((entity = media_graph_walk_next(&graph))) {
> +	media_pipeline_for_each_entity(&pipe->pipe, &iter, entity) {
>   		struct isp_video *__video;
>   
>   		media_entity_enum_set(&pipe->ent_enum, entity);
> @@ -255,10 +244,6 @@ static int isp_video_get_graph_data(struct isp_video *video,
>   			far_end = __video;
>   	}
>   
> -	mutex_unlock(&mdev->graph_mutex);
> -
> -	media_graph_walk_cleanup(&graph);
> -
>   	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>   		pipe->input = far_end;
>   		pipe->output = video;

media_pipeline_entity_iter_init() and media_pipeline_entity_iter_cleanup()?

  Tomi

Re: [PATCH v1 5/5] media: xilinx: dma: Use media_pipeline_for_each_pad()

[Tomi Valkeinen]

On 12/12/2022 16:16, Laurent Pinchart wrote:
> Replace usage of the deprecated media graph walk API with the new
> media_pipeline_for_each_pad() macro.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++-----------------
>   1 file changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
> index 0a7fd8642a65..fee02c8c85fd 100644
> --- a/drivers/media/platform/xilinx/xilinx-dma.c
> +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on)
>   static int xvip_pipeline_validate(struct xvip_pipeline *pipe,
>   				  struct xvip_dma *start)
>   {
> -	struct media_graph graph;
> -	struct media_entity *entity = &start->video.entity;
> -	struct media_device *mdev = entity->graph_obj.mdev;
> +	struct media_pipeline_pad_iter iter;
>   	unsigned int num_inputs = 0;
>   	unsigned int num_outputs = 0;
> -	int ret;
> +	struct media_pad *pad;
>   
> -	mutex_lock(&mdev->graph_mutex);
> -
> -	/* Walk the graph to locate the video nodes. */
> -	ret = media_graph_walk_init(&graph, mdev);
> -	if (ret) {
> -		mutex_unlock(&mdev->graph_mutex);
> -		return ret;
> -	}
> -
> -	media_graph_walk_start(&graph, entity);
> -
> -	while ((entity = media_graph_walk_next(&graph))) {
> +	/* Locate the video nodes in the pipeline. */
> +	media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) {
>   		struct xvip_dma *dma;
>   
> -		if (entity->function != MEDIA_ENT_F_IO_V4L)
> +		if (pad->entity->function != MEDIA_ENT_F_IO_V4L)
>   			continue;

Why do you iterate the pads here, instead of using 
media_pipeline_for_each_entity()?

  Tomi

Re: [PATCH v1 5/5] media: xilinx: dma: Use media_pipeline_for_each_pad()

[Laurent Pinchart]

Hi Tomi,

On Thu, Dec 15, 2022 at 03:29:13PM +0200, Tomi Valkeinen wrote:
> On 12/12/2022 16:16, Laurent Pinchart wrote:
> > Replace usage of the deprecated media graph walk API with the new
> > media_pipeline_for_each_pad() macro.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++-----------------
> >   1 file changed, 6 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
> > index 0a7fd8642a65..fee02c8c85fd 100644
> > --- a/drivers/media/platform/xilinx/xilinx-dma.c
> > +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> > @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on)
> >   static int xvip_pipeline_validate(struct xvip_pipeline *pipe,
> >   				  struct xvip_dma *start)
> >   {
> > -	struct media_graph graph;
> > -	struct media_entity *entity = &start->video.entity;
> > -	struct media_device *mdev = entity->graph_obj.mdev;
> > +	struct media_pipeline_pad_iter iter;
> >   	unsigned int num_inputs = 0;
> >   	unsigned int num_outputs = 0;
> > -	int ret;
> > +	struct media_pad *pad;
> >   
> > -	mutex_lock(&mdev->graph_mutex);
> > -
> > -	/* Walk the graph to locate the video nodes. */
> > -	ret = media_graph_walk_init(&graph, mdev);
> > -	if (ret) {
> > -		mutex_unlock(&mdev->graph_mutex);
> > -		return ret;
> > -	}
> > -
> > -	media_graph_walk_start(&graph, entity);
> > -
> > -	while ((entity = media_graph_walk_next(&graph))) {
> > +	/* Locate the video nodes in the pipeline. */
> > +	media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) {
> >   		struct xvip_dma *dma;
> >   
> > -		if (entity->function != MEDIA_ENT_F_IO_V4L)
> > +		if (pad->entity->function != MEDIA_ENT_F_IO_V4L)
> >   			continue;
> 
> Why do you iterate the pads here, instead of using 
> media_pipeline_for_each_entity()?

The entity iterator still iterates over all pads internally, so it's not
more efficient. It also requires explicit initialization and cleanup, so
is more complicated to use. In this specific case, the driver ignores
all pads belonging to entities that are not V4L2 video nodes
(MEDIA_ENT_F_IO_V4L). As such entities have a single pad, there's a
guarantee they will not be visited twice.

If you think this is too fragile due to the assumption that
MEDIA_ENT_F_IO_V4L entities have a single pad, I can use the entity
iterator, it's not a big deal.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 5/5] media: xilinx: dma: Use media_pipeline_for_each_pad()

[Tomi Valkeinen]

On 16/12/2022 02:36, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Thu, Dec 15, 2022 at 03:29:13PM +0200, Tomi Valkeinen wrote:
>> On 12/12/2022 16:16, Laurent Pinchart wrote:
>>> Replace usage of the deprecated media graph walk API with the new
>>> media_pipeline_for_each_pad() macro.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>    drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++-----------------
>>>    1 file changed, 6 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
>>> index 0a7fd8642a65..fee02c8c85fd 100644
>>> --- a/drivers/media/platform/xilinx/xilinx-dma.c
>>> +++ b/drivers/media/platform/xilinx/xilinx-dma.c
>>> @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on)
>>>    static int xvip_pipeline_validate(struct xvip_pipeline *pipe,
>>>    				  struct xvip_dma *start)
>>>    {
>>> -	struct media_graph graph;
>>> -	struct media_entity *entity = &start->video.entity;
>>> -	struct media_device *mdev = entity->graph_obj.mdev;
>>> +	struct media_pipeline_pad_iter iter;
>>>    	unsigned int num_inputs = 0;
>>>    	unsigned int num_outputs = 0;
>>> -	int ret;
>>> +	struct media_pad *pad;
>>>    
>>> -	mutex_lock(&mdev->graph_mutex);
>>> -
>>> -	/* Walk the graph to locate the video nodes. */
>>> -	ret = media_graph_walk_init(&graph, mdev);
>>> -	if (ret) {
>>> -		mutex_unlock(&mdev->graph_mutex);
>>> -		return ret;
>>> -	}
>>> -
>>> -	media_graph_walk_start(&graph, entity);
>>> -
>>> -	while ((entity = media_graph_walk_next(&graph))) {
>>> +	/* Locate the video nodes in the pipeline. */
>>> +	media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) {
>>>    		struct xvip_dma *dma;
>>>    
>>> -		if (entity->function != MEDIA_ENT_F_IO_V4L)
>>> +		if (pad->entity->function != MEDIA_ENT_F_IO_V4L)
>>>    			continue;
>>
>> Why do you iterate the pads here, instead of using
>> media_pipeline_for_each_entity()?
> 
> The entity iterator still iterates over all pads internally, so it's not
> more efficient. It also requires explicit initialization and cleanup, so
> is more complicated to use. In this specific case, the driver ignores
> all pads belonging to entities that are not V4L2 video nodes
> (MEDIA_ENT_F_IO_V4L). As such entities have a single pad, there's a
> guarantee they will not be visited twice.
> 
> If you think this is too fragile due to the assumption that
> MEDIA_ENT_F_IO_V4L entities have a single pad, I can use the entity
> iterator, it's not a big deal.

Well, the reason I commented was that the old code iterates entities, in 
this series you add a new way to iterate entities, and then... you don't 
use it for iterating entities =).

I think it hints that the entity iteration is not as good as one could 
wish, but I don't right away see the means to improve it. I do miss C++ 
in cases like this.

I think I would have gone with the entity iterator even if it's slightly 
more complex to use, just to highlight the point that this is iterating 
entities. But I don't mind either way.

  Tomi

Re: [PATCH v1 5/5] media: xilinx: dma: Use media_pipeline_for_each_pad()

[Laurent Pinchart]

Hi Tomi,

On Fri, Dec 16, 2022 at 08:47:13AM +0200, Tomi Valkeinen wrote:
> On 16/12/2022 02:36, Laurent Pinchart wrote:
> > On Thu, Dec 15, 2022 at 03:29:13PM +0200, Tomi Valkeinen wrote:
> >> On 12/12/2022 16:16, Laurent Pinchart wrote:
> >>> Replace usage of the deprecated media graph walk API with the new
> >>> media_pipeline_for_each_pad() macro.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>>    drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++-----------------
> >>>    1 file changed, 6 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
> >>> index 0a7fd8642a65..fee02c8c85fd 100644
> >>> --- a/drivers/media/platform/xilinx/xilinx-dma.c
> >>> +++ b/drivers/media/platform/xilinx/xilinx-dma.c
> >>> @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on)
> >>>    static int xvip_pipeline_validate(struct xvip_pipeline *pipe,
> >>>    				  struct xvip_dma *start)
> >>>    {
> >>> -	struct media_graph graph;
> >>> -	struct media_entity *entity = &start->video.entity;
> >>> -	struct media_device *mdev = entity->graph_obj.mdev;
> >>> +	struct media_pipeline_pad_iter iter;
> >>>    	unsigned int num_inputs = 0;
> >>>    	unsigned int num_outputs = 0;
> >>> -	int ret;
> >>> +	struct media_pad *pad;
> >>>    
> >>> -	mutex_lock(&mdev->graph_mutex);
> >>> -
> >>> -	/* Walk the graph to locate the video nodes. */
> >>> -	ret = media_graph_walk_init(&graph, mdev);
> >>> -	if (ret) {
> >>> -		mutex_unlock(&mdev->graph_mutex);
> >>> -		return ret;
> >>> -	}
> >>> -
> >>> -	media_graph_walk_start(&graph, entity);
> >>> -
> >>> -	while ((entity = media_graph_walk_next(&graph))) {
> >>> +	/* Locate the video nodes in the pipeline. */
> >>> +	media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) {
> >>>    		struct xvip_dma *dma;
> >>>    
> >>> -		if (entity->function != MEDIA_ENT_F_IO_V4L)
> >>> +		if (pad->entity->function != MEDIA_ENT_F_IO_V4L)
> >>>    			continue;
> >>
> >> Why do you iterate the pads here, instead of using
> >> media_pipeline_for_each_entity()?
> > 
> > The entity iterator still iterates over all pads internally, so it's not
> > more efficient. It also requires explicit initialization and cleanup, so
> > is more complicated to use. In this specific case, the driver ignores
> > all pads belonging to entities that are not V4L2 video nodes
> > (MEDIA_ENT_F_IO_V4L). As such entities have a single pad, there's a
> > guarantee they will not be visited twice.
> > 
> > If you think this is too fragile due to the assumption that
> > MEDIA_ENT_F_IO_V4L entities have a single pad, I can use the entity
> > iterator, it's not a big deal.
> 
> Well, the reason I commented was that the old code iterates entities, in 
> this series you add a new way to iterate entities, and then... you don't 
> use it for iterating entities =).
> 
> I think it hints that the entity iteration is not as good as one could 
> wish, but I don't right away see the means to improve it. I do miss C++ 
> in cases like this.

Indeed. In this specific case, it would be possible to initialize the
iterator on first use, but not to destroy it automatically if the code
breaks from the loop.

There's something else I've been thinking about: we could cache the list
of entities in the media_pipeline structure when starting the pipeline,
to facilitate iteration. The entity iterator API would become simpler
(and more efficient), but pipeline start would be more costly. Any
opinion ?

> I think I would have gone with the entity iterator even if it's slightly 
> more complex to use, just to highlight the point that this is iterating 
> entities. But I don't mind either way.

-- 
Regards,

Laurent Pinchart