[PATCH v11 01/19] media: mc: entity: Add pipeline_started/stopped ops

[Dan Scally]

Hi Jacopo

On 05/08/2025 08:45, Jacopo Mondi wrote:
> Hi Dan
> 
> On Mon, Jul 14, 2025 at 04:06:27PM +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.
>>
>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
>> ---
>> Changes in v5:
>>
>> 	- Update kerneldoc comments with Optional statement in the
>> 	  right place
>>
>> Changes in v4:
>>
>> 	- Reverted to having the iter variable
>>
>> Changes in v3:
>>
>> 	- Dropped the iter variable now that the pipeline entity
>> 	  iterator functions don't need it.
>> 	- Updated documentation to specify Optional and return
>> 	  values
>>
>> Changes in v2:
>>
>> 	- Refactored media_pipeline_started() such that the cleanup
>> 	  function for media_pipeline_entity_iter is unconditionally
>> 	  called
>> 	- Avoided using media_entity_call() helper for operation that
>> 	  has return type void to avoid compiler warnings
>> ---
>>   drivers/media/mc/mc-entity.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
>>   include/media/media-entity.h | 29 ++++++++++++++++++++++++++++
>>   2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index 045590905582054c46656e20463271b1f93fa6b4..d3443537d4304e12cb015630101efba22375c011 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -1053,6 +1053,52 @@ __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)
>> +			break;
>> +	}
>> +
>> +	media_pipeline_entity_iter_cleanup(&iter);
>> +
>> +	ret = ret == -ENOIOCTLCMD ? 0 : ret;
>> +	if (ret)
>> +		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)
>> +		if (entity->ops && entity->ops->pipeline_stopped)
>> +			entity->ops->pipeline_stopped(entity);
> 
> I was sure I asked this already, but I wasn't able to find any
> reference to this in the review of the previous version, so I'll
> re-ask (sorry if it's the second time):
> 
> why can't you use media_entity_call() here as well ?

It causes compilation errors because media_entity_call() explicitly assumes that the callback will 
return an integer, and .pipeline_stopped() returns void.

> 
>> +
>> +	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 64cf590b11343f68a456c5870ca2f32917c122f9..1e1026f65f2050bb9aa39bde68794da8d2d0a669 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:	Optional: Notify this entity that the pipeline it is a
>> + *			part of has been started.
>> + * @pipeline_stopped:	Optional: Notify this entity that the pipeline it is a
>> + *			part of has been stopped.
> 
> Why not use the same style as the other entries ?

Poor reading comprehension - I thought I had! Thanks, I'll adopt your suggestion.

> 
>   * @get_fwnode_pad:	Return the pad number based on a fwnode endpoint or
>   *			a negative value on error. This operation can be used
>   *			to map a fwnode to a media pad number. Optional.
> 
>   Or
>   * @has_pad_interdep:	Return whether two pads of the entity are
>   *			interdependent. If two pads are interdependent they are
>   *			part of the same pipeline and enabling one of the pads
>   *			means that the other pad will become "locked" and
>   *			doesn't allow configuration changes. pad0 and pad1 are
>   *			guaranteed to not both be sinks or sources. Never call
>   *			the .has_pad_interdep() operation directly, always use
>   *			media_entity_has_pad_interdep().
>   *			Optional: If the operation isn't implemented all pads
>   *			will be considered as interdependent.
> 
> Also, the existing doc uses "the entity" and not "this entity"
> 
> 
> These would then be
> 
> * @pipeline_started:	Notify the entity that the pipeline it is a
> *			part of has been started. Optional.
> * @pipeline_stopped:	Notify the entity that the pipeline it is a
> *			part of has been stopped. Optional
> 
> Question from a non-native speaker: "it is a part of" or "it is part
> of" ?

I think either would be acceptable; I can switch if it's clearer to you?

> 
>>    *
>>    * .. 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,29 @@ __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.
>> + *
>> + * Return: zero on success, or a negative error code passed through from an
>> + * entity's .pipeline_started() operation.
> 
> If you don't have specific return codes to document you could consider
> a simpler
> 
>   * Returns zero on success or a negative error code otherwise.
> 
> Up to you on this one.
> 
> With the above documentation aligned to the existing one and a
> clarification on the media_entity_call usage:
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks!
> 
> Thanks
> 
>> + */
>> +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.
>> + *
>> + * Return: zero on success, or -ENOMEM if the iterator initialisation failed.
>> + */
>> +int media_pipeline_stopped(struct media_pipeline *pipe);
>> +
>>   /**
>>    * media_pipeline_alloc_start - Mark a pipeline as streaming
>>    * @pad: Starting pad
>>
>> --
>> 2.34.1
>>
>>