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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi,

On 26/08/2024 15:40, Laurent Pinchart wrote:
> Hello,
> 
> This patch series improves the link validation helpers to make it easier
> for drivers to validate all links in a pipeline.

For the ones still missing my Rb:

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

  Tomi

> 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         |  53 +++++++--
>   include/media/v4l2-subdev.h                   |   6 +
>   5 files changed, 118 insertions(+), 76 deletions(-)
> 
> 
> base-commit: a043ea54bbb975ca9239c69fd17f430488d33522