From 530779157c06d64721b81d60f7ae2715cdf338d2 Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Fri, 3 Mar 2023 16:52:49 +0100 Subject: media: subdev: Fix validation state lockdep issue The new subdev state code has a possible deadlock scenario during link validation when the pipeline contains subdevs that support state and that do not support state. The current code locks the states of the subdevs on both ends of the link when starting the link validation, locking the sink side first, then the source. If either (or both) of the subdevs does not support state, nothing is done for that subdev at this point, and instead the locking is handled the old way, i.e. the subdev's ops do the locking internally. The issue arises when the sink doesn't support state, but source does, so the validation code locks the source for the duration of the validation, and then the sink is locked only when the get_fmt op is called. So lockdep sees the source locked first, then the sink. Later, when the streaming is started, the sink's s_stream op is called, which probably takes the subdev's lock. The op then calls the source's s_stream, which takes the source's lock. So, the sink is locked first, then the source. Note that link validation and stream starting is not done at the same time, so an actual deadlock should never happen. However, it's still a clear bug. Fix this by locking the subdev states only if both subdevs support state. In other words, we have two scenarios: 1. Both subdevs support state. Lock sink first, then source, and keep the locks while validating the link. 2. At least one of the subdevs do not support state. Take the lock only for the duration of the operation (get_fmt or looking at the routing), and release after the op is done. Obviously 1. is better, as we have a more consistent view of the states of the subdevs during validation. 2. is how it has been so far, so it's no worse than this used to be. Signed-off-by: Tomi Valkeinen Signed-off-by: Sakari Ailus Signed-off-by: Mauro Carvalho Chehab --- drivers/media/v4l2-core/v4l2-subdev.c | 82 ++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 30 deletions(-) (limited to 'drivers/media/v4l2-core/v4l2-subdev.c') diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index b10045c02f43..03dc5f68ffad 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1069,32 +1069,45 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default); static int v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream, - struct v4l2_subdev_format *fmt) + struct v4l2_subdev_format *fmt, + bool states_locked) { - if (is_media_entity_v4l2_subdev(pad->entity)) { - struct v4l2_subdev *sd = - media_entity_to_v4l2_subdev(pad->entity); + struct v4l2_subdev_state *state; + struct v4l2_subdev *sd; + int ret; - fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE; - fmt->pad = pad->index; - fmt->stream = stream; + if (!is_media_entity_v4l2_subdev(pad->entity)) { + WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L, + "Driver bug! Wrong media entity type 0x%08x, entity %s\n", + pad->entity->function, pad->entity->name); - return v4l2_subdev_call(sd, pad, get_fmt, - v4l2_subdev_get_locked_active_state(sd), - fmt); + return -EINVAL; } - WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L, - "Driver bug! Wrong media entity type 0x%08x, entity %s\n", - pad->entity->function, pad->entity->name); + sd = media_entity_to_v4l2_subdev(pad->entity); - return -EINVAL; + fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE; + fmt->pad = pad->index; + fmt->stream = stream; + + if (states_locked) + state = v4l2_subdev_get_locked_active_state(sd); + else + state = v4l2_subdev_lock_and_get_active_state(sd); + + ret = v4l2_subdev_call(sd, pad, get_fmt, state, fmt); + + if (!states_locked && state) + v4l2_subdev_unlock_state(state); + + return ret; } #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) static void __v4l2_link_validate_get_streams(struct media_pad *pad, - u64 *streams_mask) + u64 *streams_mask, + bool states_locked) { struct v4l2_subdev_route *route; struct v4l2_subdev_state *state; @@ -1104,7 +1117,11 @@ static void __v4l2_link_validate_get_streams(struct media_pad *pad, *streams_mask = 0; - state = v4l2_subdev_get_locked_active_state(subdev); + if (states_locked) + state = v4l2_subdev_get_locked_active_state(subdev); + else + state = v4l2_subdev_lock_and_get_active_state(subdev); + if (WARN_ON(!state)) return; @@ -1125,12 +1142,16 @@ static void __v4l2_link_validate_get_streams(struct media_pad *pad, *streams_mask |= BIT_ULL(route_stream); } + + if (!states_locked) + v4l2_subdev_unlock_state(state); } #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */ static void v4l2_link_validate_get_streams(struct media_pad *pad, - u64 *streams_mask) + u64 *streams_mask, + bool states_locked) { struct v4l2_subdev *subdev = media_entity_to_v4l2_subdev(pad->entity); @@ -1141,14 +1162,14 @@ static void v4l2_link_validate_get_streams(struct media_pad *pad, } #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) - __v4l2_link_validate_get_streams(pad, streams_mask); + __v4l2_link_validate_get_streams(pad, streams_mask, states_locked); #else /* This shouldn't happen */ *streams_mask = 0; #endif } -static int v4l2_subdev_link_validate_locked(struct media_link *link) +static int v4l2_subdev_link_validate_locked(struct media_link *link, bool states_locked) { struct v4l2_subdev *sink_subdev = media_entity_to_v4l2_subdev(link->sink->entity); @@ -1163,8 +1184,8 @@ static int v4l2_subdev_link_validate_locked(struct media_link *link) link->source->entity->name, link->source->index, link->sink->entity->name, link->sink->index); - v4l2_link_validate_get_streams(link->source, &source_streams_mask); - v4l2_link_validate_get_streams(link->sink, &sink_streams_mask); + v4l2_link_validate_get_streams(link->source, &source_streams_mask, states_locked); + v4l2_link_validate_get_streams(link->sink, &sink_streams_mask, states_locked); /* * It is ok to have more source streams than sink streams as extra @@ -1192,7 +1213,7 @@ static int v4l2_subdev_link_validate_locked(struct media_link *link) link->sink->entity->name, link->sink->index, stream); ret = v4l2_subdev_link_validate_get_format(link->source, stream, - &source_fmt); + &source_fmt, states_locked); if (ret < 0) { dev_dbg(dev, "Failed to get format for \"%s\":%u:%u (but that's ok)\n", @@ -1202,7 +1223,7 @@ static int v4l2_subdev_link_validate_locked(struct media_link *link) } ret = v4l2_subdev_link_validate_get_format(link->sink, stream, - &sink_fmt); + &sink_fmt, states_locked); if (ret < 0) { dev_dbg(dev, "Failed to get format for \"%s\":%u:%u (but that's ok)\n", @@ -1234,6 +1255,7 @@ int v4l2_subdev_link_validate(struct media_link *link) { struct v4l2_subdev *source_sd, *sink_sd; struct v4l2_subdev_state *source_state, *sink_state; + bool states_locked; int ret; sink_sd = media_entity_to_v4l2_subdev(link->sink->entity); @@ -1242,19 +1264,19 @@ int v4l2_subdev_link_validate(struct media_link *link) sink_state = v4l2_subdev_get_unlocked_active_state(sink_sd); source_state = v4l2_subdev_get_unlocked_active_state(source_sd); - if (sink_state) - v4l2_subdev_lock_state(sink_state); + states_locked = sink_state && source_state; - if (source_state) + if (states_locked) { + v4l2_subdev_lock_state(sink_state); v4l2_subdev_lock_state(source_state); + } - ret = v4l2_subdev_link_validate_locked(link); + ret = v4l2_subdev_link_validate_locked(link, states_locked); - if (sink_state) + if (states_locked) { v4l2_subdev_unlock_state(sink_state); - - if (source_state) v4l2_subdev_unlock_state(source_state); + } return ret; } -- cgit v1.2.3 From 55f1ecb1199000932cf82e357841cc7498ac904f Mon Sep 17 00:00:00 2001 From: Sakari Ailus Date: Fri, 24 Feb 2023 16:00:46 +0100 Subject: media: v4l: subdev: Make link validation safer Link validation currently accesses invalid pointers if the link passed to it is not between two sub-devices. This is of course a driver bug. Ignore the error but print a warning message, as this is how it used to work previously. Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()") Reported-by: Hans de Goede Signed-off-by: Sakari Ailus Tested-by: Hans de Goede Reviewed-by: Tomi Valkeinen Reviewed-by: Laurent Pinchart Signed-off-by: Mauro Carvalho Chehab --- drivers/media/v4l2-core/v4l2-subdev.c | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'drivers/media/v4l2-core/v4l2-subdev.c') diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 03dc5f68ffad..c7154a7f696a 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1258,6 +1258,16 @@ int v4l2_subdev_link_validate(struct media_link *link) bool states_locked; int ret; + if (!is_media_entity_v4l2_subdev(link->sink->entity) || + !is_media_entity_v4l2_subdev(link->source->entity)) { + pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n", + !is_media_entity_v4l2_subdev(link->sink->entity) ? + "sink" : "source", + link->source->entity->name, link->source->index, + link->sink->entity->name, link->sink->index); + return 0; + } + sink_sd = media_entity_to_v4l2_subdev(link->sink->entity); source_sd = media_entity_to_v4l2_subdev(link->source->entity); -- cgit v1.2.3 From a50ee4afc77e4df14bc146e03b3fa28daeec14a9 Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Thu, 2 Mar 2023 10:57:46 +0100 Subject: media: subdev: Use 'shall' instead of 'may' in route validation Route validation docs use the word 'may'. Change that to 'shall' for emphasis. Signed-off-by: Tomi Valkeinen Reviewed-by: Jacopo Mondi Signed-off-by: Sakari Ailus Signed-off-by: Mauro Carvalho Chehab --- drivers/media/v4l2-core/v4l2-subdev.c | 2 +- include/media/v4l2-subdev.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/media/v4l2-core/v4l2-subdev.c') diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index c7154a7f696a..4c309207c8bd 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1738,7 +1738,7 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, /* * V4L2_SUBDEV_ROUTING_NO_STREAM_MIX: Streams on the same pad - * may not be routed to streams on different pads. + * shall not be routed to streams on different pads. */ if (disallow & V4L2_SUBDEV_ROUTING_NO_STREAM_MIX) { if (remote_pads[route->sink_pad] != U32_MAX && diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 7245887ef002..f6acb055579a 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -1642,13 +1642,13 @@ u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, * enum v4l2_subdev_routing_restriction - Subdevice internal routing restrictions * * @V4L2_SUBDEV_ROUTING_NO_1_TO_N: - * an input stream may not be routed to multiple output streams (stream + * an input stream shall not be routed to multiple output streams (stream * duplication) * @V4L2_SUBDEV_ROUTING_NO_N_TO_1: - * multiple input streams may not be routed to the same output stream + * multiple input streams shall not be routed to the same output stream * (stream merging) * @V4L2_SUBDEV_ROUTING_NO_STREAM_MIX: - * streams on the same pad may not be routed to streams on different pads + * streams on the same pad shall not be routed to streams on different pads * @V4L2_SUBDEV_ROUTING_ONLY_1_TO_1: * only non-overlapping 1-to-1 stream routing is allowed (a combination of * @V4L2_SUBDEV_ROUTING_NO_1_TO_N and @V4L2_SUBDEV_ROUTING_NO_N_TO_1) -- cgit v1.2.3 From 698a619a04bee1359358d5cba552f008709b79bc Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Thu, 2 Mar 2023 10:57:47 +0100 Subject: media: subdev: Split V4L2_SUBDEV_ROUTING_NO_STREAM_MIX V4L2_SUBDEV_ROUTING_NO_STREAM_MIX routing validation flag means that all routes from a sink pad must go to the same source pad and all routes going to the same source pad must originate from the same sink pad. This does not cover all use cases. For example, if a device routes all streams from a single sink pad to any of the source pads, but streams from multiple sink pads can go to the same source pad, the current flag is too restrictive. Split the flag into two parts, V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX and V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX, which add the restriction only on one side of the device. Together they mean the same as V4L2_SUBDEV_ROUTING_NO_STREAM_MIX. Signed-off-by: Tomi Valkeinen Reviewed-by: Andy Shevchenko Reviewed-by: Jacopo Mondi Signed-off-by: Sakari Ailus Signed-off-by: Mauro Carvalho Chehab --- drivers/media/v4l2-core/v4l2-subdev.c | 15 +++++++++++---- include/media/v4l2-subdev.h | 17 ++++++++++++++--- 2 files changed, 25 insertions(+), 7 deletions(-) (limited to 'drivers/media/v4l2-core/v4l2-subdev.c') diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 4c309207c8bd..b376c47b083d 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1737,10 +1737,10 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, } /* - * V4L2_SUBDEV_ROUTING_NO_STREAM_MIX: Streams on the same pad - * shall not be routed to streams on different pads. + * V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX: all streams from a + * sink pad must be routed to a single source pad. */ - if (disallow & V4L2_SUBDEV_ROUTING_NO_STREAM_MIX) { + if (disallow & V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX) { if (remote_pads[route->sink_pad] != U32_MAX && remote_pads[route->sink_pad] != route->source_pad) { dev_dbg(sd->dev, @@ -1749,6 +1749,14 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, goto out; } + remote_pads[route->sink_pad] = route->source_pad; + } + + /* + * V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX: all streams on a + * source pad must originate from a single sink pad. + */ + if (disallow & V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX) { if (remote_pads[route->source_pad] != U32_MAX && remote_pads[route->source_pad] != route->sink_pad) { dev_dbg(sd->dev, @@ -1757,7 +1765,6 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, goto out; } - remote_pads[route->sink_pad] = route->source_pad; remote_pads[route->source_pad] = route->sink_pad; } diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index f6acb055579a..f921d8a7898e 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -1647,19 +1647,30 @@ u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, * @V4L2_SUBDEV_ROUTING_NO_N_TO_1: * multiple input streams shall not be routed to the same output stream * (stream merging) - * @V4L2_SUBDEV_ROUTING_NO_STREAM_MIX: - * streams on the same pad shall not be routed to streams on different pads + * @V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX: + * all streams from a sink pad must be routed to a single source pad + * @V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX: + * all streams on a source pad must originate from a single sink pad * @V4L2_SUBDEV_ROUTING_ONLY_1_TO_1: * only non-overlapping 1-to-1 stream routing is allowed (a combination of * @V4L2_SUBDEV_ROUTING_NO_1_TO_N and @V4L2_SUBDEV_ROUTING_NO_N_TO_1) + * @V4L2_SUBDEV_ROUTING_NO_STREAM_MIX: + * all streams from a sink pad must be routed to a single source pad, and + * that source pad shall not get routes from any other sink pad + * (a combination of @V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX and + * @V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX) */ enum v4l2_subdev_routing_restriction { V4L2_SUBDEV_ROUTING_NO_1_TO_N = BIT(0), V4L2_SUBDEV_ROUTING_NO_N_TO_1 = BIT(1), - V4L2_SUBDEV_ROUTING_NO_STREAM_MIX = BIT(2), + V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX = BIT(2), + V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX = BIT(3), V4L2_SUBDEV_ROUTING_ONLY_1_TO_1 = V4L2_SUBDEV_ROUTING_NO_1_TO_N | V4L2_SUBDEV_ROUTING_NO_N_TO_1, + V4L2_SUBDEV_ROUTING_NO_STREAM_MIX = + V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX | + V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX, }; /** -- cgit v1.2.3 From a1299df6718b718256fd9bf6d7f9bed44c826e61 Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Thu, 2 Mar 2023 10:57:48 +0100 Subject: media: subdev: Add V4L2_SUBDEV_ROUTING_NO_MULTIPLEXING A common case with subdev routing is that on the subdevice just before the DMA engines (video nodes), no multiplexing is allowed on the source pads, as the DMA engine can only handle a single stream. In some other situations one might also want to do the same check on the sink side. Add new routing validation flags to check these: V4L2_SUBDEV_ROUTING_NO_SINK_MULTIPLEXING and V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING. Signed-off-by: Tomi Valkeinen Reviewed-by: Jacopo Mondi Signed-off-by: Sakari Ailus Signed-off-by: Mauro Carvalho Chehab --- drivers/media/v4l2-core/v4l2-subdev.c | 36 ++++++++++++++++++++++++++++++++--- include/media/v4l2-subdev.h | 11 +++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) (limited to 'drivers/media/v4l2-core/v4l2-subdev.c') diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index b376c47b083d..bfb102f2121a 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1708,7 +1708,8 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, unsigned int i, j; int ret = -EINVAL; - if (disallow & V4L2_SUBDEV_ROUTING_NO_STREAM_MIX) { + if (disallow & (V4L2_SUBDEV_ROUTING_NO_STREAM_MIX | + V4L2_SUBDEV_ROUTING_NO_MULTIPLEXING)) { remote_pads = kcalloc(sd->entity.num_pads, sizeof(*remote_pads), GFP_KERNEL); if (!remote_pads) @@ -1748,8 +1749,6 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, i, "sink"); goto out; } - - remote_pads[route->sink_pad] = route->source_pad; } /* @@ -1764,7 +1763,38 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd, i, "source"); goto out; } + } + + /* + * V4L2_SUBDEV_ROUTING_NO_SINK_MULTIPLEXING: Pads on the sink + * side can not do stream multiplexing, i.e. there can be only + * a single stream in a sink pad. + */ + if (disallow & V4L2_SUBDEV_ROUTING_NO_SINK_MULTIPLEXING) { + if (remote_pads[route->sink_pad] != U32_MAX) { + dev_dbg(sd->dev, + "route %u attempts to multiplex on %s pad %u\n", + i, "sink", route->sink_pad); + goto out; + } + } + /* + * V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING: Pads on the + * source side can not do stream multiplexing, i.e. there can + * be only a single stream in a source pad. + */ + if (disallow & V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING) { + if (remote_pads[route->source_pad] != U32_MAX) { + dev_dbg(sd->dev, + "route %u attempts to multiplex on %s pad %u\n", + i, "source", route->source_pad); + goto out; + } + } + + if (remote_pads) { + remote_pads[route->sink_pad] = route->source_pad; remote_pads[route->source_pad] = route->sink_pad; } diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index f921d8a7898e..1d2877700cdb 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -1651,6 +1651,10 @@ u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, * all streams from a sink pad must be routed to a single source pad * @V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX: * all streams on a source pad must originate from a single sink pad + * @V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING: + * source pads shall not contain multiplexed streams + * @V4L2_SUBDEV_ROUTING_NO_SINK_MULTIPLEXING: + * sink pads shall not contain multiplexed streams * @V4L2_SUBDEV_ROUTING_ONLY_1_TO_1: * only non-overlapping 1-to-1 stream routing is allowed (a combination of * @V4L2_SUBDEV_ROUTING_NO_1_TO_N and @V4L2_SUBDEV_ROUTING_NO_N_TO_1) @@ -1659,18 +1663,25 @@ u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state *state, * that source pad shall not get routes from any other sink pad * (a combination of @V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX and * @V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX) + * @V4L2_SUBDEV_ROUTING_NO_MULTIPLEXING: + * no multiplexed streams allowed on either source or sink sides. */ enum v4l2_subdev_routing_restriction { V4L2_SUBDEV_ROUTING_NO_1_TO_N = BIT(0), V4L2_SUBDEV_ROUTING_NO_N_TO_1 = BIT(1), V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX = BIT(2), V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX = BIT(3), + V4L2_SUBDEV_ROUTING_NO_SINK_MULTIPLEXING = BIT(4), + V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING = BIT(5), V4L2_SUBDEV_ROUTING_ONLY_1_TO_1 = V4L2_SUBDEV_ROUTING_NO_1_TO_N | V4L2_SUBDEV_ROUTING_NO_N_TO_1, V4L2_SUBDEV_ROUTING_NO_STREAM_MIX = V4L2_SUBDEV_ROUTING_NO_SINK_STREAM_MIX | V4L2_SUBDEV_ROUTING_NO_SOURCE_STREAM_MIX, + V4L2_SUBDEV_ROUTING_NO_MULTIPLEXING = + V4L2_SUBDEV_ROUTING_NO_SINK_MULTIPLEXING | + V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING, }; /** -- cgit v1.2.3 From f57fa2959244026ea5e885249e6c55fe3f133bd7 Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen Date: Thu, 23 Mar 2023 13:58:35 +0000 Subject: media: v4l2-subdev: Add new ioctl for client capabilities Add new ioctls to set and get subdev client capabilities. Client in this context means the userspace application which opens the subdev device node. The client capabilities are stored in the file handle of the opened subdev device node, and the client must set the capabilities for each opened subdev. For now we only add a single flag, V4L2_SUBDEV_CLIENT_CAP_STREAMS, which indicates that the client is streams-aware. The reason for needing such a flag is as follows: Many structs passed via ioctls, e.g. struct v4l2_subdev_format, contain reserved fields (usually a single array field). These reserved fields can be used to extend the ioctl. The userspace is required to zero the reserved fields. We recently added a new 'stream' field to many of these structs, and the space for the field was taken from these reserved arrays. The assumption was that these new 'stream' fields are always initialized to zero if the userspace does not use them. This was a mistake, as, as mentioned above, the userspace is required to zero the _reserved_ fields. In other words, there is no requirement to zero this new stream field, and if the userspace doesn't use the field (which is the case for all userspace applications at the moment), the field may contain random data. This shows that the way the reserved fields are defined in v4l2 is, in my opinion, somewhat broken, but there is nothing to do about that. To fix this issue we need a way for the userspace to tell the kernel that the userspace has indeed set the 'stream' field, and it's fine for the kernel to access it. This is achieved with the new ioctl, which the userspace should usually use right after opening the subdev device node. Signed-off-by: Tomi Valkeinen Reviewed-by: Laurent Pinchart Signed-off-by: Sakari Ailus Signed-off-by: Mauro Carvalho Chehab --- .../userspace-api/media/v4l/user-func.rst | 1 + .../media/v4l/vidioc-subdev-g-client-cap.rst | 83 ++++++++++++++++++++++ drivers/media/v4l2-core/v4l2-subdev.c | 63 ++++++++++++++++ include/media/v4l2-subdev.h | 1 + include/uapi/linux/v4l2-subdev.h | 21 ++++++ 5 files changed, 169 insertions(+) create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst (limited to 'drivers/media/v4l2-core/v4l2-subdev.c') diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst index 228c1521f190..15ff0bf7bbe6 100644 --- a/Documentation/userspace-api/media/v4l/user-func.rst +++ b/Documentation/userspace-api/media/v4l/user-func.rst @@ -72,6 +72,7 @@ Function Reference vidioc-subdev-g-frame-interval vidioc-subdev-g-routing vidioc-subdev-g-selection + vidioc-subdev-g-client-cap vidioc-subdev-querycap vidioc-subscribe-event func-mmap diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst new file mode 100644 index 000000000000..20f12a1cc0f7 --- /dev/null +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-client-cap.rst @@ -0,0 +1,83 @@ +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later +.. c:namespace:: V4L + +.. _VIDIOC_SUBDEV_G_CLIENT_CAP: + +************************************************************ +ioctl VIDIOC_SUBDEV_G_CLIENT_CAP, VIDIOC_SUBDEV_S_CLIENT_CAP +************************************************************ + +Name +==== + +VIDIOC_SUBDEV_G_CLIENT_CAP - VIDIOC_SUBDEV_S_CLIENT_CAP - Get or set client +capabilities. + +Synopsis +======== + +.. c:macro:: VIDIOC_SUBDEV_G_CLIENT_CAP + +``int ioctl(int fd, VIDIOC_SUBDEV_G_CLIENT_CAP, struct v4l2_subdev_client_capability *argp)`` + +.. c:macro:: VIDIOC_SUBDEV_S_CLIENT_CAP + +``int ioctl(int fd, VIDIOC_SUBDEV_S_CLIENT_CAP, struct v4l2_subdev_client_capability *argp)`` + +Arguments +========= + +``fd`` + File descriptor returned by :ref:`open() `. + +``argp`` + Pointer to struct :c:type:`v4l2_subdev_client_capability`. + +Description +=========== + +These ioctls are used to get and set the client (the application using the +subdevice ioctls) capabilities. The client capabilities are stored in the file +handle of the opened subdev device node, and the client must set the +capabilities for each opened subdev separately. + +By default no client capabilities are set when a subdev device node is opened. + +The purpose of the client capabilities are to inform the kernel of the behavior +of the client, mainly related to maintaining compatibility with different +kernel and userspace versions. + +The ``VIDIOC_SUBDEV_G_CLIENT_CAP`` ioctl returns the current client capabilities +associated with the file handle ``fd``. + +The ``VIDIOC_SUBDEV_S_CLIENT_CAP`` ioctl sets client capabilities for the file +handle ``fd``. The new capabilities fully replace the current capabilities, the +ioctl can therefore also be used to remove capabilities that have previously +been set. + +``VIDIOC_SUBDEV_S_CLIENT_CAP`` modifies the struct +:c:type:`v4l2_subdev_client_capability` to reflect the capabilities that have +been accepted. A common case for the kernel not accepting a capability is that +the kernel is older than the headers the userspace uses, and thus the capability +is unknown to the kernel. + +.. flat-table:: Client Capabilities + :header-rows: 1 + + * - Capability + - Description + * - ``V4L2_SUBDEV_CLIENT_CAP_STREAMS`` + - The client is aware of streams. Setting this flag enables the use + of 'stream' fields (referring to the stream number) with various + ioctls. If this is not set (which is the default), the 'stream' fields + will be forced to 0 by the kernel. + +Return Value +============ + +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 ` chapter. + +ENOIOCTLCMD + The kernel does not support this ioctl. diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index bfb102f2121a..2ec179cd1264 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -510,8 +510,11 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, struct video_device *vdev = video_devdata(file); struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev); struct v4l2_fh *vfh = file->private_data; + struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh); bool ro_subdev = test_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags); bool streams_subdev = sd->flags & V4L2_SUBDEV_FL_STREAMS; + bool client_supports_streams = subdev_fh->client_caps & + V4L2_SUBDEV_CLIENT_CAP_STREAMS; int rval; switch (cmd) { @@ -636,6 +639,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, case VIDIOC_SUBDEV_G_FMT: { struct v4l2_subdev_format *format = arg; + if (!client_supports_streams) + format->stream = 0; + memset(format->reserved, 0, sizeof(format->reserved)); memset(format->format.reserved, 0, sizeof(format->format.reserved)); return v4l2_subdev_call(sd, pad, get_fmt, state, format); @@ -647,6 +653,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, if (format->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev) return -EPERM; + if (!client_supports_streams) + format->stream = 0; + memset(format->reserved, 0, sizeof(format->reserved)); memset(format->format.reserved, 0, sizeof(format->format.reserved)); return v4l2_subdev_call(sd, pad, set_fmt, state, format); @@ -656,6 +665,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, struct v4l2_subdev_crop *crop = arg; struct v4l2_subdev_selection sel; + if (!client_supports_streams) + crop->stream = 0; + memset(crop->reserved, 0, sizeof(crop->reserved)); memset(&sel, 0, sizeof(sel)); sel.which = crop->which; @@ -677,6 +689,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, if (crop->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev) return -EPERM; + if (!client_supports_streams) + crop->stream = 0; + memset(crop->reserved, 0, sizeof(crop->reserved)); memset(&sel, 0, sizeof(sel)); sel.which = crop->which; @@ -695,6 +710,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, case VIDIOC_SUBDEV_ENUM_MBUS_CODE: { struct v4l2_subdev_mbus_code_enum *code = arg; + if (!client_supports_streams) + code->stream = 0; + memset(code->reserved, 0, sizeof(code->reserved)); return v4l2_subdev_call(sd, pad, enum_mbus_code, state, code); @@ -703,6 +721,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: { struct v4l2_subdev_frame_size_enum *fse = arg; + if (!client_supports_streams) + fse->stream = 0; + memset(fse->reserved, 0, sizeof(fse->reserved)); return v4l2_subdev_call(sd, pad, enum_frame_size, state, fse); @@ -711,6 +732,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, case VIDIOC_SUBDEV_G_FRAME_INTERVAL: { struct v4l2_subdev_frame_interval *fi = arg; + if (!client_supports_streams) + fi->stream = 0; + memset(fi->reserved, 0, sizeof(fi->reserved)); return v4l2_subdev_call(sd, video, g_frame_interval, arg); } @@ -721,6 +745,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, if (ro_subdev) return -EPERM; + if (!client_supports_streams) + fi->stream = 0; + memset(fi->reserved, 0, sizeof(fi->reserved)); return v4l2_subdev_call(sd, video, s_frame_interval, arg); } @@ -728,6 +755,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: { struct v4l2_subdev_frame_interval_enum *fie = arg; + if (!client_supports_streams) + fie->stream = 0; + memset(fie->reserved, 0, sizeof(fie->reserved)); return v4l2_subdev_call(sd, pad, enum_frame_interval, state, fie); @@ -736,6 +766,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, case VIDIOC_SUBDEV_G_SELECTION: { struct v4l2_subdev_selection *sel = arg; + if (!client_supports_streams) + sel->stream = 0; + memset(sel->reserved, 0, sizeof(sel->reserved)); return v4l2_subdev_call( sd, pad, get_selection, state, sel); @@ -747,6 +780,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, if (sel->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev) return -EPERM; + if (!client_supports_streams) + sel->stream = 0; + memset(sel->reserved, 0, sizeof(sel->reserved)); return v4l2_subdev_call( sd, pad, set_selection, state, sel); @@ -888,6 +924,33 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, routing->which, &krouting); } + case VIDIOC_SUBDEV_G_CLIENT_CAP: { + struct v4l2_subdev_client_capability *client_cap = arg; + + client_cap->capabilities = subdev_fh->client_caps; + + return 0; + } + + case VIDIOC_SUBDEV_S_CLIENT_CAP: { + struct v4l2_subdev_client_capability *client_cap = arg; + + /* + * Clear V4L2_SUBDEV_CLIENT_CAP_STREAMS if streams API is not + * enabled. Remove this when streams API is no longer + * experimental. + */ + if (!v4l2_subdev_enable_streams_api) + client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS; + + /* Filter out unsupported capabilities */ + client_cap->capabilities &= V4L2_SUBDEV_CLIENT_CAP_STREAMS; + + subdev_fh->client_caps = client_cap->capabilities; + + return 0; + } + default: return v4l2_subdev_call(sd, core, ioctl, cmd, arg); } diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 1d2877700cdb..cfd19e72d0fc 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -1125,6 +1125,7 @@ struct v4l2_subdev_fh { struct module *owner; #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) struct v4l2_subdev_state *state; + u64 client_caps; #endif }; diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index 654d659de835..4a195b68f28f 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -233,6 +233,24 @@ struct v4l2_subdev_routing { __u32 reserved[6]; }; +/* + * The client is aware of streams. Setting this flag enables the use of 'stream' + * fields (referring to the stream number) with various ioctls. If this is not + * set (which is the default), the 'stream' fields will be forced to 0 by the + * kernel. + */ + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) + +/** + * struct v4l2_subdev_client_capability - Capabilities of the client accessing + * the subdev + * + * @capabilities: A bitmask of V4L2_SUBDEV_CLIENT_CAP_* flags. + */ +struct v4l2_subdev_client_capability { + __u64 capabilities; +}; + /* Backwards compatibility define --- to be removed */ #define v4l2_subdev_edid v4l2_edid @@ -250,6 +268,9 @@ struct v4l2_subdev_routing { #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) +#define VIDIOC_SUBDEV_G_CLIENT_CAP _IOR('V', 101, struct v4l2_subdev_client_capability) +#define VIDIOC_SUBDEV_S_CLIENT_CAP _IOWR('V', 102, struct v4l2_subdev_client_capability) + /* 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) -- cgit v1.2.3