[PATCH v5 0/6] v4l: subdev active state

[PATCH v5 0/6] v4l: subdev active state

[Tomi Valkeinen]

Hi,

This is v5 of the subdev active state series. The v4 can be found from:

https://lore.kernel.org/all/20220216130049.508664-1-tomi.valkeinen@ideasonboard.com/

Changes since v4:

- Move v4l2_subdev_call_state_active() to a new header file, include/media/v4l2-subdev-legacy.h
- Rename v4l2_subdev_lock_active_state to v4l2_subdev_lock_and_get_active_state
- Inline v4l2_subdev_lock_and_get_active_state, v4l2_subdev_lock_state, v4l2_subdev_unlock_state
- Minor doc updates
- Drop v4l2_subdev_lock_and_return_state patch

 Tomi

Tomi Valkeinen (6):
  media: subdev: rename subdev-state alloc & free
  media: subdev: add active state to struct v4l2_subdev
  media: subdev: pass also the active state to subdevs from ioctls
  media: subdev: add subdev state locking
  media: subdev: add v4l2_subdev_call_state_active()
  media: Documentation: add documentation about subdev state

 .../driver-api/media/v4l2-subdev.rst          |  75 ++++++++
 drivers/media/platform/rcar-vin/rcar-v4l2.c   |   9 +-
 drivers/media/platform/vsp1/vsp1_entity.c     |  10 +-
 drivers/media/v4l2-core/v4l2-subdev.c         | 137 ++++++++++++---
 drivers/staging/media/tegra-video/vi.c        |  10 +-
 include/media/v4l2-subdev-legacy.h            |  42 +++++
 include/media/v4l2-subdev.h                   | 163 +++++++++++++++++-
 7 files changed, 415 insertions(+), 31 deletions(-)
 create mode 100644 include/media/v4l2-subdev-legacy.h

-- 
2.25.1

[PATCH v5 1/6] media: subdev: rename subdev-state alloc & free

[Tomi Valkeinen]

v4l2_subdev_alloc_state() and v4l2_subdev_free_state() are not supposed
to be used by the drivers. However, we do have a few drivers that use
those at the moment, so we need to expose these functions for the time
being.

Prefix the functions with __ to mark the functions as internal.

At the same time, rename them to v4l2_subdev_state_alloc and
v4l2_subdev_state_free to match the style used for other functions like
video_device_alloc() and media_request_alloc().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  8 ++++++--
 drivers/media/platform/vsp1/vsp1_entity.c   |  8 ++++++--
 drivers/media/v4l2-core/v4l2-subdev.c       | 12 ++++++------
 drivers/staging/media/tegra-video/vi.c      |  8 ++++++--
 include/media/v4l2-subdev.h                 | 14 +++++++++-----
 5 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 2e60b9fce03b..da88f968c31a 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -263,7 +263,11 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 	u32 width, height;
 	int ret;
 
-	sd_state = v4l2_subdev_alloc_state(sd);
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	sd_state = __v4l2_subdev_state_alloc(sd);
 	if (IS_ERR(sd_state))
 		return PTR_ERR(sd_state);
 
@@ -299,7 +303,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 
 	rvin_format_align(vin, pix);
 done:
-	v4l2_subdev_free_state(sd_state);
+	__v4l2_subdev_state_free(sd_state);
 
 	return ret;
 }
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
index 823c15facd1b..c82b3fb7b89a 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.c
+++ b/drivers/media/platform/vsp1/vsp1_entity.c
@@ -675,7 +675,11 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
 	 * Allocate the pad configuration to store formats and selection
 	 * rectangles.
 	 */
-	entity->config = v4l2_subdev_alloc_state(&entity->subdev);
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	entity->config = __v4l2_subdev_state_alloc(&entity->subdev);
 	if (IS_ERR(entity->config)) {
 		media_entity_cleanup(&entity->subdev.entity);
 		return PTR_ERR(entity->config);
@@ -690,6 +694,6 @@ void vsp1_entity_destroy(struct vsp1_entity *entity)
 		entity->ops->destroy(entity);
 	if (entity->subdev.ctrl_handler)
 		v4l2_ctrl_handler_free(entity->subdev.ctrl_handler);
-	v4l2_subdev_free_state(entity->config);
+	__v4l2_subdev_state_free(entity->config);
 	media_entity_cleanup(&entity->subdev.entity);
 }
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 30eb50407db5..376595954db0 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -28,7 +28,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 {
 	struct v4l2_subdev_state *state;
 
-	state = v4l2_subdev_alloc_state(sd);
+	state = __v4l2_subdev_state_alloc(sd);
 	if (IS_ERR(state))
 		return PTR_ERR(state);
 
@@ -39,7 +39,7 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 
 static void subdev_fh_free(struct v4l2_subdev_fh *fh)
 {
-	v4l2_subdev_free_state(fh->state);
+	__v4l2_subdev_state_free(fh->state);
 	fh->state = NULL;
 }
 
@@ -862,7 +862,7 @@ int v4l2_subdev_link_validate(struct media_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
 
-struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd)
+struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
 {
 	struct v4l2_subdev_state *state;
 	int ret;
@@ -895,9 +895,9 @@ struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd)
 
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state);
+EXPORT_SYMBOL_GPL(__v4l2_subdev_state_alloc);
 
-void v4l2_subdev_free_state(struct v4l2_subdev_state *state)
+void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
 {
 	if (!state)
 		return;
@@ -905,7 +905,7 @@ void v4l2_subdev_free_state(struct v4l2_subdev_state *state)
 	kvfree(state->pads);
 	kfree(state);
 }
-EXPORT_SYMBOL_GPL(v4l2_subdev_free_state);
+EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
 
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index d1f43f465c22..07d368f345cd 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -507,7 +507,11 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	if (!subdev)
 		return -ENODEV;
 
-	sd_state = v4l2_subdev_alloc_state(subdev);
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	sd_state = __v4l2_subdev_state_alloc(subdev);
 	if (IS_ERR(sd_state))
 		return PTR_ERR(sd_state);
 	/*
@@ -558,7 +562,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	v4l2_fill_pix_format(pix, &fmt.format);
 	tegra_channel_fmt_align(chan, pix, fmtinfo->bpp);
 
-	v4l2_subdev_free_state(sd_state);
+	__v4l2_subdev_state_free(sd_state);
 
 	return 0;
 }
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 6c153b33bb04..5d6f56648ad6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1122,20 +1122,24 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
 int v4l2_subdev_link_validate(struct media_link *link);
 
 /**
- * v4l2_subdev_alloc_state - allocate v4l2_subdev_state
+ * __v4l2_subdev_state_alloc - allocate v4l2_subdev_state
  *
  * @sd: pointer to &struct v4l2_subdev for which the state is being allocated.
  *
- * Must call v4l2_subdev_free_state() when state is no longer needed.
+ * Must call __v4l2_subdev_state_free() when state is no longer needed.
+ *
+ * Not to be called directly by the drivers.
  */
-struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd);
+struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
 
 /**
- * v4l2_subdev_free_state - free a v4l2_subdev_state
+ * __v4l2_subdev_state_free - free a v4l2_subdev_state
  *
  * @state: v4l2_subdev_state to be freed.
+ *
+ * Not to be called directly by the drivers.
  */
-void v4l2_subdev_free_state(struct v4l2_subdev_state *state);
+void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
 
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
-- 
2.25.1

[PATCH v5 2/6] media: subdev: add active state to struct v4l2_subdev

[Tomi Valkeinen]

Add a new 'active_state' field to struct v4l2_subdev to which we can
store the active state of a subdev. This will place the subdev
configuration into a known place, allowing us to use the state directly
from the v4l2 framework, thus simplifying the drivers.

Also add functions v4l2_subdev_init_finalize() and
v4l2_subdev_cleanup(), which will allocate and free the active state.
The functions are named in a generic way so that they can be also used
for other subdev initialization work.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++
 include/media/v4l2-subdev.h           | 58 +++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 376595954db0..11a06e0aca0c 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -907,6 +907,27 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
 }
 EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
 
+int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
+{
+	struct v4l2_subdev_state *state;
+
+	state = __v4l2_subdev_state_alloc(sd);
+	if (IS_ERR(state))
+		return PTR_ERR(state);
+
+	sd->active_state = state;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
+
+void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
+{
+	__v4l2_subdev_state_free(sd->active_state);
+	sd->active_state = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 5d6f56648ad6..1bbe4383966c 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -645,6 +645,9 @@ struct v4l2_subdev_ir_ops {
  * 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
  * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
+ *
+ * Note: This struct is also used in active state, and the 'try' prefix is
+ * historical and to be removed.
  */
 struct v4l2_subdev_pad_config {
 	struct v4l2_mbus_framefmt try_fmt;
@@ -885,6 +888,9 @@ struct v4l2_subdev_platform_data {
  * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
  *		     device using v4l2_async_register_subdev_sensor().
  * @pdata: common part of subdevice platform data
+ * @active_state: Active state for the subdev (NULL for subdevs tracking the
+ *		  state internally). Initialized by calling
+ *		  v4l2_subdev_init_finalize().
  *
  * Each instance of a subdev driver should create this struct, either
  * stand-alone or embedded in a larger struct.
@@ -916,6 +922,19 @@ struct v4l2_subdev {
 	struct v4l2_async_notifier *notifier;
 	struct v4l2_async_notifier *subdev_notifier;
 	struct v4l2_subdev_platform_data *pdata;
+
+	/*
+	 * The fields below are private, and should only be accessed via
+	 * appropriate functions.
+	 */
+
+	/*
+	 * TODO: active_state should most likely be changed from a pointer to an
+	 * embedded field. For the time being it's kept as a pointer to more
+	 * easily catch uses of active_state in the cases where the driver
+	 * doesn't support it.
+	 */
+	struct v4l2_subdev_state *active_state;
 };
 
 
@@ -1141,6 +1160,45 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
  */
 void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
 
+/**
+ * v4l2_subdev_init_finalize() - Finalizes the initialization of the subdevice
+ * @sd: The subdev
+ *
+ * This function finalizes the initialization of the subdev, including
+ * allocation of the active state for the subdev.
+ *
+ * This function must be called by the subdev drivers that use the centralized
+ * active state, after the subdev struct has been initialized and
+ * media_entity_pads_init() has been called, but before registering the
+ * subdev.
+ *
+ * The user must call v4l2_subdev_cleanup() when the subdev is being removed.
+ */
+int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
+
+/**
+ * v4l2_subdev_cleanup() - Releases the resources allocated by the subdevice
+ * @sd: The subdevice
+ *
+ * This function will release the resources allocated in
+ * v4l2_subdev_init_finalize.
+ */
+void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
+
+/**
+ * v4l2_subdev_get_active_state() - Returns the active subdev state for
+ *				    subdevice
+ * @sd: The subdevice
+ *
+ * Returns the active state for the subdevice, or NULL if the subdev does not
+ * support active state.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
+{
+	return sd->active_state;
+}
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 /**
-- 
2.25.1

[PATCH v5 3/6] media: subdev: pass also the active state to subdevs from ioctls

[Tomi Valkeinen]

At the moment when a subdev op is called, the TRY subdev state
(subdev_fh->state) is passed as a parameter even for the ACTIVE case, or
alternatively a NULL can be passed for ACTIVE case. This used to make
sense, as the ACTIVE state was handled internally by the subdev drivers.

We now have a state for the ACTIVE case in a standard place, and can
pass that also to the drivers. This patch changes the subdev ioctls to
either pass the TRY or ACTIVE state to the subdev.

Unfortunately many drivers call ops from other subdevs, and implicitly
pass NULL as the state, so this is just a partial solution. A coccinelle
spatch could perhaps be created which fixes the drivers' subdev calls.

For all current upstream drivers this doesn't matter, as they do not
expect to get a valid state for ACTIVE case. But future drivers which
support multiplexed streaming and routing will depend on getting a state
for both active and try cases.

For new drivers we can mandate that the pipelines where the drivers are
used need to pass the state properly, or preferably, not call such
subdev ops at all.

However, if an existing subdev driver is changed to support multiplexed
streams, the driver has to consider cases where its ops will be called
with NULL state. The problem can easily be solved by using the
v4l2_subdev_lock_and_return_state() helper, introduced in a follow up
patch.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 64 ++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 10 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 11a06e0aca0c..b67bbce82612 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -345,6 +345,44 @@ const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
 EXPORT_SYMBOL(v4l2_subdev_call_wrappers);
 
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+
+static struct v4l2_subdev_state *
+subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
+		       unsigned int cmd, void *arg)
+{
+	u32 which;
+
+	switch (cmd) {
+	default:
+		return NULL;
+	case VIDIOC_SUBDEV_G_FMT:
+	case VIDIOC_SUBDEV_S_FMT:
+		which = ((struct v4l2_subdev_format *)arg)->which;
+		break;
+	case VIDIOC_SUBDEV_G_CROP:
+	case VIDIOC_SUBDEV_S_CROP:
+		which = ((struct v4l2_subdev_crop *)arg)->which;
+		break;
+	case VIDIOC_SUBDEV_ENUM_MBUS_CODE:
+		which = ((struct v4l2_subdev_mbus_code_enum *)arg)->which;
+		break;
+	case VIDIOC_SUBDEV_ENUM_FRAME_SIZE:
+		which = ((struct v4l2_subdev_frame_size_enum *)arg)->which;
+		break;
+	case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL:
+		which = ((struct v4l2_subdev_frame_interval_enum *)arg)->which;
+		break;
+	case VIDIOC_SUBDEV_G_SELECTION:
+	case VIDIOC_SUBDEV_S_SELECTION:
+		which = ((struct v4l2_subdev_selection *)arg)->which;
+		break;
+	}
+
+	return which == V4L2_SUBDEV_FORMAT_TRY ?
+			     subdev_fh->state :
+			     v4l2_subdev_get_active_state(sd);
+}
+
 static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 {
 	struct video_device *vdev = video_devdata(file);
@@ -352,8 +390,11 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	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);
+	struct v4l2_subdev_state *state;
 	int rval;
 
+	state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
+
 	switch (cmd) {
 	case VIDIOC_SUBDEV_QUERYCAP: {
 		struct v4l2_subdev_capability *cap = arg;
@@ -476,7 +517,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 		memset(format->reserved, 0, sizeof(format->reserved));
 		memset(format->format.reserved, 0, sizeof(format->format.reserved));
-		return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format);
+		return v4l2_subdev_call(sd, pad, get_fmt, state, format);
 	}
 
 	case VIDIOC_SUBDEV_S_FMT: {
@@ -487,7 +528,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 		memset(format->reserved, 0, sizeof(format->reserved));
 		memset(format->format.reserved, 0, sizeof(format->format.reserved));
-		return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format);
+		return v4l2_subdev_call(sd, pad, set_fmt, state, format);
 	}
 
 	case VIDIOC_SUBDEV_G_CROP: {
@@ -501,7 +542,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		sel.target = V4L2_SEL_TGT_CROP;
 
 		rval = v4l2_subdev_call(
-			sd, pad, get_selection, subdev_fh->state, &sel);
+			sd, pad, get_selection, state, &sel);
 
 		crop->rect = sel.r;
 
@@ -523,7 +564,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		sel.r = crop->rect;
 
 		rval = v4l2_subdev_call(
-			sd, pad, set_selection, subdev_fh->state, &sel);
+			sd, pad, set_selection, state, &sel);
 
 		crop->rect = sel.r;
 
@@ -534,7 +575,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		struct v4l2_subdev_mbus_code_enum *code = arg;
 
 		memset(code->reserved, 0, sizeof(code->reserved));
-		return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state,
+		return v4l2_subdev_call(sd, pad, enum_mbus_code, state,
 					code);
 	}
 
@@ -542,7 +583,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		struct v4l2_subdev_frame_size_enum *fse = arg;
 
 		memset(fse->reserved, 0, sizeof(fse->reserved));
-		return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state,
+		return v4l2_subdev_call(sd, pad, enum_frame_size, state,
 					fse);
 	}
 
@@ -567,7 +608,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		struct v4l2_subdev_frame_interval_enum *fie = arg;
 
 		memset(fie->reserved, 0, sizeof(fie->reserved));
-		return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state,
+		return v4l2_subdev_call(sd, pad, enum_frame_interval, state,
 					fie);
 	}
 
@@ -576,7 +617,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 		memset(sel->reserved, 0, sizeof(sel->reserved));
 		return v4l2_subdev_call(
-			sd, pad, get_selection, subdev_fh->state, sel);
+			sd, pad, get_selection, state, sel);
 	}
 
 	case VIDIOC_SUBDEV_S_SELECTION: {
@@ -587,7 +628,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 		memset(sel->reserved, 0, sizeof(sel->reserved));
 		return v4l2_subdev_call(
-			sd, pad, set_selection, subdev_fh->state, sel);
+			sd, pad, set_selection, state, sel);
 	}
 
 	case VIDIOC_G_EDID: {
@@ -821,10 +862,13 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
 	if (is_media_entity_v4l2_subdev(pad->entity)) {
 		struct v4l2_subdev *sd =
 			media_entity_to_v4l2_subdev(pad->entity);
+		struct v4l2_subdev_state *state;
+
+		state = v4l2_subdev_get_active_state(sd);
 
 		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
 		fmt->pad = pad->index;
-		return v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt);
+		return v4l2_subdev_call(sd, pad, get_fmt, state, fmt);
 	}
 
 	WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L,
-- 
2.25.1

[PATCH v5 4/6] media: subdev: add subdev state locking

[Tomi Valkeinen]

The V4L2 subdevs have managed without centralized locking for the state
(previously pad_config), as the try-state is supposedly safe (although I
believe two TRY ioctls for the same fd would race), and the
active-state, and its locking, is managed by the drivers internally.

We now have active-state in a centralized position, and need locking.
Strictly speaking the locking is only needed for new drivers that use
the new state, as the current drivers continue behaving as they used to.

However, active-state locking is complicated by the fact that currently
the real active-state of a subdev is split into multiple parts: the new
v4l2_subdev_state, subdev control state, and subdev's internal state.

In the future all these three states should be combined into one state
(the v4l2_subdev_state), and then a single lock for the state should be
sufficient.

But to solve the current split-state situation we need to share locks
between the three states. This is accomplished by using the same lock
management as the control handler does: we use a pointer to a mutex,
allowing the driver to override the default mutex. Thus the driver can
do e.g.:

sd->state_lock = sd->ctrl_handler->lock;

before calling v4l2_subdev_init_finalize(), resulting in sharing the
same lock between the states and the controls.

The locking model for active-state is such that any subdev op that gets
the state as a parameter expects the state to be already locked by the
caller, and expects the caller to release the lock.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
 drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
 drivers/media/v4l2-core/v4l2-subdev.c       | 58 +++++++++---
 drivers/staging/media/tegra-video/vi.c      |  4 +-
 include/media/v4l2-subdev.h                 | 97 ++++++++++++++++++++-
 5 files changed, 147 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index da88f968c31a..3759f4619a77 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -255,6 +255,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 {
 	struct v4l2_subdev *sd = vin_to_source(vin);
 	struct v4l2_subdev_state *sd_state;
+	static struct lock_class_key key;
 	struct v4l2_subdev_format format = {
 		.which = which,
 		.pad = vin->parallel.source_pad,
@@ -267,7 +268,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 	 * FIXME: Drop this call, drivers are not supposed to use
 	 * __v4l2_subdev_state_alloc().
 	 */
-	sd_state = __v4l2_subdev_state_alloc(sd);
+	sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
 	if (IS_ERR(sd_state))
 		return PTR_ERR(sd_state);
 
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
index c82b3fb7b89a..a116a3362f9e 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.c
+++ b/drivers/media/platform/vsp1/vsp1_entity.c
@@ -613,6 +613,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
 		     const char *name, unsigned int num_pads,
 		     const struct v4l2_subdev_ops *ops, u32 function)
 {
+	static struct lock_class_key key;
 	struct v4l2_subdev *subdev;
 	unsigned int i;
 	int ret;
@@ -679,7 +680,8 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
 	 * FIXME: Drop this call, drivers are not supposed to use
 	 * __v4l2_subdev_state_alloc().
 	 */
-	entity->config = __v4l2_subdev_state_alloc(&entity->subdev);
+	entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
+						   "vsp1:config->lock", &key);
 	if (IS_ERR(entity->config)) {
 		media_entity_cleanup(&entity->subdev.entity);
 		return PTR_ERR(entity->config);
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b67bbce82612..fda0925afeb3 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -27,8 +27,9 @@
 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 {
 	struct v4l2_subdev_state *state;
+	static struct lock_class_key key;
 
-	state = __v4l2_subdev_state_alloc(sd);
+	state = __v4l2_subdev_state_alloc(sd, "fh->state->lock", &key);
 	if (IS_ERR(state))
 		return PTR_ERR(state);
 
@@ -383,18 +384,15 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
 			     v4l2_subdev_get_active_state(sd);
 }
 
-static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
+static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
+			    struct v4l2_subdev_state *state)
 {
 	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);
-	struct v4l2_subdev_state *state;
 	int rval;
 
-	state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
-
 	switch (cmd) {
 	case VIDIOC_SUBDEV_QUERYCAP: {
 		struct v4l2_subdev_capability *cap = arg;
@@ -707,8 +705,24 @@ static long subdev_do_ioctl_lock(struct file *file, unsigned int cmd, void *arg)
 
 	if (lock && mutex_lock_interruptible(lock))
 		return -ERESTARTSYS;
-	if (video_is_registered(vdev))
-		ret = subdev_do_ioctl(file, cmd, arg);
+
+	if (video_is_registered(vdev)) {
+		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);
+		struct v4l2_subdev_state *state;
+
+		state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
+
+		if (state)
+			v4l2_subdev_lock_state(state);
+
+		ret = subdev_do_ioctl(file, cmd, arg, state);
+
+		if (state)
+			v4l2_subdev_unlock_state(state);
+	}
+
 	if (lock)
 		mutex_unlock(lock);
 	return ret;
@@ -864,7 +878,7 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
 			media_entity_to_v4l2_subdev(pad->entity);
 		struct v4l2_subdev_state *state;
 
-		state = v4l2_subdev_get_active_state(sd);
+		state = v4l2_subdev_get_locked_active_state(sd);
 
 		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
 		fmt->pad = pad->index;
@@ -906,7 +920,9 @@ int v4l2_subdev_link_validate(struct media_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
 
-struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
+struct v4l2_subdev_state *
+__v4l2_subdev_state_alloc(struct v4l2_subdev *sd, const char *lock_name,
+			  struct lock_class_key *lock_key)
 {
 	struct v4l2_subdev_state *state;
 	int ret;
@@ -915,6 +931,12 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
 	if (!state)
 		return ERR_PTR(-ENOMEM);
 
+	__mutex_init(&state->_lock, lock_name, lock_key);
+	if (sd->state_lock)
+		state->lock = sd->state_lock;
+	else
+		state->lock = &state->_lock;
+
 	if (sd->entity.num_pads) {
 		state->pads = kvmalloc_array(sd->entity.num_pads,
 					     sizeof(*state->pads),
@@ -925,7 +947,14 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
 		}
 	}
 
+	/*
+	 * There can be no race at this point, but we lock the state anyway to
+	 * satisfy lockdep checks.
+	 */
+	v4l2_subdev_lock_state(state);
 	ret = v4l2_subdev_call(sd, pad, init_cfg, state);
+	v4l2_subdev_unlock_state(state);
+
 	if (ret < 0 && ret != -ENOIOCTLCMD)
 		goto err;
 
@@ -946,16 +975,19 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
 	if (!state)
 		return;
 
+	mutex_destroy(&state->_lock);
+
 	kvfree(state->pads);
 	kfree(state);
 }
 EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
 
-int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
+int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
+				struct lock_class_key *key)
 {
 	struct v4l2_subdev_state *state;
 
-	state = __v4l2_subdev_state_alloc(sd);
+	state = __v4l2_subdev_state_alloc(sd, name, key);
 	if (IS_ERR(state))
 		return PTR_ERR(state);
 
@@ -963,7 +995,7 @@ int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
+EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
 
 void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
 {
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 07d368f345cd..8e184aa4c252 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -491,6 +491,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 				      struct v4l2_pix_format *pix)
 {
 	const struct tegra_video_format *fmtinfo;
+	static struct lock_class_key key;
 	struct v4l2_subdev *subdev;
 	struct v4l2_subdev_format fmt;
 	struct v4l2_subdev_state *sd_state;
@@ -511,7 +512,8 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	 * FIXME: Drop this call, drivers are not supposed to use
 	 * __v4l2_subdev_state_alloc().
 	 */
-	sd_state = __v4l2_subdev_state_alloc(subdev);
+	sd_state = __v4l2_subdev_state_alloc(subdev, "tegra:state->lock",
+					     &key);
 	if (IS_ERR(sd_state))
 		return PTR_ERR(sd_state);
 	/*
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1bbe4383966c..71d13d160d99 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -658,6 +658,8 @@ struct v4l2_subdev_pad_config {
 /**
  * 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
  *
  * This structure only needs to be passed to the pad op if the 'which' field
@@ -665,6 +667,9 @@ struct v4l2_subdev_pad_config {
  * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
  */
 struct v4l2_subdev_state {
+	/* lock for the struct v4l2_subdev_state fields */
+	struct mutex _lock;
+	struct mutex *lock;
 	struct v4l2_subdev_pad_config *pads;
 };
 
@@ -888,6 +893,9 @@ struct v4l2_subdev_platform_data {
  * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
  *		     device using v4l2_async_register_subdev_sensor().
  * @pdata: common part of subdevice platform data
+ * @state_lock: A pointer to a lock used for all the subdev's states, set by the
+ *		driver. This is	optional. If NULL, each state instance will get
+ *		a lock of its own.
  * @active_state: Active state for the subdev (NULL for subdevs tracking the
  *		  state internally). Initialized by calling
  *		  v4l2_subdev_init_finalize().
@@ -922,6 +930,7 @@ struct v4l2_subdev {
 	struct v4l2_async_notifier *notifier;
 	struct v4l2_async_notifier *subdev_notifier;
 	struct v4l2_subdev_platform_data *pdata;
+	struct mutex *state_lock;
 
 	/*
 	 * The fields below are private, and should only be accessed via
@@ -1144,12 +1153,16 @@ int v4l2_subdev_link_validate(struct media_link *link);
  * __v4l2_subdev_state_alloc - allocate v4l2_subdev_state
  *
  * @sd: pointer to &struct v4l2_subdev for which the state is being allocated.
+ * @lock_name: name of the state lock
+ * @key: lock_class_key for the lock
  *
  * Must call __v4l2_subdev_state_free() when state is no longer needed.
  *
  * Not to be called directly by the drivers.
  */
-struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
+struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd,
+						    const char *lock_name,
+						    struct lock_class_key *key);
 
 /**
  * __v4l2_subdev_state_free - free a v4l2_subdev_state
@@ -1174,7 +1187,16 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
  *
  * The user must call v4l2_subdev_cleanup() when the subdev is being removed.
  */
-int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
+#define v4l2_subdev_init_finalize(sd)                                          \
+	({                                                                     \
+		static struct lock_class_key __key;                            \
+		const char *name = KBUILD_BASENAME                             \
+			":" __stringify(__LINE__) ":sd->active_state->lock";   \
+		__v4l2_subdev_init_finalize(sd, name, &__key);                 \
+	})
+
+int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
+				struct lock_class_key *key);
 
 /**
  * v4l2_subdev_cleanup() - Releases the resources allocated by the subdevice
@@ -1191,14 +1213,83 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
  * @sd: The subdevice
  *
  * Returns the active state for the subdevice, or NULL if the subdev does not
- * support active state.
+ * support active state. If the state is not NULL, calls
+ * lockdep_assert_not_held() to issue a warning if the state is locked.
+ *
+ * This function is to be used e.g. when getting the active state for the sole
+ * purpose of passing it forward, without accessing the state fields.
  */
 static inline struct v4l2_subdev_state *
 v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
 {
+	if (sd->active_state)
+		lockdep_assert_not_held(sd->active_state->lock);
+	return sd->active_state;
+}
+
+/**
+ * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
+ *					   is locked and returns it
+ *
+ * @sd: The subdevice
+ *
+ * Returns the active state for the subdevice, or NULL if the subdev does not
+ * support active state. If the state is not NULL, calls lockdep_assert_held()
+ * to issue a warning if the state is not locked.
+ *
+ * This function is to be used when the caller knows that the active state is
+ * already locked.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
+{
+	if (sd->active_state)
+		lockdep_assert_held(sd->active_state->lock);
 	return sd->active_state;
 }
 
+/**
+ * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
+ *					     state for the subdevice
+ * @sd: The subdevice
+ *
+ * Returns the locked active state for the subdevice, or NULL if the subdev
+ * does not support active state.
+ *
+ * The state must be unlocked with v4l2_subdev_unlock_state() after use.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
+{
+	mutex_lock(sd->active_state->lock);
+
+	return sd->active_state;
+}
+
+/**
+ * v4l2_subdev_lock_state() - Locks the subdev state
+ * @state: The subdevice state
+ *
+ * Locks the given subdev state.
+ *
+ * The state must be unlocked with v4l2_subdev_unlock_state() after use.
+ */
+static inline void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
+{
+	mutex_lock(state->lock);
+}
+
+/**
+ * v4l2_subdev_unlock_state() - Unlocks the subdev state
+ * @state: The subdevice state
+ *
+ * Unlocks the given subdev state.
+ */
+static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
+{
+	mutex_unlock(state->lock);
+}
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 /**
-- 
2.25.1

[PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Tomi Valkeinen]

Add v4l2_subdev_call_state_active() macro to help calling subdev ops
that take a subdev state as a parameter. Normally the v4l2 framework
will lock and pass the correct subdev state to the subdev ops, but there
are legacy situations where this is not the case (e.g. non-MC video
device driver calling set_fmt in a source subdev).

As this macro is only needed for legacy use cases, the macro is added in
a new header file, v4l2-subdev-legacy.h.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 include/media/v4l2-subdev-legacy.h

diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
new file mode 100644
index 000000000000..6a61e579b629
--- /dev/null
+++ b/include/media/v4l2-subdev-legacy.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *  V4L2 sub-device legacy support header.
+ *
+ *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
+ */
+
+#ifndef _V4L2_SUBDEV_LEGACY_H
+#define _V4L2_SUBDEV_LEGACY_H
+
+/**
+ * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
+ *				   takes state as a parameter, passing the
+ *				   subdev its active state.
+ *
+ * @sd: pointer to the &struct v4l2_subdev
+ * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
+ *     Each element there groups a set of callbacks functions.
+ * @f: callback function to be called.
+ *     The callback functions are defined in groups, according to
+ *     each element at &struct v4l2_subdev_ops.
+ * @args: arguments for @f.
+ *
+ * This is similar to v4l2_subdev_call(), except that this version can only be
+ * used for ops that take a subdev state as a parameter. The macro will get the
+ * active state and lock it before calling the op, and unlock it after the
+ * call.
+ */
+#define v4l2_subdev_call_state_active(sd, o, f, args...)		\
+	({								\
+		int __result;						\
+		struct v4l2_subdev_state *state;			\
+		state = v4l2_subdev_get_active_state(sd);		\
+		if (state)						\
+			v4l2_subdev_lock_state(state);			\
+		__result = v4l2_subdev_call(sd, o, f, state, ##args);	\
+		if (state)						\
+			v4l2_subdev_unlock_state(state);		\
+		__result;						\
+	})
+
+#endif
-- 
2.25.1

[PATCH v5 6/6] media: Documentation: add documentation about subdev state

[Tomi Valkeinen]

Add documentation about centrally managed subdev state.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../driver-api/media/v4l2-subdev.rst          | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
index 08ea2673b19e..115211cef4d1 100644
--- a/Documentation/driver-api/media/v4l2-subdev.rst
+++ b/Documentation/driver-api/media/v4l2-subdev.rst
@@ -518,6 +518,81 @@ The :c:func:`v4l2_i2c_new_subdev` function will call
 :c:type:`i2c_board_info` structure using the ``client_type`` and the
 ``addr`` to fill it.
 
+Centrally managed subdev active state
+-------------------------------------
+
+Traditionally V4L2 subdev drivers maintained internal state for the active
+device configuration. This is often implemented as e.g. an array of struct
+v4l2_mbus_framefmt, one entry for each pad, and similarly for crop and compose
+rectangles.
+
+In addition to the active configuration, each subdev file handle has an array of
+struct v4l2_subdev_pad_config, managed by the V4L2 core, which contains the try
+configuration.
+
+To simplify the subdev drivers the V4L2 subdev API now optionally supports a
+centrally managed active configuration represented by
+:c:type:`v4l2_subdev_state`. One instance of state, which contains the active
+device configuration, is stored in the sub-device itself as part of
+the :c:type:`v4l2_subdev` structure, while the core associates a try state to
+each open file handle, to store the try configuration related to that file
+handle.
+
+Sub-device drivers can opt-in and use state to manage their active configuration
+by initializing the subdevice state with a call to v4l2_subdev_init_finalize()
+before registering the sub-device. They must also call v4l2_subdev_cleanup()
+to release all the allocated resources before unregistering the sub-device.
+The core automatically allocates and initializes a state for each open file
+handle to store the try configurations and frees it when closing the file
+handle.
+
+V4L2 sub-device operations that use both the :ref:`ACTIVE and TRY formats
+<v4l2-subdev-format-whence>` receive the correct state to operate on through
+the 'state' parameter. The state must be locked and unlocked by the
+caller by calling :c:func:`v4l2_subdev_lock_state()` and
+:c:func:`v4l2_subdev_unlock_state()`. The caller can do so by calling the subdev
+operation through the :c:func:`v4l2_subdev_call_state_active()` macro.
+
+Operations that do not receive a state parameter implicitly operate on the
+subdevice active state, which drivers can exclusively access by
+calling :c:func:`v4l2_subdev_lock_and_get_active_state()`. The sub-device active
+state must equally be released by calling :c:func:`v4l2_subdev_unlock_state()`.
+
+Drivers must never manually access the state stored in the :c:type:`v4l2_subdev`
+or in the file handle without going through the designated helpers.
+
+While the V4L2 core passes the correct try or active state to the
+subdevice operations, some existing device drivers pass a NULL state
+when calling operations with :c:func:`v4l2_subdev_call()`. This legacy
+construct causes issues with subdevice drivers that let the V4L2 core
+manage the active state, as they expect to receive the appropriate state
+as a parameter. To help the conversion of subdevice drivers to a managed
+active state without having to convert all callers at the same time, the
+:c:func:`v4l2_subdev_lock_and_return_state()` helper function can be
+used by subdevice drivers to retrieve the active state if a NULL state
+is passed to the subdevice operation.
+
+:c:func:`v4l2_subdev_lock_and_return_state()` should only be used when porting
+an existing driver to the new state management when it cannot be guaranteed
+that the current callers will pass the state properly. The function prints a
+notice when the passed state is NULL to encourage the porting of the callers
+to the new state management.
+
+The whole subdev state is in reality split into three parts: the
+v4l2_subdev_state, subdev controls and subdev driver's internal state. In the
+future these parts should be combined into a single state. For the time being
+we need a way to handle the locking for these parts. This can be accomplished
+by sharing a lock. The v4l2_ctrl_handler already supports this via its 'lock'
+pointer and the same model is used with states. The driver can do the following
+before calling v4l2_subdev_init_finalize():
+
+.. code-block:: python
+
+	sd->ctrl_handler->lock = &priv->mutex;
+	sd->state_lock = &priv->mutex;
+
+This shares the driver's private mutex between the controls and the states.
+
 V4L2 sub-device functions and data structures
 ---------------------------------------------
 
-- 
2.25.1

Re: [PATCH v5 6/6] media: Documentation: add documentation about subdev state

[Tomi Valkeinen]

On 01/03/2022 12:55, Tomi Valkeinen wrote:
> Add documentation about centrally managed subdev state.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   .../driver-api/media/v4l2-subdev.rst          | 75 +++++++++++++++++++
>   1 file changed, 75 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> index 08ea2673b19e..115211cef4d1 100644
> --- a/Documentation/driver-api/media/v4l2-subdev.rst
> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> @@ -518,6 +518,81 @@ The :c:func:`v4l2_i2c_new_subdev` function will call
>   :c:type:`i2c_board_info` structure using the ``client_type`` and the
>   ``addr`` to fill it.
>   
> +Centrally managed subdev active state
> +-------------------------------------
> +
> +Traditionally V4L2 subdev drivers maintained internal state for the active
> +device configuration. This is often implemented as e.g. an array of struct
> +v4l2_mbus_framefmt, one entry for each pad, and similarly for crop and compose
> +rectangles.
> +
> +In addition to the active configuration, each subdev file handle has an array of
> +struct v4l2_subdev_pad_config, managed by the V4L2 core, which contains the try
> +configuration.
> +
> +To simplify the subdev drivers the V4L2 subdev API now optionally supports a
> +centrally managed active configuration represented by
> +:c:type:`v4l2_subdev_state`. One instance of state, which contains the active
> +device configuration, is stored in the sub-device itself as part of
> +the :c:type:`v4l2_subdev` structure, while the core associates a try state to
> +each open file handle, to store the try configuration related to that file
> +handle.
> +
> +Sub-device drivers can opt-in and use state to manage their active configuration
> +by initializing the subdevice state with a call to v4l2_subdev_init_finalize()
> +before registering the sub-device. They must also call v4l2_subdev_cleanup()
> +to release all the allocated resources before unregistering the sub-device.
> +The core automatically allocates and initializes a state for each open file
> +handle to store the try configurations and frees it when closing the file
> +handle.
> +
> +V4L2 sub-device operations that use both the :ref:`ACTIVE and TRY formats
> +<v4l2-subdev-format-whence>` receive the correct state to operate on through
> +the 'state' parameter. The state must be locked and unlocked by the
> +caller by calling :c:func:`v4l2_subdev_lock_state()` and
> +:c:func:`v4l2_subdev_unlock_state()`. The caller can do so by calling the subdev
> +operation through the :c:func:`v4l2_subdev_call_state_active()` macro.
> +
> +Operations that do not receive a state parameter implicitly operate on the
> +subdevice active state, which drivers can exclusively access by
> +calling :c:func:`v4l2_subdev_lock_and_get_active_state()`. The sub-device active
> +state must equally be released by calling :c:func:`v4l2_subdev_unlock_state()`.
> +
> +Drivers must never manually access the state stored in the :c:type:`v4l2_subdev`
> +or in the file handle without going through the designated helpers.
> +
> +While the V4L2 core passes the correct try or active state to the
> +subdevice operations, some existing device drivers pass a NULL state
> +when calling operations with :c:func:`v4l2_subdev_call()`. This legacy
> +construct causes issues with subdevice drivers that let the V4L2 core
> +manage the active state, as they expect to receive the appropriate state
> +as a parameter. To help the conversion of subdevice drivers to a managed
> +active state without having to convert all callers at the same time, the
> +:c:func:`v4l2_subdev_lock_and_return_state()` helper function can be
> +used by subdevice drivers to retrieve the active state if a NULL state
> +is passed to the subdevice operation.
> +
> +:c:func:`v4l2_subdev_lock_and_return_state()` should only be used when porting
> +an existing driver to the new state management when it cannot be guaranteed
> +that the current callers will pass the state properly. The function prints a
> +notice when the passed state is NULL to encourage the porting of the callers
> +to the new state management.
> +
> +The whole subdev state is in reality split into three parts: the
> +v4l2_subdev_state, subdev controls and subdev driver's internal state. In the
> +future these parts should be combined into a single state. For the time being
> +we need a way to handle the locking for these parts. This can be accomplished
> +by sharing a lock. The v4l2_ctrl_handler already supports this via its 'lock'
> +pointer and the same model is used with states. The driver can do the following
> +before calling v4l2_subdev_init_finalize():
> +
> +.. code-block:: python
> +
> +	sd->ctrl_handler->lock = &priv->mutex;
> +	sd->state_lock = &priv->mutex;
> +

That is obviously supposed to be a c code-block, not python...

  Tomi

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 01, 2022 at 12:55:47PM +0200, Tomi Valkeinen wrote:
> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> that take a subdev state as a parameter. Normally the v4l2 framework
> will lock and pass the correct subdev state to the subdev ops, but there
> are legacy situations where this is not the case (e.g. non-MC video
> device driver calling set_fmt in a source subdev).
> 
> As this macro is only needed for legacy use cases, the macro is added in
> a new header file, v4l2-subdev-legacy.h.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 include/media/v4l2-subdev-legacy.h
> 
> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
> new file mode 100644
> index 000000000000..6a61e579b629
> --- /dev/null
> +++ b/include/media/v4l2-subdev-legacy.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + *  V4L2 sub-device legacy support header.
> + *
> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> + */
> +
> +#ifndef _V4L2_SUBDEV_LEGACY_H
> +#define _V4L2_SUBDEV_LEGACY_H
> +
> +/**
> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
> + *				   takes state as a parameter, passing the
> + *				   subdev its active state.
> + *
> + * @sd: pointer to the &struct v4l2_subdev
> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
> + *     Each element there groups a set of callbacks functions.
> + * @f: callback function to be called.
> + *     The callback functions are defined in groups, according to
> + *     each element at &struct v4l2_subdev_ops.
> + * @args: arguments for @f.
> + *
> + * This is similar to v4l2_subdev_call(), except that this version can only be
> + * used for ops that take a subdev state as a parameter. The macro will get the
> + * active state and lock it before calling the op, and unlock it after the

s/active state and/active state,/

> + * call.
> + */
> +#define v4l2_subdev_call_state_active(sd, o, f, args...)		\
> +	({								\
> +		int __result;						\
> +		struct v4l2_subdev_state *state;			\
> +		state = v4l2_subdev_get_active_state(sd);		\
> +		if (state)						\
> +			v4l2_subdev_lock_state(state);			\
> +		__result = v4l2_subdev_call(sd, o, f, state, ##args);	\
> +		if (state)						\
> +			v4l2_subdev_unlock_state(state);		\
> +		__result;						\
> +	})
> +
> +#endif

I think

#endif /* _V4L2_SUBDEV_LEGACY_H */

is the usual pattern. Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Sakari Ailus]

Moi,

On Wed, Mar 02, 2022 at 11:36:25AM +0200, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 01, 2022 at 12:55:47PM +0200, Tomi Valkeinen wrote:
> > Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> > that take a subdev state as a parameter. Normally the v4l2 framework
> > will lock and pass the correct subdev state to the subdev ops, but there
> > are legacy situations where this is not the case (e.g. non-MC video
> > device driver calling set_fmt in a source subdev).
> > 
> > As this macro is only needed for legacy use cases, the macro is added in
> > a new header file, v4l2-subdev-legacy.h.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >  create mode 100644 include/media/v4l2-subdev-legacy.h
> > 
> > diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
> > new file mode 100644
> > index 000000000000..6a61e579b629
> > --- /dev/null
> > +++ b/include/media/v4l2-subdev-legacy.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + *  V4L2 sub-device legacy support header.
> > + *
> > + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > + */
> > +
> > +#ifndef _V4L2_SUBDEV_LEGACY_H
> > +#define _V4L2_SUBDEV_LEGACY_H
> > +
> > +/**
> > + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
> > + *				   takes state as a parameter, passing the
> > + *				   subdev its active state.
> > + *
> > + * @sd: pointer to the &struct v4l2_subdev
> > + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
> > + *     Each element there groups a set of callbacks functions.
> > + * @f: callback function to be called.
> > + *     The callback functions are defined in groups, according to
> > + *     each element at &struct v4l2_subdev_ops.
> > + * @args: arguments for @f.
> > + *
> > + * This is similar to v4l2_subdev_call(), except that this version can only be
> > + * used for ops that take a subdev state as a parameter. The macro will get the
> > + * active state and lock it before calling the op, and unlock it after the
> 
> s/active state and/active state,/
> 
> > + * call.
> > + */
> > +#define v4l2_subdev_call_state_active(sd, o, f, args...)		\
> > +	({								\
> > +		int __result;						\
> > +		struct v4l2_subdev_state *state;			\
> > +		state = v4l2_subdev_get_active_state(sd);		\
> > +		if (state)						\
> > +			v4l2_subdev_lock_state(state);			\
> > +		__result = v4l2_subdev_call(sd, o, f, state, ##args);	\
> > +		if (state)						\
> > +			v4l2_subdev_unlock_state(state);		\
> > +		__result;						\
> > +	})
> > +
> > +#endif
> 
> I think
> 
> #endif /* _V4L2_SUBDEV_LEGACY_H */

I'll do these (plus the code block type change) when applying the patches
if there are no other comments to the series.

> 
> is the usual pattern. Apart from that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

-- 
Terveisin,

Sakari Ailus

Re: [PATCH v5 4/6] media: subdev: add subdev state locking

[Hans Verkuil]

Hi Tomi,

On 3/1/22 11:55, Tomi Valkeinen wrote:
> The V4L2 subdevs have managed without centralized locking for the state
> (previously pad_config), as the try-state is supposedly safe (although I
> believe two TRY ioctls for the same fd would race), and the
> active-state, and its locking, is managed by the drivers internally.
> 
> We now have active-state in a centralized position, and need locking.
> Strictly speaking the locking is only needed for new drivers that use
> the new state, as the current drivers continue behaving as they used to.
> 
> However, active-state locking is complicated by the fact that currently
> the real active-state of a subdev is split into multiple parts: the new
> v4l2_subdev_state, subdev control state, and subdev's internal state.
> 
> In the future all these three states should be combined into one state
> (the v4l2_subdev_state), and then a single lock for the state should be
> sufficient.
> 
> But to solve the current split-state situation we need to share locks
> between the three states. This is accomplished by using the same lock
> management as the control handler does: we use a pointer to a mutex,
> allowing the driver to override the default mutex. Thus the driver can
> do e.g.:
> 
> sd->state_lock = sd->ctrl_handler->lock;
> 
> before calling v4l2_subdev_init_finalize(), resulting in sharing the
> same lock between the states and the controls.
> 
> The locking model for active-state is such that any subdev op that gets
> the state as a parameter expects the state to be already locked by the
> caller, and expects the caller to release the lock.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
>  drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
>  drivers/media/v4l2-core/v4l2-subdev.c       | 58 +++++++++---
>  drivers/staging/media/tegra-video/vi.c      |  4 +-
>  include/media/v4l2-subdev.h                 | 97 ++++++++++++++++++++-
>  5 files changed, 147 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index da88f968c31a..3759f4619a77 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -255,6 +255,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>  {
>  	struct v4l2_subdev *sd = vin_to_source(vin);
>  	struct v4l2_subdev_state *sd_state;
> +	static struct lock_class_key key;
>  	struct v4l2_subdev_format format = {
>  		.which = which,
>  		.pad = vin->parallel.source_pad,
> @@ -267,7 +268,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>  	 * FIXME: Drop this call, drivers are not supposed to use
>  	 * __v4l2_subdev_state_alloc().
>  	 */
> -	sd_state = __v4l2_subdev_state_alloc(sd);
> +	sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
>  	if (IS_ERR(sd_state))
>  		return PTR_ERR(sd_state);
>  
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
> index c82b3fb7b89a..a116a3362f9e 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -613,6 +613,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
>  		     const char *name, unsigned int num_pads,
>  		     const struct v4l2_subdev_ops *ops, u32 function)
>  {
> +	static struct lock_class_key key;
>  	struct v4l2_subdev *subdev;
>  	unsigned int i;
>  	int ret;
> @@ -679,7 +680,8 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
>  	 * FIXME: Drop this call, drivers are not supposed to use
>  	 * __v4l2_subdev_state_alloc().
>  	 */
> -	entity->config = __v4l2_subdev_state_alloc(&entity->subdev);
> +	entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
> +						   "vsp1:config->lock", &key);
>  	if (IS_ERR(entity->config)) {
>  		media_entity_cleanup(&entity->subdev.entity);
>  		return PTR_ERR(entity->config);
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b67bbce82612..fda0925afeb3 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -27,8 +27,9 @@
>  static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>  {
>  	struct v4l2_subdev_state *state;
> +	static struct lock_class_key key;
>  
> -	state = __v4l2_subdev_state_alloc(sd);
> +	state = __v4l2_subdev_state_alloc(sd, "fh->state->lock", &key);
>  	if (IS_ERR(state))
>  		return PTR_ERR(state);
>  
> @@ -383,18 +384,15 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
>  			     v4l2_subdev_get_active_state(sd);
>  }
>  
> -static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> +static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> +			    struct v4l2_subdev_state *state)
>  {
>  	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);
> -	struct v4l2_subdev_state *state;
>  	int rval;
>  
> -	state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
> -
>  	switch (cmd) {
>  	case VIDIOC_SUBDEV_QUERYCAP: {
>  		struct v4l2_subdev_capability *cap = arg;
> @@ -707,8 +705,24 @@ static long subdev_do_ioctl_lock(struct file *file, unsigned int cmd, void *arg)
>  
>  	if (lock && mutex_lock_interruptible(lock))
>  		return -ERESTARTSYS;
> -	if (video_is_registered(vdev))
> -		ret = subdev_do_ioctl(file, cmd, arg);
> +
> +	if (video_is_registered(vdev)) {
> +		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);
> +		struct v4l2_subdev_state *state;
> +
> +		state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
> +
> +		if (state)
> +			v4l2_subdev_lock_state(state);
> +
> +		ret = subdev_do_ioctl(file, cmd, arg, state);
> +
> +		if (state)
> +			v4l2_subdev_unlock_state(state);
> +	}
> +
>  	if (lock)
>  		mutex_unlock(lock);
>  	return ret;
> @@ -864,7 +878,7 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
>  			media_entity_to_v4l2_subdev(pad->entity);
>  		struct v4l2_subdev_state *state;
>  
> -		state = v4l2_subdev_get_active_state(sd);
> +		state = v4l2_subdev_get_locked_active_state(sd);
>  
>  		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  		fmt->pad = pad->index;
> @@ -906,7 +920,9 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>  
> -struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
> +struct v4l2_subdev_state *
> +__v4l2_subdev_state_alloc(struct v4l2_subdev *sd, const char *lock_name,
> +			  struct lock_class_key *lock_key)
>  {
>  	struct v4l2_subdev_state *state;
>  	int ret;
> @@ -915,6 +931,12 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
>  	if (!state)
>  		return ERR_PTR(-ENOMEM);
>  
> +	__mutex_init(&state->_lock, lock_name, lock_key);
> +	if (sd->state_lock)
> +		state->lock = sd->state_lock;
> +	else
> +		state->lock = &state->_lock;
> +
>  	if (sd->entity.num_pads) {
>  		state->pads = kvmalloc_array(sd->entity.num_pads,
>  					     sizeof(*state->pads),
> @@ -925,7 +947,14 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
>  		}
>  	}
>  
> +	/*
> +	 * There can be no race at this point, but we lock the state anyway to
> +	 * satisfy lockdep checks.
> +	 */
> +	v4l2_subdev_lock_state(state);
>  	ret = v4l2_subdev_call(sd, pad, init_cfg, state);
> +	v4l2_subdev_unlock_state(state);
> +
>  	if (ret < 0 && ret != -ENOIOCTLCMD)
>  		goto err;
>  
> @@ -946,16 +975,19 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
>  	if (!state)
>  		return;
>  
> +	mutex_destroy(&state->_lock);
> +
>  	kvfree(state->pads);
>  	kfree(state);
>  }
>  EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
>  
> -int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
> +int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> +				struct lock_class_key *key)
>  {
>  	struct v4l2_subdev_state *state;
>  
> -	state = __v4l2_subdev_state_alloc(sd);
> +	state = __v4l2_subdev_state_alloc(sd, name, key);
>  	if (IS_ERR(state))
>  		return PTR_ERR(state);
>  
> @@ -963,7 +995,7 @@ int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
> +EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
>  
>  void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>  {
> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> index 07d368f345cd..8e184aa4c252 100644
> --- a/drivers/staging/media/tegra-video/vi.c
> +++ b/drivers/staging/media/tegra-video/vi.c
> @@ -491,6 +491,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
>  				      struct v4l2_pix_format *pix)
>  {
>  	const struct tegra_video_format *fmtinfo;
> +	static struct lock_class_key key;
>  	struct v4l2_subdev *subdev;
>  	struct v4l2_subdev_format fmt;
>  	struct v4l2_subdev_state *sd_state;
> @@ -511,7 +512,8 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
>  	 * FIXME: Drop this call, drivers are not supposed to use
>  	 * __v4l2_subdev_state_alloc().
>  	 */
> -	sd_state = __v4l2_subdev_state_alloc(subdev);
> +	sd_state = __v4l2_subdev_state_alloc(subdev, "tegra:state->lock",
> +					     &key);
>  	if (IS_ERR(sd_state))
>  		return PTR_ERR(sd_state);
>  	/*
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1bbe4383966c..71d13d160d99 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -658,6 +658,8 @@ struct v4l2_subdev_pad_config {
>  /**
>   * 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
>   *
>   * This structure only needs to be passed to the pad op if the 'which' field
> @@ -665,6 +667,9 @@ struct v4l2_subdev_pad_config {
>   * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
>   */
>  struct v4l2_subdev_state {
> +	/* lock for the struct v4l2_subdev_state fields */
> +	struct mutex _lock;
> +	struct mutex *lock;
>  	struct v4l2_subdev_pad_config *pads;
>  };
>  
> @@ -888,6 +893,9 @@ struct v4l2_subdev_platform_data {
>   * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
>   *		     device using v4l2_async_register_subdev_sensor().
>   * @pdata: common part of subdevice platform data
> + * @state_lock: A pointer to a lock used for all the subdev's states, set by the
> + *		driver. This is	optional. If NULL, each state instance will get
> + *		a lock of its own.
>   * @active_state: Active state for the subdev (NULL for subdevs tracking the
>   *		  state internally). Initialized by calling
>   *		  v4l2_subdev_init_finalize().
> @@ -922,6 +930,7 @@ struct v4l2_subdev {
>  	struct v4l2_async_notifier *notifier;
>  	struct v4l2_async_notifier *subdev_notifier;
>  	struct v4l2_subdev_platform_data *pdata;
> +	struct mutex *state_lock;
>  
>  	/*
>  	 * The fields below are private, and should only be accessed via
> @@ -1144,12 +1153,16 @@ int v4l2_subdev_link_validate(struct media_link *link);
>   * __v4l2_subdev_state_alloc - allocate v4l2_subdev_state
>   *
>   * @sd: pointer to &struct v4l2_subdev for which the state is being allocated.
> + * @lock_name: name of the state lock
> + * @key: lock_class_key for the lock
>   *
>   * Must call __v4l2_subdev_state_free() when state is no longer needed.
>   *
>   * Not to be called directly by the drivers.
>   */
> -struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
> +struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd,
> +						    const char *lock_name,
> +						    struct lock_class_key *key);
>  
>  /**
>   * __v4l2_subdev_state_free - free a v4l2_subdev_state
> @@ -1174,7 +1187,16 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
>   *
>   * The user must call v4l2_subdev_cleanup() when the subdev is being removed.
>   */
> -int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
> +#define v4l2_subdev_init_finalize(sd)                                          \
> +	({                                                                     \
> +		static struct lock_class_key __key;                            \
> +		const char *name = KBUILD_BASENAME                             \
> +			":" __stringify(__LINE__) ":sd->active_state->lock";   \
> +		__v4l2_subdev_init_finalize(sd, name, &__key);                 \
> +	})
> +
> +int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> +				struct lock_class_key *key);
>  
>  /**
>   * v4l2_subdev_cleanup() - Releases the resources allocated by the subdevice
> @@ -1191,14 +1213,83 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
>   * @sd: The subdevice
>   *
>   * Returns the active state for the subdevice, or NULL if the subdev does not
> - * support active state.
> + * support active state. If the state is not NULL, calls
> + * lockdep_assert_not_held() to issue a warning if the state is locked.
> + *
> + * This function is to be used e.g. when getting the active state for the sole
> + * purpose of passing it forward, without accessing the state fields.
>   */
>  static inline struct v4l2_subdev_state *
>  v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
>  {
> +	if (sd->active_state)
> +		lockdep_assert_not_held(sd->active_state->lock);
> +	return sd->active_state;
> +}
> +
> +/**
> + * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
> + *					   is locked and returns it
> + *
> + * @sd: The subdevice
> + *
> + * Returns the active state for the subdevice, or NULL if the subdev does not
> + * support active state. If the state is not NULL, calls lockdep_assert_held()
> + * to issue a warning if the state is not locked.
> + *
> + * This function is to be used when the caller knows that the active state is
> + * already locked.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
> +{
> +	if (sd->active_state)
> +		lockdep_assert_held(sd->active_state->lock);
>  	return sd->active_state;
>  }

I think these two function names are quite confusing.

Better options are:

v4l2_subdev_get_unlocked_active_state
v4l2_subdev_get_locked_active_state

or:

__v4l2_subdev_get_active_state (assumes unlocked state)
v4l2_subdev_get_active_state (assumes locked state)

In the current naming scheme there is no indication that v4l2_subdev_get_active_state
assumes the state is unlocked.

>  
> +/**
> + * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
> + *					     state for the subdevice
> + * @sd: The subdevice
> + *
> + * Returns the locked active state for the subdevice, or NULL if the subdev
> + * does not support active state.
> + *
> + * The state must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
> +{
> +	mutex_lock(sd->active_state->lock);
> +
> +	return sd->active_state;
> +}

I think this inline should be moved to below v4l2_subdev_lock_state...

> +
> +/**
> + * v4l2_subdev_lock_state() - Locks the subdev state
> + * @state: The subdevice state
> + *
> + * Locks the given subdev state.
> + *
> + * The state must be unlocked with v4l2_subdev_unlock_state() after use.
> + */
> +static inline void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
> +{
> +	mutex_lock(state->lock);
> +}

...since here it can use v4l2_subdev_lock_state(sd->active_state).

> +
> +/**
> + * v4l2_subdev_unlock_state() - Unlocks the subdev state
> + * @state: The subdevice state
> + *
> + * Unlocks the given subdev state.
> + */
> +static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> +{
> +	mutex_unlock(state->lock);
> +}

It can also be moved here, I have no preference.

> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  /**

Regards,

	Hans

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Hans Verkuil]

Hi Tomi,

On 3/1/22 11:55, Tomi Valkeinen wrote:
> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> that take a subdev state as a parameter. Normally the v4l2 framework
> will lock and pass the correct subdev state to the subdev ops, but there
> are legacy situations where this is not the case (e.g. non-MC video
> device driver calling set_fmt in a source subdev).
> 
> As this macro is only needed for legacy use cases, the macro is added in
> a new header file, v4l2-subdev-legacy.h.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 include/media/v4l2-subdev-legacy.h
> 
> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
> new file mode 100644
> index 000000000000..6a61e579b629
> --- /dev/null
> +++ b/include/media/v4l2-subdev-legacy.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + *  V4L2 sub-device legacy support header.
> + *
> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> + */
> +
> +#ifndef _V4L2_SUBDEV_LEGACY_H
> +#define _V4L2_SUBDEV_LEGACY_H
> +
> +/**
> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
> + *				   takes state as a parameter, passing the
> + *				   subdev its active state.
> + *
> + * @sd: pointer to the &struct v4l2_subdev
> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
> + *     Each element there groups a set of callbacks functions.
> + * @f: callback function to be called.
> + *     The callback functions are defined in groups, according to
> + *     each element at &struct v4l2_subdev_ops.
> + * @args: arguments for @f.
> + *
> + * This is similar to v4l2_subdev_call(), except that this version can only be
> + * used for ops that take a subdev state as a parameter. The macro will get the
> + * active state and lock it before calling the op, and unlock it after the
> + * call.
> + */

You should explain why this is a legacy macro and, ideally, what would need to
be done to get rid of it. The first is in the commit log, but nobody reads that :-)

But if just using it in a non-MC video device driver constitutes 'legacy' use,
then I disagree with that. There are many non-MC video device drivers, nothing
legacy about that.

Regards,

	Hans

> +#define v4l2_subdev_call_state_active(sd, o, f, args...)		\
> +	({								\
> +		int __result;						\
> +		struct v4l2_subdev_state *state;			\
> +		state = v4l2_subdev_get_active_state(sd);		\
> +		if (state)						\
> +			v4l2_subdev_lock_state(state);			\
> +		__result = v4l2_subdev_call(sd, o, f, state, ##args);	\
> +		if (state)						\
> +			v4l2_subdev_unlock_state(state);		\
> +		__result;						\
> +	})
> +
> +#endif

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Tomi Valkeinen]

Hi Hans,

On 04/03/2022 15:34, Hans Verkuil wrote:
> Hi Tomi,
> 
> On 3/1/22 11:55, Tomi Valkeinen wrote:
>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>> that take a subdev state as a parameter. Normally the v4l2 framework
>> will lock and pass the correct subdev state to the subdev ops, but there
>> are legacy situations where this is not the case (e.g. non-MC video
>> device driver calling set_fmt in a source subdev).
>>
>> As this macro is only needed for legacy use cases, the macro is added in
>> a new header file, v4l2-subdev-legacy.h.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>>   create mode 100644 include/media/v4l2-subdev-legacy.h
>>
>> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
>> new file mode 100644
>> index 000000000000..6a61e579b629
>> --- /dev/null
>> +++ b/include/media/v4l2-subdev-legacy.h
>> @@ -0,0 +1,42 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + *  V4L2 sub-device legacy support header.
>> + *
>> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> + */
>> +
>> +#ifndef _V4L2_SUBDEV_LEGACY_H
>> +#define _V4L2_SUBDEV_LEGACY_H
>> +
>> +/**
>> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
>> + *				   takes state as a parameter, passing the
>> + *				   subdev its active state.
>> + *
>> + * @sd: pointer to the &struct v4l2_subdev
>> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
>> + *     Each element there groups a set of callbacks functions.
>> + * @f: callback function to be called.
>> + *     The callback functions are defined in groups, according to
>> + *     each element at &struct v4l2_subdev_ops.
>> + * @args: arguments for @f.
>> + *
>> + * This is similar to v4l2_subdev_call(), except that this version can only be
>> + * used for ops that take a subdev state as a parameter. The macro will get the
>> + * active state and lock it before calling the op, and unlock it after the
>> + * call.
>> + */
> 
> You should explain why this is a legacy macro and, ideally, what would need to
> be done to get rid of it. The first is in the commit log, but nobody reads that :-)
> 
> But if just using it in a non-MC video device driver constitutes 'legacy' use,
> then I disagree with that. There are many non-MC video device drivers, nothing
> legacy about that.

It's difficult to define all the scenarios where this can be used, but 
the ones I can imagine fall under legacy (depending on how you define 
that, though).

I use this in CAL driver, which supports non-MC (legacy) and MC. CAL has 
a bunch of video devices (one for each DMA engine) and two CSI-2 PHY 
devices (v4l2 subdevs).

When operating in MC mode, the userspace will call, e.g., set_fmt in the 
PHY subdev, and so forth.

But in non-MC case the userspace calls VIDIOC_S_FMT in the video dev, 
and the video dev has to propagate that to the PHY subdev. I do this 
propagation using the v4l2_subdev_call_state_active macro.

I don't know if there are other drivers that support both non-MC and MC 
modes. I could also just move this macro to the CAL driver, and we could 
add this to the v4l2 framework if we see other drivers using similar 
constructs.

  Tomi

Re: [PATCH v5 4/6] media: subdev: add subdev state locking

[Tomi Valkeinen]

Hi,

On 04/03/2022 15:25, Hans Verkuil wrote:
> Hi Tomi,
> 
> On 3/1/22 11:55, Tomi Valkeinen wrote:
>> The V4L2 subdevs have managed without centralized locking for the state
>> (previously pad_config), as the try-state is supposedly safe (although I
>> believe two TRY ioctls for the same fd would race), and the
>> active-state, and its locking, is managed by the drivers internally.
>>
>> We now have active-state in a centralized position, and need locking.
>> Strictly speaking the locking is only needed for new drivers that use
>> the new state, as the current drivers continue behaving as they used to.
>>
>> However, active-state locking is complicated by the fact that currently
>> the real active-state of a subdev is split into multiple parts: the new
>> v4l2_subdev_state, subdev control state, and subdev's internal state.
>>
>> In the future all these three states should be combined into one state
>> (the v4l2_subdev_state), and then a single lock for the state should be
>> sufficient.
>>
>> But to solve the current split-state situation we need to share locks
>> between the three states. This is accomplished by using the same lock
>> management as the control handler does: we use a pointer to a mutex,
>> allowing the driver to override the default mutex. Thus the driver can
>> do e.g.:
>>
>> sd->state_lock = sd->ctrl_handler->lock;
>>
>> before calling v4l2_subdev_init_finalize(), resulting in sharing the
>> same lock between the states and the controls.
>>
>> The locking model for active-state is such that any subdev op that gets
>> the state as a parameter expects the state to be already locked by the
>> caller, and expects the caller to release the lock.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
>>   drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
>>   drivers/media/v4l2-core/v4l2-subdev.c       | 58 +++++++++---
>>   drivers/staging/media/tegra-video/vi.c      |  4 +-
>>   include/media/v4l2-subdev.h                 | 97 ++++++++++++++++++++-
>>   5 files changed, 147 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> index da88f968c31a..3759f4619a77 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> @@ -255,6 +255,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>>   {
>>   	struct v4l2_subdev *sd = vin_to_source(vin);
>>   	struct v4l2_subdev_state *sd_state;
>> +	static struct lock_class_key key;
>>   	struct v4l2_subdev_format format = {
>>   		.which = which,
>>   		.pad = vin->parallel.source_pad,
>> @@ -267,7 +268,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>>   	 * FIXME: Drop this call, drivers are not supposed to use
>>   	 * __v4l2_subdev_state_alloc().
>>   	 */
>> -	sd_state = __v4l2_subdev_state_alloc(sd);
>> +	sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
>>   	if (IS_ERR(sd_state))
>>   		return PTR_ERR(sd_state);
>>   
>> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
>> index c82b3fb7b89a..a116a3362f9e 100644
>> --- a/drivers/media/platform/vsp1/vsp1_entity.c
>> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
>> @@ -613,6 +613,7 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
>>   		     const char *name, unsigned int num_pads,
>>   		     const struct v4l2_subdev_ops *ops, u32 function)
>>   {
>> +	static struct lock_class_key key;
>>   	struct v4l2_subdev *subdev;
>>   	unsigned int i;
>>   	int ret;
>> @@ -679,7 +680,8 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity,
>>   	 * FIXME: Drop this call, drivers are not supposed to use
>>   	 * __v4l2_subdev_state_alloc().
>>   	 */
>> -	entity->config = __v4l2_subdev_state_alloc(&entity->subdev);
>> +	entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
>> +						   "vsp1:config->lock", &key);
>>   	if (IS_ERR(entity->config)) {
>>   		media_entity_cleanup(&entity->subdev.entity);
>>   		return PTR_ERR(entity->config);
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index b67bbce82612..fda0925afeb3 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -27,8 +27,9 @@
>>   static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>>   {
>>   	struct v4l2_subdev_state *state;
>> +	static struct lock_class_key key;
>>   
>> -	state = __v4l2_subdev_state_alloc(sd);
>> +	state = __v4l2_subdev_state_alloc(sd, "fh->state->lock", &key);
>>   	if (IS_ERR(state))
>>   		return PTR_ERR(state);
>>   
>> @@ -383,18 +384,15 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
>>   			     v4l2_subdev_get_active_state(sd);
>>   }
>>   
>> -static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>> +static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>> +			    struct v4l2_subdev_state *state)
>>   {
>>   	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);
>> -	struct v4l2_subdev_state *state;
>>   	int rval;
>>   
>> -	state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
>> -
>>   	switch (cmd) {
>>   	case VIDIOC_SUBDEV_QUERYCAP: {
>>   		struct v4l2_subdev_capability *cap = arg;
>> @@ -707,8 +705,24 @@ static long subdev_do_ioctl_lock(struct file *file, unsigned int cmd, void *arg)
>>   
>>   	if (lock && mutex_lock_interruptible(lock))
>>   		return -ERESTARTSYS;
>> -	if (video_is_registered(vdev))
>> -		ret = subdev_do_ioctl(file, cmd, arg);
>> +
>> +	if (video_is_registered(vdev)) {
>> +		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);
>> +		struct v4l2_subdev_state *state;
>> +
>> +		state = subdev_ioctl_get_state(sd, subdev_fh, cmd, arg);
>> +
>> +		if (state)
>> +			v4l2_subdev_lock_state(state);
>> +
>> +		ret = subdev_do_ioctl(file, cmd, arg, state);
>> +
>> +		if (state)
>> +			v4l2_subdev_unlock_state(state);
>> +	}
>> +
>>   	if (lock)
>>   		mutex_unlock(lock);
>>   	return ret;
>> @@ -864,7 +878,7 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad,
>>   			media_entity_to_v4l2_subdev(pad->entity);
>>   		struct v4l2_subdev_state *state;
>>   
>> -		state = v4l2_subdev_get_active_state(sd);
>> +		state = v4l2_subdev_get_locked_active_state(sd);
>>   
>>   		fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>   		fmt->pad = pad->index;
>> @@ -906,7 +920,9 @@ int v4l2_subdev_link_validate(struct media_link *link)
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>>   
>> -struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
>> +struct v4l2_subdev_state *
>> +__v4l2_subdev_state_alloc(struct v4l2_subdev *sd, const char *lock_name,
>> +			  struct lock_class_key *lock_key)
>>   {
>>   	struct v4l2_subdev_state *state;
>>   	int ret;
>> @@ -915,6 +931,12 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
>>   	if (!state)
>>   		return ERR_PTR(-ENOMEM);
>>   
>> +	__mutex_init(&state->_lock, lock_name, lock_key);
>> +	if (sd->state_lock)
>> +		state->lock = sd->state_lock;
>> +	else
>> +		state->lock = &state->_lock;
>> +
>>   	if (sd->entity.num_pads) {
>>   		state->pads = kvmalloc_array(sd->entity.num_pads,
>>   					     sizeof(*state->pads),
>> @@ -925,7 +947,14 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd)
>>   		}
>>   	}
>>   
>> +	/*
>> +	 * There can be no race at this point, but we lock the state anyway to
>> +	 * satisfy lockdep checks.
>> +	 */
>> +	v4l2_subdev_lock_state(state);
>>   	ret = v4l2_subdev_call(sd, pad, init_cfg, state);
>> +	v4l2_subdev_unlock_state(state);
>> +
>>   	if (ret < 0 && ret != -ENOIOCTLCMD)
>>   		goto err;
>>   
>> @@ -946,16 +975,19 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
>>   	if (!state)
>>   		return;
>>   
>> +	mutex_destroy(&state->_lock);
>> +
>>   	kvfree(state->pads);
>>   	kfree(state);
>>   }
>>   EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
>>   
>> -int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
>> +int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
>> +				struct lock_class_key *key)
>>   {
>>   	struct v4l2_subdev_state *state;
>>   
>> -	state = __v4l2_subdev_state_alloc(sd);
>> +	state = __v4l2_subdev_state_alloc(sd, name, key);
>>   	if (IS_ERR(state))
>>   		return PTR_ERR(state);
>>   
>> @@ -963,7 +995,7 @@ int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
>>   
>>   	return 0;
>>   }
>> -EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
>> +EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
>>   
>>   void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>>   {
>> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
>> index 07d368f345cd..8e184aa4c252 100644
>> --- a/drivers/staging/media/tegra-video/vi.c
>> +++ b/drivers/staging/media/tegra-video/vi.c
>> @@ -491,6 +491,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
>>   				      struct v4l2_pix_format *pix)
>>   {
>>   	const struct tegra_video_format *fmtinfo;
>> +	static struct lock_class_key key;
>>   	struct v4l2_subdev *subdev;
>>   	struct v4l2_subdev_format fmt;
>>   	struct v4l2_subdev_state *sd_state;
>> @@ -511,7 +512,8 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
>>   	 * FIXME: Drop this call, drivers are not supposed to use
>>   	 * __v4l2_subdev_state_alloc().
>>   	 */
>> -	sd_state = __v4l2_subdev_state_alloc(subdev);
>> +	sd_state = __v4l2_subdev_state_alloc(subdev, "tegra:state->lock",
>> +					     &key);
>>   	if (IS_ERR(sd_state))
>>   		return PTR_ERR(sd_state);
>>   	/*
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 1bbe4383966c..71d13d160d99 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -658,6 +658,8 @@ struct v4l2_subdev_pad_config {
>>   /**
>>    * 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
>>    *
>>    * This structure only needs to be passed to the pad op if the 'which' field
>> @@ -665,6 +667,9 @@ struct v4l2_subdev_pad_config {
>>    * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
>>    */
>>   struct v4l2_subdev_state {
>> +	/* lock for the struct v4l2_subdev_state fields */
>> +	struct mutex _lock;
>> +	struct mutex *lock;
>>   	struct v4l2_subdev_pad_config *pads;
>>   };
>>   
>> @@ -888,6 +893,9 @@ struct v4l2_subdev_platform_data {
>>    * @subdev_notifier: A sub-device notifier implicitly registered for the sub-
>>    *		     device using v4l2_async_register_subdev_sensor().
>>    * @pdata: common part of subdevice platform data
>> + * @state_lock: A pointer to a lock used for all the subdev's states, set by the
>> + *		driver. This is	optional. If NULL, each state instance will get
>> + *		a lock of its own.
>>    * @active_state: Active state for the subdev (NULL for subdevs tracking the
>>    *		  state internally). Initialized by calling
>>    *		  v4l2_subdev_init_finalize().
>> @@ -922,6 +930,7 @@ struct v4l2_subdev {
>>   	struct v4l2_async_notifier *notifier;
>>   	struct v4l2_async_notifier *subdev_notifier;
>>   	struct v4l2_subdev_platform_data *pdata;
>> +	struct mutex *state_lock;
>>   
>>   	/*
>>   	 * The fields below are private, and should only be accessed via
>> @@ -1144,12 +1153,16 @@ int v4l2_subdev_link_validate(struct media_link *link);
>>    * __v4l2_subdev_state_alloc - allocate v4l2_subdev_state
>>    *
>>    * @sd: pointer to &struct v4l2_subdev for which the state is being allocated.
>> + * @lock_name: name of the state lock
>> + * @key: lock_class_key for the lock
>>    *
>>    * Must call __v4l2_subdev_state_free() when state is no longer needed.
>>    *
>>    * Not to be called directly by the drivers.
>>    */
>> -struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
>> +struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd,
>> +						    const char *lock_name,
>> +						    struct lock_class_key *key);
>>   
>>   /**
>>    * __v4l2_subdev_state_free - free a v4l2_subdev_state
>> @@ -1174,7 +1187,16 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
>>    *
>>    * The user must call v4l2_subdev_cleanup() when the subdev is being removed.
>>    */
>> -int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
>> +#define v4l2_subdev_init_finalize(sd)                                          \
>> +	({                                                                     \
>> +		static struct lock_class_key __key;                            \
>> +		const char *name = KBUILD_BASENAME                             \
>> +			":" __stringify(__LINE__) ":sd->active_state->lock";   \
>> +		__v4l2_subdev_init_finalize(sd, name, &__key);                 \
>> +	})
>> +
>> +int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
>> +				struct lock_class_key *key);
>>   
>>   /**
>>    * v4l2_subdev_cleanup() - Releases the resources allocated by the subdevice
>> @@ -1191,14 +1213,83 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
>>    * @sd: The subdevice
>>    *
>>    * Returns the active state for the subdevice, or NULL if the subdev does not
>> - * support active state.
>> + * support active state. If the state is not NULL, calls
>> + * lockdep_assert_not_held() to issue a warning if the state is locked.
>> + *
>> + * This function is to be used e.g. when getting the active state for the sole
>> + * purpose of passing it forward, without accessing the state fields.
>>    */
>>   static inline struct v4l2_subdev_state *
>>   v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
>>   {
>> +	if (sd->active_state)
>> +		lockdep_assert_not_held(sd->active_state->lock);
>> +	return sd->active_state;
>> +}
>> +
>> +/**
>> + * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
>> + *					   is locked and returns it
>> + *
>> + * @sd: The subdevice
>> + *
>> + * Returns the active state for the subdevice, or NULL if the subdev does not
>> + * support active state. If the state is not NULL, calls lockdep_assert_held()
>> + * to issue a warning if the state is not locked.
>> + *
>> + * This function is to be used when the caller knows that the active state is
>> + * already locked.
>> + */
>> +static inline struct v4l2_subdev_state *
>> +v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
>> +{
>> +	if (sd->active_state)
>> +		lockdep_assert_held(sd->active_state->lock);
>>   	return sd->active_state;
>>   }
> 
> I think these two function names are quite confusing.
> 
> Better options are:
> 
> v4l2_subdev_get_unlocked_active_state
> v4l2_subdev_get_locked_active_state

Valid point. I changed v4l2_subdev_get_active_state to 
v4l2_subdev_get_unlocked_active_state.

> or:
> 
> __v4l2_subdev_get_active_state (assumes unlocked state)
> v4l2_subdev_get_active_state (assumes locked state)
> 
> In the current naming scheme there is no indication that v4l2_subdev_get_active_state
> assumes the state is unlocked.
> 
>>   
>> +/**
>> + * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
>> + *					     state for the subdevice
>> + * @sd: The subdevice
>> + *
>> + * Returns the locked active state for the subdevice, or NULL if the subdev
>> + * does not support active state.
>> + *
>> + * The state must be unlocked with v4l2_subdev_unlock_state() after use.
>> + */
>> +static inline struct v4l2_subdev_state *
>> +v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
>> +{
>> +	mutex_lock(sd->active_state->lock);
>> +
>> +	return sd->active_state;
>> +}
> 
> I think this inline should be moved to below v4l2_subdev_lock_state...
> 
>> +
>> +/**
>> + * v4l2_subdev_lock_state() - Locks the subdev state
>> + * @state: The subdevice state
>> + *
>> + * Locks the given subdev state.
>> + *
>> + * The state must be unlocked with v4l2_subdev_unlock_state() after use.
>> + */
>> +static inline void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
>> +{
>> +	mutex_lock(state->lock);
>> +}
> 
> ...since here it can use v4l2_subdev_lock_state(sd->active_state).
> 
>> +
>> +/**
>> + * v4l2_subdev_unlock_state() - Unlocks the subdev state
>> + * @state: The subdevice state
>> + *
>> + * Unlocks the given subdev state.
>> + */
>> +static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
>> +{
>> +	mutex_unlock(state->lock);
>> +}
> 
> It can also be moved here, I have no preference.

Ok. I re-arranged the functions a bit, and used v4l2_subdev_lock_state() 
in v4l2_subdev_lock_and_get_active_state().

I'll send a v6 after we come to a conclusion on patch 5.

  Tomi

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Hans Verkuil]

On 3/7/22 08:16, Tomi Valkeinen wrote:
> Hi Hans,
> 
> On 04/03/2022 15:34, Hans Verkuil wrote:
>> Hi Tomi,
>>
>> On 3/1/22 11:55, Tomi Valkeinen wrote:
>>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>>> that take a subdev state as a parameter. Normally the v4l2 framework
>>> will lock and pass the correct subdev state to the subdev ops, but there
>>> are legacy situations where this is not the case (e.g. non-MC video
>>> device driver calling set_fmt in a source subdev).
>>>
>>> As this macro is only needed for legacy use cases, the macro is added in
>>> a new header file, v4l2-subdev-legacy.h.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>   include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>>   create mode 100644 include/media/v4l2-subdev-legacy.h
>>>
>>> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
>>> new file mode 100644
>>> index 000000000000..6a61e579b629
>>> --- /dev/null
>>> +++ b/include/media/v4l2-subdev-legacy.h
>>> @@ -0,0 +1,42 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + *  V4L2 sub-device legacy support header.
>>> + *
>>> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> + */
>>> +
>>> +#ifndef _V4L2_SUBDEV_LEGACY_H
>>> +#define _V4L2_SUBDEV_LEGACY_H
>>> +
>>> +/**
>>> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
>>> + *                   takes state as a parameter, passing the
>>> + *                   subdev its active state.
>>> + *
>>> + * @sd: pointer to the &struct v4l2_subdev
>>> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
>>> + *     Each element there groups a set of callbacks functions.
>>> + * @f: callback function to be called.
>>> + *     The callback functions are defined in groups, according to
>>> + *     each element at &struct v4l2_subdev_ops.
>>> + * @args: arguments for @f.
>>> + *
>>> + * This is similar to v4l2_subdev_call(), except that this version can only be
>>> + * used for ops that take a subdev state as a parameter. The macro will get the
>>> + * active state and lock it before calling the op, and unlock it after the
>>> + * call.
>>> + */
>>
>> You should explain why this is a legacy macro and, ideally, what would need to
>> be done to get rid of it. The first is in the commit log, but nobody reads that :-)
>>
>> But if just using it in a non-MC video device driver constitutes 'legacy' use,
>> then I disagree with that. There are many non-MC video device drivers, nothing
>> legacy about that.
> 
> It's difficult to define all the scenarios where this can be used, but the ones I can imagine fall under legacy (depending on how you define that, though).
> 
> I use this in CAL driver, which supports non-MC (legacy) and MC. CAL has a bunch of video devices (one for each DMA engine) and two CSI-2 PHY devices (v4l2 subdevs).
> 
> When operating in MC mode, the userspace will call, e.g., set_fmt in the PHY subdev, and so forth.
> 
> But in non-MC case the userspace calls VIDIOC_S_FMT in the video dev, and the video dev has to propagate that to the PHY subdev. I do this propagation using the v4l2_subdev_call_state_active macro.
> 
> I don't know if there are other drivers that support both non-MC and MC modes. I could also just move this macro to the CAL driver, and we could add this to the v4l2 framework if we see other drivers using similar constructs.

It is common to have non-MC drivers that call set_fmt of a subdev.
Wouldn't they all need to use this helper macro? If so, then this is NOT a
legacy use, it's just a non-MC driver use.

Am I missing something?

Regards,

	Hans

> 
>  Tomi

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Tomi Valkeinen]

Hi Hans,

On 07/03/2022 10:36, Hans Verkuil wrote:
> 
> 
> On 3/7/22 08:16, Tomi Valkeinen wrote:
>> Hi Hans,
>>
>> On 04/03/2022 15:34, Hans Verkuil wrote:
>>> Hi Tomi,
>>>
>>> On 3/1/22 11:55, Tomi Valkeinen wrote:
>>>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>>>> that take a subdev state as a parameter. Normally the v4l2 framework
>>>> will lock and pass the correct subdev state to the subdev ops, but there
>>>> are legacy situations where this is not the case (e.g. non-MC video
>>>> device driver calling set_fmt in a source subdev).
>>>>
>>>> As this macro is only needed for legacy use cases, the macro is added in
>>>> a new header file, v4l2-subdev-legacy.h.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>    include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
>>>>    1 file changed, 42 insertions(+)
>>>>    create mode 100644 include/media/v4l2-subdev-legacy.h
>>>>
>>>> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
>>>> new file mode 100644
>>>> index 000000000000..6a61e579b629
>>>> --- /dev/null
>>>> +++ b/include/media/v4l2-subdev-legacy.h
>>>> @@ -0,0 +1,42 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + *  V4L2 sub-device legacy support header.
>>>> + *
>>>> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> + */
>>>> +
>>>> +#ifndef _V4L2_SUBDEV_LEGACY_H
>>>> +#define _V4L2_SUBDEV_LEGACY_H
>>>> +
>>>> +/**
>>>> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
>>>> + *                   takes state as a parameter, passing the
>>>> + *                   subdev its active state.
>>>> + *
>>>> + * @sd: pointer to the &struct v4l2_subdev
>>>> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
>>>> + *     Each element there groups a set of callbacks functions.
>>>> + * @f: callback function to be called.
>>>> + *     The callback functions are defined in groups, according to
>>>> + *     each element at &struct v4l2_subdev_ops.
>>>> + * @args: arguments for @f.
>>>> + *
>>>> + * This is similar to v4l2_subdev_call(), except that this version can only be
>>>> + * used for ops that take a subdev state as a parameter. The macro will get the
>>>> + * active state and lock it before calling the op, and unlock it after the
>>>> + * call.
>>>> + */
>>>
>>> You should explain why this is a legacy macro and, ideally, what would need to
>>> be done to get rid of it. The first is in the commit log, but nobody reads that :-)
>>>
>>> But if just using it in a non-MC video device driver constitutes 'legacy' use,
>>> then I disagree with that. There are many non-MC video device drivers, nothing
>>> legacy about that.
>>
>> It's difficult to define all the scenarios where this can be used, but the ones I can imagine fall under legacy (depending on how you define that, though).
>>
>> I use this in CAL driver, which supports non-MC (legacy) and MC. CAL has a bunch of video devices (one for each DMA engine) and two CSI-2 PHY devices (v4l2 subdevs).
>>
>> When operating in MC mode, the userspace will call, e.g., set_fmt in the PHY subdev, and so forth.
>>
>> But in non-MC case the userspace calls VIDIOC_S_FMT in the video dev, and the video dev has to propagate that to the PHY subdev. I do this propagation using the v4l2_subdev_call_state_active macro.
>>
>> I don't know if there are other drivers that support both non-MC and MC modes. I could also just move this macro to the CAL driver, and we could add this to the v4l2 framework if we see other drivers using similar constructs.
> 
> It is common to have non-MC drivers that call set_fmt of a subdev.
> Wouldn't they all need to use this helper macro? If so, then this is NOT a
> legacy use, it's just a non-MC driver use.

These non-MC drivers that call set_fmt of a subdev, they're video device 
drivers, right? In other words, there are no subdev drivers that call 
set_fmt on other subdevs?

This does get a bit complex, keeping the old and new code working 
together. In this context, I think we have three different "classes":

1. non-MC
2. MC, no state support
3. MC, state support

We have classes 1 and 2 in upstream, and 3 will be enabled with this 
series (and expanded with the streams series).

Classes 1 and 2 continue working as before. If you have a pipeline with 
only class 3 drivers, it works without any legacy "hacks". The problems 
come when you combine 1 or 2 with 3. Or possibly the problems appear 
only when combining class 1 and class 3, as class 2 drivers are not 
supposed to call subdev ops which take a state parameter on other subdevs.

A class 3 driver expects to get either a try or an active state as a 
parameter, but class 1 drivers pass NULL for the active state. If you 
write a class 3 driver and want it to work with class 1 (without any 
changes to those drivers), you must do extra plumbing in the ops 
functions, to catch the NULL state case and get & lock the state 
yourself. If you do that, this macro is not needed.

Alternatively, class 1 drivers could be changed to use this macro, so 
that a possible class 3 driver in the pipeline would work without 
additional code. But there are a lot of class 1 drivers, and thus 
modifications, and I wasn't planning to go that way.

The CAL driver I mentioned supports both class 1 and class 3 (via a 
module parameter) in the video dev driver, and the class 1 mode uses 
this macro as CAL's PHY subdev (part of the same driver) is a class 3 
subdev. The class 1 support is legacy support in CAL's case.

So... Depending on what kind of driver combinations we want to support, 
this may or may not be legacy, depending on how you define legacy =).

  Tomi

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Hans Verkuil]

Hi Tomi,

On 3/7/22 10:16, Tomi Valkeinen wrote:
> Hi Hans,
> 
> On 07/03/2022 10:36, Hans Verkuil wrote:
>>
>>
>> On 3/7/22 08:16, Tomi Valkeinen wrote:
>>> Hi Hans,
>>>
>>> On 04/03/2022 15:34, Hans Verkuil wrote:
>>>> Hi Tomi,
>>>>
>>>> On 3/1/22 11:55, Tomi Valkeinen wrote:
>>>>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>>>>> that take a subdev state as a parameter. Normally the v4l2 framework
>>>>> will lock and pass the correct subdev state to the subdev ops, but there
>>>>> are legacy situations where this is not the case (e.g. non-MC video
>>>>> device driver calling set_fmt in a source subdev).
>>>>>
>>>>> As this macro is only needed for legacy use cases, the macro is added in
>>>>> a new header file, v4l2-subdev-legacy.h.
>>>>>
>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> ---
>>>>>    include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
>>>>>    1 file changed, 42 insertions(+)
>>>>>    create mode 100644 include/media/v4l2-subdev-legacy.h
>>>>>
>>>>> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
>>>>> new file mode 100644
>>>>> index 000000000000..6a61e579b629
>>>>> --- /dev/null
>>>>> +++ b/include/media/v4l2-subdev-legacy.h
>>>>> @@ -0,0 +1,42 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + *  V4L2 sub-device legacy support header.
>>>>> + *
>>>>> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> + */
>>>>> +
>>>>> +#ifndef _V4L2_SUBDEV_LEGACY_H
>>>>> +#define _V4L2_SUBDEV_LEGACY_H
>>>>> +
>>>>> +/**
>>>>> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
>>>>> + *                   takes state as a parameter, passing the
>>>>> + *                   subdev its active state.
>>>>> + *
>>>>> + * @sd: pointer to the &struct v4l2_subdev
>>>>> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
>>>>> + *     Each element there groups a set of callbacks functions.
>>>>> + * @f: callback function to be called.
>>>>> + *     The callback functions are defined in groups, according to
>>>>> + *     each element at &struct v4l2_subdev_ops.
>>>>> + * @args: arguments for @f.
>>>>> + *
>>>>> + * This is similar to v4l2_subdev_call(), except that this version can only be
>>>>> + * used for ops that take a subdev state as a parameter. The macro will get the
>>>>> + * active state and lock it before calling the op, and unlock it after the
>>>>> + * call.
>>>>> + */
>>>>
>>>> You should explain why this is a legacy macro and, ideally, what would need to
>>>> be done to get rid of it. The first is in the commit log, but nobody reads that :-)
>>>>
>>>> But if just using it in a non-MC video device driver constitutes 'legacy' use,
>>>> then I disagree with that. There are many non-MC video device drivers, nothing
>>>> legacy about that.
>>>
>>> It's difficult to define all the scenarios where this can be used, but the ones I can imagine fall under legacy (depending on how you define that, though).
>>>
>>> I use this in CAL driver, which supports non-MC (legacy) and MC. CAL has a bunch of video devices (one for each DMA engine) and two CSI-2 PHY devices (v4l2 subdevs).
>>>
>>> When operating in MC mode, the userspace will call, e.g., set_fmt in the PHY subdev, and so forth.
>>>
>>> But in non-MC case the userspace calls VIDIOC_S_FMT in the video dev, and the video dev has to propagate that to the PHY subdev. I do this propagation using the v4l2_subdev_call_state_active macro.
>>>
>>> I don't know if there are other drivers that support both non-MC and MC modes. I could also just move this macro to the CAL driver, and we could add this to the v4l2 framework if we see other drivers using similar constructs.
>>
>> It is common to have non-MC drivers that call set_fmt of a subdev.
>> Wouldn't they all need to use this helper macro? If so, then this is NOT a
>> legacy use, it's just a non-MC driver use.
> 
> These non-MC drivers that call set_fmt of a subdev, they're video device drivers, right? In other words, there are no subdev drivers that call set_fmt on other subdevs?

Probably not, but I am not 100% certain. There are a few nested subdev cases, but I don't remember which.

> 
> This does get a bit complex, keeping the old and new code working together. In this context, I think we have three different "classes":
> 
> 1. non-MC
> 2. MC, no state support
> 3. MC, state support

Are you talking about subdev drivers or bridge drivers? It's a bit confusing. I'm assuming it can be either.

> 
> We have classes 1 and 2 in upstream, and 3 will be enabled with this series (and expanded with the streams series).
> 
> Classes 1 and 2 continue working as before. If you have a pipeline with only class 3 drivers, it works without any legacy "hacks". The problems come when you combine 1 or 2 with 3. Or possibly the problems appear only when combining class 1 and class 3, as class 2 drivers are not supposed to call subdev ops which take a state parameter on other subdevs.
> 
> A class 3 driver expects to get either a try or an active state as a parameter, but class 1 drivers pass NULL for the active state. If you write a class 3 driver and want it to work with class 1 (without any changes to those drivers), you must do extra plumbing in the ops functions, to catch the NULL state case and get & lock the state yourself. If you do that, this macro is not needed.
> 
> Alternatively, class 1 drivers could be changed to use this macro, so that a possible class 3 driver in the pipeline would work without additional code. But there are a lot of class 1 drivers, and thus modifications, and I wasn't planning to go that way.
> 
> The CAL driver I mentioned supports both class 1 and class 3 (via a module parameter) in the video dev driver, and the class 1 mode uses this macro as CAL's PHY subdev (part of the same driver) is a class 3 subdev. The class 1 support is legacy support in CAL's case.
> 
> So... Depending on what kind of driver combinations we want to support, this may or may not be legacy, depending on how you define legacy =).

Let me try to explain what my concerns are. Eventually I would really like all subdevs to be capable of working with MC bridge drivers, i.e. have state support. Bridge drivers can be either MC or non-MC.

So I would like to know:

1.1) How to convert a subdev driver to a MC state-aware subdev driver?
1.2) What is the legacy code that such a MC state-aware subdev driver has to keep in order to work with older bridge drivers that do not support such subdev drivers? (i.e. they pass NULL as the state)

2.1) How to convert a bridge driver (either non-MC or MC, but no state support) to properly support a fully converted subdev (MC state-aware) driver?
2.2) What is the legacy code that such a bridge driver has to keep in order to work with older subdev drivers that are not yet MC state-aware?

