Hi Tomi,
Thanks for the patch.
On Wed, Jul 27, 2022 at 3:37 AM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
>
> Add new versions of media_pipeline_start() and media_pipeline_stop().
> The new functions can be used by drivers that do not need to extend the
> media_pipeline with their own structs, and the new functions will handle
> allocating and freeing the media_pipeline as needed.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/mc/mc-entity.c | 49 ++++++++++++++++++++++++++++++++++++
> include/media/media-entity.h | 22 ++++++++++++++++
> 2 files changed, 71 insertions(+)
>
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 0d0d6c0dda16..b7b6c6411aa7 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -579,6 +579,55 @@ 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);
Please add NULL check here to handle the allocation failure.
> + new_pipe = true;
> + }
> +
> + ret = __media_pipeline_start(entity, pipe);
> + if (ret) {
> + if (new_pipe)
> + kfree(pipe);
> + }
> +
> + mutex_unlock(&mdev->graph_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_alloc_start);
Just a suggestion. It would be nice to extend the
media_pipeline_start() instead of adding a new function. The caller
can pass pipe as NULL as below.
media_pipeline_start(entity, NULL)
And allocation can happen inside media_pipeline_start() when pipe is NULL.
Regards,
Satish
> +
> +void media_pipeline_stop_free(struct media_entity *entity)
> +{
> + struct media_device *mdev = entity->graph_obj.mdev;
> + struct media_pipeline *pipe;
> +
> + mutex_lock(&mdev->graph_mutex);
> +
> + pipe = media_entity_pipeline(entity);
> +
> + __media_pipeline_stop(entity);
> +
> + if (pipe && pipe->start_count == 0)
> + kfree(pipe);
> +
> + mutex_unlock(&mdev->graph_mutex);
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_stop_free);
> +
> /* -----------------------------------------------------------------------------
> * Links management
> */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 6c0d0a00d58e..13a882a7839c 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -1035,6 +1035,28 @@ void media_pipeline_stop(struct media_entity *entity);
> */
> void __media_pipeline_stop(struct media_entity *entity);
>
> +/**
> + * media_pipeline_alloc_start - Mark a pipeline as streaming
> + * @entity: Starting entity
> + *
> + * media_pipeline_alloc_start() is similar to media_pipeline_start() but
> + * instead of working on a given pipeline the function will allocate a new
> + * pipeline if needed.
> + *
> + * Calls to media_pipeline_alloc_start() must be matched with
> + * media_pipeline_stop_free().
> + */
> +__must_check int media_pipeline_alloc_start(struct media_entity *entity);
> +
> +/**
> + * media_pipeline_stop_free - Mark a pipeline as not streaming
> + * @entity: Starting entity
> + *
> + * media_pipeline_stop_free() is similar to media_pipeline_stop() but will
> + * also free the pipeline when the start_count drops to 0.
> + */
> +void media_pipeline_stop_free(struct media_entity *entity);
> +
> /**
> * media_devnode_create() - creates and initializes a device node interface
> *
> --
> 2.34.1
>
