On 25/10/2023 23:07, Laurent Pinchart wrote:
> On Mon, Oct 09, 2023 at 01:56:21PM +0300, Tomi Valkeinen wrote:
>> On 03/10/2023 15:08, Sakari Ailus wrote:
>>> On VIDIOC_SUBDEV_[GS]_ROUTING, only return as many routes back to the user
>>> as there's room. Do not consider it an error if more routes existed.
>>> Simply inform the user there are more routes.
>>
>> Inform how? And I agree with Hans here. How about return ENOSPC, but the
>> kernel fills in num_routes to tell the userspace how many there actually
>> are?
>
> For VIDIOC_SUBDEV_G_ROUTING I have no objection. For
> VIDIOC_SUBDEV_S_ROUTING, however, I would prefer the ioctl to succeed if
> the routes can be applied but there's not enough space to return them
> all to the application. The application may not have an interest in
> getting the applied routes back.
For S_ROUTING, do we still want to update num_routes in that case? Even though we
return 0 since we could actually set the routes.
I think it depends a bit on the naming of these fields in v7.
How likely is it that an application would run into this anyway? I suspect a
typical app will get the routes first, then modify it.
It's worth giving this a try, but it depends a bit on how easy it is to
document it. If you need to jump through hoops to try to explain it to an
end user, then perhaps this is overly complicated.
Regards,
Hans
>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>> ---
>>> .../userspace-api/media/v4l/vidioc-subdev-g-routing.rst | 4 ----
>>> drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------
>>> 2 files changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
>>> index ced53ea5f23c..99d3c15fd759 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
>>> @@ -145,10 +145,6 @@ On success 0 is returned, on error -1 and the ``errno`` variable is set
>>> appropriately. The generic error codes are described at the
>>> :ref:`Generic Error Codes <gen-errors>` chapter.
>>>
>>> -ENOSPC
>>> - The application provided ``num_routes`` is not big enough to contain
>>> - all the available routes the subdevice exposes.
>>> -
>>> EINVAL
>>> The sink or source pad identifiers reference a non-existing pad, or reference
>>> pads of different types (ie. the sink_pad identifiers refers to a source pad).
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index 9a34e13dfd96..dd48e7e549fb 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -956,14 +956,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>>>
>>> krouting = &state->routing;
>>>
>>> - if (routing->len_routes < krouting->num_routes) {
>>> - routing->num_routes = krouting->num_routes;
>>> - return -ENOSPC;
>>> - }
>>> -
>>> memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
>>> krouting->routes,
>>> - krouting->num_routes * sizeof(*krouting->routes));
>>> + min(krouting->num_routes, routing->len_routes) *
>>> + sizeof(*krouting->routes));
>>> routing->num_routes = krouting->num_routes;
>>>
>>> return 0;
>
