[PATCH v4 0/7] v4l: subdev active state

[PATCH v4 0/7] v4l: subdev active state

[Tomi Valkeinen]

This is v4 of the subdev active state series. Changes since v3:

- Locking model change: in v3 the subdev's were responsible for locking
  their state. Now the caller of the subdev ops must lock the callee's
  state.
- Add subdev->lock which can be set by the driver. This lock will be
  used for all the states.
- Add v4l2_subdev_call_state_active() helper
- Doc changes wrt. locking

The main change here is the first bullet above. The idea for the change
came up when we realized that we will have race conditions very easily
if the state locking is handled by the subdev itself. It's not rare for
the v4l2-core to need to check, say, the routing of a subdev, and then
call a subdev op (e.g. get_fmt). In the previous version this would have
been accomplished by locking the state, looking at the routing,
unlocking, calling the subdev op, which then would lock again. Now the
v4l2-core can lock the state for the duration of the operation. This
also simplifies driver code nicely.

In the longer time frame we hope that we can improve the locking even
more by locking all the relevant subdev states in the pipeline at the
beginning of the operation. E.g. when the streaming is started the
v4l2-core would lock all the subdev states that are part of that
pipeline before calling s_stream.

I dropped the reviewed-bys from "media: subdev: add subdev state
locking" and "media: subdev: Add v4l2_subdev_lock_and_return_state()" as
their operation changed, and I think they need a fresh review.

I also dropped the "media: subdev: rename v4l2_subdev_pad_config.try_*
fields" just to simplify the series management. It's one of the cleanups
that can be easily done after the dust has settled.

 Tomi

