[v2,5/7] media: v4l2-subdev: Support hybrid links in v4l2_subdev_link_validate()

  The v4l2_subdev_link_validate() helper function is meant to be used as a
drop-in implementation of a V4L2 subdev entity .link_validate() handler.
It supports subdev-to-subdev links only, and complains if one end of the
link is not a subdev. This forces drivers that have video output devices
connected to subdevs to implement a custom .link_validate() handler,
calling v4l2_subdev_link_validate() for the subdev-to-subdev links, and
performing manual link validation for the video-to-subdev links.

Video devices embed a media entity, and therefore also have a
.link_validate() operation. For video capture devices, the operation
should be manually implemented by drivers for validate the
subdev-to-video links. For video output devices, on the other hand, that
operation is never called, as link validation is performed in the
context of the sink entity.

As a result, we end up forcing drivers to implement a custom
.link_validate() handler for subdevs connected to video output devices,
when the video devices provide an operation that could be used for that
purpose.

To improve that situation, make v4l2_subdev_link_validate() delegate
link validation to the source's .link_validate() operation when the link
source is a video device and the link sink is a subdev. This allows
broader usage of v4l2_subdev_link_validate(), and simplifies drivers by
making video device link validation easy to implement in the video
device .link_validate(), regardless of whether the video device is an
output device or a capture device.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++----
 include/media/v4l2-subdev.h           |  6 ++++
 2 files changed, 41 insertions(+), 5 deletions(-)
  

Hi,

On 22/08/2024 18:45, Laurent Pinchart wrote:
> The v4l2_subdev_link_validate() helper function is meant to be used as a
> drop-in implementation of a V4L2 subdev entity .link_validate() handler.
> It supports subdev-to-subdev links only, and complains if one end of the
> link is not a subdev. This forces drivers that have video output devices
> connected to subdevs to implement a custom .link_validate() handler,
> calling v4l2_subdev_link_validate() for the subdev-to-subdev links, and
> performing manual link validation for the video-to-subdev links.
> 
> Video devices embed a media entity, and therefore also have a
> .link_validate() operation. For video capture devices, the operation
> should be manually implemented by drivers for validate the
> subdev-to-video links. For video output devices, on the other hand, that
> operation is never called, as link validation is performed in the
> context of the sink entity.
> 
> As a result, we end up forcing drivers to implement a custom
> .link_validate() handler for subdevs connected to video output devices,
> when the video devices provide an operation that could be used for that
> purpose.
> 
> To improve that situation, make v4l2_subdev_link_validate() delegate
> link validation to the source's .link_validate() operation when the link
> source is a video device and the link sink is a subdev. This allows
> broader usage of v4l2_subdev_link_validate(), and simplifies drivers by
> making video device link validation easy to implement in the video
> device .link_validate(), regardless of whether the video device is an
> output device or a capture device.

Maybe mention this patch in the previous patch's desc to answer the 
question I sent =)

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++----
>   include/media/v4l2-subdev.h           |  6 ++++
>   2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index d3196042d5c5..32ffebae4d17 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1450,13 +1450,43 @@ int v4l2_subdev_link_validate(struct media_link *link)
>   	if (WARN_ON_ONCE(!is_media_entity_v4l2_subdev(link->sink->entity)))
>   		return -EINVAL;
>   
> -	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> -		pr_warn_once("source of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> -			     link->source->entity->name, link->source->index,
> -			     link->sink->entity->name, link->sink->index);
> -		return 0;
> +	/*
> +	 * If the source is a video device, delegate link validation to it. This
> +	 * allows usage of this helper for subdev connected to a video output
> +	 * device, provided that the driver implement the video output device's
> +	 * .link_validate() operation.
> +	 */
> +	if (is_media_entity_v4l2_video_device(link->source->entity)) {
> +		struct media_entity *source = link->source->entity;
> +
> +		if (!source->ops || !source->ops->link_validate) {
> +			/*
> +			 * Many existing drivers do not implement the required
> +			 * .link_validate() operation for their video devices.
> +			 * Print a warning to get the drivers fixed, and return
> +			 * 0 to avoid breaking userspace. This should
> +			 * eventually be turned into a WARN_ON() when all
> +			 * drivers will have been fixed.
> +			 */
> +			pr_warn_once("video device '%s' does not implement .link_validate(), driver bug!\n",
> +				     source->name);
> +			return 0;
> +		}
> +
> +		/* Avoid infinite loops. */

Maybe this could elaborate a bit, and say that non-subdev drivers should 
not use v4l2_subdev_link_validate, but some do (?) or might use it by 
mistake, and this catches the driver bug.

> +		if (WARN_ON(source->ops->link_validate == v4l2_subdev_link_validate))
> +			return -EINVAL;

This might still be risky. The driver could implement its own validate, 
which does some checks and then calls v4l2_subdev_link_validate(). But 
I'm sure that'll get get noticed =).

  Tomi

> +
> +		return source->ops->link_validate(link);
>   	}
>   
> +	/*
> +	 * If the source is still not a subdev, usage of this helper is a clear
> +	 * driver bug.
> +	 */
> +	if (WARN_ON(!is_media_entity_v4l2_subdev(link->source->entity)))
> +		return -EINVAL;
> +
>   	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>   	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>   
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index bd235d325ff9..8daa0929865c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1250,6 +1250,12 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
>    * calls v4l2_subdev_link_validate_default() to ensure that
>    * width, height and the media bus pixel code are equal on both
>    * source and sink of the link.
> + *
> + * The function can be used as a drop-in &media_entity_ops.link_validate
> + * implementation for v4l2_subdev instances. It supports all links between
> + * subdevs, as well as links between subdevs and video devices, provided that
> + * the video devices also implement their &media_entity_ops.link_validate
> + * operation.
>    */
>   int v4l2_subdev_link_validate(struct media_link *link);
>
  

