Re: [PATCH v4 22/29] media: mc: Add MEDIA_LNK_FL_VALIDATE_LATE

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sakari

On Wed, Apr 08, 2026 at 06:39:31PM +0300, Sakari Ailus wrote:
> Add MEDIA_LNK_FL_VALIDATE_LATE flag to support late validation of links.
> This is serving the use case where video devices are configured and
> started streaming indepenently of each other but this sequence may be run

s/indepenently/independently

> in series, in such a way that a video device in a pipeline starts
> streaming before another one is configured.
>
> Before this flag, drivers have resorted to implementing the link
> validation separately for the video nodes as part of streaming start
> sequence.
>
> media_pipeline_start() shall be called on each leaf entity connected to
> the graph with a link where MEDIA_LNK_FL_VALIDATE_LATE is set before
> uphardware operation.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  .../media/mediactl/media-ioc-setup-link.rst   |  4 +
>  .../media/mediactl/media-types.rst            |  5 ++
>  drivers/media/mc/mc-entity.c                  | 82 ++++++++++++++++++-
>  include/uapi/linux/media.h                    |  1 +
>  4 files changed, 88 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/mediactl/media-ioc-setup-link.rst b/Documentation/userspace-api/media/mediactl/media-ioc-setup-link.rst
> index 23208300cb61..7c2bced57e77 100644
> --- a/Documentation/userspace-api/media/mediactl/media-ioc-setup-link.rst
> +++ b/Documentation/userspace-api/media/mediactl/media-ioc-setup-link.rst
> @@ -49,6 +49,10 @@ Only links marked with the ``DYNAMIC`` link flag can be enabled/disabled
>  while streaming media data. Attempting to enable or disable a streaming
>  non-dynamic link will return an ``EBUSY`` error code.
>
> +Pipeline validation may be delayed for links marked with the ``VALIDATE_LATE``

Is it helpful to say "created" instead of "marked" to highlight that
this flag is set by drivers or is it clear enough from the context ?

> +flag until actual hardware operation even if the rest of the pipeline would be

operation or operations ?

> +validated at an earlier point of time.
> +
>  If the specified link can't be found the driver returns with an ``EINVAL``
>  error code.
>
> diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst
> index 6332e8395263..d6a690655a01 100644
> --- a/Documentation/userspace-api/media/mediactl/media-types.rst
> +++ b/Documentation/userspace-api/media/mediactl/media-types.rst
> @@ -391,6 +391,7 @@ must be set for every pad.
>  .. _MEDIA-LNK-FL-ENABLED:
>  .. _MEDIA-LNK-FL-IMMUTABLE:
>  .. _MEDIA-LNK-FL-DYNAMIC:
> +.. _MEDIA-LNK-FL-VALIDATE-LATE:
>  .. _MEDIA-LNK-FL-LINK-TYPE:
>
>  .. flat-table:: Media link flags
> @@ -410,6 +411,10 @@ must be set for every pad.
>         -  The link enabled state can be modified during streaming. This flag
>  	  is set by drivers and is read-only for applications.
>
> +    *  -  ``MEDIA_LNK_FL_VALIDATE_LATE``
> +       -  The validation of the link may be delayed up to until the start of

Not an English native speaker here, but "delayed up to until" sounds more
complicated than just "delayed to"

