Re: [PATCH 15/30] media: entity: Look for indirect routes

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 3521 bytes --]

Hi Sakari,

On Thu, Aug 23, 2018 at 03:25:29PM +0200, Niklas Söderlund wrote:
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> Two pads are considered having an active route for the purpose of
> has_route() if an indirect active route can be found between the two pads.
> An simple example of this is that a source pad has an active route to
> another source pad if both of the pads have an active route to the same
> sink pad.
>
> Make media_entity_has_route() return true in that case, and do not rely on
> drivers performing this by themselves.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-entity.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 3912bc75651fe0b7..29e732a130667ae9 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -240,6 +240,9 @@ EXPORT_SYMBOL_GPL(media_entity_pads_init);
>  bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
>  			    unsigned int pad1)
>  {
> +	unsigned int i;
> +	bool has_route;
> +
>  	if (pad0 >= entity->num_pads || pad1 >= entity->num_pads)
>  		return false;
>
> @@ -253,7 +256,34 @@ bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
>  	    && entity->pads[pad1].flags & MEDIA_PAD_FL_SINK)
>  		swap(pad0, pad1);
>
> -	return entity->ops->has_route(entity, pad0, pad1);
> +	has_route = entity->ops->has_route(entity, pad0, pad1);
> +	/* A direct route is returned immediately */
> +	if (has_route ||
> +	    (entity->pads[pad0].flags & MEDIA_PAD_FL_SINK &&
> +	     entity->pads[pad1].flags & MEDIA_PAD_FL_SOURCE))

I'm not sure I get why you're using the || condition here.

I suspect it is in order to make sure the driver implementation of
ops->has_route() only checks for direct links, but wouldn't it be
better to clarify the function semantics instead of catching
'non-direct' routes returned here?

More generally, why not call ops->has_route() only in case pad0 and
pad1 have different type, and check for indirect links otherwise?

> +		return true;
> +
> +	/* Look for indirect routes */
> +	for (i = 0; i < entity->num_pads; i++) {
> +		if (i == pad0 || i == pad1)
> +			continue;

This is correct, but the following check catches this condition as
well I think.

I assume if we fall down here pad0 and pad1 are of the same type and
if 'i' is one of the two pads it will have the same type as pad0, and
you're checking for this here below.

> +
> +		/*
> +		 * There are no direct routes between same types of
> +		 * pads, so skip checking this route
> +		 */
> +		if (!((entity->pads[pad0].flags ^ entity->pads[i].flags) &
> +		      (MEDIA_PAD_FL_SOURCE | MEDIA_PAD_FL_SINK)))
> +			continue;
> +
> +		/* Is there an indirect route? */
> +		if (entity->ops->has_route(entity, i, pad0) &&
> +		    entity->ops->has_route(entity, i, pad1))

What guarantees here that i is a sink and pad0/pad1 are sources? It seems
the 'has_route' operation wants this, otherwise I don't see a reason
for "[PATCH 09/30] media: entity: Swap pads if route is checked from
source to sink". Did I miss something from that patch?

Sorry, lot of questions. I might be confused :/
Thanks
   j

> +			return true;
> +	}
> +
> +	return false;
> +
>  }
>  EXPORT_SYMBOL_GPL(media_entity_has_route);
>
> --
> 2.18.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]