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.
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?
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.
Tomi
