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@xxxxxxxxxxxxxxxx>
> > ---
> > 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
