[PATCH v2 0/7] media: v4l2: Improve media link validation

[PATCH v2 0/7] media: v4l2: Improve media link validation

[Laurent Pinchart]

Hello,

(Resending to the right mailing list, sorry for the noise)

This patch series improves the link validation helpers to make it easier
for drivers to validate all links in a pipeline.

The vast majority of drivers use the v4l2_subdev_link_validate()
function as their .link_validate() handler for subdevs. This correctly
validates subdev-to-subdev links. For links between subdevs and video
capture devices, a few drivers implement the .link_validate() operation
of their video devices, while others implement manual validation in
their .streamon() handler, or don't implement validation at all. Links
between video output devices are in an even worse state, as the link
validation logic at pipeline start time does not call the
.link_validate() operation on the source side of a link, leaving manual
validation in .streamon() the only option. Adding insult to injury,
v4l2_subdev_link_validate() prints a warning when the link's source is
not a subdev, which forces drivers to implement a manual subdev link
validation handler for subdevs connected to output video nodes.

It turns out that v4l2_subdev_link_validate() is even used in the
.link_validate() implementation of video devices by two drivers. This is
clearly wrong, and is addressed by patches 1/7 to 3/7. Note that patch
3/7 should ideally implement real validation of the link between the
subdev and video capture device, but that requires understanding of the
driver's logic as well as testing, so I have left it out as an exercise
for the driver's maintainer. The patch doesn't introduce any regression.

Patch 4/7 then starts refactoring the v4l2_subdev_link_validate() to
separate the current warning in two categories, with a WARN_ON() for an
issue that indicates a clear driver bug (and does not occur in any
driver in mainline at the moment), and keeping the pr_warn_once() for
other issues that are present in multiple drivers.

Patch 5/7 continues with expanding v4l2_subdev_link_validate() to better
support links from video output devices to subdevs, delegating the
validation to the video output device's .link_validate() operation. This
allows using v4l2_subdev_link_validate() for all subdevs in a driver,
regardless of whether they are connected to other subdevs, video capture
devices or video output devices, and implementing all the link
validation for video devices in their .link_validate() operation.

Patches 6/7 and 7/7 then address the v4l2_subdev_link_validate() warning
for the vsp1 driver. Patch 6/7 silences the warning. This is
unfortunately done with a workaround, as the ideal implementation,
moving all validation for video devices to their .link_validate()
operation as showcased in patch 7/7, breaks operation of the vsp1 unit
test suite. While that is fixable in the test suite, it indicates that
other userspace applications may also break as a result.

To my great sadness, I had to mark patch 7/7 as [DNI]. This does not
make the v4l2_subdev_link_validate() improvements in patch 5/7
pointless, as they are useful for new drivers, as well as drivers that
don't include multiple video devices in a pipeline.

Laurent Pinchart (7):
  media: microchip-isc: Drop v4l2_subdev_link_validate() for video
    devices
  media: sun4i_csi: Implement link validate for sun4i_csi subdev
  media: sun4i_csi: Don't use v4l2_subdev_link_validate() for video
    device
  media: v4l2-subdev: Refactor warnings in v4l2_subdev_link_validate()
  media: v4l2-subdev: Support hybrid links in
    v4l2_subdev_link_validate()
  media: renesas: vsp1: Implement .link_validate() for video devices
  [DNI] media: renesas: vsp1: Validate all links through
    .link_validate()

 .../platform/microchip/microchip-isc-base.c   |  19 +---
 .../media/platform/renesas/vsp1/vsp1_video.c  | 104 +++++++++---------
 .../platform/sunxi/sun4i-csi/sun4i_csi.c      |  12 ++
 drivers/media/v4l2-core/v4l2-subdev.c         |  50 +++++++--
 include/media/v4l2-subdev.h                   |   6 +
 5 files changed, 115 insertions(+), 76 deletions(-)


base-commit: a043ea54bbb975ca9239c69fd17f430488d33522
-- 
Regards,

Laurent Pinchart

[PATCH v2 1/7] media: microchip-isc: Drop v4l2_subdev_link_validate() for video devices

[Laurent Pinchart]

The v4l2_subdev_link_validate() function is a helper designed to
validate links whose sink is a subdev. When called on a link whose sink
is a video device, it only prints a warning and returns. As the
microchip-isc driver implements manual validate of the subdev to video
device link, we can just dropp the v4l2_subdev_link_validate() to avoid
the warning.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../platform/microchip/microchip-isc-base.c   | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
index f3a5cbacadbe..28e56f6a695d 100644
--- a/drivers/media/platform/microchip/microchip-isc-base.c
+++ b/drivers/media/platform/microchip/microchip-isc-base.c
@@ -902,8 +902,11 @@ static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
 	return 0;
 }
 
-static int isc_validate(struct isc_device *isc)
+static int isc_link_validate(struct media_link *link)
 {
+	struct video_device *vdev =
+		media_entity_to_video_device(link->sink->entity);
+	struct isc_device *isc = video_get_drvdata(vdev);
 	int ret;
 	int i;
 	struct isc_format *sd_fmt = NULL;
@@ -1906,20 +1909,6 @@ int microchip_isc_pipeline_init(struct isc_device *isc)
 }
 EXPORT_SYMBOL_GPL(microchip_isc_pipeline_init);
 
-static int isc_link_validate(struct media_link *link)
-{
-	struct video_device *vdev =
-		media_entity_to_video_device(link->sink->entity);
-	struct isc_device *isc = video_get_drvdata(vdev);
-	int ret;
-
-	ret = v4l2_subdev_link_validate(link);
-	if (ret)
-		return ret;
-
-	return isc_validate(isc);
-}
-
 static const struct media_entity_operations isc_entity_operations = {
 	.link_validate = isc_link_validate,
 };
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/7] media: microchip-isc: Drop v4l2_subdev_link_validate() for video devices

[Tomi Valkeinen]

Hi,

On 22/08/2024 18:45, Laurent Pinchart wrote:
> The v4l2_subdev_link_validate() function is a helper designed to
> validate links whose sink is a subdev. When called on a link whose sink
> is a video device, it only prints a warning and returns. As the
> microchip-isc driver implements manual validate of the subdev to video
> device link, we can just dropp the v4l2_subdev_link_validate() to avoid
> the warning.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   .../platform/microchip/microchip-isc-base.c   | 19 ++++---------------
>   1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index f3a5cbacadbe..28e56f6a695d 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -902,8 +902,11 @@ static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
>   	return 0;
>   }
>   
> -static int isc_validate(struct isc_device *isc)
> +static int isc_link_validate(struct media_link *link)
>   {
> +	struct video_device *vdev =
> +		media_entity_to_video_device(link->sink->entity);
> +	struct isc_device *isc = video_get_drvdata(vdev);
>   	int ret;
>   	int i;
>   	struct isc_format *sd_fmt = NULL;
> @@ -1906,20 +1909,6 @@ int microchip_isc_pipeline_init(struct isc_device *isc)
>   }
>   EXPORT_SYMBOL_GPL(microchip_isc_pipeline_init);
>   
> -static int isc_link_validate(struct media_link *link)
> -{
> -	struct video_device *vdev =
> -		media_entity_to_video_device(link->sink->entity);
> -	struct isc_device *isc = video_get_drvdata(vdev);
> -	int ret;
> -
> -	ret = v4l2_subdev_link_validate(link);
> -	if (ret)
> -		return ret;
> -
> -	return isc_validate(isc);
> -}
> -
>   static const struct media_entity_operations isc_entity_operations = {
>   	.link_validate = isc_link_validate,
>   };

Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

  Tomi

[PATCH v2 2/7] media: sun4i_csi: Implement link validate for sun4i_csi subdev

[Laurent Pinchart]

The sun4i_csi driver doesn't implement link validation for the subdev it
registers, leaving the link between the subdev and its source
unvalidated. Fix it, using the v4l2_subdev_link_validate() helper.

Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
index 097a3a08ef7d..dbb26c7b2f8d 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
@@ -39,6 +39,10 @@ static const struct media_entity_operations sun4i_csi_video_entity_ops = {
 	.link_validate = v4l2_subdev_link_validate,
 };
 
+static const struct media_entity_operations sun4i_csi_subdev_entity_ops = {
+	.link_validate = v4l2_subdev_link_validate,
+};
+
 static int sun4i_csi_notify_bound(struct v4l2_async_notifier *notifier,
 				  struct v4l2_subdev *subdev,
 				  struct v4l2_async_connection *asd)
