diff mbox series

[v5,11/24] media: entity: Skip link validation for pads to which there is no route to

Message ID 20210415130450.421168-12-tomi.valkeinen@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series v4l: subdev internal routing | expand

Commit Message

Tomi Valkeinen April 15, 2021, 1:04 p.m. UTC 
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.

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(-)

Comments

Laurent Pinchart April 18, 2021, 6:06 p.m. UTC | #1 
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;
>
Sakari Ailus April 20, 2021, 11:41 a.m. UTC | #2 
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;
> >  
>
Tomi Valkeinen April 23, 2021, 12:37 p.m. UTC | #3 
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
Sakari Ailus April 29, 2021, 12:06 p.m. UTC | #4 
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.
Laurent Pinchart April 29, 2021, 2:10 p.m. UTC | #5 
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.
diff mbox series

Patch

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;