On 29/08/2022 19:57, Laurent Pinchart wrote:
Hi Tomi,
Thank you for the patch.
On Wed, Aug 10, 2022 at 03:11:00PM +0300, Tomi Valkeinen wrote:
Add new variant of media_pipeline_start(), media_pipeline_alloc_start().
media_pipeline_alloc_start() can be used by drivers that do not need to
extend the media_pipeline. The function will either use the pipeline
already associated with the entity, if such exists, or allocate a new
pipeline.
When media_pipeline_stop() is called and the pipeline's use count drops
to zero, the pipeline is automatically freed.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
drivers/media/mc/mc-entity.c | 41 ++++++++++++++++++++++++++++++
drivers/media/v4l2-core/v4l2-dev.c | 11 ++++++++
include/media/media-entity.h | 14 ++++++++++
include/media/v4l2-dev.h | 14 ++++++++++
4 files changed, 80 insertions(+)
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index b1abaa333d13..d277eed11caf 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -529,6 +529,8 @@ void __media_pipeline_stop(struct media_entity *entity)
media_graph_walk_cleanup(graph);
+ if (pipe->allocated)
+ kfree(pipe);
}
EXPORT_SYMBOL_GPL(__media_pipeline_stop);
@@ -542,6 +544,45 @@ void media_pipeline_stop(struct media_entity *entity)
}
EXPORT_SYMBOL_GPL(media_pipeline_stop);
+__must_check int media_pipeline_alloc_start(struct media_entity *entity)
+{
+ struct media_device *mdev = entity->graph_obj.mdev;
+ struct media_pipeline *pipe;
+ int ret;
+ bool new_pipe = false;
+
+ mutex_lock(&mdev->graph_mutex);
+
+ /*
+ * Is the entity already part of a pipeline? If not, we need to allocate
+ * a pipe.
+ */
+ pipe = media_entity_pipeline(entity);
+ if (!pipe) {
+ pipe = kzalloc(sizeof(*pipe), GFP_KERNEL);
+
You can drop the blank line.
Ok.
+ if (!pipe) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ new_pipe = true;
+ pipe->allocated = true;
+ }
+
+ ret = __media_pipeline_start(entity, pipe);
+ if (ret) {
+ if (new_pipe)
+ kfree(pipe);
If new_pipe was a media_pipeline pointer, initialized to NULL and set to
pipe when you allocate the pipe, you could write this
if (ret)
kfree(new_pipe);
Yes, that's slightly neater.
I don't mind much either way.
I have no objection against this patch, but I don't think it aligns well
with what I was imagining as further evolutions of the API. I would like
the pipe to be turned into an object similar to a DRM atomic commit.
There will thus never be a need for drivers to allocate the pipeline, it
will be done by the framework. We can rework (and likely drop) this
function when that happens.
True, but isn't this a bit in the right direction? This goes a bit
towards automating the pipeline management. Having the drivers use this
instead of doing the management themselves should make it easier to
eventually move to a framework managed model.
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Thanks!
Tomi
