| Message ID | 20220810121122.3149086-13-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l: routing and streams support | expand |
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@ideasonboard.com> > --- > 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. > + 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); 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. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + } > + > +out: > + mutex_unlock(&mdev->graph_mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(media_pipeline_alloc_start); > + > /* ----------------------------------------------------------------------------- > * Links management > */ > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > index 7f933ff89fd4..945bb867a4c1 100644 > --- a/drivers/media/v4l2-core/v4l2-dev.c > +++ b/drivers/media/v4l2-core/v4l2-dev.c > @@ -1143,6 +1143,17 @@ void __video_device_pipeline_stop(struct video_device *vdev) > } > EXPORT_SYMBOL_GPL(__video_device_pipeline_stop); > > +__must_check int video_device_pipeline_alloc_start(struct video_device *vdev) > +{ > + struct media_entity *entity = &vdev->entity; > + > + if (entity->num_pads != 1) > + return -ENODEV; > + > + return media_pipeline_alloc_start(entity); > +} > +EXPORT_SYMBOL_GPL(video_device_pipeline_alloc_start); > + > struct media_pipeline *video_device_pipeline(struct video_device *vdev) > { > struct media_entity *entity = &vdev->entity; > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index 3438954892b7..3d0d5e3c3333 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -104,6 +104,7 @@ struct media_graph { > * @graph: Media graph walk during pipeline start / stop > */ > struct media_pipeline { > + bool allocated; This should be documented. > int start_count; > struct media_graph graph; > }; > @@ -1028,6 +1029,19 @@ 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 use an existing pipeline if > + * the entity is already part of a pipeline, or allocate a new pipeline. > + * > + * Calls to media_pipeline_alloc_start() must be matched with > + * media_pipeline_stop(). > + */ > +__must_check int media_pipeline_alloc_start(struct media_entity *entity); > + > /** > * media_devnode_create() - creates and initializes a device node interface > * > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h > index 4ccc24f5918a..29eb61244d1f 100644 > --- a/include/media/v4l2-dev.h > +++ b/include/media/v4l2-dev.h > @@ -609,6 +609,20 @@ void video_device_pipeline_stop(struct video_device *vdev); > */ > void __video_device_pipeline_stop(struct video_device *vdev); > > +/** > + * video_device_pipeline_alloc_start - Mark a pipeline as streaming > + * @vdev: Starting video device > + * > + * video_device_pipeline_alloc_start() is similar to video_device_pipeline_start() > + * but instead of working on a given pipeline the function will use an > + * existing pipeline if the video device is already part of a pipeline, or > + * allocate a new pipeline. > + * > + * Calls to video_device_pipeline_alloc_start() must be matched with > + * video_device_pipeline_stop(). > + */ > +__must_check int video_device_pipeline_alloc_start(struct video_device *vdev); > + > /** > * video_device_pipeline - Get the media pipeline a video device is part of > * @vdev: The video device
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@ideasonboard.com> >> --- >> 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@ideasonboard.com> Thanks! Tomi
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@ideasonboard.com> > >> --- > >> 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@ideasonboard.com>
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); + + 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); + } + +out: + mutex_unlock(&mdev->graph_mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(media_pipeline_alloc_start); + /* ----------------------------------------------------------------------------- * Links management */ diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c index 7f933ff89fd4..945bb867a4c1 100644 --- a/drivers/media/v4l2-core/v4l2-dev.c +++ b/drivers/media/v4l2-core/v4l2-dev.c @@ -1143,6 +1143,17 @@ void __video_device_pipeline_stop(struct video_device *vdev) } EXPORT_SYMBOL_GPL(__video_device_pipeline_stop); +__must_check int video_device_pipeline_alloc_start(struct video_device *vdev) +{ + struct media_entity *entity = &vdev->entity; + + if (entity->num_pads != 1) + return -ENODEV; + + return media_pipeline_alloc_start(entity); +} +EXPORT_SYMBOL_GPL(video_device_pipeline_alloc_start); + struct media_pipeline *video_device_pipeline(struct video_device *vdev) { struct media_entity *entity = &vdev->entity; diff --git a/include/media/media-entity.h b/include/media/media-entity.h index 3438954892b7..3d0d5e3c3333 100644 --- a/include/media/media-entity.h +++ b/include/media/media-entity.h @@ -104,6 +104,7 @@ struct media_graph { * @graph: Media graph walk during pipeline start / stop */ struct media_pipeline { + bool allocated; int start_count; struct media_graph graph; }; @@ -1028,6 +1029,19 @@ 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 use an existing pipeline if + * the entity is already part of a pipeline, or allocate a new pipeline. + * + * Calls to media_pipeline_alloc_start() must be matched with + * media_pipeline_stop(). + */ +__must_check int media_pipeline_alloc_start(struct media_entity *entity); + /** * media_devnode_create() - creates and initializes a device node interface * diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h index 4ccc24f5918a..29eb61244d1f 100644 --- a/include/media/v4l2-dev.h +++ b/include/media/v4l2-dev.h @@ -609,6 +609,20 @@ void video_device_pipeline_stop(struct video_device *vdev); */ void __video_device_pipeline_stop(struct video_device *vdev); +/** + * video_device_pipeline_alloc_start - Mark a pipeline as streaming + * @vdev: Starting video device + * + * video_device_pipeline_alloc_start() is similar to video_device_pipeline_start() + * but instead of working on a given pipeline the function will use an + * existing pipeline if the video device is already part of a pipeline, or + * allocate a new pipeline. + * + * Calls to video_device_pipeline_alloc_start() must be matched with + * video_device_pipeline_stop(). + */ +__must_check int video_device_pipeline_alloc_start(struct video_device *vdev); + /** * video_device_pipeline - Get the media pipeline a video device is part of * @vdev: The video device
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@ideasonboard.com> --- 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(+)