Tomi Valkeinen (7):
  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_lock_and_return_state()
  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         | 157 +++++++++++--
 drivers/staging/media/tegra-video/vi.c        |  10 +-
 include/media/v4l2-subdev.h                   | 220 +++++++++++++++++-
 6 files changed, 450 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH v4 1/7] 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 v4 2/7] 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 v4 3/7] 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 v4 4/7] 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>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
 drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
 drivers/media/v4l2-core/v4l2-subdev.c       | 78 +++++++++++++++----
 drivers/staging/media/tegra-video/vi.c      |  4 +-
 include/media/v4l2-subdev.h                 | 85 ++++++++++++++++++++-
 5 files changed, 155 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..0df9bbe1819d 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)
 {
@@ -972,6 +1004,26 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
 
+struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
+{
+	mutex_lock(sd->active_state->lock);
+
+	return sd->active_state;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);
+
+void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
+{
+	mutex_lock(state->lock);
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_lock_state);
+
+void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
+{
+	mutex_unlock(state->lock);
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
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..8d089a2dbd32 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,71 @@ 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_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.
+ */
+struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
+
+/**
+ * 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.
+ */
+void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
+
+/**
+ * v4l2_subdev_unlock_state() - Unlocks the subdev state
+ * @state: The subdevice state
+ *
+ * Unlocks the given subdev state.
+ */
+void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 /**
-- 
2.25.1

[PATCH v4 5/7] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Tomi Valkeinen]

All suitable subdev ops are now passed either the TRY or the ACTIVE
state by the v4l2 core. However, other subdev drivers can still call the
ops passing NULL as the state, implying the active case.

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 here.

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

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 8d089a2dbd32..84c02488d53f 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1278,6 +1278,44 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
  */
 void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
 
+/**
+ * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
+ * @sd: subdevice
+ * @state: subdevice state as passed to the subdev op
+ *
+ * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
+ * NULL as the state parameter, as subdevs always used to have their active
+ * state stored privately.
+ *
+ * However, newer state-aware subdev drivers, which store their active state in
+ * a common place, subdev->active_state, expect to always get a proper state as
+ * a parameter.
+ *
+ * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
+ * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
+ * issue by using subdev->active_state in case the passed state is NULL.
+ *
+ * This is a temporary helper function, and should be removed when we can ensure
+ * that all drivers pass proper state when calling other subdevs.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state)
+{
+	if (state)
+		return state;
+
+	dev_notice_once(sd->dev,
+		"The provided state is NULL. Adjusting to the subdev active state.\n"
+		"Please consider porting your driver to the new state management API.\n");
+
+	state = sd->active_state;
+
+	v4l2_subdev_lock_state(state);
+
+	return state;
+}
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 /**
-- 
2.25.1

[PATCH v4 6/7] 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.

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

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 84c02488d53f..0db61203c8d9 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1359,6 +1359,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
 		__result;						\
 	})
 
+/**
+ * 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;						\
+	})
+
 /**
  * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
  *
-- 
2.25.1

[PATCH v4 7/7] 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..997d76134b50 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 is expected to be locked and unlocked by the
+caller by calling :c:func:`v4l2_subdev_lock_state()` and
+:c:func:`v4l2_subdev_unlock_state()`. Alternatively the caller can  use
+:c:func:`v4l2_subdev_call_state_active()` macro to call the op.
+
+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_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 it's '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 v4 6/7] media: subdev: add v4l2_subdev_call_state_active()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Feb 16, 2022 at 03:00:48PM +0200, Tomi Valkeinen wrote:
> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> that take a subdev state as a parameter.

We should not encourage subdev drivers to call into each other. There
may be some valid use cases for such a helper at this point, namely in
the .start_streaming() implementation in a vb2 queue, but even then, I
think it would be best to rework the pipeline start API to lock the
states of all subdevs in the pipeline (I've already improved pipeline
start on top of your streams series, the locking isn't there yet, but I
could give it a try).

On the other hand, this macro could help identifying drivers that handle
locking manually, helping reworking them once the pipeline start API
handles locking too.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  include/media/v4l2-subdev.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 84c02488d53f..0db61203c8d9 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1359,6 +1359,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
>  		__result;						\
>  	})
>  
> +/**
> + * 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;						\
> +	})
> +
>  /**
>   * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
>   *

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/7] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Feb 16, 2022 at 03:00:47PM +0200, Tomi Valkeinen wrote:
> All suitable subdev ops are now passed either the TRY or the ACTIVE
> state by the v4l2 core. However, other subdev drivers can still call the
> ops passing NULL as the state, implying the active case.
> 
> 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 here.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  include/media/v4l2-subdev.h | 38 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8d089a2dbd32..84c02488d53f 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1278,6 +1278,44 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
>   */
>  void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
>  
> +/**
> + * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
> + * @sd: subdevice
> + * @state: subdevice state as passed to the subdev op
> + *
> + * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
> + * NULL as the state parameter, as subdevs always used to have their active
> + * state stored privately.
> + *
> + * However, newer state-aware subdev drivers, which store their active state in
> + * a common place, subdev->active_state, expect to always get a proper state as
> + * a parameter.
> + *
> + * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
> + * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
> + * issue by using subdev->active_state in case the passed state is NULL.
> + *
> + * This is a temporary helper function, and should be removed when we can ensure
> + * that all drivers pass proper state when calling other subdevs.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state)
> +{
> +	if (state)
> +		return state;
> +
> +	dev_notice_once(sd->dev,
> +		"The provided state is NULL. Adjusting to the subdev active state.\n"
> +		"Please consider porting your driver to the new state management API.\n");
> +
> +	state = sd->active_state;
> +
> +	v4l2_subdev_lock_state(state);

This looks strange, if the state parameter is not NULL then you don't
lock the state, otherwise you do. How is the caller of this function
expected to unlock the state ? I'm tempted to drop this helper and push
harder for porting drivers to the new API.

> +
> +	return state;
> +}
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/7] media: subdev: add subdev state locking

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Feb 16, 2022 at 03:00:46PM +0200, 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       | 78 +++++++++++++++----
>  drivers/staging/media/tegra-video/vi.c      |  4 +-
>  include/media/v4l2-subdev.h                 | 85 ++++++++++++++++++++-
>  5 files changed, 155 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..0df9bbe1819d 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)
>  {
> @@ -972,6 +1004,26 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
>  
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
> +{
> +	mutex_lock(sd->active_state->lock);
> +
> +	return sd->active_state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);
> +
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
> +{
> +	mutex_lock(state->lock);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_state);
> +
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> +{
> +	mutex_unlock(state->lock);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> 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..8d089a2dbd32 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,71 @@ 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_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.
> + */
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
> +
> +/**
> + * 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.
> + */
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> +
> +/**
> + * v4l2_subdev_unlock_state() - Unlocks the subdev state
> + * @state: The subdevice state
> + *
> + * Unlocks the given subdev state.
> + */
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/7] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Tomi Valkeinen]

Hi,