The code needed for 1.2 and 2.2 (helper functions/macros) is legacy code, and can be marked as such.

If this is clear, then we can work towards converting both subdev and bridge drivers and eventually (might take years!) get rid of the legacy code.

Removing support for case 2 is probably something that we want to do sooner than later.

For the CAL driver I do not consider non-MC support as legacy. It's legacy in the context of the CAL driver only, but API-wise it is not since there are many non-MC bridge drivers.

Converting all subdev drivers to support the same MC state mechanism is time consuming, but we've done such things before.

Regards,

	Hans

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Tomi Valkeinen]

On 07/03/2022 11:51, Hans Verkuil wrote:
> Hi Tomi,
> 
> On 3/7/22 10:16, Tomi Valkeinen wrote:
>> Hi Hans,
>>
>> On 07/03/2022 10:36, Hans Verkuil wrote:
>>>
>>>
>>> On 3/7/22 08:16, Tomi Valkeinen wrote:
>>>> Hi Hans,
>>>>
>>>> On 04/03/2022 15:34, Hans Verkuil wrote:
>>>>> Hi Tomi,
>>>>>
>>>>> On 3/1/22 11:55, Tomi Valkeinen wrote:
>>>>>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>>>>>> that take a subdev state as a parameter. Normally the v4l2 framework
>>>>>> will lock and pass the correct subdev state to the subdev ops, but there
>>>>>> are legacy situations where this is not the case (e.g. non-MC video
>>>>>> device driver calling set_fmt in a source subdev).
>>>>>>
>>>>>> As this macro is only needed for legacy use cases, the macro is added in
>>>>>> a new header file, v4l2-subdev-legacy.h.
>>>>>>
>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>> ---
>>>>>>     include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 42 insertions(+)
>>>>>>     create mode 100644 include/media/v4l2-subdev-legacy.h
>>>>>>
>>>>>> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
>>>>>> new file mode 100644
>>>>>> index 000000000000..6a61e579b629
>>>>>> --- /dev/null
>>>>>> +++ b/include/media/v4l2-subdev-legacy.h
>>>>>> @@ -0,0 +1,42 @@
>>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>>> +/*
>>>>>> + *  V4L2 sub-device legacy support header.
>>>>>> + *
>>>>>> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef _V4L2_SUBDEV_LEGACY_H
>>>>>> +#define _V4L2_SUBDEV_LEGACY_H
>>>>>> +
>>>>>> +/**
>>>>>> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
>>>>>> + *                   takes state as a parameter, passing the
>>>>>> + *                   subdev its active state.
>>>>>> + *
>>>>>> + * @sd: pointer to the &struct v4l2_subdev
>>>>>> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
>>>>>> + *     Each element there groups a set of callbacks functions.
>>>>>> + * @f: callback function to be called.
>>>>>> + *     The callback functions are defined in groups, according to
>>>>>> + *     each element at &struct v4l2_subdev_ops.
>>>>>> + * @args: arguments for @f.
>>>>>> + *
>>>>>> + * This is similar to v4l2_subdev_call(), except that this version can only be
>>>>>> + * used for ops that take a subdev state as a parameter. The macro will get the
>>>>>> + * active state and lock it before calling the op, and unlock it after the
>>>>>> + * call.
>>>>>> + */
>>>>>
>>>>> You should explain why this is a legacy macro and, ideally, what would need to
>>>>> be done to get rid of it. The first is in the commit log, but nobody reads that :-)
>>>>>
>>>>> But if just using it in a non-MC video device driver constitutes 'legacy' use,
>>>>> then I disagree with that. There are many non-MC video device drivers, nothing
>>>>> legacy about that.
>>>>
>>>> It's difficult to define all the scenarios where this can be used, but the ones I can imagine fall under legacy (depending on how you define that, though).
>>>>
>>>> I use this in CAL driver, which supports non-MC (legacy) and MC. CAL has a bunch of video devices (one for each DMA engine) and two CSI-2 PHY devices (v4l2 subdevs).
>>>>
>>>> When operating in MC mode, the userspace will call, e.g., set_fmt in the PHY subdev, and so forth.
>>>>
>>>> But in non-MC case the userspace calls VIDIOC_S_FMT in the video dev, and the video dev has to propagate that to the PHY subdev. I do this propagation using the v4l2_subdev_call_state_active macro.
>>>>
>>>> I don't know if there are other drivers that support both non-MC and MC modes. I could also just move this macro to the CAL driver, and we could add this to the v4l2 framework if we see other drivers using similar constructs.
>>>
>>> It is common to have non-MC drivers that call set_fmt of a subdev.
>>> Wouldn't they all need to use this helper macro? If so, then this is NOT a
>>> legacy use, it's just a non-MC driver use.
>>
>> These non-MC drivers that call set_fmt of a subdev, they're video device drivers, right? In other words, there are no subdev drivers that call set_fmt on other subdevs?
> 
> Probably not, but I am not 100% certain. There are a few nested subdev cases, but I don't remember which.
> 
>>
>> This does get a bit complex, keeping the old and new code working together. In this context, I think we have three different "classes":
>>
>> 1. non-MC
>> 2. MC, no state support
>> 3. MC, state support
> 
> Are you talking about subdev drivers or bridge drivers? It's a bit confusing. I'm assuming it can be either.

