Re: [PATCH v5 05/16] media: mali-c55: Add Mali-C55 ISP driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux