On Mon, Nov 13, 2023 at 03:03:39PM +0200, Tomi Valkeinen wrote:
> On 13/11/2023 12:17, Laurent Pinchart wrote:
> > Before v6.3, the v4l2_subdev_link_validate() helper would ignore links
> > whose source was a video device and sink a subdev. The helper was (and
>
> Didn't it also ignore links where the sink is a video device?
__media_pipeline_start() calls .link_validate() to validate links on the
sink side of the subdev only, so video devices connected to a source pad
don't cause any issue.
> > still is) used by drivers in such cases, in particular for subdevs with
> > multiple sink pads, some connected to video devices and some to other
> > subdevs.
> >
> > Then, commit a6b995ed03ff ("media: subdev: use streams in
> > v4l2_subdev_link_validate()") assumed the entities on the two sides of a
> > link are both subdevs, and caused crashes in drivers that use the helper
> > with subdev sink pads connected to video devices. Commit 55f1ecb11990
> > ("media: v4l: subdev: Make link validation safer"), merged in v6.4,
> > fixed the crash by adding an explicit check with a pr_warn_once(),
> > mentioning a driver bug.
> >
> > Links between a subdev and a video device need to be validated, and
> > v4l2_subdev_link_validate() can't handle that. Drivers typically handle
> > this validation manually at stream start time (either in the .streamon()
> > ioctl handler, or in the .start_streaming() vb2 queue operation),
> > instead of implementing a custom .link_validate() handler. Forbidding
> > usage of v4l2_subdev_link_validate() as the .link_validate() handler
> > would thus force all subdev drivers that mix source links to subdev and
> > video devices to implement a custom .link_validate() handler that
> > returns immediately for the video device links and call
> > v4l2_subdev_link_validate() for subdev links. This would create lots of
> > duplicated code for no real gain. Instead, relax the check in
> > v4l2_ctrl_modify_range() to ignore links from a video device to a subdev
>
> v4l2_ctrl_modify_range? copy-paste error?
Yes that's a nince one, I wonder how I managed to get it wrong :-)
> Should the check also be relaxed wrt. video device as a sink?
See above.
> > without printing any warning.
>
> As Sakari said, it would make sense to use .link_validate() to validate
> all links.
I think I could get convinced to do so. I'll reply to Sakari's other
e-mail in the thread.
> But if this warning is getting printed at the moment, then I think this
> is a valid fix (maybe with the sink side handled too, if needed).
If we decide it's really a driver issue, then I suppose keeping a
pr_warn_once() could help getting things fixed. That's how I noticed the
problem.
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 30 ++++++++++++++++++++-------
> > 1 file changed, 23 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 67d43206ce32..b00be1d57e05 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -1356,15 +1356,31 @@ int v4l2_subdev_link_validate(struct media_link *link)
> > struct v4l2_subdev *source_sd, *sink_sd;
> > struct v4l2_subdev_state *source_state, *sink_state;
> > bool states_locked;
> > + bool is_sink_subdev;
> > + bool is_source_subdev;
> > 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",
> > - link->source->entity->name, link->source->index,
> > - link->sink->entity->name, link->sink->index);
> > + is_sink_subdev = is_media_entity_v4l2_subdev(link->sink->entity);
> > + is_source_subdev = is_media_entity_v4l2_subdev(link->source->entity);
> > +
> > + if (!is_sink_subdev || !is_source_subdev) {
> > + /*
> > + * Do not print the warning if the source is a video device and
> > + * the sink a subdev. This is a valid use case, to allow usage
> > + * of this helper by subdev drivers that have multiple sink
> > + * pads, some connected to video devices and some connected to
> > + * other subdevs. The video device to subdev link is typically
> > + * validated manually by the driver at stream start time in such
> > + * cases.
> > + */
> > + if (!is_sink_subdev ||
> > + !is_media_entity_v4l2_video_device(link->source->entity))
> > + pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> > + !is_sink_subdev ? "sink" : "source",
> > + link->source->entity->name,
> > + link->source->index,
> > + link->sink->entity->name,
> > + link->sink->index);
> > return 0;
> > }
> >
--
Regards,
Laurent Pinchart