Yes, I mean either one.

>> We have classes 1 and 2 in upstream, and 3 will be enabled with this series (and expanded with the streams series).
>>
>> Classes 1 and 2 continue working as before. If you have a pipeline with only class 3 drivers, it works without any legacy "hacks". The problems come when you combine 1 or 2 with 3. Or possibly the problems appear only when combining class 1 and class 3, as class 2 drivers are not supposed to call subdev ops which take a state parameter on other subdevs.
>>
>> A class 3 driver expects to get either a try or an active state as a parameter, but class 1 drivers pass NULL for the active state. If you write a class 3 driver and want it to work with class 1 (without any changes to those drivers), you must do extra plumbing in the ops functions, to catch the NULL state case and get & lock the state yourself. If you do that, this macro is not needed.
>>
>> Alternatively, class 1 drivers could be changed to use this macro, so that a possible class 3 driver in the pipeline would work without additional code. But there are a lot of class 1 drivers, and thus modifications, and I wasn't planning to go that way.
>>
>> The CAL driver I mentioned supports both class 1 and class 3 (via a module parameter) in the video dev driver, and the class 1 mode uses this macro as CAL's PHY subdev (part of the same driver) is a class 3 subdev. The class 1 support is legacy support in CAL's case.
>>
>> So... Depending on what kind of driver combinations we want to support, this may or may not be legacy, depending on how you define legacy =).
> 
> Let me try to explain what my concerns are. Eventually I would really like all subdevs to be capable of working with MC bridge drivers, i.e. have state support. Bridge drivers can be either MC or non-MC.
> 
> So I would like to know:
> 
> 1.1) How to convert a subdev driver to a MC state-aware subdev driver?

