On 29/07/2022 11:30, Satish Nagireddy wrote:
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.
Oops...
+ 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.
Hmm, yes, that is an option, although I'm not quite happy with such a call either. Passing a NULL there could be a driver bug, and with such a change that would not be caught.
But we also need to free the memory, for which I have the media_pipeline_stop_free(). If we go with your suggestion, I think we can add a boolean to struct media_pipeline which tells if it was allocated by the media_pipeline_start(), and then media_pipeline_stop() can free it.
This would allow us to keep the existing calls and not add any new ones. Tomi
