From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-media@vger.kernel.org, tomi.valkeinen@ideasonboard.com,
bingbu.cao@intel.com, hongju.wang@intel.com, hverkuil@xs4all.nl,
Andrey Konovalov <andrey.konovalov@linaro.org>,
Jacopo Mondi <jacopo.mondi@ideasonboard.com>,
Dmitry Perchanov <dmitry.perchanov@intel.com>,
"Ng, Khai Wen" <khai.wen.ng@intel.com>
Subject: Re: [PATCH v4 19/23] media: v4l: subdev: Add len_routes field to struct v4l2_subdev_routing
Date: Tue, 26 Sep 2023 00:01:00 +0300 [thread overview]
Message-ID: <20230925210100.GA18981@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20230922142239.259425-20-sakari.ailus@linux.intel.com>
Hi Sakari,
Thank you for the patch.
On Fri, Sep 22, 2023 at 05:22:35PM +0300, 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.
This cause a regression in all upstream drivers that call
v4l2_subdev_set_routing(). You need to patch the following files:
drivers/media/i2c/ds90ub913.c
drivers/media/i2c/ds90ub953.c
drivers/media/i2c/ds90ub960.c
drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
It's a bit error-prone though. Consider the following change:
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
index 792f031e032a..c1c9e7018f24 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
@@ -201,6 +201,7 @@ static int mxc_isi_crossbar_init_cfg(struct v4l2_subdev *sd,
route->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE;
}
+ routing.len_routes = xbar->num_sources;
routing.num_routes = xbar->num_sources;
routing.routes = routes;
It's easy to forget to set len_routes :-S I wonder if we could do
better.
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> .../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.
> +
> +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)
> * - 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
> * - __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 cb9ca08bf11c..2042ad4ef5f1 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;
> +
> 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 7c34243ffed9..04597dd3a6e2 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
> * @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];
> };
>
> /*
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2023-09-25 21:00 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-22 14:22 [PATCH v4 00/20] Generic line based metadata support, internal pads Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 01/23] media: mc: Add INTERNAL pad flag Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 02/23] media: uapi: Add generic serial metadata mbus formats Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 03/23] media: uapi: Document which mbus format fields are valid for metadata Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 04/23] media: uapi: Add generic 8-bit metadata format definitions Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 05/23] media: v4l: Support line-based metadata capture Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 06/23] media: uapi: ccs: Add media bus code for MIPI CCS embedded data Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 07/23] media: Documentation: ccs: Document routing Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 08/23] media: Documentation: Additional streams generally don't harm capture Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 09/23] media: Documentation: Document embedded data guidelines for camera sensors Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 10/23] media: Documentation: v4l: Document source routes Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 11/23] media: Documentation: Document S_ROUTING behaviour Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 12/23] media: v4l: subdev: Add helpers for format, crop and compose pointers Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 13/23] media: ccs: Move ccs_validate_csi_data_format up Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 14/23] media: ccs: Support frame descriptors Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 15/23] media: ccs: Add support for embedded data stream Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 16/23] media: Add MIPI CSI-2 generic long packet type definition Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 17/23] media: v4l: subdev: Move G_ROUTING handling below S_ROUTING Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 18/23] media: v4l: subdev: Copy argument back to user also for S_ROUTING Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 19/23] media: v4l: subdev: Add len_routes field to struct v4l2_subdev_routing Sakari Ailus
2023-09-25 21:01 ` Laurent Pinchart [this message]
2023-09-25 21:34 ` Laurent Pinchart
2023-09-26 5:56 ` Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 20/23] media: v4l: subdev: Return routes set using S_ROUTING Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 21/23] media: uapi: Allow a larger number of routes than there's room for Sakari Ailus
2023-09-25 21:32 ` Laurent Pinchart
2023-09-26 5:56 ` Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 22/23] media: v4l: subdev: Add trivial set_routing support Sakari Ailus
2023-09-22 14:22 ` [PATCH v4 23/23] media: uapi: v4l: subdev: Enable streams API Sakari Ailus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230925210100.GA18981@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=andrey.konovalov@linaro.org \
--cc=bingbu.cao@intel.com \
--cc=dmitry.perchanov@intel.com \
--cc=hongju.wang@intel.com \
--cc=hverkuil@xs4all.nl \
--cc=jacopo.mondi@ideasonboard.com \
--cc=khai.wen.ng@intel.com \
--cc=linux-media@vger.kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=tomi.valkeinen@ideasonboard.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox