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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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);
> >   

-- 
Regards,

Laurent Pinchart