@@ -214,6 +218,7 @@ static int sun4i_csi_probe(struct platform_device *pdev)
 	subdev->internal_ops = &sun4i_csi_subdev_internal_ops;
 	subdev->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+	subdev->entity.ops = &sun4i_csi_subdev_entity_ops;
 	subdev->owner = THIS_MODULE;
 	snprintf(subdev->name, sizeof(subdev->name), "sun4i-csi-0");
 	v4l2_set_subdevdata(subdev, csi);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/7] media: sun4i_csi: Implement link validate for sun4i_csi subdev

[Tomi Valkeinen]

Hi,

On 22/08/2024 18:45, Laurent Pinchart wrote:
> The sun4i_csi driver doesn't implement link validation for the subdev it
> registers, leaving the link between the subdev and its source
> unvalidated. Fix it, using the v4l2_subdev_link_validate() helper.
> 
> Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Chen-Yu Tsai <wens@csie.org>
> ---
>   drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> index 097a3a08ef7d..dbb26c7b2f8d 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> @@ -39,6 +39,10 @@ static const struct media_entity_operations sun4i_csi_video_entity_ops = {
>   	.link_validate = v4l2_subdev_link_validate,
>   };
>   
> +static const struct media_entity_operations sun4i_csi_subdev_entity_ops = {
> +	.link_validate = v4l2_subdev_link_validate,
> +};
> +
>   static int sun4i_csi_notify_bound(struct v4l2_async_notifier *notifier,
>   				  struct v4l2_subdev *subdev,
>   				  struct v4l2_async_connection *asd)
> @@ -214,6 +218,7 @@ static int sun4i_csi_probe(struct platform_device *pdev)
>   	subdev->internal_ops = &sun4i_csi_subdev_internal_ops;
>   	subdev->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>   	subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> +	subdev->entity.ops = &sun4i_csi_subdev_entity_ops;
>   	subdev->owner = THIS_MODULE;
>   	snprintf(subdev->name, sizeof(subdev->name), "sun4i-csi-0");
>   	v4l2_set_subdevdata(subdev, csi);

Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

  Tomi

[PATCH v2 3/7] media: sun4i_csi: Don't use v4l2_subdev_link_validate() for video device

[Laurent Pinchart]

The v4l2_subdev_link_validate() function is a helper designed to
validate links whose sink is a subdev. When called on a link whose sink
is a video device, it only prints a warning and returns. Its usage in
the sun4i_csi driver is wrong, leaving the link from the sub4i_csi
subdev to the capture video device unvalidated.

Planned improvements to the v4l2_subdev_link_validate() function will
turn the warning into an error, breaking the sun4i_csi driver. As an
interim measure, move the warning to the sun4i_csi driver in a custom
validation handler, and drop the call to the helper.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
index dbb26c7b2f8d..d07e980aba61 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
@@ -35,8 +35,15 @@ struct sun4i_csi_traits {
 	bool has_isp;
 };
 
+static int sun4i_csi_video_link_validate(struct media_link *link)
+{
+	dev_warn_once(link->graph_obj.mdev->dev,
+		      "Driver bug: link validation not implemented\n");
+	return 0;
+}
+
 static const struct media_entity_operations sun4i_csi_video_entity_ops = {
-	.link_validate = v4l2_subdev_link_validate,
+	.link_validate = sun4i_csi_video_link_validate,
 };
 
 static const struct media_entity_operations sun4i_csi_subdev_entity_ops = {
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/7] media: sun4i_csi: Don't use v4l2_subdev_link_validate() for video device

[Tomi Valkeinen]

Hi,

