| Message ID | 20210415130450.421168-12-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | v4l: subdev internal routing | expand |
Hi Tomi and Sakari, Thank you for the patch. There's an extra "to" in the subject line. On Thu, Apr 15, 2021 at 04:04:37PM +0300, Tomi Valkeinen wrote: > From: Sakari Ailus <sakari.ailus@linux.intel.com> > > Links are validated along the pipeline which is about to start streaming. > Not all the pads in entities that are traversed along that pipeline are > part of the pipeline, however. Skip the link validation for such pads, > and while at there rename "other_pad" to "local_pad" to convey the fact > the route to be checked is internal to the entity. Both "pad" and "local_pad" are local. I would have kept the "other_pad" variable name. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/mc/mc-entity.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 28d7fd254c77..fe6cb743c85c 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -482,12 +482,17 @@ __must_check int __media_pipeline_start(struct media_pad *pad, > bitmap_fill(has_no_links, entity->num_pads); > > list_for_each_entry(link, &entity->links, list) { > - struct media_pad *other_pad = > + struct media_pad *local_pad = > link->sink->entity == entity ? > link->sink : link->source; > > + /* Ignore pads to which there is no route. */ > + if (!media_entity_has_route(entity, pad->index, > + local_pad->index)) > + continue; > + > /* Mark that a pad is connected by a link. */ > - bitmap_clear(has_no_links, other_pad->index, 1); > + bitmap_clear(has_no_links, local_pad->index, 1); > > /* > * Pads that either do not need to connect or > @@ -496,13 +501,13 @@ __must_check int __media_pipeline_start(struct media_pad *pad, > */ > if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) || > link->flags & MEDIA_LNK_FL_ENABLED) > - bitmap_set(active, other_pad->index, 1); > + bitmap_set(active, local_pad->index, 1); > > /* > * Link validation will only take place for > * sink ends of the link that are enabled. > */ > - if (link->sink != other_pad || > + if (link->sink != local_pad || > !(link->flags & MEDIA_LNK_FL_ENABLED)) > continue; > -- Regards, Laurent Pinchart
Hi Laurent, On Sun, Apr 18, 2021 at 09:06:11PM +0300, Laurent Pinchart wrote: > Hi Tomi and Sakari, > > Thank you for the patch. > > There's an extra "to" in the subject line. > > On Thu, Apr 15, 2021 at 04:04:37PM +0300, Tomi Valkeinen wrote: > > From: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Links are validated along the pipeline which is about to start streaming. > > Not all the pads in entities that are traversed along that pipeline are > > part of the pipeline, however. Skip the link validation for such pads, > > and while at there rename "other_pad" to "local_pad" to convey the fact > > the route to be checked is internal to the entity. > > Both "pad" and "local_pad" are local. I would have kept the "other_pad" The pad that in the remote entity is not local. The other one could be called remote_pad though. > variable name. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/mc/mc-entity.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > > index 28d7fd254c77..fe6cb743c85c 100644 > > --- a/drivers/media/mc/mc-entity.c > > +++ b/drivers/media/mc/mc-entity.c > > @@ -482,12 +482,17 @@ __must_check int __media_pipeline_start(struct media_pad *pad, > > bitmap_fill(has_no_links, entity->num_pads); > > > > list_for_each_entry(link, &entity->links, list) { > > - struct media_pad *other_pad = > > + struct media_pad *local_pad = > > link->sink->entity == entity ? > > link->sink : link->source; > > > > + /* Ignore pads to which there is no route. */ > > + if (!media_entity_has_route(entity, pad->index, > > + local_pad->index)) > > + continue; > > + > > /* Mark that a pad is connected by a link. */ > > - bitmap_clear(has_no_links, other_pad->index, 1); > > + bitmap_clear(has_no_links, local_pad->index, 1); > > > > /* > > * Pads that either do not need to connect or > > @@ -496,13 +501,13 @@ __must_check int __media_pipeline_start(struct media_pad *pad, > > */ > > if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) || > > link->flags & MEDIA_LNK_FL_ENABLED) > > - bitmap_set(active, other_pad->index, 1); > > + bitmap_set(active, local_pad->index, 1); > > > > /* > > * Link validation will only take place for > > * sink ends of the link that are enabled. > > */ > > - if (link->sink != other_pad || > > + if (link->sink != local_pad || > > !(link->flags & MEDIA_LNK_FL_ENABLED)) > > continue; > > > -- Sakari Ailus
On 20/04/2021 14:41, Sakari Ailus wrote: > Hi Laurent, > > On Sun, Apr 18, 2021 at 09:06:11PM +0300, Laurent Pinchart wrote: >> Hi Tomi and Sakari, >> >> Thank you for the patch. >> >> There's an extra "to" in the subject line. >> >> On Thu, Apr 15, 2021 at 04:04:37PM +0300, Tomi Valkeinen wrote: >>> From: Sakari Ailus <sakari.ailus@linux.intel.com> >>> >>> Links are validated along the pipeline which is about to start streaming. >>> Not all the pads in entities that are traversed along that pipeline are >>> part of the pipeline, however. Skip the link validation for such pads, >>> and while at there rename "other_pad" to "local_pad" to convey the fact >>> the route to be checked is internal to the entity. >> >> Both "pad" and "local_pad" are local. I would have kept the "other_pad" > > The pad that in the remote entity is not local. The other one could be > called remote_pad though. I'm not sure what you mean here. Aren't both pad and local_pad pads of a single entity here? If so, I think Laurent's comment makes sense, and other_pad is a better name. Tomi
Moi, On Fri, Apr 23, 2021 at 03:37:03PM +0300, Tomi Valkeinen wrote: > On 20/04/2021 14:41, Sakari Ailus wrote: > > Hi Laurent, > > > > On Sun, Apr 18, 2021 at 09:06:11PM +0300, Laurent Pinchart wrote: > > > Hi Tomi and Sakari, > > > > > > Thank you for the patch. > > > > > > There's an extra "to" in the subject line. > > > > > > On Thu, Apr 15, 2021 at 04:04:37PM +0300, Tomi Valkeinen wrote: > > > > From: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > > > > > Links are validated along the pipeline which is about to start streaming. > > > > Not all the pads in entities that are traversed along that pipeline are > > > > part of the pipeline, however. Skip the link validation for such pads, > > > > and while at there rename "other_pad" to "local_pad" to convey the fact > > > > the route to be checked is internal to the entity. > > > > > > Both "pad" and "local_pad" are local. I would have kept the "other_pad" > > > > The pad that in the remote entity is not local. The other one could be > > called remote_pad though. > > I'm not sure what you mean here. Aren't both pad and local_pad pads of a > single entity here? If so, I think Laurent's comment makes sense, and > other_pad is a better name. I guess you could argue for either. I'm fine dropping the patch. -- Sakari Ailus
Hi Sakari, On Thu, Apr 29, 2021 at 03:06:12PM +0300, Sakari Ailus wrote: > On Fri, Apr 23, 2021 at 03:37:03PM +0300, Tomi Valkeinen wrote: > > On 20/04/2021 14:41, Sakari Ailus wrote: > > > On Sun, Apr 18, 2021 at 09:06:11PM +0300, Laurent Pinchart wrote: > > > > Hi Tomi and Sakari, > > > > > > > > Thank you for the patch. > > > > > > > > There's an extra "to" in the subject line. > > > > > > > > On Thu, Apr 15, 2021 at 04:04:37PM +0300, Tomi Valkeinen wrote: > > > > > From: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > > > > > > > Links are validated along the pipeline which is about to start streaming. > > > > > Not all the pads in entities that are traversed along that pipeline are > > > > > part of the pipeline, however. Skip the link validation for such pads, > > > > > and while at there rename "other_pad" to "local_pad" to convey the fact > > > > > the route to be checked is internal to the entity. > > > > > > > > Both "pad" and "local_pad" are local. I would have kept the "other_pad" > > > > > > The pad that in the remote entity is not local. The other one could be > > > called remote_pad though. > > > > I'm not sure what you mean here. Aren't both pad and local_pad pads of a > > single entity here? If so, I think Laurent's comment makes sense, and > > other_pad is a better name. > > I guess you could argue for either. I'm fine dropping the patch. I think the patch is good, it's just the extra rename that puzzled me. -- Regards, Laurent Pinchart
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c index 28d7fd254c77..fe6cb743c85c 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -482,12 +482,17 @@ __must_check int __media_pipeline_start(struct media_pad *pad, bitmap_fill(has_no_links, entity->num_pads); list_for_each_entry(link, &entity->links, list) { - struct media_pad *other_pad = + struct media_pad *local_pad = link->sink->entity == entity ? link->sink : link->source; + /* Ignore pads to which there is no route. */ + if (!media_entity_has_route(entity, pad->index, + local_pad->index)) + continue; + /* Mark that a pad is connected by a link. */ - bitmap_clear(has_no_links, other_pad->index, 1); + bitmap_clear(has_no_links, local_pad->index, 1); /* * Pads that either do not need to connect or @@ -496,13 +501,13 @@ __must_check int __media_pipeline_start(struct media_pad *pad, */ if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) || link->flags & MEDIA_LNK_FL_ENABLED) - bitmap_set(active, other_pad->index, 1); + bitmap_set(active, local_pad->index, 1); /* * Link validation will only take place for * sink ends of the link that are enabled. */ - if (link->sink != other_pad || + if (link->sink != local_pad || !(link->flags & MEDIA_LNK_FL_ENABLED)) continue;