| Message ID | 20220301161156.1119557-23-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | v4l: routing and streams support | expand |
Hi Tomasz, On 07/07/2022 12:15, Tomasz Figa wrote: > Hi Tomi, Laurent, > > On Tue, Mar 01, 2022 at 06:11:42PM +0200, Tomi Valkeinen wrote: >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Add support for subdev internal routing. A route is defined as a single >> stream from a sink pad to a source pad. >> >> The userspace can configure the routing via two new ioctls, >> VIDIOC_SUBDEV_G_ROUTING and VIDIOC_SUBDEV_S_ROUTING, and subdevs can >> implement the functionality with v4l2_subdev_pad_ops.set_routing(). > > Thanks for the patch! Please check my comment inline. > >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> >> - Add sink and source streams for multiplexed links >> - Copy the argument back in case of an error. This is needed to let the >> caller know the number of routes. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> >> - Expand and refine documentation. >> - Make the 'routes' pointer a __u64 __user pointer so that a compat32 >> version of the ioctl is not required. >> - Add struct v4l2_subdev_krouting to be used for subdevice operations. >> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> >> - Fix typecasing warnings >> - Check sink & source pad types >> - Add 'which' field >> - Add V4L2_SUBDEV_ROUTE_FL_SOURCE >> - Routing to subdev state >> - Dropped get_routing subdev op >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/v4l2-core/v4l2-ioctl.c | 25 ++++++++- >> drivers/media/v4l2-core/v4l2-subdev.c | 73 +++++++++++++++++++++++++++ >> include/media/v4l2-subdev.h | 22 ++++++++ >> include/uapi/linux/v4l2-subdev.h | 57 +++++++++++++++++++++ >> 4 files changed, 176 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >> index 642cb90f457c..add3b28d446e 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -16,6 +16,7 @@ >> #include <linux/kernel.h> >> #include <linux/version.h> >> >> +#include <linux/v4l2-subdev.h> >> #include <linux/videodev2.h> >> >> #include <media/v4l2-common.h> >> @@ -3093,6 +3094,21 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, >> ret = 1; >> break; >> } >> + >> + case VIDIOC_SUBDEV_G_ROUTING: >> + case VIDIOC_SUBDEV_S_ROUTING: { >> + struct v4l2_subdev_routing *routing = parg; >> + >> + if (routing->num_routes > 256) > > Should we define and document this constant? It's just an arbitrary high number as a sanity check. We don't have any specific reason to limit the number of routes. If we publicize it to the userspace, then the userspace might depend on it. On the other hand, failing mystically when num-routes > 256 is also not so nice (not that we have any drivers that go there, 8 has been the max so far). I think we should just keep it here internally, and try to make sure to increase it if we ever get drivers that might support more routes. >> + return -EINVAL; >> + >> + *user_ptr = u64_to_user_ptr(routing->routes); >> + *kernel_ptr = (void **)&routing->routes; >> + *array_size = sizeof(struct v4l2_subdev_route) >> + * routing->num_routes; >> + ret = 1; >> + break; >> + } >> } >> >> return ret; >> @@ -3356,8 +3372,15 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, >> /* >> * Some ioctls can return an error, but still have valid >> * results that must be returned. >> + * >> + * FIXME: subdev IOCTLS are partially handled here and partially in >> + * v4l2-subdev.c and the 'always_copy' flag can only be set for IOCTLS >> + * defined here as part of the 'v4l2_ioctls' array. As >> + * VIDIOC_SUBDEV_G_ROUTING needs to return results to applications even >> + * in case of failure, but it is not defined here as part of the >> + * 'v4l2_ioctls' array, insert an ad-hoc check to address that. >> */ >> - if (err < 0 && !always_copy) >> + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) >> goto out; >> >> out_array_args: >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 3ad24093abe9..89c97bde4575 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -377,6 +377,10 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh, >> case VIDIOC_SUBDEV_S_SELECTION: >> which = ((struct v4l2_subdev_selection *)arg)->which; >> break; >> + case VIDIOC_SUBDEV_G_ROUTING: >> + case VIDIOC_SUBDEV_S_ROUTING: >> + which = ((struct v4l2_subdev_routing *)arg)->which; >> + break; >> } >> >> return which == V4L2_SUBDEV_FORMAT_TRY ? >> @@ -692,6 +696,74 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, >> case VIDIOC_SUBDEV_QUERYSTD: >> return v4l2_subdev_call(sd, video, querystd, arg); >> >> + case VIDIOC_SUBDEV_G_ROUTING: { >> + struct v4l2_subdev_routing *routing = arg; >> + struct v4l2_subdev_krouting *krouting; >> + >> + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)) >> + return -ENOIOCTLCMD; >> + >> + memset(routing->reserved, 0, sizeof(routing->reserved)); >> + >> + krouting = &state->routing; >> + >> + if (routing->num_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)); >> + routing->num_routes = krouting->num_routes; >> + >> + return 0; >> + } >> + >> + case VIDIOC_SUBDEV_S_ROUTING: { >> + struct v4l2_subdev_routing *routing = arg; >> + struct v4l2_subdev_route *routes = >> + (struct v4l2_subdev_route *)(uintptr_t)routing->routes; >> + struct v4l2_subdev_krouting krouting = {}; >> + unsigned int i; >> + >> + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)) >> + return -ENOIOCTLCMD; >> + >> + if (routing->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev) >> + return -EPERM; >> + >> + memset(routing->reserved, 0, sizeof(routing->reserved)); >> + >> + for (i = 0; i < routing->num_routes; ++i) { >> + const struct v4l2_subdev_route *route = &routes[i]; >> + const struct media_pad *pads = sd->entity.pads; >> + >> + /* Do not check sink pad for source routes */ >> + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE)) { >> + if (route->sink_pad >= sd->entity.num_pads) >> + return -EINVAL; >> + >> + if (!(pads[route->sink_pad].flags & >> + MEDIA_PAD_FL_SINK)) >> + return -EINVAL; >> + } >> + >> + if (route->source_pad >= sd->entity.num_pads) >> + return -EINVAL; >> + >> + if (!(pads[route->source_pad].flags & >> + MEDIA_PAD_FL_SOURCE)) >> + return -EINVAL; >> + } >> + >> + krouting.num_routes = routing->num_routes; >> + krouting.routes = routes; >> + >> + return v4l2_subdev_call(sd, pad, set_routing, state, >> + routing->which, &krouting); >> + } >> + >> default: >> return v4l2_subdev_call(sd, core, ioctl, cmd, arg); >> } >> @@ -979,6 +1051,7 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state) >> >> mutex_destroy(&state->_lock); >> >> + kfree(state->routing.routes); > > Do we have any guarantee that this array was allocated with kmalloc()? > Maybe kvfree() could be more appropriate here? The array is allocated with kmemdup() in v4l2_subdev_set_routing(). Tomi
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 642cb90f457c..add3b28d446e 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -16,6 +16,7 @@ #include <linux/kernel.h> #include <linux/version.h> +#include <linux/v4l2-subdev.h> #include <linux/videodev2.h> #include <media/v4l2-common.h> @@ -3093,6 +3094,21 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, ret = 1; break; } + + case VIDIOC_SUBDEV_G_ROUTING: + case VIDIOC_SUBDEV_S_ROUTING: { + struct v4l2_subdev_routing *routing = parg; + + if (routing->num_routes > 256) + return -EINVAL; + + *user_ptr = u64_to_user_ptr(routing->routes); + *kernel_ptr = (void **)&routing->routes; + *array_size = sizeof(struct v4l2_subdev_route) + * routing->num_routes; + ret = 1; + break; + } } return ret; @@ -3356,8 +3372,15 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, /* * Some ioctls can return an error, but still have valid * results that must be returned. + * + * FIXME: subdev IOCTLS are partially handled here and partially in + * v4l2-subdev.c and the 'always_copy' flag can only be set for IOCTLS + * defined here as part of the 'v4l2_ioctls' array. As + * VIDIOC_SUBDEV_G_ROUTING needs to return results to applications even + * in case of failure, but it is not defined here as part of the + * 'v4l2_ioctls' array, insert an ad-hoc check to address that. */ - if (err < 0 && !always_copy) + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) goto out; out_array_args: diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 3ad24093abe9..89c97bde4575 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -377,6 +377,10 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh, case VIDIOC_SUBDEV_S_SELECTION: which = ((struct v4l2_subdev_selection *)arg)->which; break; + case VIDIOC_SUBDEV_G_ROUTING: + case VIDIOC_SUBDEV_S_ROUTING: + which = ((struct v4l2_subdev_routing *)arg)->which; + break; } return which == V4L2_SUBDEV_FORMAT_TRY ? @@ -692,6 +696,74 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, case VIDIOC_SUBDEV_QUERYSTD: return v4l2_subdev_call(sd, video, querystd, arg); + case VIDIOC_SUBDEV_G_ROUTING: { + struct v4l2_subdev_routing *routing = arg; + struct v4l2_subdev_krouting *krouting; + + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)) + return -ENOIOCTLCMD; + + memset(routing->reserved, 0, sizeof(routing->reserved)); + + krouting = &state->routing; + + if (routing->num_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)); + routing->num_routes = krouting->num_routes; + + return 0; + } + + case VIDIOC_SUBDEV_S_ROUTING: { + struct v4l2_subdev_routing *routing = arg; + struct v4l2_subdev_route *routes = + (struct v4l2_subdev_route *)(uintptr_t)routing->routes; + struct v4l2_subdev_krouting krouting = {}; + unsigned int i; + + if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)) + return -ENOIOCTLCMD; + + if (routing->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev) + return -EPERM; + + memset(routing->reserved, 0, sizeof(routing->reserved)); + + for (i = 0; i < routing->num_routes; ++i) { + const struct v4l2_subdev_route *route = &routes[i]; + const struct media_pad *pads = sd->entity.pads; + + /* Do not check sink pad for source routes */ + if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE)) { + if (route->sink_pad >= sd->entity.num_pads) + return -EINVAL; + + if (!(pads[route->sink_pad].flags & + MEDIA_PAD_FL_SINK)) + return -EINVAL; + } + + if (route->source_pad >= sd->entity.num_pads) + return -EINVAL; + + if (!(pads[route->source_pad].flags & + MEDIA_PAD_FL_SOURCE)) + return -EINVAL; + } + + krouting.num_routes = routing->num_routes; + krouting.routes = routes; + + return v4l2_subdev_call(sd, pad, set_routing, state, + routing->which, &krouting); + } + default: return v4l2_subdev_call(sd, core, ioctl, cmd, arg); } @@ -979,6 +1051,7 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state) mutex_destroy(&state->_lock); + kfree(state->routing.routes); kvfree(state->pads); kfree(state); } diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index ccb4ce9ea6ff..2630bb4c0459 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -697,12 +697,26 @@ struct v4l2_subdev_pad_config { struct v4l2_rect try_compose; }; +/** + * struct v4l2_subdev_krouting - subdev routing table + * + * @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 num_routes; + struct v4l2_subdev_route *routes; +}; + /** * struct v4l2_subdev_state - Used for storing subdev state information. * * @_lock: default for 'lock' * @lock: mutex for the state. May be replaced by the user. * @pads: &struct v4l2_subdev_pad_config array + * @routing: routing table for the subdev * * This structure only needs to be passed to the pad op if the 'which' field * of the main argument is set to %V4L2_SUBDEV_FORMAT_TRY. For @@ -713,6 +727,7 @@ struct v4l2_subdev_state { struct mutex _lock; struct mutex *lock; struct v4l2_subdev_pad_config *pads; + struct v4l2_subdev_krouting routing; }; /** @@ -765,6 +780,9 @@ struct v4l2_subdev_state { * this operation as close as possible to stream on time. The * operation shall fail if the pad index it has been called on * is not valid or in case of unrecoverable failures. + * + * @set_routing: enable or disable data connection routes described in the + * subdevice routing table. */ struct v4l2_subdev_pad_ops { int (*init_cfg)(struct v4l2_subdev *sd, @@ -807,6 +825,10 @@ struct v4l2_subdev_pad_ops { struct v4l2_mbus_frame_desc *fd); int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, struct v4l2_mbus_config *config); + int (*set_routing)(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, + enum v4l2_subdev_format_whence which, + struct v4l2_subdev_krouting *route); }; /** diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index d91ab6f22fa7..1ec3141bf860 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -191,6 +191,61 @@ struct v4l2_subdev_capability { /* The v4l2 sub-device supports multiplexed streams. */ #define V4L2_SUBDEV_CAP_MPLEXED 0x00000002 +/* + * Is the route active? An active route will start when streaming is enabled + * on a video node. + */ +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE _BITUL(0) + +/* + * Is the route immutable, i.e. can it be activated and inactivated? + * Set by the driver. + */ +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE _BITUL(1) + +/* + * Is the route a source endpoint? A source endpoint route refers to a stream + * generated internally by the subdevice (usually a sensor), and thus there + * is no sink-side endpoint for the route. The sink_pad and sink_stream + * fields are unused. + * Set by the driver. + */ +#define V4L2_SUBDEV_ROUTE_FL_SOURCE _BITUL(2) + +/** + * struct v4l2_subdev_route - A route inside a subdev + * + * @sink_pad: the sink pad index + * @sink_stream: the sink stream identifier + * @source_pad: the source pad index + * @source_stream: the source stream identifier + * @flags: route flags V4L2_SUBDEV_ROUTE_FL_* + * @reserved: drivers and applications must zero this array + */ +struct v4l2_subdev_route { + __u32 sink_pad; + __u32 sink_stream; + __u32 source_pad; + __u32 source_stream; + __u32 flags; + __u32 reserved[5]; +}; + +/** + * 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 + * @routes: pointer to the routes array + * @reserved: drivers and applications must zero this array + */ +struct v4l2_subdev_routing { + __u32 which; + __u32 num_routes; + __u64 routes; + __u32 reserved[6]; +}; + /* Backwards compatibility define --- to be removed */ #define v4l2_subdev_edid v4l2_edid @@ -206,6 +261,8 @@ struct v4l2_subdev_capability { #define VIDIOC_SUBDEV_S_CROP _IOWR('V', 60, struct v4l2_subdev_crop) #define VIDIOC_SUBDEV_G_SELECTION _IOWR('V', 61, struct v4l2_subdev_selection) #define VIDIOC_SUBDEV_S_SELECTION _IOWR('V', 62, struct v4l2_subdev_selection) +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) /* The following ioctls are identical to the ioctls in videodev2.h */ #define VIDIOC_SUBDEV_G_STD _IOR('V', 23, v4l2_std_id) #define VIDIOC_SUBDEV_S_STD _IOW('V', 24, v4l2_std_id)