I presume you mean how to convert an MC subdev driver to state-aware MC 
driver. If you have a non-MC subdev driver, then that first needs to be 
converted to an MC driver, which is out of scope here.

Here's an example commit where I convert OV10635 to streams:

https://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git/commit/?h=streams/work-v11&id=ffb08af15f04ef2ce0a00bb356e957c839d6bccb

However, the commit is on top of the streams series and I add support 
for multiple streams in addition to the active state, so 1) it's not as 
simple as it could be (e.g. get_frame_desc() and set_routing() ops could 
be left out), and 2) it's not using the old-style v4l2_subdev_pad_config 
(which is the only option on top of this series), but the new routes & 
streams.

So the example doesn't quite answer to your question... I haven't looked 
at implementing a driver which would be state-aware but not 
streams-aware, but I think it's essentially just:

- Use v4l2_subdev_init_finalize() to create the active state storage
- Always use state->pads instead of something stored in the driver's 
private data for active case.

> 1.2) What is the legacy code that such a MC state-aware subdev driver has to keep in order to work with older bridge drivers that do not support such subdev drivers? (i.e. they pass NULL as the state)

I believe the only extra code needed is to handle the state == NULL 
case. This means adding code to each subdev op which has the state as a 
parameter, and doing, perhaps, something like this:

