Hi Laurent,
On Sun, Jun 16, 2024 at 11:38:07PM +0300, Laurent Pinchart wrote:
> On Sun, Jun 09, 2024 at 06:21:52AM +0000, Sakari Ailus wrote:
> > On Thu, Jun 06, 2024 at 10:10:14PM +0300, Tomi Valkeinen wrote:
> > > On 06/06/2024 20:53, Laurent Pinchart wrote:
> > > > > > > > + return -EINVAL;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + active_sink = route->sink_pad;
> > > > > > > > + }
> > > > > > > > + if (active_sink == UINT_MAX) {
> > > > > > > > + dev_err(rzr->mali_c55->dev, "One route has to be active");
> > > > > > > > + return -EINVAL;
> > > > > > > > + }
> > > > > >
> > > > > > The recommended handling of invalid routing is to adjust the routing
> > > > > > table, not to return errors.
> > > > >
> > > > > How should I adjust it ? The error here is due to the fact multiple
> > > > > routes are set as active, which one should I make active ? the first
> > > > > one ? Should I go and reset the flags in the subdev_route for the one
> > > > > that has to be made non-active ?
> > > >
> > > > The same way you would adjust an invalid format, you can pick the route
> > > > you consider should be the default.
> > > >
> > > > I'd like Sakari's and Tomi's opinions on this, as it's a new API and the
> > > > behaviour is still a bit in flux.
> > >
> > > Well... My opinion is that the driver adjusting the given config parameters
> > > (for any ioctl) is awful and should be deprecated. If the user asks for X,
> > > and the driver adjusts it and returns Y, then the user has two options:
> > > fail, because it didn't get X (after possibly laborious field by field
> > > checks), or shrug it's virtual shoulders and accept Y and hope that things
> > > still work even though it wanted X.
> >
> > This is still often the only way to tell what the hardware can do as the
> > limitations in different cases (cropping and scaling for instance) can be
> > arbitrary. The other option is that the user space has to know the hardware
> > capabilities without them being available from the kernel.
>
> For some parameters that make sense (we don't have a try mechanism for
> ISP parameters buffers for instance), but when it comes to configuring a
> pipeline, I think a parameters adjustment model is needed when we don't
> have means to expose constraints in a generic way to userspace. The
> question is in which category routing falls.
>
> > There could be cases of IOCTLs where returning an error if what was
> > requested can't be performed exactly is workable in general, but then again
> > having consistency across IOCTL behaviour is very beneficial as well.
> >
> > If you need something exactly, then I think you should check after the
> > IOCTL that this is what you also got, beyond the IOCTL succeeding.
>
> I do agree with Tomi that this kind of check can be annoying for
> applications. In cases where checking the result would be complex, and
> where there is very little use case for receiving anything but the exact
> configuration you asked for, adjusting the parameters could increase the
> implementation complexity on both the kernel side and userspace side for
> no or very little benefit.
>
> > > But maybe that was an answer to a question you didn't really ask =).
> > >
> > > I think setting it to default routing in case of an error is as fine as any
> > > other "fix" for the routing. It won't work anyway.
> > >
> > > But if the function sets default routing and returns 0 here, why would it
> > > return an error from v4l2_subdev_routing_validate()? Should it just set
> > > default routing in that case too? So should set_routing() ever return an
> > > error, if we can just set the default routing?
>
> That's a good point. I asked myself the same question after sending my
> previous e-mail, and wondered if anyone else would notice too :-)
>
> > S_ROUTING is a bit special as it deals with multiple routes and the user
> > space does have a way to add them incrementally.
> >
> > Perhaps we should document better what the driver is expected to to correct
> > the routes?
>
> We should document the expected behaviour clearly. After agreeing on the
> expected behaviour, that is.
>
> > I'd think routes may be added by the driver (as some of them cannot be
> > disabled for instance) but if a requested route cannot be created, that
> > should probably be an error.
> >
> > I've copied my current (with all the pending patches) documentation here
> > <URL:https://www.retiisi.eu/~sailus/v4l2/tmp/streams-doc/userspace-api/media/v4l/dev-subdev.html#streams-multiplexed-media-pads-and-internal-routing>.
> >
> > The text does not elaborate what exactly a driver could or should do, apart
> > from specifying the condition for EINVAL. I think we should specify this in
>
> I don't see mentions of EINVAL related to streams there, am I missing
> something ?
>
> > greater detail. My original thought wws the adjustment would be done by
> > adding static routes omitted by the caller, not trying to come up with e.g.
> > valid pad/stream pairs when user provided invalid ones.
> >
> > Could this correction functionality be limited to returning static routes?
>
> That would make userspace a tad simpler, and wouldn't be hard to do in
> the kernel, but I wonder if departing from the rule that invalid routing
> tables result in an error is worth it for such a small gain.
I'm just referring to our previous decision on the matter. :-)
Of course an application can do G_ROUTING, toggle the routes it needs to
and call S_ROUTING again, in order to be (fairly) certain it'll succeed.
Say, if an application wants to enable an embedded data route, then it'll
be required to supply the route for the image data as well, even if there's
no configuration that could be made for that route.
I'm thinking of fairly generic code here, if a device requires special
routing setup, it'll need the user space to be aware of it.
>
> > > In the VIDIOC_SUBDEV_S_ROUTING doc we do list some cases where EINVAL or
> > > E2BIG is returned. But only a few, and I think
> > > v4l2_subdev_routing_validate() will return errors for many other cases too.
> > >
> > > For what it's worth, the drivers I have written just return an error. It's
> > > simple for the driver and the user and works. If the consensus is that the
> > > drivers should instead set the default routing, or somehow mangle the given
> > > routing to an acceptable form, I can update those drivers accordingly.
> > >
> > > But we probably need to update the docs too to be a bit more clear what
> > > VIDIOC_SUBDEV_S_ROUTING will do (although are the other ioctls any
> > > clearer?).
> > >
> > > All that said, I think it's still a bit case-by-case. I don't think the
> > > drivers should always return an error if they get a routing table that's not
> > > 100% perfect. E.g. if a device supports two static routes, but the second
> > > one can be enabled or disabled, the driver should still accept a routing
> > > table from the user with only the first route present. Etc.
> > >
> > > For the specific case in this patch... I'd prefer returning an error, or if
> > > that's not ok, set default routing.
> >
> > Not modifying the routing table is another option as well but it may
> > require separating validating user-provided routes and applying the routes
>
> I'm not sure to follow you here. By not modifying the routing table, do
> you mean returning an error ? Why would that require separation of
> validation and configuration ?
If a driver has already made changes to its routing table, it's a bad idea
to return an error to the user. In this case changes shouldn't be made.
>
> > to the sub-device state. The default could be useful in principle, too, for
> > routing-unaware applications but they won't be calling S_ROUTING anyway.
>
--
Kind regards,
Sakari Ailus
