| Message ID | 20250519140403.443915-2-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] media: mc: entity: Add pipeline_started/stopped ops | expand |
Hi Dan On Mon, May 19, 2025 at 03:04:01PM +0100, Daniel Scally wrote: > Add two new members to struct media_entity_operations, along with new > functions in media-entity.c to traverse a media pipeline and call the > new operations. The new functions are intended to be used to signal > to a media pipeline that it has fully started, with the entity ops > allowing drivers to define some action to be taken when those > conditions are met. > > The combination of the new functions and operations allows drivers > which are part of a multi-driver pipeline to delay actually starting > streaming until all of the conditions for streaming succcessfully are > met across all drivers. Maybe s/succcessfully are/are successfully/ Or was this (the three 'c' apart) intentional ? > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > drivers/media/mc/mc-entity.c | 45 ++++++++++++++++++++++++++++++++++++ > include/media/media-entity.h | 24 +++++++++++++++++++ > 2 files changed, 69 insertions(+) > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c > index 045590905582..e36b1710669d 100644 > --- a/drivers/media/mc/mc-entity.c > +++ b/drivers/media/mc/mc-entity.c > @@ -1053,6 +1053,51 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe, > } > EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next); > > +int media_pipeline_started(struct media_pipeline *pipe) > +{ > + struct media_pipeline_entity_iter iter; > + struct media_entity *entity; > + int ret; > + > + ret = media_pipeline_entity_iter_init(pipe, &iter); > + if (ret) > + return ret; > + > + media_pipeline_for_each_entity(pipe, &iter, entity) { > + ret = media_entity_call(entity, pipeline_started); > + if (ret && ret != -ENOIOCTLCMD) > + goto err_notify_stopped; > + } > + > + media_pipeline_entity_iter_cleanup(&iter); > + > + return ret == -ENOIOCTLCMD ? 0 : ret; Shouldn't you just return 0 ? If a ret < 0 is encoutered in the loop we just to the below label > + > +err_notify_stopped: > + media_pipeline_stopped(pipe); Do you need to media_pipeline_entity_iter_cleanup() ? > + return ret; > +} > +EXPORT_SYMBOL_GPL(media_pipeline_started); > + > +int media_pipeline_stopped(struct media_pipeline *pipe) > +{ > + struct media_pipeline_entity_iter iter; > + struct media_entity *entity; > + int ret; > + > + ret = media_pipeline_entity_iter_init(pipe, &iter); > + if (ret) > + return ret; > + > + media_pipeline_for_each_entity(pipe, &iter, entity) > + media_entity_call(entity, pipeline_stopped); > + > + media_pipeline_entity_iter_cleanup(&iter); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(media_pipeline_stopped); > + > /* ----------------------------------------------------------------------------- > * Links management > */ > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index 64cf590b1134..e858326b95cb 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -269,6 +269,10 @@ struct media_pad { > * media_entity_has_pad_interdep(). > * Optional: If the operation isn't implemented all pads > * will be considered as interdependent. > + * @pipeline_started: Notify this entity that the pipeline it is a part of has > + * been started > + * @pipeline_stopped: Notify this entity that the pipeline it is a part of has > + * been stopped > * > * .. note:: > * > @@ -284,6 +288,8 @@ struct media_entity_operations { > int (*link_validate)(struct media_link *link); > bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0, > unsigned int pad1); > + int (*pipeline_started)(struct media_entity *entity); > + void (*pipeline_stopped)(struct media_entity *entity); > }; > > /** > @@ -1261,6 +1267,24 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe, > entity != NULL; \ > entity = __media_pipeline_entity_iter_next((pipe), iter, entity)) > > +/** > + * media_pipeline_started - Inform entities in a pipeline that it has started > + * @pipe: The pipeline > + * > + * Iterate on all entities in a media pipeline and call their pipeline_started > + * member of media_entity_operations. > + */ > +int media_pipeline_started(struct media_pipeline *pipe); > + > +/** > + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped > + * @pipe: The pipeline > + * > + * Iterate on all entities in a media pipeline and call their pipeline_stopped > + * member of media_entity_operations. > + */ > +int media_pipeline_stopped(struct media_pipeline *pipe); > + All good, but I don't see these operations being used at all in this series ? > /** > * media_pipeline_alloc_start - Mark a pipeline as streaming > * @pad: Starting pad > -- > 2.34.1 > >
Hi Jacopo On 21/05/2025 16:18, Jacopo Mondi wrote: > Hi Dan > > On Mon, May 19, 2025 at 03:04:01PM +0100, Daniel Scally wrote: >> Add two new members to struct media_entity_operations, along with new >> functions in media-entity.c to traverse a media pipeline and call the >> new operations. The new functions are intended to be used to signal >> to a media pipeline that it has fully started, with the entity ops >> allowing drivers to define some action to be taken when those >> conditions are met. >> >> The combination of the new functions and operations allows drivers >> which are part of a multi-driver pipeline to delay actually starting >> streaming until all of the conditions for streaming succcessfully are >> met across all drivers. > Maybe s/succcessfully are/are successfully/ > > Or was this (the three 'c' apart) intentional ? It was intentional as-is but I think it reads just fine the other way round too; happy to change to that. > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> drivers/media/mc/mc-entity.c | 45 ++++++++++++++++++++++++++++++++++++ >> include/media/media-entity.h | 24 +++++++++++++++++++ >> 2 files changed, 69 insertions(+) >> >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >> index 045590905582..e36b1710669d 100644 >> --- a/drivers/media/mc/mc-entity.c >> +++ b/drivers/media/mc/mc-entity.c >> @@ -1053,6 +1053,51 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe, >> } >> EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next); >> >> +int media_pipeline_started(struct media_pipeline *pipe) >> +{ >> + struct media_pipeline_entity_iter iter; >> + struct media_entity *entity; >> + int ret; >> + >> + ret = media_pipeline_entity_iter_init(pipe, &iter); >> + if (ret) >> + return ret; >> + >> + media_pipeline_for_each_entity(pipe, &iter, entity) { >> + ret = media_entity_call(entity, pipeline_started); >> + if (ret && ret != -ENOIOCTLCMD) >> + goto err_notify_stopped; >> + } >> + >> + media_pipeline_entity_iter_cleanup(&iter); >> + >> + return ret == -ENOIOCTLCMD ? 0 : ret; > Shouldn't you just return 0 ? If a ret < 0 is encoutered in the loop > we just to the below label Oh yeah; good point. > >> + >> +err_notify_stopped: >> + media_pipeline_stopped(pipe); > Do you need to media_pipeline_entity_iter_cleanup() ? Yes - thanks. > >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(media_pipeline_started); >> + >> +int media_pipeline_stopped(struct media_pipeline *pipe) >> +{ >> + struct media_pipeline_entity_iter iter; >> + struct media_entity *entity; >> + int ret; >> + >> + ret = media_pipeline_entity_iter_init(pipe, &iter); >> + if (ret) >> + return ret; >> + >> + media_pipeline_for_each_entity(pipe, &iter, entity) >> + media_entity_call(entity, pipeline_stopped); >> + >> + media_pipeline_entity_iter_cleanup(&iter); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(media_pipeline_stopped); >> + >> /* ----------------------------------------------------------------------------- >> * Links management >> */ >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >> index 64cf590b1134..e858326b95cb 100644 >> --- a/include/media/media-entity.h >> +++ b/include/media/media-entity.h >> @@ -269,6 +269,10 @@ struct media_pad { >> * media_entity_has_pad_interdep(). >> * Optional: If the operation isn't implemented all pads >> * will be considered as interdependent. >> + * @pipeline_started: Notify this entity that the pipeline it is a part of has >> + * been started >> + * @pipeline_stopped: Notify this entity that the pipeline it is a part of has >> + * been stopped >> * >> * .. note:: >> * >> @@ -284,6 +288,8 @@ struct media_entity_operations { >> int (*link_validate)(struct media_link *link); >> bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0, >> unsigned int pad1); >> + int (*pipeline_started)(struct media_entity *entity); >> + void (*pipeline_stopped)(struct media_entity *entity); >> }; >> >> /** >> @@ -1261,6 +1267,24 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe, >> entity != NULL; \ >> entity = __media_pipeline_entity_iter_next((pipe), iter, entity)) >> >> +/** >> + * media_pipeline_started - Inform entities in a pipeline that it has started >> + * @pipe: The pipeline >> + * >> + * Iterate on all entities in a media pipeline and call their pipeline_started >> + * member of media_entity_operations. >> + */ >> +int media_pipeline_started(struct media_pipeline *pipe); >> + >> +/** >> + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped >> + * @pipe: The pipeline >> + * >> + * Iterate on all entities in a media pipeline and call their pipeline_stopped >> + * member of media_entity_operations. >> + */ >> +int media_pipeline_stopped(struct media_pipeline *pipe); >> + > All good, but I don't see these operations being used at all in this > series ? Ah, yeah...organisational deficiency on my part; it's used by the ISP and IVC drivers in the series' which are based on this one, and it's kinda related in that it's required to get the multiple drivers registering video devices into the same media device to play nicely together, so I bundled them into the same series. I could send it alone, or update the series cover message to make more clear it's encompassing a few things? > >> /** >> * media_pipeline_alloc_start - Mark a pipeline as streaming >> * @pad: Starting pad >> -- >> 2.34.1 >> >>
Hi Nicolas - thanks for the comment On 22/05/2025 20:43, Nicolas Dufresne wrote: > Hi, > > Le lundi 19 mai 2025 à 15:04 +0100, Daniel Scally a écrit : >> Add two new members to struct media_entity_operations, along with new >> functions in media-entity.c to traverse a media pipeline and call the >> new operations. The new functions are intended to be used to signal >> to a media pipeline that it has fully started, with the entity ops >> allowing drivers to define some action to be taken when those >> conditions are met. >> >> The combination of the new functions and operations allows drivers >> which are part of a multi-driver pipeline to delay actually starting >> streaming until all of the conditions for streaming succcessfully are >> met across all drivers. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> drivers/media/mc/mc-entity.c | 45 ++++++++++++++++++++++++++++++++++++ >> include/media/media-entity.h | 24 +++++++++++++++++++ >> 2 files changed, 69 insertions(+) >> >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c >> index 045590905582..e36b1710669d 100644 >> --- a/drivers/media/mc/mc-entity.c >> +++ b/drivers/media/mc/mc-entity.c >> @@ -1053,6 +1053,51 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe, >> } >> EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next); >> >> +int media_pipeline_started(struct media_pipeline *pipe) >> +{ >> + struct media_pipeline_entity_iter iter; >> + struct media_entity *entity; >> + int ret; >> + >> + ret = media_pipeline_entity_iter_init(pipe, &iter); >> + if (ret) >> + return ret; >> + >> + media_pipeline_for_each_entity(pipe, &iter, entity) { >> + ret = media_entity_call(entity, pipeline_started); >> + if (ret && ret != -ENOIOCTLCMD) >> + goto err_notify_stopped; >> + } > Would this be more useful if it had a specified traversal order ? Perhaps > sink to source traversal? Hmm...maybe? I don't think that it would matter either way for my particular use case, but I can have a look at giving it a specific order. > > Nicolas > >> + >> + media_pipeline_entity_iter_cleanup(&iter); >> + >> + return ret == -ENOIOCTLCMD ? 0 : ret; >> + >> +err_notify_stopped: >> + media_pipeline_stopped(pipe); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(media_pipeline_started); >> + >> +int media_pipeline_stopped(struct media_pipeline *pipe) >> +{ >> + struct media_pipeline_entity_iter iter; >> + struct media_entity *entity; >> + int ret; >> + >> + ret = media_pipeline_entity_iter_init(pipe, &iter); >> + if (ret) >> + return ret; >> + >> + media_pipeline_for_each_entity(pipe, &iter, entity) >> + media_entity_call(entity, pipeline_stopped); >> + >> + media_pipeline_entity_iter_cleanup(&iter); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(media_pipeline_stopped); >> + >> /* ----------------------------------------------------------------------------- >> * Links management >> */ >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >> index 64cf590b1134..e858326b95cb 100644 >> --- a/include/media/media-entity.h >> +++ b/include/media/media-entity.h >> @@ -269,6 +269,10 @@ struct media_pad { >> * media_entity_has_pad_interdep(). >> * Optional: If the operation isn't implemented all pads >> * will be considered as interdependent. >> + * @pipeline_started: Notify this entity that the pipeline it is a part of has >> + * been started >> + * @pipeline_stopped: Notify this entity that the pipeline it is a part of has >> + * been stopped >> * >> * .. note:: >> * >> @@ -284,6 +288,8 @@ struct media_entity_operations { >> int (*link_validate)(struct media_link *link); >> bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0, >> unsigned int pad1); >> + int (*pipeline_started)(struct media_entity *entity); >> + void (*pipeline_stopped)(struct media_entity *entity); >> }; >> >> /** >> @@ -1261,6 +1267,24 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe, >> entity != NULL; \ >> entity = __media_pipeline_entity_iter_next((pipe), iter, entity)) >> >> +/** >> + * media_pipeline_started - Inform entities in a pipeline that it has started >> + * @pipe: The pipeline >> + * >> + * Iterate on all entities in a media pipeline and call their pipeline_started >> + * member of media_entity_operations. >> + */ >> +int media_pipeline_started(struct media_pipeline *pipe); >> + >> +/** >> + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped >> + * @pipe: The pipeline >> + * >> + * Iterate on all entities in a media pipeline and call their pipeline_stopped >> + * member of media_entity_operations. >> + */ >> +int media_pipeline_stopped(struct media_pipeline *pipe); >> + >> /** >> * media_pipeline_alloc_start - Mark a pipeline as streaming >> * @pad: Starting pad
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c index 045590905582..e36b1710669d 100644 --- a/drivers/media/mc/mc-entity.c +++ b/drivers/media/mc/mc-entity.c @@ -1053,6 +1053,51 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe, } EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next); +int media_pipeline_started(struct media_pipeline *pipe) +{ + struct media_pipeline_entity_iter iter; + struct media_entity *entity; + int ret; + + ret = media_pipeline_entity_iter_init(pipe, &iter); + if (ret) + return ret; + + media_pipeline_for_each_entity(pipe, &iter, entity) { + ret = media_entity_call(entity, pipeline_started); + if (ret && ret != -ENOIOCTLCMD) + goto err_notify_stopped; + } + + media_pipeline_entity_iter_cleanup(&iter); + + return ret == -ENOIOCTLCMD ? 0 : ret; + +err_notify_stopped: + media_pipeline_stopped(pipe); + return ret; +} +EXPORT_SYMBOL_GPL(media_pipeline_started); + +int media_pipeline_stopped(struct media_pipeline *pipe) +{ + struct media_pipeline_entity_iter iter; + struct media_entity *entity; + int ret; + + ret = media_pipeline_entity_iter_init(pipe, &iter); + if (ret) + return ret; + + media_pipeline_for_each_entity(pipe, &iter, entity) + media_entity_call(entity, pipeline_stopped); + + media_pipeline_entity_iter_cleanup(&iter); + + return 0; +} +EXPORT_SYMBOL_GPL(media_pipeline_stopped); + /* ----------------------------------------------------------------------------- * Links management */ diff --git a/include/media/media-entity.h b/include/media/media-entity.h index 64cf590b1134..e858326b95cb 100644 --- a/include/media/media-entity.h +++ b/include/media/media-entity.h @@ -269,6 +269,10 @@ struct media_pad { * media_entity_has_pad_interdep(). * Optional: If the operation isn't implemented all pads * will be considered as interdependent. + * @pipeline_started: Notify this entity that the pipeline it is a part of has + * been started + * @pipeline_stopped: Notify this entity that the pipeline it is a part of has + * been stopped * * .. note:: * @@ -284,6 +288,8 @@ struct media_entity_operations { int (*link_validate)(struct media_link *link); bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0, unsigned int pad1); + int (*pipeline_started)(struct media_entity *entity); + void (*pipeline_stopped)(struct media_entity *entity); }; /** @@ -1261,6 +1267,24 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe, entity != NULL; \ entity = __media_pipeline_entity_iter_next((pipe), iter, entity)) +/** + * media_pipeline_started - Inform entities in a pipeline that it has started + * @pipe: The pipeline + * + * Iterate on all entities in a media pipeline and call their pipeline_started + * member of media_entity_operations. + */ +int media_pipeline_started(struct media_pipeline *pipe); + +/** + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped + * @pipe: The pipeline + * + * Iterate on all entities in a media pipeline and call their pipeline_stopped + * member of media_entity_operations. + */ +int media_pipeline_stopped(struct media_pipeline *pipe); + /** * media_pipeline_alloc_start - Mark a pipeline as streaming * @pad: Starting pad
Add two new members to struct media_entity_operations, along with new functions in media-entity.c to traverse a media pipeline and call the new operations. The new functions are intended to be used to signal to a media pipeline that it has fully started, with the entity ops allowing drivers to define some action to be taken when those conditions are met. The combination of the new functions and operations allows drivers which are part of a multi-driver pipeline to delay actually starting streaming until all of the conditions for streaming succcessfully are met across all drivers. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- drivers/media/mc/mc-entity.c | 45 ++++++++++++++++++++++++++++++++++++ include/media/media-entity.h | 24 +++++++++++++++++++ 2 files changed, 69 insertions(+)