On 16/02/2022 23:38, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Feb 16, 2022 at 03:00:47PM +0200, Tomi Valkeinen wrote:
>> All suitable subdev ops are now passed either the TRY or the ACTIVE
>> state by the v4l2 core. However, other subdev drivers can still call the
>> ops passing NULL as the state, implying the active case.
>>
>> 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 here.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   include/media/v4l2-subdev.h | 38 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 8d089a2dbd32..84c02488d53f 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1278,6 +1278,44 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
>>    */
>>   void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
>>   
>> +/**
>> + * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
>> + * @sd: subdevice
>> + * @state: subdevice state as passed to the subdev op
>> + *
>> + * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
>> + * NULL as the state parameter, as subdevs always used to have their active
>> + * state stored privately.
>> + *
>> + * However, newer state-aware subdev drivers, which store their active state in
>> + * a common place, subdev->active_state, expect to always get a proper state as
>> + * a parameter.
>> + *
>> + * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
>> + * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
>> + * issue by using subdev->active_state in case the passed state is NULL.
>> + *
>> + * This is a temporary helper function, and should be removed when we can ensure
>> + * that all drivers pass proper state when calling other subdevs.
>> + */
>> +static inline struct v4l2_subdev_state *
>> +v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
>> +				  struct v4l2_subdev_state *state)
>> +{
>> +	if (state)
>> +		return state;
>> +
>> +	dev_notice_once(sd->dev,
>> +		"The provided state is NULL. Adjusting to the subdev active state.\n"
>> +		"Please consider porting your driver to the new state management API.\n");
>> +
>> +	state = sd->active_state;
>> +
>> +	v4l2_subdev_lock_state(state);
> 
> This looks strange, if the state parameter is not NULL then you don't
> lock the state, otherwise you do. How is the caller of this function
> expected to unlock the state ? I'm tempted to drop this helper and push
> harder for porting drivers to the new API.

Indeed, I didn't think it through. Handling this with the new locking 
model is a bit more difficult. The driver could do something like this:

static int my_subdev_op(sd, _state)
{
	state = _state ? _state : v4l2_subdev_lock_active_state();

	...

	if (!_state)
		v4l2_subdev_unlock_state(state);

	return 0;
}

But hiding that behind a helper is not so easy.

Perhaps it is better to drop this, and change the calling subdev drivers 
to use the v4l2_subdev_call_state_active() helper.

  Tomi

Re: [PATCH v4 6/7] media: subdev: add v4l2_subdev_call_state_active()

[Tomi Valkeinen]

On 16/02/2022 23:30, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Feb 16, 2022 at 03:00:48PM +0200, Tomi Valkeinen wrote:
>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>> that take a subdev state as a parameter.
> 
> We should not encourage subdev drivers to call into each other. There
> may be some valid use cases for such a helper at this point, namely in
> the .start_streaming() implementation in a vb2 queue, but even then, I
> think it would be best to rework the pipeline start API to lock the
> states of all subdevs in the pipeline (I've already improved pipeline
> start on top of your streams series, the locking isn't there yet, but I
> could give it a try).
> 
> On the other hand, this macro could help identifying drivers that handle
> locking manually, helping reworking them once the pipeline start API
> handles locking too.

I use this in the cal-video.c to implement the legacy non-MC support. 
That is a bit special case, and I could do that with a CAL internal 
helper. So I'm fine with dropping this if there won't be other users.

  Tomi

Re: [PATCH v4 5/7] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Laurent Pinchart]

Hi Tomi,

