Re: [PATCH v2 09/30] media: entity: Swap pads if route is checked from source to sink

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

On Mon, Feb 18, 2019 at 10:21:07AM +0100, Jacopo Mondi wrote:
> On Tue, Jan 22, 2019 at 05:20:30PM +0200, Laurent Pinchart wrote:
> > 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?

I fear this will be confusing for subdevs, as they would have to
implement part of the operation.

Could it be that the subdev has_route operation isn't the best API for
the job, if it gets that complex ? I wonder if it would be easier to
create another operation that takes a pad index as argument, and returns
the list of pads (possibly as a bitmask ?) or connected pads.
media_entity_has_route() could easily be implemented on top of that, and
these new semantics may be easier for subdevs to implement.

> >>> 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?

I think that's the worst of both worlds from a subdev point of view :-)

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