my_set_fmt(sd, _state, fmt)
{
	state = _state

	if (!_state)
		state = v4l2_subdev_lock_and_get_active_state(sd);

	... use 'state' here ...

	if (!_state)
		v4l2_subdev_unlock_state(state);
}

Maybe we can somehow macro-ify the above, which creates a wrapper for 
the op. Or, as I mentioned, we could try to change all the drivers that 
do those calls, so that they use the macro in this patch instead.

> 2.1) How to convert a bridge driver (either non-MC or MC, but no state support) to properly support a fully converted subdev (MC state-aware) driver?

Converting non-MC driver to an MC driver is out of the context here, as 
it's not related to the active state. An MC bridge driver should work 
fine with state-aware subdev drivers, as the bridge driver should not 
call any of the subdev's state-related ops.

To make a non-MC bridge driver support state-aware subdev drivers, they 
can use the macro in this patch.

> 2.2) What is the legacy code that such a bridge driver has to keep in order to work with older subdev drivers that are not yet MC state-aware?

The older subdev drivers should keep working without any extra code.

> The code needed for 1.2 and 2.2 (helper functions/macros) is legacy code, and can be marked as such.
> > If this is clear, then we can work towards converting both subdev and 
bridge drivers and eventually (might take years!) get rid of the legacy 
code.
> 
> Removing support for case 2 is probably something that we want to do sooner than later.
> 
> For the CAL driver I do not consider non-MC support as legacy. It's legacy in the context of the CAL driver only, but API-wise it is not since there are many non-MC bridge drivers.

That's true, but also, non-MC bridge drivers do not need to use this 
function if the subdev drivers use the method shown in 1.2. I think this 
is the question here:

- Change all the callers and use the macro in this patch. Then the macro 
is not legacy.
- Change the callees, in which case this macro is needed only in some 
cases where, for whatever reason, a specific callee has not been changed 
(yet?). In this case it's legacy.

Changing the callers would be a nicer option, I think, but I also fear 
that it's very difficult and easily brings in bugs. I haven't looked 
closely, but I think it would be a big patch.

And it's not clear to me if there's a benefit: do all those drivers ever 
need to interact a state-aware subdev driver? If they do, maybe there 
are only a few such subdev drivers, and it's not a big issue to have the 
lock/unlock code in those state-aware subdev drivers. All the other 
state-aware subdev drivers would not need the legacy support code as 
they're used only in more modern pipelines.

  Tomi

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Tomi Valkeinen]

Hi Hans,