On Thu, Feb 17, 2022 at 06:05:18AM +0200, Tomi Valkeinen wrote:
> On 16/02/2022 23:38, Laurent Pinchart wrote:
> > On Wed, Feb 16, 2022 at 03:00:47PM +0200, Tomi Valkeinen wrote:
> >> All suitable subdev ops are now passed either the TRY or the ACTIVE
> >> state by the v4l2 core. However, other subdev drivers can still call the
> >> ops passing NULL as the state, implying the active case.
> >>
> >> 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 here.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   include/media/v4l2-subdev.h | 38 +++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 38 insertions(+)
> >>
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 8d089a2dbd32..84c02488d53f 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -1278,6 +1278,44 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> >>    */
> >>   void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> >>   
> >> +/**
> >> + * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
> >> + * @sd: subdevice
> >> + * @state: subdevice state as passed to the subdev op
> >> + *
> >> + * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
> >> + * NULL as the state parameter, as subdevs always used to have their active
> >> + * state stored privately.
> >> + *
> >> + * However, newer state-aware subdev drivers, which store their active state in
> >> + * a common place, subdev->active_state, expect to always get a proper state as
> >> + * a parameter.
> >> + *
> >> + * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
> >> + * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
> >> + * issue by using subdev->active_state in case the passed state is NULL.
> >> + *
> >> + * This is a temporary helper function, and should be removed when we can ensure
> >> + * that all drivers pass proper state when calling other subdevs.
> >> + */
> >> +static inline struct v4l2_subdev_state *
> >> +v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
> >> +				  struct v4l2_subdev_state *state)
> >> +{
> >> +	if (state)
> >> +		return state;
> >> +
> >> +	dev_notice_once(sd->dev,
> >> +		"The provided state is NULL. Adjusting to the subdev active state.\n"
> >> +		"Please consider porting your driver to the new state management API.\n");
> >> +
> >> +	state = sd->active_state;
> >> +
> >> +	v4l2_subdev_lock_state(state);
> > 
> > This looks strange, if the state parameter is not NULL then you don't
> > lock the state, otherwise you do. How is the caller of this function
> > expected to unlock the state ? I'm tempted to drop this helper and push
> > harder for porting drivers to the new API.
> 
> Indeed, I didn't think it through. Handling this with the new locking 
> model is a bit more difficult. The driver could do something like this:
> 
> static int my_subdev_op(sd, _state)
> {
> 	state = _state ? _state : v4l2_subdev_lock_active_state();
> 
> 	...
> 
> 	if (!_state)
> 		v4l2_subdev_unlock_state(state);
> 
> 	return 0;
> }
> 
> But hiding that behind a helper is not so easy.
> 
> Perhaps it is better to drop this, and change the calling subdev drivers 
> to use the v4l2_subdev_call_state_active() helper.

I think that would be better, yes.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 6/7] media: subdev: add v4l2_subdev_call_state_active()

[Laurent Pinchart]

Hi Tomi,

On Thu, Feb 17, 2022 at 06:19:15AM +0200, Tomi Valkeinen wrote:
> On 16/02/2022 23:30, Laurent Pinchart wrote:
> > On Wed, Feb 16, 2022 at 03:00:48PM +0200, Tomi Valkeinen wrote:
> >> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> >> that take a subdev state as a parameter.
> > 
> > We should not encourage subdev drivers to call into each other. There
> > may be some valid use cases for such a helper at this point, namely in
> > the .start_streaming() implementation in a vb2 queue, but even then, I
> > think it would be best to rework the pipeline start API to lock the
> > states of all subdevs in the pipeline (I've already improved pipeline
> > start on top of your streams series, the locking isn't there yet, but I
> > could give it a try).
> > 
> > On the other hand, this macro could help identifying drivers that handle
> > locking manually, helping reworking them once the pipeline start API
> > handles locking too.
> 
> I use this in the cal-video.c to implement the legacy non-MC support. 
> That is a bit special case, and I could do that with a CAL internal 
> helper. So I'm fine with dropping this if there won't be other users.

Ah yes, for legacy support we indeed need something. There are likely
other drivers that will need this then (why did we write legacy code in
the first place, really ? :-D). We could keep the macro, but we should
then document it as not being for non-legacy use cases, and not for new
drivers. It would be even nicer if we could enforce that at runtime.

Actually, thinking more about this, how about moving this macro to a new
v4l2-subdev-legacy.h header ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 7/7] media: Documentation: add documentation about subdev state

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Feb 16, 2022 at 03:00:49PM +0200, 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..997d76134b50 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 is expected to be locked and unlocked by the

s/is expected to be/must be/

> +caller by calling :c:func:`v4l2_subdev_lock_state()` and
> +:c:func:`v4l2_subdev_unlock_state()`. Alternatively the caller can  use
> +:c:func:`v4l2_subdev_call_state_active()` macro to call the op.

I'd write the last sentence as

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_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 it's 'lock'

s/it's/its/

> +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
>  ---------------------------------------------
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/7] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Hans Verkuil]