On 22/08/2024 18:45, Laurent Pinchart wrote:
> The v4l2_subdev_link_validate() function is a helper designed to
> validate links whose sink is a subdev. When called on a link whose sink
> is a video device, it only prints a warning and returns. Its usage in
> the sun4i_csi driver is wrong, leaving the link from the sub4i_csi
> subdev to the capture video device unvalidated.
> 
> Planned improvements to the v4l2_subdev_link_validate() function will
> turn the warning into an error, breaking the sun4i_csi driver. As an
> interim measure, move the warning to the sun4i_csi driver in a custom
> validation handler, and drop the call to the helper.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Chen-Yu Tsai <wens@csie.org>
> ---
>   drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> index dbb26c7b2f8d..d07e980aba61 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> @@ -35,8 +35,15 @@ struct sun4i_csi_traits {
>   	bool has_isp;
>   };
>   
> +static int sun4i_csi_video_link_validate(struct media_link *link)
> +{
> +	dev_warn_once(link->graph_obj.mdev->dev,
> +		      "Driver bug: link validation not implemented\n");
> +	return 0;
> +}
> +
>   static const struct media_entity_operations sun4i_csi_video_entity_ops = {
> -	.link_validate = v4l2_subdev_link_validate,
> +	.link_validate = sun4i_csi_video_link_validate,
>   };
>   
>   static const struct media_entity_operations sun4i_csi_subdev_entity_ops = {

I fear this might just leave it broken, but I don't have a better idea.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

  Tomi

Re: [PATCH v2 3/7] media: sun4i_csi: Don't use v4l2_subdev_link_validate() for video device

[Laurent Pinchart]

On Mon, Aug 26, 2024 at 02:04:44PM +0300, Tomi Valkeinen wrote:
> On 22/08/2024 18:45, Laurent Pinchart wrote:
> > The v4l2_subdev_link_validate() function is a helper designed to
> > validate links whose sink is a subdev. When called on a link whose sink
> > is a video device, it only prints a warning and returns. Its usage in
> > the sun4i_csi driver is wrong, leaving the link from the sub4i_csi
> > subdev to the capture video device unvalidated.
> > 
> > Planned improvements to the v4l2_subdev_link_validate() function will
> > turn the warning into an error, breaking the sun4i_csi driver. As an
> > interim measure, move the warning to the sun4i_csi driver in a custom
> > validation handler, and drop the call to the helper.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Acked-by: Chen-Yu Tsai <wens@csie.org>
> > ---
> >   drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > index dbb26c7b2f8d..d07e980aba61 100644
> > --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > @@ -35,8 +35,15 @@ struct sun4i_csi_traits {
> >   	bool has_isp;
> >   };
> >   
> > +static int sun4i_csi_video_link_validate(struct media_link *link)
> > +{
> > +	dev_warn_once(link->graph_obj.mdev->dev,
> > +		      "Driver bug: link validation not implemented\n");
> > +	return 0;
> > +}
> > +
> >   static const struct media_entity_operations sun4i_csi_video_entity_ops = {
> > -	.link_validate = v4l2_subdev_link_validate,
> > +	.link_validate = sun4i_csi_video_link_validate,
> >   };
> >   
> >   static const struct media_entity_operations sun4i_csi_subdev_entity_ops = {
> 
> I fear this might just leave it broken, but I don't have a better idea.

I think it would be fairly easy to implement support for proper
validation in the driver, but it would require access to a device for
testing. This patch doesn't worsen the issue, or make fixing it more
difficult, so I think it's OK for now until someone can send a real fix.

> Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/7] media: v4l2-subdev: Refactor warnings in v4l2_subdev_link_validate()

[Laurent Pinchart]

The v4l2_subdev_link_validate() function prints a one-time warning if it
gets called on a link whose source or sink is not a subdev. As links get
validated in the context of their sink, a call to the helper when the
link's sink is not a subdev indicates that the driver has set its
.link_validate() handler to v4l2_subdev_link_validate() on a non-subdev
entity, which is a clear driver bug. On the other hand, the link's
source not being a subdev indicates that the helper is used for a subdev
connected to a video output device, which is a lesser issue, if only
because this is currently common practice.

There are no drivers left in the kernel that use
v4l2_subdev_link_validate() in a context where it may get called on a
non-subdev sink. Replace the pr_warn_once() with a WARN_ON_ONCE() in
this case to make sure that new offenders won't be introduced.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v1:

- Switch from WARN_ON() to WARN_ON_ONCE()
---
 drivers/media/v4l2-core/v4l2-subdev.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 7c5812d55315..d3196042d5c5 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1443,11 +1443,15 @@ int v4l2_subdev_link_validate(struct media_link *link)
 	bool states_locked;
 	int ret;
 
-	if (!is_media_entity_v4l2_subdev(link->sink->entity) ||
-	    !is_media_entity_v4l2_subdev(link->source->entity)) {
-		pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
-			     !is_media_entity_v4l2_subdev(link->sink->entity) ?
-			     "sink" : "source",
+	/*
+	 * Links are validated in the context of the sink entity. Usage of this
+	 * helper on a sink that is not a subdev is a clear driver bug.
+	 */
+	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;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/7] media: v4l2-subdev: Refactor warnings in v4l2_subdev_link_validate()

[Tomi Valkeinen]

Hi,

On 22/08/2024 18:45, Laurent Pinchart wrote:
> The v4l2_subdev_link_validate() function prints a one-time warning if it
> gets called on a link whose source or sink is not a subdev. As links get
> validated in the context of their sink, a call to the helper when the
> link's sink is not a subdev indicates that the driver has set its
> .link_validate() handler to v4l2_subdev_link_validate() on a non-subdev
> entity, which is a clear driver bug. On the other hand, the link's
> source not being a subdev indicates that the helper is used for a subdev
> connected to a video output device, which is a lesser issue, if only
> because this is currently common practice.

Hmm, what does this mean... So we have a sink subdev, which might be 
linked to a source subdev (in which case everything is fine), or it 
might be linked to a non-subdev source.

Why is it a bug to be linked to a non-subdev source? And if it is a bug, 
why is ok (only pr_warn_once)?

  Tomi

> There are no drivers left in the kernel that use
> v4l2_subdev_link_validate() in a context where it may get called on a
> non-subdev sink. Replace the pr_warn_once() with a WARN_ON_ONCE() in
> this case to make sure that new offenders won't be introduced.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Switch from WARN_ON() to WARN_ON_ONCE()
> ---
>   drivers/media/v4l2-core/v4l2-subdev.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 7c5812d55315..d3196042d5c5 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1443,11 +1443,15 @@ int v4l2_subdev_link_validate(struct media_link *link)
>   	bool states_locked;
>   	int ret;
>   
> -	if (!is_media_entity_v4l2_subdev(link->sink->entity) ||
> -	    !is_media_entity_v4l2_subdev(link->source->entity)) {
> -		pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> -			     !is_media_entity_v4l2_subdev(link->sink->entity) ?
> -			     "sink" : "source",
> +	/*
> +	 * Links are validated in the context of the sink entity. Usage of this
> +	 * helper on a sink that is not a subdev is a clear driver bug.
> +	 */
> +	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;

Re: [PATCH v2 4/7] media: v4l2-subdev: Refactor warnings in v4l2_subdev_link_validate()

[Laurent Pinchart]

Hi Tomi,

On Mon, Aug 26, 2024 at 02:10:57PM +0300, Tomi Valkeinen wrote:
> On 22/08/2024 18:45, Laurent Pinchart wrote:
> > The v4l2_subdev_link_validate() function prints a one-time warning if it
> > gets called on a link whose source or sink is not a subdev. As links get
> > validated in the context of their sink, a call to the helper when the
> > link's sink is not a subdev indicates that the driver has set its
> > .link_validate() handler to v4l2_subdev_link_validate() on a non-subdev
> > entity, which is a clear driver bug. On the other hand, the link's
> > source not being a subdev indicates that the helper is used for a subdev
> > connected to a video output device, which is a lesser issue, if only
> > because this is currently common practice.
> 
> Hmm, what does this mean... So we have a sink subdev, which might be 
> linked to a source subdev (in which case everything is fine), or it 
> might be linked to a non-subdev source.
> 
> Why is it a bug to be linked to a non-subdev source? And if it is a bug, 
> why is ok (only pr_warn_once)?
>
> > There are no drivers left in the kernel that use
> > v4l2_subdev_link_validate() in a context where it may get called on a
> > non-subdev sink. Replace the pr_warn_once() with a WARN_ON_ONCE() in
> > this case to make sure that new offenders won't be introduced.

I'll add here

A subsequent change will improve the v4l2_subdev_link_validate() helper
to properly support validating video device to subdev links.

> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Switch from WARN_ON() to WARN_ON_ONCE()
> > ---
> >   drivers/media/v4l2-core/v4l2-subdev.c | 14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 7c5812d55315..d3196042d5c5 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -1443,11 +1443,15 @@ int v4l2_subdev_link_validate(struct media_link *link)
> >   	bool states_locked;
> >   	int ret;
> >   
> > -	if (!is_media_entity_v4l2_subdev(link->sink->entity) ||
> > -	    !is_media_entity_v4l2_subdev(link->source->entity)) {
> > -		pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> > -			     !is_media_entity_v4l2_subdev(link->sink->entity) ?
> > -			     "sink" : "source",
> > +	/*
> > +	 * Links are validated in the context of the sink entity. Usage of this
> > +	 * helper on a sink that is not a subdev is a clear driver bug.
> > +	 */
> > +	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;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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

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. */
+		if (WARN_ON(source->ops->link_validate == v4l2_subdev_link_validate))
+			return -EINVAL;
+
+		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

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

[Tomi Valkeinen]

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

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

[Laurent Pinchart]

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

[PATCH v2 6/7] media: renesas: vsp1: Implement .link_validate() for video devices

[Laurent Pinchart]

The v4l2_subdev_link_validate() helper prints a warning if the
.link_validate() operation is not implemented for video devices
connected to the subdevs. Implement the operation to silence the
warning.

Ideally validation of the link between the video device and the subdev
should be implemented in that operation. That would however break
userspace that does not configure formats on all video devices before
starting streaming. While this mode of operation may not be considered
valid by the V4L2 API specification (interpretation differ), it is
nonetheless supported by the vsp1 driver at the moment and used by at
least the vsp1 unit test suite, and possibly other userspace
applciations. Removing it would be a regression.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../media/platform/renesas/vsp1/vsp1_video.c  | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
index fdb46ec0c872..e728f9f5160e 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
@@ -1081,6 +1081,27 @@ static const struct v4l2_file_operations vsp1_video_fops = {
 	.mmap = vb2_fop_mmap,
 };
 
+/* -----------------------------------------------------------------------------
+ * Media entity operations
+ */
+
+static int vsp1_video_link_validate(struct media_link *link)
+{
+	/*
+	 * Ideally, link validation should be implemented here instead of
+	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
+	 * manually. That would however break userspace that start one video
+	 * device before configures formats on other video devices in the
+	 * pipeline. This operation is just a no-op to silence the warnings
+	 * from v4l2_subdev_link_validate().
+	 */
+	return 0;
+}
+
+static const struct media_entity_operations vsp1_video_media_ops = {
+	.link_validate = vsp1_video_link_validate,
+};
+
 /* -----------------------------------------------------------------------------
  * Suspend and Resume
  */
@@ -1215,6 +1236,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
 
 	/* ... and the video node... */
 	video->video.v4l2_dev = &video->vsp1->v4l2_dev;
+	video->video.entity.ops = &vsp1_video_media_ops;
 	video->video.fops = &vsp1_video_fops;
 	snprintf(video->video.name, sizeof(video->video.name), "%s %s",
 		 rwpf->entity.subdev.name, direction);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/7] media: renesas: vsp1: Implement .link_validate() for video devices

[Tomi Valkeinen]

Hi,

On 22/08/2024 18:45, Laurent Pinchart wrote:
> The v4l2_subdev_link_validate() helper prints a warning if the
> .link_validate() operation is not implemented for video devices
> connected to the subdevs. Implement the operation to silence the
> warning.
> 
> Ideally validation of the link between the video device and the subdev
> should be implemented in that operation. That would however break
> userspace that does not configure formats on all video devices before
> starting streaming. While this mode of operation may not be considered
> valid by the V4L2 API specification (interpretation differ), it is
> nonetheless supported by the vsp1 driver at the moment and used by at
> least the vsp1 unit test suite, and possibly other userspace
> applciations. Removing it would be a regression.

"applications"

If the media graph is validated when the first stream is enabled, does 
that mean that when the graph is "enabled", we can never change e.g. the 
resolution, even for streams that have not been enabled and even if all 
the drivers would support this?

The pad interdependency should help there, right? Would it help here, too?

  Tomi

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   .../media/platform/renesas/vsp1/vsp1_video.c  | 22 +++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> index fdb46ec0c872..e728f9f5160e 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> @@ -1081,6 +1081,27 @@ static const struct v4l2_file_operations vsp1_video_fops = {
>   	.mmap = vb2_fop_mmap,
>   };
>   
> +/* -----------------------------------------------------------------------------
> + * Media entity operations
> + */
> +
> +static int vsp1_video_link_validate(struct media_link *link)
> +{
> +	/*
> +	 * Ideally, link validation should be implemented here instead of
> +	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
> +	 * manually. That would however break userspace that start one video
> +	 * device before configures formats on other video devices in the
> +	 * pipeline. This operation is just a no-op to silence the warnings
> +	 * from v4l2_subdev_link_validate().
> +	 */
> +	return 0;
> +}
> +
> +static const struct media_entity_operations vsp1_video_media_ops = {
> +	.link_validate = vsp1_video_link_validate,
> +};
> +
>   /* -----------------------------------------------------------------------------
>    * Suspend and Resume
>    */
> @@ -1215,6 +1236,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
>   
>   	/* ... and the video node... */
>   	video->video.v4l2_dev = &video->vsp1->v4l2_dev;
> +	video->video.entity.ops = &vsp1_video_media_ops;
>   	video->video.fops = &vsp1_video_fops;
>   	snprintf(video->video.name, sizeof(video->video.name), "%s %s",
>   		 rwpf->entity.subdev.name, direction);

Re: [PATCH v2 6/7] media: renesas: vsp1: Implement .link_validate() for video devices

[Laurent Pinchart]

Hi Tomi,

On Mon, Aug 26, 2024 at 02:49:44PM +0300, Tomi Valkeinen wrote:
> On 22/08/2024 18:45, Laurent Pinchart wrote:
> > The v4l2_subdev_link_validate() helper prints a warning if the
> > .link_validate() operation is not implemented for video devices
> > connected to the subdevs. Implement the operation to silence the
> > warning.
> > 
> > Ideally validation of the link between the video device and the subdev
> > should be implemented in that operation. That would however break
> > userspace that does not configure formats on all video devices before
> > starting streaming. While this mode of operation may not be considered
> > valid by the V4L2 API specification (interpretation differ), it is
> > nonetheless supported by the vsp1 driver at the moment and used by at
> > least the vsp1 unit test suite, and possibly other userspace
> > applciations. Removing it would be a regression.
> 
> "applications"
> 
> If the media graph is validated when the first stream is enabled, does 
> that mean that when the graph is "enabled", we can never change e.g. the 
> resolution, even for streams that have not been enabled and even if all 
> the drivers would support this?

In the general case, yes. Drivers can already support this use case by
implementing their own link validation. Improvements to generic helpers
could also be possible in the future.

> The pad interdependency should help there, right? Would it help here, too?

Possibly, yes.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >   .../media/platform/renesas/vsp1/vsp1_video.c  | 22 +++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > index fdb46ec0c872..e728f9f5160e 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > @@ -1081,6 +1081,27 @@ static const struct v4l2_file_operations vsp1_video_fops = {
> >   	.mmap = vb2_fop_mmap,
> >   };
> >   
> > +/* -----------------------------------------------------------------------------
> > + * Media entity operations
> > + */
> > +
> > +static int vsp1_video_link_validate(struct media_link *link)
> > +{
> > +	/*
> > +	 * Ideally, link validation should be implemented here instead of
> > +	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
> > +	 * manually. That would however break userspace that start one video
> > +	 * device before configures formats on other video devices in the
> > +	 * pipeline. This operation is just a no-op to silence the warnings
> > +	 * from v4l2_subdev_link_validate().
> > +	 */
> > +	return 0;
> > +}
> > +
> > +static const struct media_entity_operations vsp1_video_media_ops = {
> > +	.link_validate = vsp1_video_link_validate,
> > +};
> > +
> >   /* -----------------------------------------------------------------------------
> >    * Suspend and Resume
> >    */
> > @@ -1215,6 +1236,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
> >   
> >   	/* ... and the video node... */
> >   	video->video.v4l2_dev = &video->vsp1->v4l2_dev;
> > +	video->video.entity.ops = &vsp1_video_media_ops;
> >   	video->video.fops = &vsp1_video_fops;
> >   	snprintf(video->video.name, sizeof(video->video.name), "%s %s",
> >   		 rwpf->entity.subdev.name, direction);

-- 
Regards,

Laurent Pinchart

[PATCH v2 7/7] [DNI] media: renesas: vsp1: Validate all links through .link_validate()

[Laurent Pinchart]

Move validation of the links between video devices and subdevs,
performed manually in vsp1_video_streamon(), to the video device
.link_validate() handler.

This is how drivers should be implemented, but sadly, doing so for the
vsp1 driver could break userspace, introducing a regression. This patch
serves as an example to showcase usage of the .link_validate()
operation, but should not be merged.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../media/platform/renesas/vsp1/vsp1_video.c  | 98 +++++++------------
 1 file changed, 37 insertions(+), 61 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
index e728f9f5160e..14575698bbe7 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
@@ -45,51 +45,6 @@
  * Helper functions
  */
 
-static struct v4l2_subdev *
-vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
-{
-	struct media_pad *remote;
-
-	remote = media_pad_remote_pad_first(local);
-	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
-		return NULL;
-
-	if (pad)
-		*pad = remote->index;
-
-	return media_entity_to_v4l2_subdev(remote->entity);
-}
-
-static int vsp1_video_verify_format(struct vsp1_video *video)
-{
-	struct v4l2_subdev_format fmt = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-	struct v4l2_subdev *subdev;
-	int ret;
-
-	subdev = vsp1_video_remote_subdev(&video->pad, &fmt.pad);
-	if (subdev == NULL)
-		return -EINVAL;
-
-	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
-	if (ret < 0)
-		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
-
-	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
-	    video->rwpf->format.height != fmt.format.height ||
-	    video->rwpf->format.width != fmt.format.width) {
-		dev_dbg(video->vsp1->dev,
-			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
-			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
-			video->rwpf->format.height, fmt.format.code,
-			fmt.format.width, fmt.format.height);
-		return -EPIPE;
-	}
-
-	return 0;
-}
-
 static int __vsp1_video_try_format(struct vsp1_video *video,
 				   struct v4l2_pix_format_mplane *pix,
 				   const struct vsp1_format_info **fmtinfo)
@@ -991,14 +946,6 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	/*
-	 * Verify that the configured format matches the output of the connected
-	 * subdev.
-	 */
-	ret = vsp1_video_verify_format(video);
-	if (ret < 0)
-		goto err_stop;
-
 	/* Start the queue. */
 	ret = vb2_streamon(&video->queue, type);
 	if (ret < 0)
@@ -1087,14 +1034,43 @@ static const struct v4l2_file_operations vsp1_video_fops = {
 
 static int vsp1_video_link_validate(struct media_link *link)
 {
-	/*
-	 * Ideally, link validation should be implemented here instead of
-	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
-	 * manually. That would however break userspace that start one video
-	 * device before configures formats on other video devices in the
-	 * pipeline. This operation is just a no-op to silence the warnings
-	 * from v4l2_subdev_link_validate().
-	 */
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	struct v4l2_subdev *subdev;
+	struct media_entity *entity;
+	struct media_pad *remote;
+	struct vsp1_video *video;
+	int ret;
+
+	if (is_media_entity_v4l2_video_device(link->source->entity)) {
+		entity = link->source->entity;
+		remote = link->sink;
+	} else {
+		entity = link->sink->entity;
+		remote = link->source;
+	}
+
+	fmt.pad = remote->index;
+
+	subdev = media_entity_to_v4l2_subdev(remote->entity);
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret < 0)
+		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
+
+	video = to_vsp1_video(media_entity_to_video_device(entity));
+
+	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
+	    video->rwpf->format.height != fmt.format.height ||
+	    video->rwpf->format.width != fmt.format.width) {
+		dev_dbg(video->vsp1->dev,
+			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
+			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
+			video->rwpf->format.height, fmt.format.code,
+			fmt.format.width, fmt.format.height);
+		return -EPIPE;
+	}
+
 	return 0;
 }
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 7/7] [DNI] media: renesas: vsp1: Validate all links through .link_validate()

