Hi Laurent, Sakari,

On Tue, Jan 22, 2019 at 05:20:30PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tue, Jan 22, 2019 at 05:15:06PM +0200, Sakari Ailus wrote:
> > On Wed, Jan 16, 2019 at 12:57:43AM +0200, Laurent Pinchart wrote:
> > >>
> > >> This way the pads are always passed to the has_route() op sink pad first.
> > >> Makes sense.
> > >
> > > Is there anything in the API that mandates one pad to be a sink and the
> > > other pad to the a source ? I had designed the operation to allow
> > > sink-sink and source-source connections to be checked too.
> >
> > Do you have a use case in mind for sink--sink or source--source routes? The
> > routes are about flows of data, so I'd presume only source--sink or
> > sink--source routes are meaningful.
> >
> > If you did, then the driver would have to handle that by itself. This still
> > simplifies the implementation for drivers that do not.
>
> I don't have use cases for such routes, but we use the has_route
> operation when traversing pipelines, and at that point we need to get
> all the internally connected pads. In another patch in this series you
> implement a helper function that handles this, but its implementation
> isn't complete. I explained in my review of that patch that I fear a
> correct generic implementation would become quite complex, while the
> complexity should be easy to handle on the driver side as the code can
> then be specialized for the case at hand.
>

As a compromise, in v3 I'm thinking of maintaining support for the
most common case of two sources connected to the same sink, as
Sakari's patch does, but let more complex cases be handled by the
driver implementation of has_route().

Ack?

> > > If your goal is to simplify the implementation of the .has_route()
> > > operation in drivers, I would instead sort pad0 and pad1 by value.
> >
> > That'd be another option to make the order deterministic for the driver.
> > I'm fine with that as well.
> >

In v3 I have taken both suggestions in: try the "sink then source" order
first, then order by index in case the pads are of the same time. This
needs to be documented in has_route() operation definition though.

Would that be fine with you?

Thanks
   j

> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >> ---
> > >>  drivers/media/media-entity.c | 4 ++++
> > >>  1 file changed, 4 insertions(+)
> > >>
> > >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > >> index 3c0e7425c8983b45..33f00e35ccd92c6f 100644
> > >> --- a/drivers/media/media-entity.c
> > >> +++ b/drivers/media/media-entity.c
> > >> @@ -249,6 +249,10 @@ bool media_entity_has_route(struct media_entity *entity, unsigned int pad0,
> > >>  	if (!entity->ops || !entity->ops->has_route)
> > >>  		return true;
> > >>
> > >> +	if (entity->pads[pad0].flags & MEDIA_PAD_FL_SOURCE
> > >> +	    && entity->pads[pad1].flags & MEDIA_PAD_FL_SINK)
> > >> +		swap(pad0, pad1);
> > >> +
> > >>  	return entity->ops->has_route(entity, pad0, pad1);
> > >>  }
> > >>  EXPORT_SYMBOL_GPL(media_entity_has_route);
>
> --
> Regards,
>
> Laurent Pinchart