On 2/16/22 14:00, Tomi Valkeinen wrote:
> All suitable subdev ops are now passed either the TRY or the ACTIVE
> state by the v4l2 core. However, other subdev drivers can still call the
> ops passing NULL as the state, implying the active case.
> 
> 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 here.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> ---
>  include/media/v4l2-subdev.h | 38 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 8d089a2dbd32..84c02488d53f 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1278,6 +1278,44 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
>   */
>  void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
>  
> +/**
> + * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
> + * @sd: subdevice
> + * @state: subdevice state as passed to the subdev op
> + *
> + * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
> + * NULL as the state parameter, as subdevs always used to have their active
> + * state stored privately.
> + *
> + * However, newer state-aware subdev drivers, which store their active state in
> + * a common place, subdev->active_state, expect to always get a proper state as
> + * a parameter.
> + *
> + * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
> + * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
> + * issue by using subdev->active_state in case the passed state is NULL.
> + *
> + * This is a temporary helper function, and should be removed when we can ensure
> + * that all drivers pass proper state when calling other subdevs.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state)
> +{
> +	if (state)
> +		return state;
> +
> +	dev_notice_once(sd->dev,
> +		"The provided state is NULL. Adjusting to the subdev active state.\n"
> +		"Please consider porting your driver to the new state management API.\n");
> +
> +	state = sd->active_state;
> +
> +	v4l2_subdev_lock_state(state);
> +
> +	return state;
> +}
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  /**

Re: [PATCH v4 6/7] media: subdev: add v4l2_subdev_call_state_active()

[Hans Verkuil]

On 2/16/22 14:00, Tomi Valkeinen wrote:
> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> that take a subdev state as a parameter.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> ---
>  include/media/v4l2-subdev.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 84c02488d53f..0db61203c8d9 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1359,6 +1359,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
>  		__result;						\
>  	})
>  
> +/**
> + * 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;						\
> +	})
> +
>  /**
>   * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
>   *

Re: [PATCH v4 5/7] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Hans Verkuil]

On 2/28/22 09:59, Hans Verkuil wrote:
> 
> 
> On 2/16/22 14:00, Tomi Valkeinen wrote:
>> All suitable subdev ops are now passed either the TRY or the ACTIVE
>> state by the v4l2 core. However, other subdev drivers can still call the
>> ops passing NULL as the state, implying the active case.
>>
>> 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 here.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Ignore this, I should have read Laurent's comments first. I agree with him.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>> ---
>>  include/media/v4l2-subdev.h | 38 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 8d089a2dbd32..84c02488d53f 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1278,6 +1278,44 @@ void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
>>   */
>>  void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
>>  
>> +/**
>> + * v4l2_subdev_lock_and_return_state() - Gets locked try or active subdev state
>> + * @sd: subdevice
>> + * @state: subdevice state as passed to the subdev op
>> + *
>> + * Due to legacy reasons, when subdev drivers call ops in other subdevs they use
>> + * NULL as the state parameter, as subdevs always used to have their active
>> + * state stored privately.
>> + *
>> + * However, newer state-aware subdev drivers, which store their active state in
>> + * a common place, subdev->active_state, expect to always get a proper state as
>> + * a parameter.
>> + *
>> + * These state-aware drivers can use v4l2_subdev_lock_and_return_state() instead
>> + * of v4l2_subdev_lock_state(). v4l2_subdev_lock_and_return_state() solves the
>> + * issue by using subdev->active_state in case the passed state is NULL.
>> + *
>> + * This is a temporary helper function, and should be removed when we can ensure
>> + * that all drivers pass proper state when calling other subdevs.
>> + */
>> +static inline struct v4l2_subdev_state *
>> +v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
>> +				  struct v4l2_subdev_state *state)
>> +{
>> +	if (state)
>> +		return state;
>> +
>> +	dev_notice_once(sd->dev,
>> +		"The provided state is NULL. Adjusting to the subdev active state.\n"
>> +		"Please consider porting your driver to the new state management API.\n");
>> +
>> +	state = sd->active_state;
>> +
>> +	v4l2_subdev_lock_state(state);
>> +
>> +	return state;
>> +}
>> +
>>  #endif /* CONFIG_MEDIA_CONTROLLER */
>>  
>>  /**

Re: [PATCH v4 6/7] media: subdev: add v4l2_subdev_call_state_active()

[Hans Verkuil]

On 2/28/22 09:59, Hans Verkuil wrote:
> 
> 
> On 2/16/22 14:00, Tomi Valkeinen wrote:
>> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
>> that take a subdev state as a parameter.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Hmm, after reading Laurent's comments I also think I was a bit too hasty.
It should either be moved to a v4l2-subdev-legacy.h header (are there other
legacy functions that could be moved there as well?) or it should have 'legacy'
or something like that in the name.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>> ---
>>  include/media/v4l2-subdev.h | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 84c02488d53f..0db61203c8d9 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1359,6 +1359,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
>>  		__result;						\
>>  	})
>>  
>> +/**
>> + * 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;						\
>> +	})
>> +
>>  /**
>>   * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
>>   *

Re: [PATCH v4 6/7] media: subdev: add v4l2_subdev_call_state_active()

[Laurent Pinchart]

On Mon, Feb 28, 2022 at 10:10:00AM +0100, Hans Verkuil wrote:
> On 2/28/22 09:59, Hans Verkuil wrote:
> > On 2/16/22 14:00, Tomi Valkeinen wrote:
> >> Add v4l2_subdev_call_state_active() macro to help calling subdev ops
> >> that take a subdev state as a parameter.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > 
> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Hmm, after reading Laurent's comments I also think I was a bit too hasty.
> It should either be moved to a v4l2-subdev-legacy.h header (are there other
> legacy functions that could be moved there as well?)

I think so (I'll need to have a look though), and I expect there will be
more in the future in any case :-)

> or it should have 'legacy'
> or something like that in the name.
> 
> >> ---
> >>  include/media/v4l2-subdev.h | 31 +++++++++++++++++++++++++++++++
> >>  1 file changed, 31 insertions(+)
> >>
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 84c02488d53f..0db61203c8d9 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -1359,6 +1359,37 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
> >>  		__result;						\
> >>  	})
> >>  
> >> +/**
> >> + * 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;						\
> >> +	})
> >> +
> >>  /**
> >>   * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
> >>   *

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/7] media: subdev: add subdev state locking

[Hans Verkuil]

On 2/16/22 14:00, 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>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
>  drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
>  drivers/media/v4l2-core/v4l2-subdev.c       | 78 +++++++++++++++----
>  drivers/staging/media/tegra-video/vi.c      |  4 +-
>  include/media/v4l2-subdev.h                 | 85 ++++++++++++++++++++-
>  5 files changed, 155 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..0df9bbe1819d 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)
>  {
> @@ -972,6 +1004,26 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
>  
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
> +{
> +	mutex_lock(sd->active_state->lock);
> +
> +	return sd->active_state;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);

I don't like this function very much. First of all, call v4l2_subdev_lock_state()
instead of mutex_lock, that signals that the normal state lock function is used.

The naming is poor since this suggests that the active_state is just locked
when it actually is also returned. So v4l2_subdev_lock_and_return_active_state()
is really the correct name. Long, yes, but at least it is clear what it does.

I also think this is better done as a static inline.

But really, I wonder if we need this helper at all. Can't drivers just call
v4l2_subdev_lock_state(sd->active_state) and then use sd->active_state?

I think that's much more understandable, and it avoids having confusing
lock helper functions. More on this below in the header.

> +
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
> +{
> +	mutex_lock(state->lock);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_state);
> +
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> +{
> +	mutex_unlock(state->lock);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);

To be honest, I think these two functions could also be moved to the header
as a static inline. It will be a bit more efficient and for developers reading
the header it will be easier to understand what is going on.

> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> 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..8d089a2dbd32 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,71 @@ 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;
>  }

Do we really need these two functions? I can't help feeling that this is
overkill and that is becomes quite confusing to have all these similarly
names functions.

It's a bit of a grey area admittedly, but it does confuse me a bit.

Regards,

	Hans

>  
> +/**
> + * v4l2_subdev_lock_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.
> + */
> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
> +
> +/**
> + * 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.
> + */
> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> +
> +/**
> + * v4l2_subdev_unlock_state() - Unlocks the subdev state
> + * @state: The subdevice state
> + *
> + * Unlocks the given subdev state.
> + */
> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  /**

Re: [PATCH v4 4/7] media: subdev: add subdev state locking

[Laurent Pinchart]

Hi Hans,

On Mon, Feb 28, 2022 at 11:05:09AM +0100, Hans Verkuil wrote:
> On 2/16/22 14:00, 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>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
> >  drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
> >  drivers/media/v4l2-core/v4l2-subdev.c       | 78 +++++++++++++++----
> >  drivers/staging/media/tegra-video/vi.c      |  4 +-
> >  include/media/v4l2-subdev.h                 | 85 ++++++++++++++++++++-
> >  5 files changed, 155 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..0df9bbe1819d 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)
> >  {
> > @@ -972,6 +1004,26 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
> >  
> > +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
> > +{
> > +	mutex_lock(sd->active_state->lock);
> > +
> > +	return sd->active_state;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);
> 
> I don't like this function very much. First of all, call v4l2_subdev_lock_state()
> instead of mutex_lock, that signals that the normal state lock function is used.
> 
> The naming is poor since this suggests that the active_state is just locked
> when it actually is also returned. So v4l2_subdev_lock_and_return_active_state()
> is really the correct name. Long, yes, but at least it is clear what it does.

I agree the name isn't perfect. How about
v4l2_subdev_lock_and_get_active_state() ?

> I also think this is better done as a static inline.
> 
> But really, I wonder if we need this helper at all. Can't drivers just call
> v4l2_subdev_lock_state(sd->active_state) and then use sd->active_state?
> 
> I think that's much more understandable, and it avoids having confusing
> lock helper functions. More on this below in the header.

Drivers should never access that active_state field manually, that's a
hard rule. All accesses should go through accessors. No expection, so
it's easy to enforce the rule.

This accessor isn't meant to stay. It is here to help the transition to
active states, and will also allow quickly identifying through grep
where more work is required to move drivers to the correct API.

> > +
> > +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
> > +{
> > +	mutex_lock(state->lock);
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_state);
> > +
> > +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> > +{
> > +	mutex_unlock(state->lock);
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
> 
> To be honest, I think these two functions could also be moved to the header
> as a static inline. It will be a bit more efficient

Note that I expect in the future a switch to ww_mutex, so this will
become more complex. That being said, we can start with inline
functions, and then rework that.

> and for developers reading
> the header it will be easier to understand what is going on.

Driver developers shouldn't care how this is implemented (and should
very very much *never* touch this mutex directly, or do anything funky
with it).

> > +
> >  #endif /* CONFIG_MEDIA_CONTROLLER */
> >  
> >  void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> > 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..8d089a2dbd32 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,71 @@ 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;
> >  }
> 
> Do we really need these two functions? I can't help feeling that this is
> overkill and that is becomes quite confusing to have all these similarly
> names functions.
> 
> It's a bit of a grey area admittedly, but it does confuse me a bit.

