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.
> > /**
> > * 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