[Tomi Valkeinen]

Hi,

On 22/08/2024 18:45, Laurent Pinchart wrote:
> Move validation of the links between video devices and subdevs,
> performed manually in vsp1_video_streamon(), to the video device
> .link_validate() handler.
> 
> This is how drivers should be implemented, but sadly, doing so for the
> vsp1 driver could break userspace, introducing a regression. This patch
> serves as an example to showcase usage of the .link_validate()
> operation, but should not be merged.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   .../media/platform/renesas/vsp1/vsp1_video.c  | 98 +++++++------------
>   1 file changed, 37 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> index e728f9f5160e..14575698bbe7 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> @@ -45,51 +45,6 @@
>    * Helper functions
>    */
>   
> -static struct v4l2_subdev *
> -vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
> -{
> -	struct media_pad *remote;
> -
> -	remote = media_pad_remote_pad_first(local);
> -	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> -		return NULL;
> -
> -	if (pad)
> -		*pad = remote->index;
> -
> -	return media_entity_to_v4l2_subdev(remote->entity);
> -}
> -
> -static int vsp1_video_verify_format(struct vsp1_video *video)
> -{
> -	struct v4l2_subdev_format fmt = {
> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> -	};
> -	struct v4l2_subdev *subdev;
> -	int ret;
> -
> -	subdev = vsp1_video_remote_subdev(&video->pad, &fmt.pad);
> -	if (subdev == NULL)
> -		return -EINVAL;
> -
> -	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> -	if (ret < 0)
> -		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> -
> -	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
> -	    video->rwpf->format.height != fmt.format.height ||
> -	    video->rwpf->format.width != fmt.format.width) {
> -		dev_dbg(video->vsp1->dev,
> -			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
> -			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
> -			video->rwpf->format.height, fmt.format.code,
> -			fmt.format.width, fmt.format.height);
> -		return -EPIPE;
> -	}
> -
> -	return 0;
> -}
> -
>   static int __vsp1_video_try_format(struct vsp1_video *video,
>   				   struct v4l2_pix_format_mplane *pix,
>   				   const struct vsp1_format_info **fmtinfo)
> @@ -991,14 +946,6 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>   
>   	mutex_unlock(&mdev->graph_mutex);
>   
> -	/*
> -	 * Verify that the configured format matches the output of the connected
> -	 * subdev.
> -	 */
> -	ret = vsp1_video_verify_format(video);
> -	if (ret < 0)
> -		goto err_stop;
> -
>   	/* Start the queue. */
>   	ret = vb2_streamon(&video->queue, type);
>   	if (ret < 0)
> @@ -1087,14 +1034,43 @@ static const struct v4l2_file_operations vsp1_video_fops = {
>   
>   static int vsp1_video_link_validate(struct media_link *link)
>   {
> -	/*
> -	 * Ideally, link validation should be implemented here instead of
> -	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
> -	 * manually. That would however break userspace that start one video
> -	 * device before configures formats on other video devices in the
> -	 * pipeline. This operation is just a no-op to silence the warnings
> -	 * from v4l2_subdev_link_validate().
> -	 */
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	struct v4l2_subdev *subdev;
> +	struct media_entity *entity;
> +	struct media_pad *remote;
> +	struct vsp1_video *video;
> +	int ret;
> +
> +	if (is_media_entity_v4l2_video_device(link->source->entity)) {
> +		entity = link->source->entity;
> +		remote = link->sink;
> +	} else {
> +		entity = link->sink->entity;
> +		remote = link->source;
> +	}

This looks a bit odd. So this device can be either a source and a sink?

This made me also wonder about the .link_validate(). It's the only 
media_entity_operations op that does not get the media_entity as a 
parameter. Which here means the driver has to go and "guess" whether it 
is the source or the sink of the given link.

I wonder if there's a reason why .link_validate() doesn't have the 
media_entity parameter?

> +
> +	fmt.pad = remote->index;
> +
> +	subdev = media_entity_to_v4l2_subdev(remote->entity);
> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> +	if (ret < 0)
> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> +
> +	video = to_vsp1_video(media_entity_to_video_device(entity));
> +
> +	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
> +	    video->rwpf->format.height != fmt.format.height ||
> +	    video->rwpf->format.width != fmt.format.width) {
> +		dev_dbg(video->vsp1->dev,
> +			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
> +			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
> +			video->rwpf->format.height, fmt.format.code,
> +			fmt.format.width, fmt.format.height);
> +		return -EPIPE;
> +	}

Why don't we have a common videodev state which could be used to do 
these validations in a common function? =)

