| Message ID | 20211213232849.40071-4-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] media: media.h: Add new media link type | expand |
Hi Sakari On 14/12/2021 15:01, Sakari Ailus wrote: > Hi Daniel, > > On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote: >> When iterating over the media graph, don't follow links that are not >> pad-to-pad links. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes since the rfc: >> >> - new patch >> >> drivers/media/mc/mc-entity.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >> index d79eb88bc167..aeddc3f6310e 100644 >> --- a/drivers/media/mc/mc-entity.c >> +++ b/drivers/media/mc/mc-entity.c >> @@ -325,6 +325,14 @@ static void media_graph_walk_iter(struct media_graph *graph) >> >> link = list_entry(link_top(graph), typeof(*link), list); >> >> + /* If the link is not a pad-to-pad link, don't follow it */ > This comment should mention data links, not pad-to-pad links. I wondered about the terminology of this actually...since we create those links with media_create_pad_link(), and they're called pad-to-pad links in the documentation [1], but in other cases called data links. Do we need to fix those other references too? [1] https://www.kernel.org/doc/html/v5.0/media/kapi/mc-core.html#links > > Seems fine apart from this. > >> + if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_DATA_LINK) { >> + link_top(graph) = link_top(graph)->next; >> + dev_dbg(entity->graph_obj.mdev->dev, "walk: skipping %s link\n", >> + link_type(link)); >> + return; >> + } >> + >> /* The link is not enabled so we do not follow. */ >> if (!(link->flags & MEDIA_LNK_FL_ENABLED)) { >> link_top(graph) = link_top(graph)->next;
Hi Daniel, On Tue, Dec 14, 2021 at 04:14:21PM +0000, Daniel Scally wrote: > Hi Sakari > > On 14/12/2021 15:01, Sakari Ailus wrote: > > Hi Daniel, > > > > On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote: > >> When iterating over the media graph, don't follow links that are not > >> pad-to-pad links. > >> > >> Signed-off-by: Daniel Scally <djrscally@gmail.com> > >> --- > >> Changes since the rfc: > >> > >> - new patch > >> > >> drivers/media/mc/mc-entity.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > >> index d79eb88bc167..aeddc3f6310e 100644 > >> --- a/drivers/media/mc/mc-entity.c > >> +++ b/drivers/media/mc/mc-entity.c > >> @@ -325,6 +325,14 @@ static void media_graph_walk_iter(struct media_graph *graph) > >> > >> link = list_entry(link_top(graph), typeof(*link), list); > >> > >> + /* If the link is not a pad-to-pad link, don't follow it */ > > This comment should mention data links, not pad-to-pad links. > > > I wondered about the terminology of this actually...since we create > those links with media_create_pad_link(), and they're called pad-to-pad > links in the documentation [1], but in other cases called data links. Do > we need to fix those other references too? > > > > [1] https://www.kernel.org/doc/html/v5.0/media/kapi/mc-core.html#links Good point. There were only one type of links before the interface links were introduced. Some of the documentation seems to discuss pad links whereas the corresponding macro name is MEDIA_LNK_FL_DATA_LINK. What the links really represent is flow of data. It would be good to align this, although that should probably be done in a different context from this patchset.
Hi Sakari On 14/12/2021 21:22, Sakari Ailus wrote: > Hi Daniel, > > On Tue, Dec 14, 2021 at 04:14:21PM +0000, Daniel Scally wrote: >> Hi Sakari >> >> On 14/12/2021 15:01, Sakari Ailus wrote: >>> Hi Daniel, >>> >>> On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote: >>>> When iterating over the media graph, don't follow links that are not >>>> pad-to-pad links. >>>> >>>> Signed-off-by: Daniel Scally <djrscally@gmail.com> >>>> --- >>>> Changes since the rfc: >>>> >>>> - new patch >>>> >>>> drivers/media/mc/mc-entity.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >>>> index d79eb88bc167..aeddc3f6310e 100644 >>>> --- a/drivers/media/mc/mc-entity.c >>>> +++ b/drivers/media/mc/mc-entity.c >>>> @@ -325,6 +325,14 @@ static void media_graph_walk_iter(struct media_graph *graph) >>>> >>>> link = list_entry(link_top(graph), typeof(*link), list); >>>> >>>> + /* If the link is not a pad-to-pad link, don't follow it */ >>> This comment should mention data links, not pad-to-pad links. >> >> I wondered about the terminology of this actually...since we create >> those links with media_create_pad_link(), and they're called pad-to-pad >> links in the documentation [1], but in other cases called data links. Do >> we need to fix those other references too? >> >> >> >> [1] https://www.kernel.org/doc/html/v5.0/media/kapi/mc-core.html#links > Good point. > > There were only one type of links before the interface links were > introduced. Some of the documentation seems to discuss pad links whereas > the corresponding macro name is MEDIA_LNK_FL_DATA_LINK. What the links > really represent is flow of data. > > It would be good to align this, although that should probably be done in a > different context from this patchset. > Ack; I'll fix the comment as you suggested for now
On Tue, Dec 14, 2021 at 05:01:23PM +0200, Sakari Ailus wrote: > On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote: > > When iterating over the media graph, don't follow links that are not > > pad-to-pad links. > > > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > > --- > > Changes since the rfc: > > > > - new patch > > > > drivers/media/mc/mc-entity.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > > index d79eb88bc167..aeddc3f6310e 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -325,6 +325,14 @@ static void media_graph_walk_iter(struct media_graph *graph) > > > > link = list_entry(link_top(graph), typeof(*link), list); > > > > + /* If the link is not a pad-to-pad link, don't follow it */ > > This comment should mention data links, not pad-to-pad links. > > Seems fine apart from this. > > > + if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_DATA_LINK) { > > + link_top(graph) = link_top(graph)->next; > > + dev_dbg(entity->graph_obj.mdev->dev, "walk: skipping %s link\n", > > + link_type(link)); I would drop the debug message. The other messages in this function can be useful to figure out why graph walk doesn't behave like expected (reporting, for instance, that a disabled link is not traversed), but I don't think there's much value in indicating we're skipping non-data links. With these issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + return; > > + } > > + > > /* The link is not enabled so we do not follow. */ > > if (!(link->flags & MEDIA_LNK_FL_ENABLED)) { > > link_top(graph) = link_top(graph)->next;
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c index d79eb88bc167..aeddc3f6310e 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -325,6 +325,14 @@ static void media_graph_walk_iter(struct media_graph *graph) link = list_entry(link_top(graph), typeof(*link), list); + /* If the link is not a pad-to-pad link, don't follow it */ + if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_DATA_LINK) { + link_top(graph) = link_top(graph)->next; + dev_dbg(entity->graph_obj.mdev->dev, "walk: skipping %s link\n", + link_type(link)); + return; + } + /* The link is not enabled so we do not follow. */ if (!(link->flags & MEDIA_LNK_FL_ENABLED)) { link_top(graph) = link_top(graph)->next;
When iterating over the media graph, don't follow links that are not pad-to-pad links. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes since the rfc: - new patch drivers/media/mc/mc-entity.c | 8 ++++++++ 1 file changed, 8 insertions(+)