On 07/03/2022 16:00, Tomi Valkeinen wrote:
> On 07/03/2022 11:51, Hans Verkuil wrote:
>> Hi Tomi,
>>
>> On 3/7/22 10:16, Tomi Valkeinen wrote:
>>> Hi Hans,
>>>
>>> On 07/03/2022 10:36, Hans Verkuil wrote:
>>>>
>>>>
>>>> On 3/7/22 08:16, Tomi Valkeinen wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On 04/03/2022 15:34, Hans Verkuil wrote:
>>>>>> Hi Tomi,
>>>>>>
>>>>>> On 3/1/22 11:55, Tomi Valkeinen wrote:
>>>>>>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>>>>>>> that take a subdev state as a parameter. Normally the v4l2 framework
>>>>>>> will lock and pass the correct subdev state to the subdev ops, 
>>>>>>> but there
>>>>>>> are legacy situations where this is not the case (e.g. non-MC video
>>>>>>> device driver calling set_fmt in a source subdev).
>>>>>>>
>>>>>>> As this macro is only needed for legacy use cases, the macro is 
>>>>>>> added in
>>>>>>> a new header file, v4l2-subdev-legacy.h.
>>>>>>>
>>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>> ---
>>>>>>>     include/media/v4l2-subdev-legacy.h | 42 
>>>>>>> ++++++++++++++++++++++++++++++
>>>>>>>     1 file changed, 42 insertions(+)
>>>>>>>     create mode 100644 include/media/v4l2-subdev-legacy.h
>>>>>>>
>>>>>>> diff --git a/include/media/v4l2-subdev-legacy.h 
>>>>>>> b/include/media/v4l2-subdev-legacy.h
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..6a61e579b629
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/media/v4l2-subdev-legacy.h
>>>>>>> @@ -0,0 +1,42 @@
>>>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>>>> +/*
>>>>>>> + *  V4L2 sub-device legacy support header.
>>>>>>> + *
>>>>>>> + *  Copyright (C) 2022  Tomi Valkeinen 
>>>>>>> <tomi.valkeinen@ideasonboard.com>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef _V4L2_SUBDEV_LEGACY_H
>>>>>>> +#define _V4L2_SUBDEV_LEGACY_H
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * v4l2_subdev_call_state_active - call an operation of a 
>>>>>>> v4l2_subdev which
>>>>>>> + *                   takes state as a parameter, passing the
>>>>>>> + *                   subdev its active state.
>>>>>>> + *
>>>>>>> + * @sd: pointer to the &struct v4l2_subdev
>>>>>>> + * @o: name of the element at &struct v4l2_subdev_ops that 
>>>>>>> contains @f.
>>>>>>> + *     Each element there groups a set of callbacks functions.
>>>>>>> + * @f: callback function to be called.
>>>>>>> + *     The callback functions are defined in groups, according to
>>>>>>> + *     each element at &struct v4l2_subdev_ops.
>>>>>>> + * @args: arguments for @f.
>>>>>>> + *
>>>>>>> + * This is similar to v4l2_subdev_call(), except that this 
>>>>>>> version can only be
>>>>>>> + * used for ops that take a subdev state as a parameter. The 
>>>>>>> macro will get the
>>>>>>> + * active state and lock it before calling the op, and unlock it 
>>>>>>> after the
>>>>>>> + * call.
>>>>>>> + */
>>>>>>
>>>>>> You should explain why this is a legacy macro and, ideally, what 
>>>>>> would need to
>>>>>> be done to get rid of it. The first is in the commit log, but 
>>>>>> nobody reads that :-)
>>>>>>
>>>>>> But if just using it in a non-MC video device driver constitutes 
>>>>>> 'legacy' use,
>>>>>> then I disagree with that. There are many non-MC video device 
>>>>>> drivers, nothing
>>>>>> legacy about that.
>>>>>
>>>>> It's difficult to define all the scenarios where this can be used, 
>>>>> but the ones I can imagine fall under legacy (depending on how you 
>>>>> define that, though).
>>>>>
>>>>> I use this in CAL driver, which supports non-MC (legacy) and MC. 
>>>>> CAL has a bunch of video devices (one for each DMA engine) and two 
>>>>> CSI-2 PHY devices (v4l2 subdevs).
>>>>>
>>>>> When operating in MC mode, the userspace will call, e.g., set_fmt 
>>>>> in the PHY subdev, and so forth.
>>>>>
>>>>> But in non-MC case the userspace calls VIDIOC_S_FMT in the video 
>>>>> dev, and the video dev has to propagate that to the PHY subdev. I 
>>>>> do this propagation using the v4l2_subdev_call_state_active macro.
>>>>>
>>>>> I don't know if there are other drivers that support both non-MC 
>>>>> and MC modes. I could also just move this macro to the CAL driver, 
>>>>> and we could add this to the v4l2 framework if we see other drivers 
>>>>> using similar constructs.
>>>>
>>>> It is common to have non-MC drivers that call set_fmt of a subdev.
>>>> Wouldn't they all need to use this helper macro? If so, then this is 
>>>> NOT a
>>>> legacy use, it's just a non-MC driver use.
>>>
>>> These non-MC drivers that call set_fmt of a subdev, they're video 
>>> device drivers, right? In other words, there are no subdev drivers 
>>> that call set_fmt on other subdevs?
>>
>> Probably not, but I am not 100% certain. There are a few nested subdev 
>> cases, but I don't remember which.
>>
>>>
>>> This does get a bit complex, keeping the old and new code working 
>>> together. In this context, I think we have three different "classes":
>>>
>>> 1. non-MC
>>> 2. MC, no state support
>>> 3. MC, state support
>>
>> Are you talking about subdev drivers or bridge drivers? It's a bit 
>> confusing. I'm assuming it can be either.
> 
> Yes, I mean either one.
> 
>>> We have classes 1 and 2 in upstream, and 3 will be enabled with this 
>>> series (and expanded with the streams series).
>>>
>>> Classes 1 and 2 continue working as before. If you have a pipeline 
>>> with only class 3 drivers, it works without any legacy "hacks". The 
>>> problems come when you combine 1 or 2 with 3. Or possibly the 
>>> problems appear only when combining class 1 and class 3, as class 2 
>>> drivers are not supposed to call subdev ops which take a state 
>>> parameter on other subdevs.
>>>
>>> A class 3 driver expects to get either a try or an active state as a 
>>> parameter, but class 1 drivers pass NULL for the active state. If you 
>>> write a class 3 driver and want it to work with class 1 (without any 
>>> changes to those drivers), you must do extra plumbing in the ops 
>>> functions, to catch the NULL state case and get & lock the state 
>>> yourself. If you do that, this macro is not needed.
>>>
>>> Alternatively, class 1 drivers could be changed to use this macro, so 
>>> that a possible class 3 driver in the pipeline would work without 
>>> additional code. But there are a lot of class 1 drivers, and thus 
>>> modifications, and I wasn't planning to go that way.
>>>
>>> The CAL driver I mentioned supports both class 1 and class 3 (via a 
>>> module parameter) in the video dev driver, and the class 1 mode uses 
>>> this macro as CAL's PHY subdev (part of the same driver) is a class 3 
>>> subdev. The class 1 support is legacy support in CAL's case.
>>>
>>> So... Depending on what kind of driver combinations we want to 
>>> support, this may or may not be legacy, depending on how you define 
>>> legacy =).
>>
>> Let me try to explain what my concerns are. Eventually I would really 
>> like all subdevs to be capable of working with MC bridge drivers, i.e. 
>> have state support. Bridge drivers can be either MC or non-MC.
>>
>> So I would like to know:
>>
>> 1.1) How to convert a subdev driver to a MC state-aware subdev driver?
> 
> I presume you mean how to convert an MC subdev driver to state-aware MC 
> driver. If you have a non-MC subdev driver, then that first needs to be 
> converted to an MC driver, which is out of scope here.
> 
> Here's an example commit where I convert OV10635 to streams:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git/commit/?h=streams/work-v11&id=ffb08af15f04ef2ce0a00bb356e957c839d6bccb 
> 
> 
> However, the commit is on top of the streams series and I add support 
> for multiple streams in addition to the active state, so 1) it's not as 
> simple as it could be (e.g. get_frame_desc() and set_routing() ops could 
> be left out), and 2) it's not using the old-style v4l2_subdev_pad_config 
> (which is the only option on top of this series), but the new routes & 
> streams.
> 
> So the example doesn't quite answer to your question... I haven't looked 
> at implementing a driver which would be state-aware but not 
> streams-aware, but I think it's essentially just:
> 
> - Use v4l2_subdev_init_finalize() to create the active state storage
> - Always use state->pads instead of something stored in the driver's 
> private data for active case.
> 
>> 1.2) What is the legacy code that such a MC state-aware subdev driver 
>> has to keep in order to work with older bridge drivers that do not 
>> support such subdev drivers? (i.e. they pass NULL as the state)
> 
> I believe the only extra code needed is to handle the state == NULL 
> case. This means adding code to each subdev op which has the state as a 
> parameter, and doing, perhaps, something like this:
> 
> my_set_fmt(sd, _state, fmt)
> {
>      state = _state
> 
>      if (!_state)
>          state = v4l2_subdev_lock_and_get_active_state(sd);
> 
>      ... use 'state' here ...
> 
>      if (!_state)
>          v4l2_subdev_unlock_state(state);
> }
> 
> Maybe we can somehow macro-ify the above, which creates a wrapper for 
> the op. Or, as I mentioned, we could try to change all the drivers that 
> do those calls, so that they use the macro in this patch instead.
> 
>> 2.1) How to convert a bridge driver (either non-MC or MC, but no state 
>> support) to properly support a fully converted subdev (MC state-aware) 
>> driver?
> 
> Converting non-MC driver to an MC driver is out of the context here, as 
> it's not related to the active state. An MC bridge driver should work 
> fine with state-aware subdev drivers, as the bridge driver should not 
> call any of the subdev's state-related ops.
> 
> To make a non-MC bridge driver support state-aware subdev drivers, they 
> can use the macro in this patch.
> 
>> 2.2) What is the legacy code that such a bridge driver has to keep in 
>> order to work with older subdev drivers that are not yet MC state-aware?
> 
> The older subdev drivers should keep working without any extra code.
> 
>> The code needed for 1.2 and 2.2 (helper functions/macros) is legacy 
>> code, and can be marked as such.
>> > If this is clear, then we can work towards converting both subdev and 
> bridge drivers and eventually (might take years!) get rid of the legacy 
> code.
>>
>> Removing support for case 2 is probably something that we want to do 
>> sooner than later.
>>
>> For the CAL driver I do not consider non-MC support as legacy. It's 
>> legacy in the context of the CAL driver only, but API-wise it is not 
>> since there are many non-MC bridge drivers.
> 
> That's true, but also, non-MC bridge drivers do not need to use this 
> function if the subdev drivers use the method shown in 1.2. I think this 
> is the question here:
> 
> - Change all the callers and use the macro in this patch. Then the macro 
> is not legacy.
> - Change the callees, in which case this macro is needed only in some 
> cases where, for whatever reason, a specific callee has not been changed 
> (yet?). In this case it's legacy.
> 
> Changing the callers would be a nicer option, I think, but I also fear 
> that it's very difficult and easily brings in bugs. I haven't looked 
> closely, but I think it would be a big patch.
> 
> And it's not clear to me if there's a benefit: do all those drivers ever 
> need to interact a state-aware subdev driver? If they do, maybe there 
> are only a few such subdev drivers, and it's not a big issue to have the 
> lock/unlock code in those state-aware subdev drivers. All the other 
> state-aware subdev drivers would not need the legacy support code as 
> they're used only in more modern pipelines.

Did the above explain the situation enough? I'm ok with both options 
(changing the callers or callees), but I also think we don't need to 
make the decision know.

  Tomi

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Hans Verkuil]

On 07/03/2022 15:00, Tomi Valkeinen wrote:
> On 07/03/2022 11:51, Hans Verkuil wrote:
>> Hi Tomi,
>>
>> On 3/7/22 10:16, Tomi Valkeinen wrote:
>>> Hi Hans,
>>>
>>> On 07/03/2022 10:36, Hans Verkuil wrote:
>>>>
>>>>
>>>> On 3/7/22 08:16, Tomi Valkeinen wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On 04/03/2022 15:34, Hans Verkuil wrote:
>>>>>> Hi Tomi,
>>>>>>
>>>>>> On 3/1/22 11:55, Tomi Valkeinen wrote:
>>>>>>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>>>>>>> that take a subdev state as a parameter. Normally the v4l2 framework
>>>>>>> will lock and pass the correct subdev state to the subdev ops, but there
>>>>>>> are legacy situations where this is not the case (e.g. non-MC video
>>>>>>> device driver calling set_fmt in a source subdev).
>>>>>>>
>>>>>>> As this macro is only needed for legacy use cases, the macro is added in
>>>>>>> a new header file, v4l2-subdev-legacy.h.
>>>>>>>
>>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>> ---
>>>>>>>     include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
>>>>>>>     1 file changed, 42 insertions(+)
>>>>>>>     create mode 100644 include/media/v4l2-subdev-legacy.h
>>>>>>>
>>>>>>> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..6a61e579b629
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/media/v4l2-subdev-legacy.h
>>>>>>> @@ -0,0 +1,42 @@
>>>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>>>> +/*
>>>>>>> + *  V4L2 sub-device legacy support header.
>>>>>>> + *
>>>>>>> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef _V4L2_SUBDEV_LEGACY_H
>>>>>>> +#define _V4L2_SUBDEV_LEGACY_H
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
>>>>>>> + *                   takes state as a parameter, passing the
>>>>>>> + *                   subdev its active state.
>>>>>>> + *
>>>>>>> + * @sd: pointer to the &struct v4l2_subdev
>>>>>>> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
>>>>>>> + *     Each element there groups a set of callbacks functions.
>>>>>>> + * @f: callback function to be called.
>>>>>>> + *     The callback functions are defined in groups, according to
>>>>>>> + *     each element at &struct v4l2_subdev_ops.
>>>>>>> + * @args: arguments for @f.
>>>>>>> + *
>>>>>>> + * This is similar to v4l2_subdev_call(), except that this version can only be
>>>>>>> + * used for ops that take a subdev state as a parameter. The macro will get the
>>>>>>> + * active state and lock it before calling the op, and unlock it after the
>>>>>>> + * call.
>>>>>>> + */
>>>>>>
>>>>>> You should explain why this is a legacy macro and, ideally, what would need to
>>>>>> be done to get rid of it. The first is in the commit log, but nobody reads that :-)
>>>>>>
>>>>>> But if just using it in a non-MC video device driver constitutes 'legacy' use,
>>>>>> then I disagree with that. There are many non-MC video device drivers, nothing
>>>>>> legacy about that.
>>>>>
>>>>> It's difficult to define all the scenarios where this can be used, but the ones I can imagine fall under legacy (depending on how you define that, though).
>>>>>
>>>>> I use this in CAL driver, which supports non-MC (legacy) and MC. CAL has a bunch of video devices (one for each DMA engine) and two CSI-2 PHY devices (v4l2 subdevs).
>>>>>
>>>>> When operating in MC mode, the userspace will call, e.g., set_fmt in the PHY subdev, and so forth.
>>>>>
>>>>> But in non-MC case the userspace calls VIDIOC_S_FMT in the video dev, and the video dev has to propagate that to the PHY subdev. I do this propagation using the v4l2_subdev_call_state_active macro.
>>>>>
>>>>> I don't know if there are other drivers that support both non-MC and MC modes. I could also just move this macro to the CAL driver, and we could add this to the v4l2 framework if we see other
>>>>> drivers using similar constructs.
>>>>
>>>> It is common to have non-MC drivers that call set_fmt of a subdev.
>>>> Wouldn't they all need to use this helper macro? If so, then this is NOT a
>>>> legacy use, it's just a non-MC driver use.
>>>
>>> These non-MC drivers that call set_fmt of a subdev, they're video device drivers, right? In other words, there are no subdev drivers that call set_fmt on other subdevs?
>>
>> Probably not, but I am not 100% certain. There are a few nested subdev cases, but I don't remember which.
>>
>>>
>>> This does get a bit complex, keeping the old and new code working together. In this context, I think we have three different "classes":
>>>
>>> 1. non-MC
>>> 2. MC, no state support
>>> 3. MC, state support
>>
>> Are you talking about subdev drivers or bridge drivers? It's a bit confusing. I'm assuming it can be either.
> 
> Yes, I mean either one.
> 
>>> We have classes 1 and 2 in upstream, and 3 will be enabled with this series (and expanded with the streams series).
>>>
>>> Classes 1 and 2 continue working as before. If you have a pipeline with only class 3 drivers, it works without any legacy "hacks". The problems come when you combine 1 or 2 with 3. Or possibly the
>>> problems appear only when combining class 1 and class 3, as class 2 drivers are not supposed to call subdev ops which take a state parameter on other subdevs.
>>>
>>> A class 3 driver expects to get either a try or an active state as a parameter, but class 1 drivers pass NULL for the active state. If you write a class 3 driver and want it to work with class 1
>>> (without any changes to those drivers), you must do extra plumbing in the ops functions, to catch the NULL state case and get & lock the state yourself. If you do that, this macro is not needed.
>>>
>>> Alternatively, class 1 drivers could be changed to use this macro, so that a possible class 3 driver in the pipeline would work without additional code. But there are a lot of class 1 drivers, and
>>> thus modifications, and I wasn't planning to go that way.
>>>
>>> The CAL driver I mentioned supports both class 1 and class 3 (via a module parameter) in the video dev driver, and the class 1 mode uses this macro as CAL's PHY subdev (part of the same driver) is
>>> a class 3 subdev. The class 1 support is legacy support in CAL's case.
>>>
>>> So... Depending on what kind of driver combinations we want to support, this may or may not be legacy, depending on how you define legacy =).
>>
>> Let me try to explain what my concerns are. Eventually I would really like all subdevs to be capable of working with MC bridge drivers, i.e. have state support. Bridge drivers can be either MC or
>> non-MC.
>>
>> So I would like to know:
>>
>> 1.1) How to convert a subdev driver to a MC state-aware subdev driver?
> 
> I presume you mean how to convert an MC subdev driver to state-aware MC driver. If you have a non-MC subdev driver, then that first needs to be converted to an MC driver, which is out of scope here.
> 
> Here's an example commit where I convert OV10635 to streams:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git/commit/?h=streams/work-v11&id=ffb08af15f04ef2ce0a00bb356e957c839d6bccb
> 
> However, the commit is on top of the streams series and I add support for multiple streams in addition to the active state, so 1) it's not as simple as it could be (e.g. get_frame_desc() and
> set_routing() ops could be left out), and 2) it's not using the old-style v4l2_subdev_pad_config (which is the only option on top of this series), but the new routes & streams.
> 
> So the example doesn't quite answer to your question... I haven't looked at implementing a driver which would be state-aware but not streams-aware, but I think it's essentially just:
> 
> - Use v4l2_subdev_init_finalize() to create the active state storage
> - Always use state->pads instead of something stored in the driver's private data for active case.

Can you make a patch that converts ov5648.c (or a similar driver) to use
sd->active_state? I'd like to see how that looks.

> 
>> 1.2) What is the legacy code that such a MC state-aware subdev driver has to keep in order to work with older bridge drivers that do not support such subdev drivers? (i.e. they pass NULL as the state)
> 
> I believe the only extra code needed is to handle the state == NULL case. This means adding code to each subdev op which has the state as a parameter, and doing, perhaps, something like this:
> 
> my_set_fmt(sd, _state, fmt)
> {
>     state = _state
> 
>     if (!_state)
>         state = v4l2_subdev_lock_and_get_active_state(sd);
> 
>     ... use 'state' here ...
> 
>     if (!_state)
>         v4l2_subdev_unlock_state(state);
> }
> 
> Maybe we can somehow macro-ify the above, which creates a wrapper for the op. Or, as I mentioned, we could try to change all the drivers that do those calls, so that they use the macro in this patch
> instead.

Right. So to confirm: the only reason a state-ware subdev set_fmt op is called
with a NULL state pointer is if it is called from non-MC bridge drivers? And if
we would update those bridge drivers to always pass a non-NULL state pointer,
then such subdevs no longer need to care about this and can just use the state.

If so, then that's the way forward.