Fwiw:
Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

  Tomi

Re: [PATCH v2 7/7] [DNI] media: renesas: vsp1: Validate all links through .link_validate()

[Laurent Pinchart]

Hi Tomi,

On Mon, Aug 26, 2024 at 02:43:47PM +0300, Tomi Valkeinen wrote:
> On 22/08/2024 18:45, Laurent Pinchart wrote:
> > Move validation of the links between video devices and subdevs,
> > performed manually in vsp1_video_streamon(), to the video device
> > .link_validate() handler.
> > 
> > This is how drivers should be implemented, but sadly, doing so for the
> > vsp1 driver could break userspace, introducing a regression. This patch
> > serves as an example to showcase usage of the .link_validate()
> > operation, but should not be merged.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >   .../media/platform/renesas/vsp1/vsp1_video.c  | 98 +++++++------------
> >   1 file changed, 37 insertions(+), 61 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > index e728f9f5160e..14575698bbe7 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > @@ -45,51 +45,6 @@
> >    * Helper functions
> >    */
> >   
> > -static struct v4l2_subdev *
> > -vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
> > -{
> > -	struct media_pad *remote;
> > -
> > -	remote = media_pad_remote_pad_first(local);
> > -	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> > -		return NULL;
> > -
> > -	if (pad)
> > -		*pad = remote->index;
> > -
> > -	return media_entity_to_v4l2_subdev(remote->entity);
> > -}
> > -
> > -static int vsp1_video_verify_format(struct vsp1_video *video)
> > -{
> > -	struct v4l2_subdev_format fmt = {
> > -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > -	};
> > -	struct v4l2_subdev *subdev;
> > -	int ret;
> > -
> > -	subdev = vsp1_video_remote_subdev(&video->pad, &fmt.pad);
> > -	if (subdev == NULL)
> > -		return -EINVAL;
> > -
> > -	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> > -	if (ret < 0)
> > -		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> > -
> > -	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
> > -	    video->rwpf->format.height != fmt.format.height ||
> > -	    video->rwpf->format.width != fmt.format.width) {
> > -		dev_dbg(video->vsp1->dev,
> > -			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
> > -			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
> > -			video->rwpf->format.height, fmt.format.code,
> > -			fmt.format.width, fmt.format.height);
> > -		return -EPIPE;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >   static int __vsp1_video_try_format(struct vsp1_video *video,
> >   				   struct v4l2_pix_format_mplane *pix,
> >   				   const struct vsp1_format_info **fmtinfo)
> > @@ -991,14 +946,6 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
> >   
> >   	mutex_unlock(&mdev->graph_mutex);
> >   
> > -	/*
> > -	 * Verify that the configured format matches the output of the connected
> > -	 * subdev.
> > -	 */
> > -	ret = vsp1_video_verify_format(video);
> > -	if (ret < 0)
> > -		goto err_stop;
> > -
> >   	/* Start the queue. */
> >   	ret = vb2_streamon(&video->queue, type);
> >   	if (ret < 0)
> > @@ -1087,14 +1034,43 @@ static const struct v4l2_file_operations vsp1_video_fops = {
> >   
> >   static int vsp1_video_link_validate(struct media_link *link)
> >   {
> > -	/*
> > -	 * Ideally, link validation should be implemented here instead of
> > -	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
> > -	 * manually. That would however break userspace that start one video
> > -	 * device before configures formats on other video devices in the
> > -	 * pipeline. This operation is just a no-op to silence the warnings
> > -	 * from v4l2_subdev_link_validate().
> > -	 */
> > +	struct v4l2_subdev_format fmt = {
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +	};
> > +	struct v4l2_subdev *subdev;
> > +	struct media_entity *entity;
> > +	struct media_pad *remote;
> > +	struct vsp1_video *video;
> > +	int ret;
> > +
> > +	if (is_media_entity_v4l2_video_device(link->source->entity)) {
> > +		entity = link->source->entity;
> > +		remote = link->sink;
> > +	} else {
> > +		entity = link->sink->entity;
> > +		remote = link->source;
> > +	}
> 
> This looks a bit odd. So this device can be either a source and a sink?

Correct. The VSP has both capture and output video devices, and this
helper function is used for both.

> This made me also wonder about the .link_validate(). It's the only 
> media_entity_operations op that does not get the media_entity as a 
> parameter. Which here means the driver has to go and "guess" whether it 
> is the source or the sink of the given link.
> 
> I wonder if there's a reason why .link_validate() doesn't have the 
> media_entity parameter?

Because it validates a link. Which of the sink or source entity would
you pass to the function ?

> > +
> > +	fmt.pad = remote->index;
> > +
> > +	subdev = media_entity_to_v4l2_subdev(remote->entity);
> > +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> > +	if (ret < 0)
> > +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> > +
> > +	video = to_vsp1_video(media_entity_to_video_device(entity));
> > +
> > +	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
> > +	    video->rwpf->format.height != fmt.format.height ||
> > +	    video->rwpf->format.width != fmt.format.width) {
> > +		dev_dbg(video->vsp1->dev,
> > +			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
> > +			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
> > +			video->rwpf->format.height, fmt.format.code,
> > +			fmt.format.width, fmt.format.height);
> > +		return -EPIPE;
> > +	}
> 
> Why don't we have a common videodev state which could be used to do 
> these validations in a common function? =)

Because you haven't sent patches yet ;-)

But jokes aside, because there's no 1:1 mapping between media bus codes
and pixel formats, so drivers have to validate at least that part.

> Fwiw:
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 7/7] [DNI] media: renesas: vsp1: Validate all links through .link_validate()

[Tomi Valkeinen]

