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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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