On Tue, Sep 06, 2022 at 12:58:45PM +0000, Sakari Ailus wrote: > On Tue, Sep 06, 2022 at 12:19:19PM +0300, Tomi Valkeinen wrote: > > On 06/09/2022 11:44, Laurent Pinchart wrote: > > > On Wed, Aug 31, 2022 at 05:21:58PM +0300, Tomi Valkeinen wrote: > > > > On 29/08/2022 20:18, Laurent Pinchart wrote: > > > > > On Wed, Aug 10, 2022 at 03:11:02PM +0300, Tomi Valkeinen wrote: > > > > > > From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > > > > > > > > > > [Note: the code is mostly from Laurent but the patch description is from Tomi] > > > > > > > > > > I'll review the description then :-) > > > > > > > > > > > The media_pipeline_start() and media_pipeline_stop() functions use the > > > > > > media graph walk API to traverse the graph and validate the pipeline. > > > > > > The graph walk traverses the media graph following links between the > > > > > > entities. > > > > > > > > > > > > Also, while the pipeline can't change between the start and stop calls, > > > > > > the graph is walked again from scratch at stop time, or any time a > > > > > > driver needs to inspect the pipeline. > > > > > > > > > > > > With the upcoming multiplexed streams support we will need a bit more > > > > > > intelligent pipeline construction, as e.g. an entity may be passing > > > > > > through two independent streams via separate pads, in which case those > > > > > > > > > > Did you mean "as e.g. two independent streams may be passing through a > > > > > single entity via separate pads" ? > > > > > > > > > > > pads should not be part of the same pipeline. > > > > > > > > > > > > This patch essentially rewrites the media_pipeline_start/stop so that > > > > > > a pipeline is defined as a set of pads instead of entities and the media > > > > > > graph traversal considers the pad interdependencies when choosing which > > > > > > links to follow. > > > > > > > > > > > > Currently all the entity's pads are considered as interdependent. This > > > > > > means that the behavior with all the current drivers stays the same, but > > > > > > in the future we can define a more fine-grained pipeline construction. > > > > > > > > > > > > Additionally the media pipeline's pads are cached at > > > > > > media_pipeline_start() time, and re-used at media_pipeline_stop() which > > > > > > avoid the need to re-walk the whole graph as the previous implementation > > > > > > did. > > > > > > > > > > > > Also, caching pads in the pipeline can serve in the future as the > > > > > > foundation to provide a better API than the media graph walk to drivers > > > > > > to iterate over pads and entities in the pipeline. > > > > > > > > > > > > Note that the old media_pipeline_start/stop used the media graph walk > > > > > > API. The new version does not use the media graph walk API, but instead > > > > > > a new implementation. > > > > > > > > > > > > There are two reason for not changing the graph walk: it proved to be > > > > > > rather difficult to change the graph walk to have the features > > > > > > implemented in this patch, and second, this keeps the backward > > > > > > compatibility of the graph walk as there are users of the graph walk API > > > > > > > > > > > > The long term plan is that all the existing code would be converted to > > > > > > use the new cached pipeline, thus allowing us to remove the graph walk. > > > > > > > > > > Could you mark the graph walk API as deprecated in this patch, or in a > > > > > subsequent patch in the series ? I think I did in a previous version, > > > > > but I may recall incorrectly. > > > > > > > > I didn't mark the graph walk API as deprecated in v14 series. We can do > > > > it on top, but I'm not sure if it's a valid thing to say yet. Have we > > > > tried converting any graph walk uses to the new pipeline code? We could > > > > well have code missing that prevents the conversion. > > > > > > I'd like to avoid new users of the graph walk API. This requires two > > > patches that you haven't included or squashed in v14 though, namely > > > > > > 20a31d49bd75 media: mc: entity: Add entity iterator for media_pipeline > > > 50659eb74afc media: mc: entity: Add pad iterator for media_pipeline > > > > > > that you can find at > > > > > > git://linuxtv.org/pinchartl/media.git streams/v6.0/v11 > > > > > > Should I submit that on top of v14 for inclusion in v6.1 too ? > > > > Both look fine to me with a quick glance, and I'm ok with adding these on > > top of v14 (or I can pick them for v15 if there's one). > > > > I don't think we have any code that uses them, have you tested them? > > They're from Laurent, why would you test them? :-) http://events17.linuxfoundation.org/sites/events/files/slides/20170601-ossj.pdf > I've taken the 16 first patches (v14) to my tree, will send a PR shortly. -- Regards, Laurent Pinchart
