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@xxxxxxxxxxxxxxxx>
---
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
