On 28/11/2023 14:30, Sakari Ailus wrote:
> Hi Hans,
>
> Thank you for the review. I somehow missed replying to this earlier, before
> posting v7.
>
> On Thu, Oct 05, 2023 at 01:00:15PM +0200, Hans Verkuil wrote:
>> On 03/10/2023 14:08, Sakari Ailus wrote:
>>> The len_routes field is used to tell the size of the routes array in
>>> struct v4l2_subdev_routing. This way the number of routes returned from
>>> S_ROUTING IOCTL may be larger than the number of routes provided, in case
>>> there are more routes returned by the driver.
>>>
>>> Note that this changes the (now-disabled) UAPI, users must be updated.
>>
>> With "now-disabled" you mean "still disabled", right?
>>
>> So:
>>
>> "Note that this uAPI is still disabled in the code, so this change can
>> safely be done. Anyone who manually patched the code to enable this uAPI
>> must update their code."
>
> I can add that.
>
>>
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>> ---
>>> .../media/v4l/vidioc-subdev-g-routing.rst | 31 ++++++++++++-------
>>> drivers/media/v4l2-core/v4l2-ioctl.c | 4 +--
>>> drivers/media/v4l2-core/v4l2-subdev.c | 6 +++-
>>> include/media/v4l2-subdev.h | 2 ++
>>> include/uapi/linux/v4l2-subdev.h | 8 +++--
>>> 5 files changed, 34 insertions(+), 17 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 72677a280cd6..9a9765ddc316 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
>>> @@ -46,20 +46,26 @@ with the ``VIDIOC_SUBDEV_S_ROUTING`` ioctl, by adding or removing routes and
>>> setting or clearing flags of the ``flags`` field of a
>>> struct :c:type:`v4l2_subdev_route`.
>>>
>>> -All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
>>> -means that the userspace must reconfigure all streams after calling the ioctl
>>> -with e.g. ``VIDIOC_SUBDEV_S_FMT``.
>>> +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is
>>> +called. This means that the userspace must reconfigure all streams after calling
>>> +the ioctl with e.g. ``VIDIOC_SUBDEV_S_FMT``.
>>>
>>> Only subdevices which have both sink and source pads can support routing.
>>>
>>> -When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
>>> -provided ``num_routes`` is not big enough to contain all the available routes
>>> -the subdevice exposes, drivers return the ENOSPC error code and adjust the
>>> -value of the ``num_routes`` field. Application should then reserve enough memory
>>> -for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
>>> +The ``num_routes`` field is used to denote the number of routes set (set by user
>>> +space on ``VIDIOC_SUBDEV_S_ROUTING`` argument) on the routing table as well as
>>> +the number of routes returned back from both IOCTLs. The ``len_routes``
>>> +signifies the number of routes that can fit into the ``routes`` array. The
>>> +userspace shall set ``len_routes`` for both IOCTLs and ``num_routes`` for
>>> +``VIDIOC_SUBDEV_S_ROUTING``.
>>>
>>> -On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the
>>> -``num_routes`` field to reflect the actual number of routes returned.
>>> +On a ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the ``num_routes``
>>> +field to reflect the actual number of routes known by the driver. ``num_routes``
>>> +larger than ``len_routes`` in both IOCTLs. In this ``len_routes`` were returned
>>> +back to the userspace. This is not an error.
>>
>> This paragraph is garbled.
>
> Oops. This intended to read:
>
> On a ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the ``num_routes``
> field to reflect the actual number of routes known by the driver.
> ``num_routes`` larger than ``len_routes`` may be returned by both IOCTLs to
> indicate there are more routes than fits to the ``routes`` array. In this
> case first ``len_routes`` were returned back to the userspace in the
> ``routes`` array. This is not considered as an error.
>
>>
>>> +
>>> +Also ``VIDIOC_SUBDEV_S_ROUTING`` may return more route than the user provided in
>>> +``num_routes`` field due to e.g. hardware properties.
>>>
>>> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>>>
>>> @@ -74,6 +80,9 @@ On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the
>>> - ``which``
>>> - Format to modified, from enum
>>> :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
>>> + * - __u32
>>> + - ``len_routes``
>>> + - The length of the array (as in memory reserved for the array)
>>
>> So is this in bytes or in number of route entries?
>>
>> I think 'len_routes' is a terribly confusing name.
>>
>> How about 'max_num_routes'? Or 'max_route_elems/entries'?
>>
>>> * - struct :c:type:`v4l2_subdev_route`
>>> - ``routes[]``
>>> - Array of struct :c:type:`v4l2_subdev_route` entries
>>> @@ -81,7 +90,7 @@ On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the
>>> - ``num_routes``
>>> - Number of entries of the routes array
>>
>> This is now a very confusing field description.
>>
>> How about:
>>
>> 'max_num_routes': Maximum number of entries that can fit in the routes array
>> 'num_routes': Actual number of entries stored in the routes array.
>>
>>> * - __u32
>>> - - ``reserved``\ [5]
>>> + - ``reserved``\ [11]
>>> - Reserved for future extensions. Applications and drivers must set
>>> the array to zero.
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 6921a72566df..1e3da9d64958 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -3155,13 +3155,13 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>>> case VIDIOC_SUBDEV_S_ROUTING: {
>>> struct v4l2_subdev_routing *routing = parg;
>>>
>>> - if (routing->num_routes > 256)
>>> + if (routing->len_routes > 256)
>>> return -E2BIG;
>>>
>>> *user_ptr = u64_to_user_ptr(routing->routes);
>>> *kernel_ptr = (void **)&routing->routes;
>>> *array_size = sizeof(struct v4l2_subdev_route)
>>> - * routing->num_routes;
>>> + * routing->len_routes;
>>> ret = 1;
>>> break;
>>> }
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index 614ff0031831..bd1e8205913c 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -903,6 +903,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>>> if (routing->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
>>> return -EPERM;
>>>
>>> + if (routing->num_routes > routing->len_routes)
>>> + return -EINVAL;
> 0> > +
>>> memset(routing->reserved, 0, sizeof(routing->reserved));
>>>
>>> for (i = 0; i < routing->num_routes; ++i) {
>>> @@ -929,6 +932,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>>> }
>>>
>>> krouting.num_routes = routing->num_routes;
>>> + krouting.len_routes = routing->len_routes;
>>> krouting.routes = routes;
>>>
>>> return v4l2_subdev_call(sd, pad, set_routing, state,
>>> @@ -949,7 +953,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>>>
>>> krouting = &state->routing;
>>>
>>> - if (routing->num_routes < krouting->num_routes) {
>>> + if (routing->len_routes < krouting->num_routes) {
>>> routing->num_routes = krouting->num_routes;
>>> return -ENOSPC;
>>> }
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index e49e8af2fb52..baaa81a9497e 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -741,12 +741,14 @@ struct v4l2_subdev_stream_configs {
>>> /**
>>> * struct v4l2_subdev_krouting - subdev routing table
>>> *
>>> + * @len_routes: length of routes array, in routes
>>> * @num_routes: number of routes
>>> * @routes: &struct v4l2_subdev_route
>>> *
>>> * This structure contains the routing table for a subdev.
>>> */
>>> struct v4l2_subdev_krouting {
>>> + unsigned int len_routes;
>>> unsigned int num_routes;
>>> struct v4l2_subdev_route *routes;
>>> };
>>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
>>> index 4a195b68f28f..b57fb89caa9e 100644
>>> --- a/include/uapi/linux/v4l2-subdev.h
>>> +++ b/include/uapi/linux/v4l2-subdev.h
>>> @@ -222,15 +222,17 @@ struct v4l2_subdev_route {
>>> * struct v4l2_subdev_routing - Subdev routing information
>>> *
>>> * @which: configuration type (from enum v4l2_subdev_format_whence)
>>> - * @num_routes: the total number of routes in the routes array
>>> + * @len_routes: the length of the routes array, in routes
>>> * @routes: pointer to the routes array
>>> + * @num_routes: the total number of routes in the routes array
>
> I'll change this to reflect there may be more routes than fits to the
> array.
>
>>> * @reserved: drivers and applications must zero this array
>>> */
>>> struct v4l2_subdev_routing {
>>> __u32 which;
>>> - __u32 num_routes;
>>> + __u32 len_routes;
>>> __u64 routes;
>>> - __u32 reserved[6];
>>> + __u32 num_routes;
>>> + __u32 reserved[11];
>>> };
>>>
>>> /*
>>
>> I'm going with a NACK for the 'len_routes' name, it's a really, really
>> bad name.
>>
>> So:
>>
>> Nacked-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>
> That's bit harsh judgement due to a differing opinion on a field name. I
> think len_routes is fine as it's the length of the routes array, not the
> number of entries in it but I'm totally fine changing it.
>
> max_num_routes isn't great either as num_routes can effectively be larger
> than max_num_routes.
>
> How about "size_routes", for instance? Or "max_elems_routes"?
>
I think we should keep the meaning of num_routes as exactly that: the
number of routes that can be stored in the routes array. This is consistent
with the struct media_v2_topology use of the num_ fields.
What you want is to be able to tell the driver how many of the routes
in the routes array should be used for setting routes.
How about adding a field "set_routes" instead? It is ignored by G_ROUTING,
and S_ROUTING will set the first 'set_routes' elements in the 'routes'
array. I'm not sure if set_routes should be set to 0 upon return: does
the ordering of the routes change? Or are the first 'set_routes' elements
the same as what the user put in originally?
If set_routes > num_routes, then return -EINVAL. If set_routes is 0, would
that clear all routing?
With a naming scheme like this it can also be used if we ever implement a
S_TOPOLOGY ioctl.
Regards,
Hans
