mbox series

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

Message ID 20240822153527.25320-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
Headers show
Series media: v4l2: Improve media link validation | expand

Message

Laurent Pinchart Aug. 22, 2024, 3:35 p.m. UTC 
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

Comments

Laurent Pinchart Aug. 22, 2024, 3:46 p.m. UTC | #1 
This patch series was sent to libcamera-devel instead of linux-media.
I've now fixed that mistake, sorry the noise. Please reply to the series
sent to the right mailing list.

On Thu, Aug 22, 2024 at 06:35:20PM +0300, 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.
> 
> 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