[v12,15/30] media: subdev: add v4l2_subdev_set_routing helper()
Commit Message
Add a helper function to set the subdev routing. The helper can be used
from subdev driver's set_routing op to store the routing table.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
include/media/v4l2-subdev.h | 16 ++++++++++++++++
2 files changed, 43 insertions(+)
Comments
Moi,
On Wed, Jul 27, 2022 at 01:36:24PM +0300, Tomi Valkeinen wrote:
> Add a helper function to set the subdev routing. The helper can be used
> from subdev driver's set_routing op to store the routing table.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
> include/media/v4l2-subdev.h | 16 ++++++++++++++++
> 2 files changed, 43 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index befd00ebf381..0b841cf74c74 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1176,6 +1176,33 @@ int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_get_fmt);
>
> +int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_krouting *routing)
> +{
> + struct v4l2_subdev_krouting *dst = &state->routing;
> + const struct v4l2_subdev_krouting *src = routing;
> +
> + lockdep_assert_held(state->lock);
> +
> + kfree(dst->routes);
> + dst->routes = NULL;
> + dst->num_routes = 0;
Shouldn't you keep the old routing around until memory allocation (and
anything else that can fail) has succeeded?
> +
> + if (src->num_routes > 0) {
> + dst->routes = kmemdup(src->routes,
> + src->num_routes * sizeof(*src->routes),
> + GFP_KERNEL);
> + if (!dst->routes)
> + return -ENOMEM;
> +
> + dst->num_routes = src->num_routes;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing);
> +
> #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>
> #endif /* CONFIG_MEDIA_CONTROLLER */
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index decd1e1c3d39..37081a2e6697 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1407,6 +1407,22 @@ v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
> int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format);
>
> +/**
> + * v4l2_subdev_set_routing() - Set given routing to subdev state
> + * @sd: The subdevice
> + * @state: The subdevice state
> + * @routing: Routing that will be copied to subdev state
> + *
> + * This will release old routing table (if any) from the state, allocate
> + * enough space for the given routing, and copy the routing.
> + *
> + * This can be used from the subdev driver's set_routing op, after validating
> + * the routing.
> + */
> +int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_krouting *routing);
> +
> #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>
> #endif /* CONFIG_MEDIA_CONTROLLER */
On 01/08/2022 09:59, Sakari Ailus wrote:
> Moi,
>
> On Wed, Jul 27, 2022 at 01:36:24PM +0300, Tomi Valkeinen wrote:
>> Add a helper function to set the subdev routing. The helper can be used
>> from subdev driver's set_routing op to store the routing table.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
>> include/media/v4l2-subdev.h | 16 ++++++++++++++++
>> 2 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index befd00ebf381..0b841cf74c74 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -1176,6 +1176,33 @@ int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>> }
>> EXPORT_SYMBOL_GPL(v4l2_subdev_get_fmt);
>>
>> +int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_krouting *routing)
>> +{
>> + struct v4l2_subdev_krouting *dst = &state->routing;
>> + const struct v4l2_subdev_krouting *src = routing;
>> +
>> + lockdep_assert_held(state->lock);
>> +
>> + kfree(dst->routes);
>> + dst->routes = NULL;
>> + dst->num_routes = 0;
>
> Shouldn't you keep the old routing around until memory allocation (and
> anything else that can fail) has succeeded?
Indeed, good catch! I think v4l2_subdev_init_stream_configs() also needs
improving.
Tomi
@@ -1176,6 +1176,33 @@ int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
}
EXPORT_SYMBOL_GPL(v4l2_subdev_get_fmt);
+int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_krouting *routing)
+{
+ struct v4l2_subdev_krouting *dst = &state->routing;
+ const struct v4l2_subdev_krouting *src = routing;
+
+ lockdep_assert_held(state->lock);
+
+ kfree(dst->routes);
+ dst->routes = NULL;
+ dst->num_routes = 0;
+
+ if (src->num_routes > 0) {
+ dst->routes = kmemdup(src->routes,
+ src->num_routes * sizeof(*src->routes),
+ GFP_KERNEL);
+ if (!dst->routes)
+ return -ENOMEM;
+
+ dst->num_routes = src->num_routes;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing);
+
#endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
#endif /* CONFIG_MEDIA_CONTROLLER */
@@ -1407,6 +1407,22 @@ v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
struct v4l2_subdev_format *format);
+/**
+ * v4l2_subdev_set_routing() - Set given routing to subdev state
+ * @sd: The subdevice
+ * @state: The subdevice state
+ * @routing: Routing that will be copied to subdev state
+ *
+ * This will release old routing table (if any) from the state, allocate
+ * enough space for the given routing, and copy the routing.
+ *
+ * This can be used from the subdev driver's set_routing op, after validating
+ * the routing.
+ */
+int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_krouting *routing);
+
#endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
#endif /* CONFIG_MEDIA_CONTROLLER */