> 
>> 2.1) How to convert a bridge driver (either non-MC or MC, but no state support) to properly support a fully converted subdev (MC state-aware) driver?
> 
> Converting non-MC driver to an MC driver is out of the context here, as it's not related to the active state. An MC bridge driver should work fine with state-aware subdev drivers, as the bridge driver
> should not call any of the subdev's state-related ops.
> 
> To make a non-MC bridge driver support state-aware subdev drivers, they can use the macro in this patch.

That sounds good to me.

The only legacy bit about the macro, though, is the fact that v4l2_subdev_get_active_state()
can return NULL: that indicates that the subdev driver isn't properly state-aware.

So I think this should be a regular macro in v4l2-subdev.h.

Perhaps with a comment mentioning that v4l2_subdev_get_active_state() can be replaced by
v4l2_subdev_lock_and_get_active_state() once all subdevs are state-aware.

> 
>> 2.2) What is the legacy code that such a bridge driver has to keep in order to work with older subdev drivers that are not yet MC state-aware?
> 
> The older subdev drivers should keep working without any extra code.
> 
>> The code needed for 1.2 and 2.2 (helper functions/macros) is legacy code, and can be marked as such.
>> > If this is clear, then we can work towards converting both subdev and 
> bridge drivers and eventually (might take years!) get rid of the legacy code.
>>
>> Removing support for case 2 is probably something that we want to do sooner than later.
>>
>> For the CAL driver I do not consider non-MC support as legacy. It's legacy in the context of the CAL driver only, but API-wise it is not since there are many non-MC bridge drivers.
> 
> That's true, but also, non-MC bridge drivers do not need to use this function if the subdev drivers use the method shown in 1.2. I think this is the question here:
> 
> - Change all the callers and use the macro in this patch. Then the macro is not legacy.
> - Change the callees, in which case this macro is needed only in some cases where, for whatever reason, a specific callee has not been changed (yet?). In this case it's legacy.
> 
> Changing the callers would be a nicer option, I think, but I also fear that it's very difficult and easily brings in bugs. I haven't looked closely, but I think it would be a big patch.

I believe changing the callers is the correct approach. Next to that I want to slowly
convert all subdevs to be state-aware (i.e. use sd->active_state etc). Based on past
experience it is a really bad idea to have all these variations in how subdevs work.
That should be minimized.

I suspect that modifying the callers isn't as bad as you might think. It is also the
sane approach: passing the active_state to the subdev really *is* what you want to do.
It is not a hack, it is the right thing. Adding a workaround in a subdev where a
NULL state is handled separately is, however, a hack, and a hack that is all to easy
to forget to implement in new drivers.

> 
> And it's not clear to me if there's a benefit: do all those drivers ever need to interact a state-aware subdev driver? If they do, maybe there are only a few such subdev drivers, and it's not a big
> issue to have the lock/unlock code in those state-aware subdev drivers. All the other state-aware subdev drivers would not need the legacy support code as they're used only in more modern pipelines.

You never know that: anyone can hook any sensor to a non-MC driver in a product.
Just because there are no such boards in the mainline kernel doesn't mean that
such boards don't exist.

I hope this helps, and apologies for the delay in replying.

Regards,

	Hans

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Tomi Valkeinen]

On 22/03/2022 11:23, Hans Verkuil wrote:

> Can you make a patch that converts ov5648.c (or a similar driver) to use
> sd->active_state? I'd like to see how that looks.

I have ov5640, I can convert that one.
> Right. So to confirm: the only reason a state-ware subdev set_fmt op is called
> with a NULL state pointer is if it is called from non-MC bridge drivers? And if

I hope so. There could be MC bridge or subdev drivers that call set_fmt 
(or get_fmt) on other subdevs. I cannot figure out why that would be 
done, though, and it sounds wrong to me. Afaik, with MC, set_fmt (and 
other such calls) always come from the userspace, and each subdev is 
configured separately from the others.

But if there are such drivers, they'll get fixed along the non-MC bridge 
drivers.

> we would update those bridge drivers to always pass a non-NULL state pointer,
> then such subdevs no longer need to care about this and can just use the state.

That's correct.

>>> 2.1) How to convert a bridge driver (either non-MC or MC, but no state support) to properly support a fully converted subdev (MC state-aware) driver?
>>
>> Converting non-MC driver to an MC driver is out of the context here, as it's not related to the active state. An MC bridge driver should work fine with state-aware subdev drivers, as the bridge driver
>> should not call any of the subdev's state-related ops.
>>
>> To make a non-MC bridge driver support state-aware subdev drivers, they can use the macro in this patch.
> 
> That sounds good to me.
> 
> The only legacy bit about the macro, though, is the fact that v4l2_subdev_get_active_state()
> can return NULL: that indicates that the subdev driver isn't properly state-aware.
> 
> So I think this should be a regular macro in v4l2-subdev.h.
> 
> Perhaps with a comment mentioning that v4l2_subdev_get_active_state() can be replaced by
> v4l2_subdev_lock_and_get_active_state() once all subdevs are state-aware.

Yes, that's true. I'll add the comment, and move this back to v4l2-subdev.h.

>>> 2.2) What is the legacy code that such a bridge driver has to keep in order to work with older subdev drivers that are not yet MC state-aware?
>>
>> The older subdev drivers should keep working without any extra code.
>>
>>> The code needed for 1.2 and 2.2 (helper functions/macros) is legacy code, and can be marked as such.
>>>> If this is clear, then we can work towards converting both subdev and
>> bridge drivers and eventually (might take years!) get rid of the legacy code.
>>>
>>> Removing support for case 2 is probably something that we want to do sooner than later.
>>>
>>> For the CAL driver I do not consider non-MC support as legacy. It's legacy in the context of the CAL driver only, but API-wise it is not since there are many non-MC bridge drivers.
>>
>> That's true, but also, non-MC bridge drivers do not need to use this function if the subdev drivers use the method shown in 1.2. I think this is the question here:
>>
>> - Change all the callers and use the macro in this patch. Then the macro is not legacy.
>> - Change the callees, in which case this macro is needed only in some cases where, for whatever reason, a specific callee has not been changed (yet?). In this case it's legacy.
>>
>> Changing the callers would be a nicer option, I think, but I also fear that it's very difficult and easily brings in bugs. I haven't looked closely, but I think it would be a big patch.
> 
> I believe changing the callers is the correct approach. Next to that I want to slowly
> convert all subdevs to be state-aware (i.e. use sd->active_state etc). Based on past
> experience it is a really bad idea to have all these variations in how subdevs work.
> That should be minimized.
> 
> I suspect that modifying the callers isn't as bad as you might think. It is also the
> sane approach: passing the active_state to the subdev really *is* what you want to do.
> It is not a hack, it is the right thing. Adding a workaround in a subdev where a
> NULL state is handled separately is, however, a hack, and a hack that is all to easy
> to forget to implement in new drivers.

Ok.

I'll send a v6 soon with the changes we've discussed, and I'll take a 
look at ov5640.c and changing the callers.

  Tomi

Re: [PATCH v5 5/6] media: subdev: add v4l2_subdev_call_state_active()

[Laurent Pinchart]

Hi Tomi,

On Mon, Mar 07, 2022 at 04:00:53PM +0200, Tomi Valkeinen wrote:
> On 07/03/2022 11:51, Hans Verkuil wrote:
> > On 3/7/22 10:16, Tomi Valkeinen wrote:
> >> On 07/03/2022 10:36, Hans Verkuil wrote:
> >>> On 3/7/22 08:16, Tomi Valkeinen wrote:
> >>>> On 04/03/2022 15:34, Hans Verkuil wrote:
> >>>>> On 3/1/22 11:55, Tomi Valkeinen wrote:
> >>>>>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> >>>>>> that take a subdev state as a parameter. Normally the v4l2 framework
> >>>>>> will lock and pass the correct subdev state to the subdev ops, but there
> >>>>>> are legacy situations where this is not the case (e.g. non-MC video
> >>>>>> device driver calling set_fmt in a source subdev).
> >>>>>>
> >>>>>> As this macro is only needed for legacy use cases, the macro is added in
> >>>>>> a new header file, v4l2-subdev-legacy.h.
> >>>>>>
> >>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>>> ---
> >>>>>>     include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
> >>>>>>     1 file changed, 42 insertions(+)
> >>>>>>     create mode 100644 include/media/v4l2-subdev-legacy.h
> >>>>>>
> >>>>>> diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..6a61e579b629
> >>>>>> --- /dev/null
> >>>>>> +++ b/include/media/v4l2-subdev-legacy.h
> >>>>>> @@ -0,0 +1,42 @@
> >>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>>>>> +/*
> >>>>>> + *  V4L2 sub-device legacy support header.
> >>>>>> + *
> >>>>>> + *  Copyright (C) 2022  Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>>> + */
> >>>>>> +
> >>>>>> +#ifndef _V4L2_SUBDEV_LEGACY_H
> >>>>>> +#define _V4L2_SUBDEV_LEGACY_H
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
> >>>>>> + *                   takes state as a parameter, passing the
> >>>>>> + *                   subdev its active state.
> >>>>>> + *
> >>>>>> + * @sd: pointer to the &struct v4l2_subdev
> >>>>>> + * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
> >>>>>> + *     Each element there groups a set of callbacks functions.
> >>>>>> + * @f: callback function to be called.
> >>>>>> + *     The callback functions are defined in groups, according to
> >>>>>> + *     each element at &struct v4l2_subdev_ops.
> >>>>>> + * @args: arguments for @f.
> >>>>>> + *
> >>>>>> + * This is similar to v4l2_subdev_call(), except that this version can only be
> >>>>>> + * used for ops that take a subdev state as a parameter. The macro will get the
> >>>>>> + * active state and lock it before calling the op, and unlock it after the
> >>>>>> + * call.
> >>>>>> + */
> >>>>>
> >>>>> You should explain why this is a legacy macro and, ideally, what would need to
> >>>>> be done to get rid of it. The first is in the commit log, but nobody reads that :-)
> >>>>>
> >>>>> But if just using it in a non-MC video device driver constitutes 'legacy' use,
> >>>>> then I disagree with that. There are many non-MC video device drivers, nothing
> >>>>> legacy about that.
> >>>>
> >>>> It's difficult to define all the scenarios where this can be
> >>>> used, but the ones I can imagine fall under legacy (depending on
> >>>> how you define that, though).
> >>>>
> >>>> I use this in CAL driver, which supports non-MC (legacy) and MC.
> >>>> CAL has a bunch of video devices (one for each DMA engine) and
> >>>> two CSI-2 PHY devices (v4l2 subdevs).
> >>>>
> >>>> When operating in MC mode, the userspace will call, e.g., set_fmt
> >>>> in the PHY subdev, and so forth.
> >>>>
> >>>> But in non-MC case the userspace calls VIDIOC_S_FMT in the video
> >>>> dev, and the video dev has to propagate that to the PHY subdev. I
> >>>> do this propagation using the v4l2_subdev_call_state_active
> >>>> macro.
> >>>>
> >>>> I don't know if there are other drivers that support both non-MC
> >>>> and MC modes. I could also just move this macro to the CAL
> >>>> driver, and we could add this to the v4l2 framework if we see
> >>>> other drivers using similar constructs.
> >>>
> >>> It is common to have non-MC drivers that call set_fmt of a subdev.
> >>> Wouldn't they all need to use this helper macro? If so, then this is NOT a
> >>> legacy use, it's just a non-MC driver use.
> >>
> >> These non-MC drivers that call set_fmt of a subdev, they're video
> >> device drivers, right? In other words, there are no subdev drivers
> >> that call set_fmt on other subdevs?
> > 
> > Probably not, but I am not 100% certain. There are a few nested
> > subdev cases, but I don't remember which.
> > 
> >> This does get a bit complex, keeping the old and new code working
> >> together. In this context, I think we have three different
> >> "classes":
> >>
> >> 1. non-MC
> >> 2. MC, no state support
> >> 3. MC, state support
> > 
> > Are you talking about subdev drivers or bridge drivers? It's a bit
> > confusing. I'm assuming it can be either.
> 
> Yes, I mean either one.
> 
> >> We have classes 1 and 2 in upstream, and 3 will be enabled with
> >> this series (and expanded with the streams series).
> >>
> >> Classes 1 and 2 continue working as before. If you have a pipeline
> >> with only class 3 drivers, it works without any legacy "hacks". The
> >> problems come when you combine 1 or 2 with 3. Or possibly the
> >> problems appear only when combining class 1 and class 3, as class 2
> >> drivers are not supposed to call subdev ops which take a state
> >> parameter on other subdevs.
> >>
> >> A class 3 driver expects to get either a try or an active state as
> >> a parameter, but class 1 drivers pass NULL for the active state. If
> >> you write a class 3 driver and want it to work with class 1
> >> (without any changes to those drivers), you must do extra plumbing
> >> in the ops functions, to catch the NULL state case and get & lock
> >> the state yourself. If you do that, this macro is not needed.
> >>
> >> Alternatively, class 1 drivers could be changed to use this macro,
> >> so that a possible class 3 driver in the pipeline would work
> >> without additional code. But there are a lot of class 1 drivers,
> >> and thus modifications, and I wasn't planning to go that way.
> >>
> >> The CAL driver I mentioned supports both class 1 and class 3 (via a
> >> module parameter) in the video dev driver, and the class 1 mode
> >> uses this macro as CAL's PHY subdev (part of the same driver) is a
> >> class 3 subdev. The class 1 support is legacy support in CAL's
> >> case.
> >>
> >> So... Depending on what kind of driver combinations we want to
> >> support, this may or may not be legacy, depending on how you define
> >> legacy =).
> > 
> > Let me try to explain what my concerns are. Eventually I would
> > really like all subdevs to be capable of working with MC bridge
> > drivers, i.e. have state support. Bridge drivers can be either MC or
> > non-MC.
> > 
> > So I would like to know:
> > 
> > 1.1) How to convert a subdev driver to a MC state-aware subdev driver?
> 
> I presume you mean how to convert an MC subdev driver to state-aware MC 
> driver. If you have a non-MC subdev driver, then that first needs to be 
> converted to an MC driver, which is out of scope here.
> 
> Here's an example commit where I convert OV10635 to streams:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git/commit/?h=streams/work-v11&id=ffb08af15f04ef2ce0a00bb356e957c839d6bccb
> 
> However, the commit is on top of the streams series and I add support 
> for multiple streams in addition to the active state, so 1) it's not as 
> simple as it could be (e.g. get_frame_desc() and set_routing() ops could 
> be left out), and 2) it's not using the old-style v4l2_subdev_pad_config 
> (which is the only option on top of this series), but the new routes & 
> streams.
> 
> So the example doesn't quite answer to your question... I haven't looked 
> at implementing a driver which would be state-aware but not 
> streams-aware, but I think it's essentially just:
> 
> - Use v4l2_subdev_init_finalize() to create the active state storage
> - Always use state->pads instead of something stored in the driver's 
> private data for active case.
> 
> > 1.2) What is the legacy code that such a MC state-aware subdev
> > driver has to keep in order to work with older bridge drivers that
> > do not support such subdev drivers? (i.e. they pass NULL as the
> > state)
> 
> I believe the only extra code needed is to handle the state == NULL 
> case. This means adding code to each subdev op which has the state as a 
> parameter, and doing, perhaps, something like this:
> 
> my_set_fmt(sd, _state, fmt)
> {
> 	state = _state
> 
> 	if (!_state)
> 		state = v4l2_subdev_lock_and_get_active_state(sd);
> 
> 	... use 'state' here ...
> 
> 	if (!_state)
> 		v4l2_subdev_unlock_state(state);
> }
> 
> Maybe we can somehow macro-ify the above, which creates a wrapper for 
> the op. Or, as I mentioned, we could try to change all the drivers that 
> do those calls, so that they use the macro in this patch instead.
>
> > 2.1) How to convert a bridge driver (either non-MC or MC, but no
> > state support) to properly support a fully converted subdev (MC
> > state-aware) driver?
> 
> Converting non-MC driver to an MC driver is out of the context here, as 
> it's not related to the active state. An MC bridge driver should work 
> fine with state-aware subdev drivers, as the bridge driver should not 
> call any of the subdev's state-related ops.
> 
> To make a non-MC bridge driver support state-aware subdev drivers, they 
> can use the macro in this patch.
> 
> > 2.2) What is the legacy code that such a bridge driver has to keep
> > in order to work with older subdev drivers that are not yet MC
> > state-aware?
> 
> The older subdev drivers should keep working without any extra code.
> 
> > The code needed for 1.2 and 2.2 (helper functions/macros) is legacy
> > code, and can be marked as such.
> > 
> > > If this is clear, then we can work towards converting both subdev
> > > and bridge drivers and eventually (might take years!) get rid of
> > > the legacy code.
> > 
> > Removing support for case 2 is probably something that we want to do
> > sooner than later.
> > 
> > For the CAL driver I do not consider non-MC support as legacy. It's
> > legacy in the context of the CAL driver only, but API-wise it is not
> > since there are many non-MC bridge drivers.
> 
> That's true, but also, non-MC bridge drivers do not need to use this 
> function if the subdev drivers use the method shown in 1.2. I think this 
> is the question here:
> 
> - Change all the callers and use the macro in this patch. Then the macro 
> is not legacy.
> - Change the callees, in which case this macro is needed only in some 
> cases where, for whatever reason, a specific callee has not been changed 
> (yet?). In this case it's legacy.
> 
> Changing the callers would be a nicer option, I think, but I also fear 
> that it's very difficult and easily brings in bugs. I haven't looked 
> closely, but I think it would be a big patch.

Could this be done in the existing wrappers (v4l2_subdev_call_wrappers,
in drivers/media/v4l2-core/v4l2-subdev.c) ?

> And it's not clear to me if there's a benefit: do all those drivers ever 
> need to interact a state-aware subdev driver? If they do, maybe there 
> are only a few such subdev drivers, and it's not a big issue to have the 
> lock/unlock code in those state-aware subdev drivers. All the other 
> state-aware subdev drivers would not need the legacy support code as 
> they're used only in more modern pipelines.

-- 
Regards,

Laurent Pinchart