Hi Tomi,

On Mon, Aug 26, 2024 at 02:23:06PM +0300, Tomi Valkeinen wrote:
> On 22/08/2024 18:45, Laurent Pinchart wrote:
> > The v4l2_subdev_link_validate() helper function is meant to be used as a
> > drop-in implementation of a V4L2 subdev entity .link_validate() handler.
> > It supports subdev-to-subdev links only, and complains if one end of the
> > link is not a subdev. This forces drivers that have video output devices
> > connected to subdevs to implement a custom .link_validate() handler,
> > calling v4l2_subdev_link_validate() for the subdev-to-subdev links, and
> > performing manual link validation for the video-to-subdev links.
> > 
> > Video devices embed a media entity, and therefore also have a
> > .link_validate() operation. For video capture devices, the operation
> > should be manually implemented by drivers for validate the
> > subdev-to-video links. For video output devices, on the other hand, that
> > operation is never called, as link validation is performed in the
> > context of the sink entity.
> > 
> > As a result, we end up forcing drivers to implement a custom
> > .link_validate() handler for subdevs connected to video output devices,
> > when the video devices provide an operation that could be used for that
> > purpose.
> > 
> > To improve that situation, make v4l2_subdev_link_validate() delegate
> > link validation to the source's .link_validate() operation when the link
> > source is a video device and the link sink is a subdev. This allows
> > broader usage of v4l2_subdev_link_validate(), and simplifies drivers by
> > making video device link validation easy to implement in the video
> > device .link_validate(), regardless of whether the video device is an
> > output device or a capture device.
> 
> Maybe mention this patch in the previous patch's desc to answer the 
> question I sent =)

I'll do so.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >   drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++----
> >   include/media/v4l2-subdev.h           |  6 ++++
> >   2 files changed, 41 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index d3196042d5c5..32ffebae4d17 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -1450,13 +1450,43 @@ int v4l2_subdev_link_validate(struct media_link *link)
> >   	if (WARN_ON_ONCE(!is_media_entity_v4l2_subdev(link->sink->entity)))
> >   		return -EINVAL;
> >   
> > -	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> > -		pr_warn_once("source of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> > -			     link->source->entity->name, link->source->index,
> > -			     link->sink->entity->name, link->sink->index);
> > -		return 0;
> > +	/*
> > +	 * If the source is a video device, delegate link validation to it. This
> > +	 * allows usage of this helper for subdev connected to a video output
> > +	 * device, provided that the driver implement the video output device's
> > +	 * .link_validate() operation.
> > +	 */
> > +	if (is_media_entity_v4l2_video_device(link->source->entity)) {
> > +		struct media_entity *source = link->source->entity;
> > +
> > +		if (!source->ops || !source->ops->link_validate) {
> > +			/*
> > +			 * Many existing drivers do not implement the required
> > +			 * .link_validate() operation for their video devices.
> > +			 * Print a warning to get the drivers fixed, and return
> > +			 * 0 to avoid breaking userspace. This should
> > +			 * eventually be turned into a WARN_ON() when all
> > +			 * drivers will have been fixed.
> > +			 */
> > +			pr_warn_once("video device '%s' does not implement .link_validate(), driver bug!\n",
> > +				     source->name);
> > +			return 0;
> > +		}
> > +
> > +		/* Avoid infinite loops. */
> 
> Maybe this could elaborate a bit, and say that non-subdev drivers should 
> not use v4l2_subdev_link_validate, but some do (?) or might use it by 
> mistake, and this catches the driver bug.

I'll expand the comment to

                /*
                 * Avoid infinite loops in case a video device incorrectly uses
                 * this helper function as its .link_validate() handler.
                 */

> > +		if (WARN_ON(source->ops->link_validate == v4l2_subdev_link_validate))
> > +			return -EINVAL;
> 
> This might still be risky. The driver could implement its own validate, 
> which does some checks and then calls v4l2_subdev_link_validate(). But 
> I'm sure that'll get get noticed =).

Yes, that would still lead to an infinite loop, but there's only so much
we can do against foot-shooting.

> > +
> > +		return source->ops->link_validate(link);
> >   	}
> >   
> > +	/*
> > +	 * If the source is still not a subdev, usage of this helper is a clear
> > +	 * driver bug.
> > +	 */
> > +	if (WARN_ON(!is_media_entity_v4l2_subdev(link->source->entity)))
> > +		return -EINVAL;
> > +
> >   	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
> >   	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
> >   
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index bd235d325ff9..8daa0929865c 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -1250,6 +1250,12 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
> >    * calls v4l2_subdev_link_validate_default() to ensure that
> >    * width, height and the media bus pixel code are equal on both
> >    * source and sink of the link.
> > + *
> > + * The function can be used as a drop-in &media_entity_ops.link_validate
> > + * implementation for v4l2_subdev instances. It supports all links between
> > + * subdevs, as well as links between subdevs and video devices, provided that
> > + * the video devices also implement their &media_entity_ops.link_validate
> > + * operation.
> >    */
> >   int v4l2_subdev_link_validate(struct media_link *link);
> >