As mentioned above, I think there will be room for simplication after
the transition. It may take a while though. In the meantime, being able
to catch locking issues is useful in my opinion.

> > +/**
> > + * v4l2_subdev_lock_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.
> > + */
> > +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
> > +
> > +/**
> > + * 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.
> > + */
> > +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
> > +
> > +/**
> > + * v4l2_subdev_unlock_state() - Unlocks the subdev state
> > + * @state: The subdevice state
> > + *
> > + * Unlocks the given subdev state.
> > + */
> > +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
> > +
> >  #endif /* CONFIG_MEDIA_CONTROLLER */
> >  
> >  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/7] media: subdev: add subdev state locking

[Hans Verkuil]

Hi Laurent,

On 2/28/22 11:14, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Feb 28, 2022 at 11:05:09AM +0100, Hans Verkuil wrote:
>> On 2/16/22 14:00, 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>
>>> ---
>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
>>>  drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
>>>  drivers/media/v4l2-core/v4l2-subdev.c       | 78 +++++++++++++++----
>>>  drivers/staging/media/tegra-video/vi.c      |  4 +-
>>>  include/media/v4l2-subdev.h                 | 85 ++++++++++++++++++++-
>>>  5 files changed, 155 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..0df9bbe1819d 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)
>>>  {
>>> @@ -972,6 +1004,26 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>>>  }
>>>  EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
>>>  
>>> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
>>> +{
>>> +	mutex_lock(sd->active_state->lock);
>>> +
>>> +	return sd->active_state;
>>> +}
>>> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);
>>
>> I don't like this function very much. First of all, call v4l2_subdev_lock_state()
>> instead of mutex_lock, that signals that the normal state lock function is used.
>>
>> The naming is poor since this suggests that the active_state is just locked
>> when it actually is also returned. So v4l2_subdev_lock_and_return_active_state()
>> is really the correct name. Long, yes, but at least it is clear what it does.
> 
> I agree the name isn't perfect. How about
> v4l2_subdev_lock_and_get_active_state() ?

