Moi,
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.
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.
>
> 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?
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?
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
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?
>
> 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
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.
--
Terveisin,
Sakari Ailus
