Hi Sakari,
On Mon, Nov 13, 2023 at 01:53:24PM +0000, Sakari Ailus wrote:
> On Mon, Nov 13, 2023 at 01:37:54PM +0200, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 11:06:34AM +0000, Sakari Ailus wrote:
> > > Hi Laurent,
> > >
> > > Thanks for the patch.
> > >
> > > On Mon, Nov 13, 2023 at 12:17:18PM +0200, 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
> > > > 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
> > >
> > > While some do the validation as part of the streamon callback, it'd be
> > > nicer to move this to the link_validate callback instead: this is what the
> > > callback is for.
> >
> > Is there any driver that does so already ?
>
> Based on a quick look IPU3-CIO2 and vimc do that. I guess there indeed are
> also other drivers besides the vsp driver that do not, on the other hand.
> But this is what I would have expected drivers to do all along: the
> link_validate callback is where you validate the formats across the link
> match, independently of whether the link is between sub-devices or a
> sub-device and something else.
I suppose that could work. It would be a bit cumbersome for drivers
whose subdevs have sink pads connected to other subdevs and sink pads
connected to video devices, but that's not the majority of subdevs, and
probably not the end of the world.
> > > I'd presume not may drivers depend on
> > > v4l2_subdev_link_validate() fail silently on non-subdevices as the issue
> > > hasn't been reported before while the patch that seems to have broken this
> > > was merged in 6.3.
> >
> > At least the VSP1 driver depends on it. The i.MX8MP ISI driver does as
> > well, it got merged in v6.4, so the crash in v6.3 didn't get noticed.
> > I'd be surprised if the omap3isp driver also didn't depend on this
> > behaviour when operating in M2M mode.
> >
> > > Not failing silently in link_validate also ensures the validation gets
> > > done: there have been drivers (more than one) that have simply missed the
> > > link validation due to the issue (non-sub-device entity on one end) being
> > > silently ignored by default.
> >
> > If there's a consensus that subdev to video device link validation need
> > to be done in the .link_validate() operation, then work needs to be done
> > to make that happen. I'd like to see at least one proof of concept
> > implemented, to make sure there are no unforeseen issues. Are you
> > volunteering ? :-)
>
> I'd hope the driver authors to provide fixes: they have the hardware to
> test, too.
I'll give it a try with the VSP1 driver.
> > In the meantime, most drivers (possibly all) validate that link manually
> > outside of the .link_validate() operation, so this is the de facto
> > typical mode of operation. I don't think it can be considered as a
>
> That is not entirely correct. While such drivers exist, these are still a
> subset of all drivers (performing link validation between sub-device and
> video nodes).
It's a subset indeed, but it seems to still be the majority.
> > driver bug until we make a reasonable effort to convert drivers to a new
> > model. It's not fair to just add a pr_warn_once() without having done
> > some of the work needed to move to a new model.
>
> I don't think the model really got changed. The documentation implies this,
> it does not explicitly document what the driver is expected to do. I can
> write a documentation patch for this.
The model got changed in practice, a situation that was previously
allowed and considered valid by reviewers now generates a warning.
> Given that there are surprisingly more than one driver that seem to depend
> on this, at the moment, I'm fine with reverting back to ignoring the error.
> But we should still print a warning to signal the driver should be fixed.
I'd say "moved to the new recommended model" instead of "fixed".
> I can address the omap3isp driver. The IPU6 ISYS driver already uses
> link_validate callback.
Did you get a chance to address omap3isp ? :-)
> Also cc Hans.
>
> > > > 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
> > > > without printing any warning.
> > > >
> > > > 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
