Re: [PATCH v13 14/34] media: mc: entity: Rewrite media_pipeline_start()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux