On 29/08/2022 20:18, Laurent Pinchart wrote:
Hi Tomi,
Thank you for the patch.
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" ?
Yes. That's a bit more understandable.
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 don't remember removing deprecation comments, but who knows...
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index d277eed11caf..df8e82be2bcc 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -225,6 +225,27 @@ EXPORT_SYMBOL_GPL(media_entity_pads_init);
* Graph traversal
*/
+/*
+ * This function checks the interdependency inside the entity between @pad0
+ * and @pad1. If two pads are interdependent they are part of the same pipeline
+ * and enabling one of the pads means that the other pad will become "locked"
+ * and doesn't allow configuration changes.
+ *
+ * For the time being all pads are considered interdependent.
+ */
+static bool media_entity_has_pad_interdep(struct media_entity *entity,
I won't bikeshed this name as it can easily be changed later if needed.
Thank you ;).
@@ -966,10 +1318,29 @@ EXPORT_SYMBOL_GPL(media_entity_get_fwnode_pad);
struct media_pipeline *media_entity_pipeline(struct media_entity *entity)
{
- return entity->pipe;
+ struct media_pipeline *pipe;
+ struct media_pad *pad;
+
+ if (entity->num_pads == 0)
+ return NULL;
+
+ pipe = entity->pads->pipe;
+
+ media_entity_for_each_pad(entity, pad) {
+ if (WARN_ON(pad->pipe != pipe))
+ return NULL;
+ }
This wasn't done in v12, could you explain the rationale ? The
documentation of media_entity_pipeline() should probably be updated too.
This is just a sanity check.
The doc says "This function is to be used only for entities that do not
support multiple pipelines". So the pipeline must be the same for all
pads, and if that's not the case, something is horribly wrong.
However, now that I think about it, can we have NULL pipes in some pads,
if those pads are e.g. not linked... Then I think the check should be
(pipe && pad->pipe && pipe != pad->pipe).
But if that's the case, does this even work... The code expects the
first pad to contain the pipeline for the entity. If the first pad's
pipe can be NULL, but, say, second and fourth pads contain the pipeline
(as those pads are actually used), then this is quite broken.
Tomi
