| Message ID | 20220727103639.581567-9-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | v4l: routing and streams support | expand |
Hi Tomi, Thanks for the patch. On Wed, Jul 27, 2022 at 3:37 AM Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> 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@ideasonboard.com> > --- > 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 >
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@ideasonboard.com> 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@ideasonboard.com> >> --- >> 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
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); + 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); + +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 *
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@ideasonboard.com> --- drivers/media/mc/mc-entity.c | 49 ++++++++++++++++++++++++++++++++++++ include/media/media-entity.h | 22 ++++++++++++++++ 2 files changed, 71 insertions(+)