| Message ID | 20221212141621.724-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: Replace media graph walk in several drivers | expand |
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
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
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
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
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(+)