Hi Laurent,
On Wed, Jan 16, 2019 at 01:41:08AM +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Fri, Nov 02, 2018 at 12:31:29AM +0100, Niklas Söderlund wrote:
> > From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > 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 42977634d7102852..e45fc2549017615a 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))
> > + return true;
>
> This will return true if pad0 is a sink and pad1 a source, regardless of
> has_route. I don't think that was intended. return has_route; would be
> an easy fix.
Good find. Yes, I think the condition apart from has_route itself is to be
dropped.
>
> > +
> > + /* Look for indirect routes */
> > + for (i = 0; i < entity->num_pads; i++) {
> > + if (i == pad0 || i == pad1)
> > + continue;
> > +
> > + /*
> > + * 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))
> > + return true;
> > + }
>
> Isn't this best implemented in drivers ? I fear the complexity you need
> here isn't worth it, especially given that you would also need to
> support cases such as
>
> Pads 0, 1 and 2 are sink, pads 3 and 4 are sources. has_route(0, 3),
> has_route(1, 3), has_route(1, 4) and has_route(2, 4) are all true,
> has_route(0, 4) and has_route(2, 3) are all false.
> media_entity_has_route(0, 2) should return true.
Yes, this is not entirely generic, but it'd cover the vast majority of the
cases. It needs to be documented as such. I'd keep it, to avoid drivers
from having to cope with checks for two source pads when both are routed to
a common sink. I presume this is one of the most common cases. I'm also
fine with dropping it for now, to keep the API simple. But we should then
reconsider this if that'd simplify drivers.
>
> > + return false;
> > +
> > }
> > EXPORT_SYMBOL_GPL(media_entity_has_route);
> >
>
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