> +	  hardware operation.
> +
>      *  -  ``MEDIA_LNK_FL_LINK_TYPE``
>         -  This is a bitmask that defines the type of the link. The following
>  	  link types are currently supported:
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 287eded356bb..5b0162f81425 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -771,7 +771,7 @@ static int media_pipeline_populate(struct media_pipeline *pipe,
>  static int
>  __media_pipeline_validate_one(struct media_pad *origin,
>  			      struct media_pad *pad, struct media_link *link,
> -			      bool *has_enabled_link)
> +			      bool *has_enabled_link, bool skip_validation)
>  {
>  	struct media_device *mdev = origin->graph_obj.mdev;
>  	struct media_entity *entity = pad->entity;
> @@ -784,6 +784,9 @@ __media_pipeline_validate_one(struct media_pad *origin,
>  	if (has_enabled_link)
>  		*has_enabled_link = true;
>
> +	if (skip_validation)
> +		return 0;
> +
>  	/* Skip validation if the current pad isn't the sink pad of the link. */
>  	if (link->sink != pad)
>  		return 0;
> @@ -825,11 +828,48 @@ __must_check int __media_pipeline_start(struct media_pad *origin,
>  		return -EINVAL;
>
>  	/*
> -	 * If the pipeline has already been started, it is guaranteed to be
> -	 * valid, so just increase the start count.
> +	 * Increase start count on pipelines that have been validated
> +	 * earlier. Also check links with the VALIDATE_LATE flag here.
>  	 */
>  	if (pipe->start_count) {
> +		struct media_link *link;
> +
> +		link = __media_entity_next_link(origin->entity, NULL,
> +						MEDIA_LNK_FL_DATA_LINK);
> +		if (link && link->flags & MEDIA_LNK_FL_VALIDATE_LATE) {
> +			struct media_link *link2 =
> +				__media_entity_next_link(origin->entity, link,
> +							 MEDIA_LNK_FL_DATA_LINK);
> +			bool has_enabled_link = false;
> +
> +			/*
> +			 * Only a single pad is allowed for VALIDATE_LATE
> +			 * links. That pad needs to have exactly one link.
> +			 */
> +			if (origin->entity->num_pads != 1)
> +				return -EINVAL;
> +
> +			if (!link || link2)
> +				return -EINVAL;

This is a development error, isn't it ? Is it worth an error message ?

> +
> +			dev_dbg(mdev->dev,
> +				"Validating pad '%s':%u late\n",
> +				origin->entity->name, origin->index);
> +
> +			ret = __media_pipeline_validate_one(link->sink,
> +							    link->sink, link,
> +							    &has_enabled_link,
> +							    false);
> +			if (ret)
> +				return ret;
> +
> +			if (origin->flags & MEDIA_PAD_FL_MUST_CONNECT &&
> +			    !has_enabled_link)
> +				return -ENOLINK;
> +		}
> +
>  		pipe->start_count++;
> +
>  		return 0;
>  	}
>
> @@ -873,12 +913,19 @@ __must_check int __media_pipeline_start(struct media_pad *origin,
>  		 * the connected sink pad to avoid duplicating checks.
>  		 */
>  		for_each_media_entity_data_link(entity, link) {
> +			/* Skip late-validated links not connected to origin. */
> +			bool skip_validation =
> +				link->flags & MEDIA_LNK_FL_VALIDATE_LATE &&
> +				link->sink != origin &&
> +				link->source != origin;
> +
>  			/* Skip links unrelated to the current pad. */
>  			if (link->sink != pad && link->source != pad)
>  				continue;
>
>  			ret = __media_pipeline_validate_one(origin, pad, link,
> -							    &has_enabled_link);
> +							    &has_enabled_link,
> +							    skip_validation);
>  			if (ret)
>  				goto error;
>  		}
> @@ -1158,6 +1205,33 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
>  	if (WARN_ON(!(sink->pads[sink_pad].flags & MEDIA_PAD_FL_SINK)))
>  		return -EINVAL;
>
> +	/*
> +	 * With the late validate flag, either source or sink shall have exactly
> +	 * one pad and no links before this one. Similarly, no links may be
> +	 * added to entities with a single pad and an existing late-validated
> +	 * link.
> +	 */
> +	if (flags & MEDIA_LNK_FL_VALIDATE_LATE) {
> +		if (!(source->num_pads == 1 && !source->num_links) &&
> +		    !(sink->num_pads == 1 && !sink->num_links))
> +			return -EINVAL;

Ah, so the above error check for which I suggested an error message
cannot happen, right ?

> +	} else {
> +		struct media_entity *entities[] = { source, sink };
> +
> +		for (unsigned int i = 0; i < ARRAY_SIZE(entities); i++) {
> +			if (entities[i]->num_pads != 1)
> +				continue;
> +
> +			struct media_link *__link =
> +				__media_entity_next_link(entities[i], NULL,
> +							 MEDIA_LNK_FL_DATA_LINK);
> +
> +			if (__link &&
> +			    __link->flags & MEDIA_LNK_FL_VALIDATE_LATE)
> +				return -EINVAL;
> +		}
> +	}
> +
>  	link = media_add_link(&source->links);
>  	if (link == NULL)
>  		return -ENOMEM;
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 1c80b1d6bbaf..c96e2118ea99 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -219,6 +219,7 @@ struct media_pad_desc {
>  #define MEDIA_LNK_FL_ENABLED			(1U << 0)
>  #define MEDIA_LNK_FL_IMMUTABLE			(1U << 1)
>  #define MEDIA_LNK_FL_DYNAMIC			(1U << 2)
> +#define MEDIA_LNK_FL_VALIDATE_LATE		(1U << 3)

Nice, with the above minors addressed, if applicable
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>
>  #define MEDIA_LNK_FL_LINK_TYPE			(0xf << 28)
>  #  define MEDIA_LNK_FL_DATA_LINK		(0U << 28)
> --
> 2.47.3
>
>