On 26/08/2024 15:18, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Aug 26, 2024 at 02:43:47PM +0300, Tomi Valkeinen wrote:
>> On 22/08/2024 18:45, Laurent Pinchart wrote:
>>> Move validation of the links between video devices and subdevs,
>>> performed manually in vsp1_video_streamon(), to the video device
>>> .link_validate() handler.
>>>
>>> This is how drivers should be implemented, but sadly, doing so for the
>>> vsp1 driver could break userspace, introducing a regression. This patch
>>> serves as an example to showcase usage of the .link_validate()
>>> operation, but should not be merged.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>    .../media/platform/renesas/vsp1/vsp1_video.c  | 98 +++++++------------
>>>    1 file changed, 37 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
>>> index e728f9f5160e..14575698bbe7 100644
>>> --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
>>> +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
>>> @@ -45,51 +45,6 @@
>>>     * Helper functions
>>>     */
>>>    
>>> -static struct v4l2_subdev *
>>> -vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
>>> -{
>>> -	struct media_pad *remote;
>>> -
>>> -	remote = media_pad_remote_pad_first(local);
>>> -	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
>>> -		return NULL;
>>> -
>>> -	if (pad)
>>> -		*pad = remote->index;
>>> -
>>> -	return media_entity_to_v4l2_subdev(remote->entity);
>>> -}
>>> -
>>> -static int vsp1_video_verify_format(struct vsp1_video *video)
>>> -{
>>> -	struct v4l2_subdev_format fmt = {
>>> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>> -	};
>>> -	struct v4l2_subdev *subdev;
>>> -	int ret;
>>> -
>>> -	subdev = vsp1_video_remote_subdev(&video->pad, &fmt.pad);
>>> -	if (subdev == NULL)
>>> -		return -EINVAL;
>>> -
>>> -	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>>> -	if (ret < 0)
>>> -		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
>>> -
>>> -	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
>>> -	    video->rwpf->format.height != fmt.format.height ||
>>> -	    video->rwpf->format.width != fmt.format.width) {
>>> -		dev_dbg(video->vsp1->dev,
>>> -			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
>>> -			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
>>> -			video->rwpf->format.height, fmt.format.code,
>>> -			fmt.format.width, fmt.format.height);
>>> -		return -EPIPE;
>>> -	}
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>    static int __vsp1_video_try_format(struct vsp1_video *video,
>>>    				   struct v4l2_pix_format_mplane *pix,
>>>    				   const struct vsp1_format_info **fmtinfo)
>>> @@ -991,14 +946,6 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>>>    
>>>    	mutex_unlock(&mdev->graph_mutex);
>>>    
>>> -	/*
>>> -	 * Verify that the configured format matches the output of the connected
>>> -	 * subdev.
>>> -	 */
>>> -	ret = vsp1_video_verify_format(video);
>>> -	if (ret < 0)
>>> -		goto err_stop;
>>> -
>>>    	/* Start the queue. */
>>>    	ret = vb2_streamon(&video->queue, type);
>>>    	if (ret < 0)
>>> @@ -1087,14 +1034,43 @@ static const struct v4l2_file_operations vsp1_video_fops = {
>>>    
>>>    static int vsp1_video_link_validate(struct media_link *link)
>>>    {
>>> -	/*
>>> -	 * Ideally, link validation should be implemented here instead of
>>> -	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
>>> -	 * manually. That would however break userspace that start one video
>>> -	 * device before configures formats on other video devices in the
>>> -	 * pipeline. This operation is just a no-op to silence the warnings
>>> -	 * from v4l2_subdev_link_validate().
>>> -	 */
>>> +	struct v4l2_subdev_format fmt = {
>>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>> +	};
>>> +	struct v4l2_subdev *subdev;
>>> +	struct media_entity *entity;
>>> +	struct media_pad *remote;
>>> +	struct vsp1_video *video;
>>> +	int ret;
>>> +
>>> +	if (is_media_entity_v4l2_video_device(link->source->entity)) {
>>> +		entity = link->source->entity;
>>> +		remote = link->sink;
>>> +	} else {
>>> +		entity = link->sink->entity;
>>> +		remote = link->source;
>>> +	}
>>
>> This looks a bit odd. So this device can be either a source and a sink?
> 
> Correct. The VSP has both capture and output video devices, and this
> helper function is used for both.
> 
>> This made me also wonder about the .link_validate(). It's the only
>> media_entity_operations op that does not get the media_entity as a
>> parameter. Which here means the driver has to go and "guess" whether it
>> is the source or the sink of the given link.
>>
>> I wonder if there's a reason why .link_validate() doesn't have the
>> media_entity parameter?
> 
> Because it validates a link. Which of the sink or source entity would
> you pass to the function ?

The one where the op is defined.

  Tomi

>>> +
>>> +	fmt.pad = remote->index;
>>> +
>>> +	subdev = media_entity_to_v4l2_subdev(remote->entity);
>>> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>>> +	if (ret < 0)
>>> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
>>> +
>>> +	video = to_vsp1_video(media_entity_to_video_device(entity));
>>> +
>>> +	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
>>> +	    video->rwpf->format.height != fmt.format.height ||
>>> +	    video->rwpf->format.width != fmt.format.width) {
>>> +		dev_dbg(video->vsp1->dev,
>>> +			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
>>> +			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
>>> +			video->rwpf->format.height, fmt.format.code,
>>> +			fmt.format.width, fmt.format.height);
>>> +		return -EPIPE;
>>> +	}
>>
>> Why don't we have a common videodev state which could be used to do
>> these validations in a common function? =)
> 
> Because you haven't sent patches yet ;-)
> 
> But jokes aside, because there's no 1:1 mapping between media bus codes
> and pixel formats, so drivers have to validate at least that part.
> 
>> Fwiw:
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 

Re: [PATCH v2 7/7] [DNI] media: renesas: vsp1: Validate all links through .link_validate()

[Laurent Pinchart]