That's certainly better, but then v4l2_subdev_lock_and_return_state should
also be renamed to v4l2_subdev_lock_and_get_state() to stay consistent.

> 
>> I also think this is better done as a static inline.
>>
>> But really, I wonder if we need this helper at all. Can't drivers just call
>> v4l2_subdev_lock_state(sd->active_state) and then use sd->active_state?
>>
>> I think that's much more understandable, and it avoids having confusing
>> lock helper functions. More on this below in the header.
> 
> Drivers should never access that active_state field manually, that's a
> hard rule. All accesses should go through accessors. No expection, so
> it's easy to enforce the rule.
> 
> This accessor isn't meant to stay. It is here to help the transition to
> active states, and will also allow quickly identifying through grep
> where more work is required to move drivers to the correct API.

OK. Perhaps this should be mentioned in the function comments.

Actually, I think that is a good point: make it clear in the header and
perhaps in the source implementation as well if a function is temporary,
and explain what needs to be done to eventually be able to remove it.

This probably also ties in with a v4l2-subdev-legacy.h header.

> 
>>> +
>>> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
>>> +{
>>> +	mutex_lock(state->lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_state);
>>> +
>>> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
>>> +{
>>> +	mutex_unlock(state->lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
>>
>> To be honest, I think these two functions could also be moved to the header
>> as a static inline. It will be a bit more efficient
> 
> Note that I expect in the future a switch to ww_mutex, so this will
> become more complex. That being said, we can start with inline
> functions, and then rework that.
> 
>> and for developers reading
>> the header it will be easier to understand what is going on.
> 
> Driver developers shouldn't care how this is implemented (and should
> very very much *never* touch this mutex directly, or do anything funky
> with it).

If things go wrong, they really do care about what mutex is involved :-)

> 
>>> +
>>>  #endif /* CONFIG_MEDIA_CONTROLLER */
>>>  
>>>  void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>>> 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..8d089a2dbd32 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,71 @@ 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;
>>>  }
>>
>> Do we really need these two functions? I can't help feeling that this is
>> overkill and that is becomes quite confusing to have all these similarly
>> names functions.
>>
>> It's a bit of a grey area admittedly, but it does confuse me a bit.
> 
> As mentioned above, I think there will be room for simplication after
> the transition. It may take a while though. In the meantime, being able
> to catch locking issues is useful in my opinion.

