Hi Tomi,
On Tue, Aug 30, 2022 at 02:08:53PM +0300, Tomi Valkeinen wrote:
> On 29/08/2022 19:57, Laurent Pinchart wrote:
> > 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.
It's hard for me at this point to tell how much this would help, I don't
have enough visibility. I think it doesn't go in the wrong direction, so
it's OK :-)
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
--
Regards,
Laurent Pinchart