On Mon, Aug 26, 2024 at 03:22:02PM +0300, Tomi Valkeinen wrote:
> On 26/08/2024 15:18, Laurent Pinchart wrote:
> > Hi Tomi,
> > 
> > On Mon, Aug 26, 2024 at 02:43:47PM +0300, Tomi Valkeinen wrote:
> >> On 22/08/2024 18:45, Laurent Pinchart wrote:
> >>> Move validation of the links between video devices and subdevs,
> >>> performed manually in vsp1_video_streamon(), to the video device
> >>> .link_validate() handler.
> >>>
> >>> This is how drivers should be implemented, but sadly, doing so for the
> >>> vsp1 driver could break userspace, introducing a regression. This patch
> >>> serves as an example to showcase usage of the .link_validate()
> >>> operation, but should not be merged.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>>    .../media/platform/renesas/vsp1/vsp1_video.c  | 98 +++++++------------
> >>>    1 file changed, 37 insertions(+), 61 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> >>> index e728f9f5160e..14575698bbe7 100644
> >>> --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
> >>> +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> >>> @@ -45,51 +45,6 @@
> >>>     * Helper functions
> >>>     */
> >>>    
> >>> -static struct v4l2_subdev *
> >>> -vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
> >>> -{
> >>> -	struct media_pad *remote;
> >>> -
> >>> -	remote = media_pad_remote_pad_first(local);
> >>> -	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> >>> -		return NULL;
> >>> -
> >>> -	if (pad)
> >>> -		*pad = remote->index;
> >>> -
> >>> -	return media_entity_to_v4l2_subdev(remote->entity);
> >>> -}
> >>> -
> >>> -static int vsp1_video_verify_format(struct vsp1_video *video)
> >>> -{
> >>> -	struct v4l2_subdev_format fmt = {
> >>> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>> -	};
> >>> -	struct v4l2_subdev *subdev;
> >>> -	int ret;
> >>> -
> >>> -	subdev = vsp1_video_remote_subdev(&video->pad, &fmt.pad);
> >>> -	if (subdev == NULL)
> >>> -		return -EINVAL;
> >>> -
> >>> -	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> >>> -	if (ret < 0)
> >>> -		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> >>> -
> >>> -	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
> >>> -	    video->rwpf->format.height != fmt.format.height ||
> >>> -	    video->rwpf->format.width != fmt.format.width) {
> >>> -		dev_dbg(video->vsp1->dev,
> >>> -			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
> >>> -			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
> >>> -			video->rwpf->format.height, fmt.format.code,
> >>> -			fmt.format.width, fmt.format.height);
> >>> -		return -EPIPE;
> >>> -	}
> >>> -
> >>> -	return 0;
> >>> -}
> >>> -
> >>>    static int __vsp1_video_try_format(struct vsp1_video *video,
> >>>    				   struct v4l2_pix_format_mplane *pix,
> >>>    				   const struct vsp1_format_info **fmtinfo)
> >>> @@ -991,14 +946,6 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
> >>>    
> >>>    	mutex_unlock(&mdev->graph_mutex);
> >>>    
> >>> -	/*
> >>> -	 * Verify that the configured format matches the output of the connected
> >>> -	 * subdev.
> >>> -	 */
> >>> -	ret = vsp1_video_verify_format(video);
> >>> -	if (ret < 0)
> >>> -		goto err_stop;
> >>> -
> >>>    	/* Start the queue. */
> >>>    	ret = vb2_streamon(&video->queue, type);
> >>>    	if (ret < 0)
> >>> @@ -1087,14 +1034,43 @@ static const struct v4l2_file_operations vsp1_video_fops = {
> >>>    
> >>>    static int vsp1_video_link_validate(struct media_link *link)
> >>>    {
> >>> -	/*
> >>> -	 * Ideally, link validation should be implemented here instead of
> >>> -	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
> >>> -	 * manually. That would however break userspace that start one video
> >>> -	 * device before configures formats on other video devices in the
> >>> -	 * pipeline. This operation is just a no-op to silence the warnings
> >>> -	 * from v4l2_subdev_link_validate().
> >>> -	 */
> >>> +	struct v4l2_subdev_format fmt = {
> >>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>> +	};
> >>> +	struct v4l2_subdev *subdev;
> >>> +	struct media_entity *entity;
> >>> +	struct media_pad *remote;
> >>> +	struct vsp1_video *video;
> >>> +	int ret;
> >>> +
> >>> +	if (is_media_entity_v4l2_video_device(link->source->entity)) {
> >>> +		entity = link->source->entity;
> >>> +		remote = link->sink;
> >>> +	} else {
> >>> +		entity = link->sink->entity;
> >>> +		remote = link->source;
> >>> +	}
> >>
> >> This looks a bit odd. So this device can be either a source and a sink?
> > 
> > Correct. The VSP has both capture and output video devices, and this
> > helper function is used for both.
> > 
> >> This made me also wonder about the .link_validate(). It's the only
> >> media_entity_operations op that does not get the media_entity as a
> >> parameter. Which here means the driver has to go and "guess" whether it
> >> is the source or the sink of the given link.
> >>
> >> I wonder if there's a reason why .link_validate() doesn't have the
> >> media_entity parameter?
> > 
> > Because it validates a link. Which of the sink or source entity would
> > you pass to the function ?
> 
> The one where the op is defined.

With the exception of links from video output devices to subdevs, it's
always the sink. I don't expect most drivers to use a single validation
function for the capture and output video devices, so I don't think
passing the entity pointer is that crucial. We could however improve
this on top in case it ends up being a common use case.

> >>> +
> >>> +	fmt.pad = remote->index;
> >>> +
> >>> +	subdev = media_entity_to_v4l2_subdev(remote->entity);
> >>> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> >>> +	if (ret < 0)
> >>> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> >>> +
> >>> +	video = to_vsp1_video(media_entity_to_video_device(entity));
> >>> +
> >>> +	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
> >>> +	    video->rwpf->format.height != fmt.format.height ||
> >>> +	    video->rwpf->format.width != fmt.format.width) {
> >>> +		dev_dbg(video->vsp1->dev,
> >>> +			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
> >>> +			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
> >>> +			video->rwpf->format.height, fmt.format.code,
> >>> +			fmt.format.width, fmt.format.height);
> >>> +		return -EPIPE;
> >>> +	}
> >>
> >> Why don't we have a common videodev state which could be used to do
> >> these validations in a common function? =)
> > 
> > Because you haven't sent patches yet ;-)
> > 
> > But jokes aside, because there's no 1:1 mapping between media bus codes
> > and pixel formats, so drivers have to validate at least that part.
> > 
> >> Fwiw:
> >> Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 7/7] [DNI] media: renesas: vsp1: Validate all links through .link_validate()

[Tomi Valkeinen]