OK.

Regards,

	Hans

> 
>>> +/**
>>> + * v4l2_subdev_lock_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.
>>> + */
>>> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
>>> +
>>> +/**
>>> + * 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.
>>> + */
>>> +void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
>>> +
>>> +/**
>>> + * v4l2_subdev_unlock_state() - Unlocks the subdev state
>>> + * @state: The subdevice state
>>> + *
>>> + * Unlocks the given subdev state.
>>> + */
>>> +void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
>>> +
>>>  #endif /* CONFIG_MEDIA_CONTROLLER */
>>>  
>>>  /**
> 

Re: [PATCH v4 4/7] media: subdev: add subdev state locking

[Tomi Valkeinen]

Hi,

On 28/02/2022 12:14, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Feb 28, 2022 at 11:05:09AM +0100, Hans Verkuil wrote:
>> On 2/16/22 14:00, 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>
>>> ---
>>>   drivers/media/platform/rcar-vin/rcar-v4l2.c |  3 +-
>>>   drivers/media/platform/vsp1/vsp1_entity.c   |  4 +-
>>>   drivers/media/v4l2-core/v4l2-subdev.c       | 78 +++++++++++++++----
>>>   drivers/staging/media/tegra-video/vi.c      |  4 +-
>>>   include/media/v4l2-subdev.h                 | 85 ++++++++++++++++++++-
>>>   5 files changed, 155 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..0df9bbe1819d 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)
>>>   {
>>> @@ -972,6 +1004,26 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>>>   }
>>>   EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
>>>   
>>> +struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
>>> +{
>>> +	mutex_lock(sd->active_state->lock);
>>> +
>>> +	return sd->active_state;
>>> +}
>>> +EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);
>>
>> I don't like this function very much. First of all, call v4l2_subdev_lock_state()
>> instead of mutex_lock, that signals that the normal state lock function is used.
>>
>> The naming is poor since this suggests that the active_state is just locked
>> when it actually is also returned. So v4l2_subdev_lock_and_return_active_state()
>> is really the correct name. Long, yes, but at least it is clear what it does.
> 
> I agree the name isn't perfect. How about
> v4l2_subdev_lock_and_get_active_state() ?
> 
>> I also think this is better done as a static inline.
>>
>> But really, I wonder if we need this helper at all. Can't drivers just call
>> v4l2_subdev_lock_state(sd->active_state) and then use sd->active_state?
>>
>> I think that's much more understandable, and it avoids having confusing
>> lock helper functions. More on this below in the header.
> 
> Drivers should never access that active_state field manually, that's a
> hard rule. All accesses should go through accessors. No expection, so
> it's easy to enforce the rule.
> 
> This accessor isn't meant to stay. It is here to help the transition to
> active states, and will also allow quickly identifying through grep
> where more work is required to move drivers to the correct API.

Why wouldn't v4l2_subdev_lock_and_get_active_state() stay? I think it's 
the function to use when e.g. a driver gets a callback from debugfs.

  Tomi