On 26/08/2024 15:25, Laurent Pinchart wrote:
> On Mon, Aug 26, 2024 at 03:22:02PM +0300, Tomi Valkeinen wrote:
>> On 26/08/2024 15:18, Laurent Pinchart wrote:
>>> Hi Tomi,
>>>
>>> On Mon, Aug 26, 2024 at 02:43:47PM +0300, Tomi Valkeinen wrote:
>>>> On 22/08/2024 18:45, Laurent Pinchart wrote:
>>>>> Move validation of the links between video devices and subdevs,
>>>>> performed manually in vsp1_video_streamon(), to the video device
>>>>> .link_validate() handler.
>>>>>
>>>>> This is how drivers should be implemented, but sadly, doing so for the
>>>>> vsp1 driver could break userspace, introducing a regression. This patch
>>>>> serves as an example to showcase usage of the .link_validate()
>>>>> operation, but should not be merged.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>>> ---
>>>>>     .../media/platform/renesas/vsp1/vsp1_video.c  | 98 +++++++------------
>>>>>     1 file changed, 37 insertions(+), 61 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
>>>>> index e728f9f5160e..14575698bbe7 100644
>>>>> --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
>>>>> +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
>>>>> @@ -45,51 +45,6 @@
>>>>>      * Helper functions
>>>>>      */
>>>>>     
>>>>> -static struct v4l2_subdev *
>>>>> -vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
>>>>> -{
>>>>> -	struct media_pad *remote;
>>>>> -
>>>>> -	remote = media_pad_remote_pad_first(local);
>>>>> -	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
>>>>> -		return NULL;
>>>>> -
>>>>> -	if (pad)
>>>>> -		*pad = remote->index;
>>>>> -
>>>>> -	return media_entity_to_v4l2_subdev(remote->entity);
>>>>> -}
>>>>> -
>>>>> -static int vsp1_video_verify_format(struct vsp1_video *video)
>>>>> -{
>>>>> -	struct v4l2_subdev_format fmt = {
>>>>> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>>> -	};
>>>>> -	struct v4l2_subdev *subdev;
>>>>> -	int ret;
>>>>> -
>>>>> -	subdev = vsp1_video_remote_subdev(&video->pad, &fmt.pad);
>>>>> -	if (subdev == NULL)
>>>>> -		return -EINVAL;
>>>>> -
>>>>> -	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>>>>> -	if (ret < 0)
>>>>> -		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
>>>>> -
>>>>> -	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
>>>>> -	    video->rwpf->format.height != fmt.format.height ||
>>>>> -	    video->rwpf->format.width != fmt.format.width) {
>>>>> -		dev_dbg(video->vsp1->dev,
>>>>> -			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
>>>>> -			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
>>>>> -			video->rwpf->format.height, fmt.format.code,
>>>>> -			fmt.format.width, fmt.format.height);
>>>>> -		return -EPIPE;
>>>>> -	}
>>>>> -
>>>>> -	return 0;
>>>>> -}
>>>>> -
>>>>>     static int __vsp1_video_try_format(struct vsp1_video *video,
>>>>>     				   struct v4l2_pix_format_mplane *pix,
>>>>>     				   const struct vsp1_format_info **fmtinfo)
>>>>> @@ -991,14 +946,6 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>>>>>     
>>>>>     	mutex_unlock(&mdev->graph_mutex);
>>>>>     
>>>>> -	/*
>>>>> -	 * Verify that the configured format matches the output of the connected
>>>>> -	 * subdev.
>>>>> -	 */
>>>>> -	ret = vsp1_video_verify_format(video);
>>>>> -	if (ret < 0)
>>>>> -		goto err_stop;
>>>>> -
>>>>>     	/* Start the queue. */
>>>>>     	ret = vb2_streamon(&video->queue, type);
>>>>>     	if (ret < 0)
>>>>> @@ -1087,14 +1034,43 @@ static const struct v4l2_file_operations vsp1_video_fops = {
>>>>>     
>>>>>     static int vsp1_video_link_validate(struct media_link *link)
>>>>>     {
>>>>> -	/*
>>>>> -	 * Ideally, link validation should be implemented here instead of
>>>>> -	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
>>>>> -	 * manually. That would however break userspace that start one video
>>>>> -	 * device before configures formats on other video devices in the
>>>>> -	 * pipeline. This operation is just a no-op to silence the warnings
>>>>> -	 * from v4l2_subdev_link_validate().
>>>>> -	 */
>>>>> +	struct v4l2_subdev_format fmt = {
>>>>> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>>> +	};
>>>>> +	struct v4l2_subdev *subdev;
>>>>> +	struct media_entity *entity;
>>>>> +	struct media_pad *remote;
>>>>> +	struct vsp1_video *video;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (is_media_entity_v4l2_video_device(link->source->entity)) {
>>>>> +		entity = link->source->entity;
>>>>> +		remote = link->sink;
>>>>> +	} else {
>>>>> +		entity = link->sink->entity;
>>>>> +		remote = link->source;
>>>>> +	}
>>>>
>>>> This looks a bit odd. So this device can be either a source and a sink?
>>>
>>> Correct. The VSP has both capture and output video devices, and this
>>> helper function is used for both.
>>>
>>>> This made me also wonder about the .link_validate(). It's the only
>>>> media_entity_operations op that does not get the media_entity as a
>>>> parameter. Which here means the driver has to go and "guess" whether it
>>>> is the source or the sink of the given link.
>>>>
>>>> I wonder if there's a reason why .link_validate() doesn't have the
>>>> media_entity parameter?
>>>
>>> Because it validates a link. Which of the sink or source entity would
>>> you pass to the function ?
>>
>> The one where the op is defined.
> 
> With the exception of links from video output devices to subdevs, it's
> always the sink. I don't expect most drivers to use a single validation
> function for the capture and output video devices, so I don't think
> passing the entity pointer is that crucial. We could however improve
> this on top in case it ends up being a common use case.

It's an issue even if the driver only acts as a sink or source, but I 
agree, not crucial at all.

Looking at the patch 1 of this series:

+static int isc_link_validate(struct media_link *link)
  {
+	struct video_device *vdev =
+		media_entity_to_video_device(link->sink->entity);

that should be:

static int isc_link_validate(struct media_entity *ent, struct media_link 
*link)
{
	struct video_device *vdev = media_entity_to_video_device(ent);

In any case, that's a topic for another series =).

  Tomi

[PATCH v2 0/7] media: v4l2: Improve media link validation

[Laurent Pinchart]

Hello,

This patch series improves the link validation helpers to make it easier
for drivers to validate all links in a pipeline.

The vast majority of drivers use the v4l2_subdev_link_validate()
function as their .link_validate() handler for subdevs. This correctly
validates subdev-to-subdev links. For links between subdevs and video
capture devices, a few drivers implement the .link_validate() operation
of their video devices, while others implement manual validation in
their .streamon() handler, or don't implement validation at all. Links
between video output devices are in an even worse state, as the link
validation logic at pipeline start time does not call the
.link_validate() operation on the source side of a link, leaving manual
validation in .streamon() the only option. Adding insult to injury,
v4l2_subdev_link_validate() prints a warning when the link's source is
not a subdev, which forces drivers to implement a manual subdev link
validation handler for subdevs connected to output video nodes.

It turns out that v4l2_subdev_link_validate() is even used in the
.link_validate() implementation of video devices by two drivers. This is
clearly wrong, and is addressed by patches 1/7 to 3/7. Note that patch
3/7 should ideally implement real validation of the link between the
subdev and video capture device, but that requires understanding of the
driver's logic as well as testing, so I have left it out as an exercise
for the driver's maintainer. The patch doesn't introduce any regression.

Patch 4/7 then starts refactoring the v4l2_subdev_link_validate() to
separate the current warning in two categories, with a WARN_ON() for an
issue that indicates a clear driver bug (and does not occur in any
driver in mainline at the moment), and keeping the pr_warn_once() for
other issues that are present in multiple drivers.

Patch 5/7 continues with expanding v4l2_subdev_link_validate() to better
support links from video output devices to subdevs, delegating the
validation to the video output device's .link_validate() operation. This
allows using v4l2_subdev_link_validate() for all subdevs in a driver,
regardless of whether they are connected to other subdevs, video capture
devices or video output devices, and implementing all the link
validation for video devices in their .link_validate() operation.

Patches 6/7 and 7/7 then address the v4l2_subdev_link_validate() warning
for the vsp1 driver. Patch 6/7 silences the warning. This is
unfortunately done with a workaround, as the ideal implementation,
moving all validation for video devices to their .link_validate()
operation as showcased in patch 7/7, breaks operation of the vsp1 unit
test suite. While that is fixable in the test suite, it indicates that
other userspace applications may also break as a result.

To my great sadness, I had to mark patch 7/7 as [DNI]. This does not
make the v4l2_subdev_link_validate() improvements in patch 5/7
pointless, as they are useful for new drivers, as well as drivers that
don't include multiple video devices in a pipeline.

Laurent Pinchart (7):
  media: microchip-isc: Drop v4l2_subdev_link_validate() for video
    devices
  media: sun4i_csi: Implement link validate for sun4i_csi subdev
  media: sun4i_csi: Don't use v4l2_subdev_link_validate() for video
    device
  media: v4l2-subdev: Refactor warnings in v4l2_subdev_link_validate()
  media: v4l2-subdev: Support hybrid links in
    v4l2_subdev_link_validate()
  media: renesas: vsp1: Implement .link_validate() for video devices
  [DNI] media: renesas: vsp1: Validate all links through
    .link_validate()

 .../platform/microchip/microchip-isc-base.c   |  19 +---
 .../media/platform/renesas/vsp1/vsp1_video.c  | 104 +++++++++---------
 .../platform/sunxi/sun4i-csi/sun4i_csi.c      |  12 ++
 drivers/media/v4l2-core/v4l2-subdev.c         |  50 +++++++--
 include/media/v4l2-subdev.h                   |   6 +
 5 files changed, 115 insertions(+), 76 deletions(-)


base-commit: a043ea54bbb975ca9239c69fd17f430488d33522
-- 
Regards,

Laurent Pinchart

[PATCH v2 7/7] [DNI] media: renesas: vsp1: Validate all links through .link_validate()

[Laurent Pinchart]

Move validation of the links between video devices and subdevs,
performed manually in vsp1_video_streamon(), to the video device
.link_validate() handler.

This is how drivers should be implemented, but sadly, doing so for the
vsp1 driver could break userspace, introducing a regression. This patch
serves as an example to showcase usage of the .link_validate()
operation, but should not be merged.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../media/platform/renesas/vsp1/vsp1_video.c  | 98 +++++++------------
 1 file changed, 37 insertions(+), 61 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
index e728f9f5160e..14575698bbe7 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
@@ -45,51 +45,6 @@
  * Helper functions
  */
 
-static struct v4l2_subdev *
-vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
-{
-	struct media_pad *remote;
-
-	remote = media_pad_remote_pad_first(local);
-	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
-		return NULL;
-
-	if (pad)
-		*pad = remote->index;
-
-	return media_entity_to_v4l2_subdev(remote->entity);
-}
-
-static int vsp1_video_verify_format(struct vsp1_video *video)
-{
-	struct v4l2_subdev_format fmt = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-	struct v4l2_subdev *subdev;
-	int ret;
-
-	subdev = vsp1_video_remote_subdev(&video->pad, &fmt.pad);
-	if (subdev == NULL)
-		return -EINVAL;
-
-	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
-	if (ret < 0)
-		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
-
-	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
-	    video->rwpf->format.height != fmt.format.height ||
-	    video->rwpf->format.width != fmt.format.width) {
-		dev_dbg(video->vsp1->dev,
-			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
-			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
-			video->rwpf->format.height, fmt.format.code,
-			fmt.format.width, fmt.format.height);
-		return -EPIPE;
-	}
-
-	return 0;
-}
-
 static int __vsp1_video_try_format(struct vsp1_video *video,
 				   struct v4l2_pix_format_mplane *pix,
 				   const struct vsp1_format_info **fmtinfo)
@@ -991,14 +946,6 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	/*
-	 * Verify that the configured format matches the output of the connected
-	 * subdev.
-	 */
-	ret = vsp1_video_verify_format(video);
-	if (ret < 0)
-		goto err_stop;
-
 	/* Start the queue. */
 	ret = vb2_streamon(&video->queue, type);
 	if (ret < 0)
@@ -1087,14 +1034,43 @@ static const struct v4l2_file_operations vsp1_video_fops = {
 
 static int vsp1_video_link_validate(struct media_link *link)
 {
-	/*
-	 * Ideally, link validation should be implemented here instead of
-	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
-	 * manually. That would however break userspace that start one video
-	 * device before configures formats on other video devices in the
-	 * pipeline. This operation is just a no-op to silence the warnings
-	 * from v4l2_subdev_link_validate().
-	 */
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	struct v4l2_subdev *subdev;
+	struct media_entity *entity;
+	struct media_pad *remote;
+	struct vsp1_video *video;
+	int ret;
+
+	if (is_media_entity_v4l2_video_device(link->source->entity)) {
+		entity = link->source->entity;
+		remote = link->sink;
+	} else {
+		entity = link->sink->entity;
+		remote = link->source;
+	}
+
+	fmt.pad = remote->index;
+
+	subdev = media_entity_to_v4l2_subdev(remote->entity);
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret < 0)
+		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
+
+	video = to_vsp1_video(media_entity_to_video_device(entity));
+
+	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
+	    video->rwpf->format.height != fmt.format.height ||
+	    video->rwpf->format.width != fmt.format.width) {
+		dev_dbg(video->vsp1->dev,
+			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
+			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
+			video->rwpf->format.height, fmt.format.code,
+			fmt.format.width, fmt.format.height);
+		return -EPIPE;
+	}
+
 	return 0;
 }
 
-- 
Regards,

Laurent Pinchart