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

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

[Tomi Valkeinen]

Hi,

v2 of the subdev active state series. I've fixed this according to
comments from Hans and Jacopo:

- doc improvements
- drop extra #include
- use active_state in two places instead of state
- improve the notice in v4l2_subdev_lock_and_return_state()

 Tomi

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

 .../driver-api/media/v4l2-subdev.rst          |  57 +++++++
 drivers/media/platform/rcar-vin/rcar-v4l2.c   |   9 +-
 drivers/media/platform/vsp1/vsp1_entity.c     |  10 +-
 drivers/media/v4l2-core/v4l2-subdev.c         | 125 +++++++++++++--
 drivers/staging/media/tegra-video/vi.c        |  10 +-
 include/media/v4l2-subdev.h                   | 150 +++++++++++++++++-
 6 files changed, 334 insertions(+), 27 deletions(-)

-- 
2.25.1

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

[Tomi Valkeinen]

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

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  4 ++--
 drivers/media/platform/vsp1/vsp1_entity.c   |  4 ++--
 drivers/media/v4l2-core/v4l2-subdev.c       | 12 ++++++------
 drivers/staging/media/tegra-video/vi.c      |  4 ++--
 include/media/v4l2-subdev.h                 | 10 +++++-----
 5 files changed, 17 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..5aae01456c04 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -263,7 +263,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 	u32 width, height;
 	int ret;
 
-	sd_state = v4l2_subdev_alloc_state(sd);
+	sd_state = __v4l2_subdev_state_alloc(sd);
 	if (IS_ERR(sd_state))
 		return PTR_ERR(sd_state);
 
@@ -299,7 +299,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..869cadc1468d 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.c
+++ b/drivers/media/platform/vsp1/vsp1_entity.c
@@ -675,7 +675,7 @@ 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);
+	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 +690,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 5d27a27cc2f2..fe49c86a9b02 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;
 }
 
@@ -870,7 +870,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;
@@ -903,9 +903,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;
@@ -913,7 +913,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 69d9787d5338..b01f19e0f332 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -507,7 +507,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	if (!subdev)
 		return -ENODEV;
 
-	sd_state = v4l2_subdev_alloc_state(subdev);
+	sd_state = __v4l2_subdev_state_alloc(subdev);
 	if (IS_ERR(sd_state))
 		return PTR_ERR(sd_state);
 	/*
@@ -558,7 +558,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 95ec18c2f49c..e52bf508c75b 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1135,20 +1135,20 @@ 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.
  */
-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.
  */
-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 v2 2/6] media: subdev: add active state to struct v4l2_subdev

[Tomi Valkeinen]

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 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 fe49c86a9b02..de160140d63b 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
 	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
+
+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);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index e52bf508c75b..eddf72768e10 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;
@@ -898,6 +901,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.
@@ -929,6 +935,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;
 };
 
 
@@ -1217,4 +1236,43 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
 void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
 			      const struct v4l2_event *ev);
 
+/**
+ * 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 needed 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
-- 
2.25.1

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3151 bytes --]

Hi Tomi,

I love your patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on tegra/for-next v5.16-rc6 next-20211220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/v4l-subdev-active-state/20211217-215244
base:   git://linuxtv.org/media_tree.git master
config: mips-loongson2k_defconfig (https://download.01.org/0day-ci/archive/20211221/202112210321.dlOWsxJG-lkp(a)intel.com/config)
compiler: mips64el-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/728c6f8f7c038fd0c851160b170cdc08f1999b38
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tomi-Valkeinen/v4l-subdev-active-state/20211217-215244
        git checkout 728c6f8f7c038fd0c851160b170cdc08f1999b38
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips SHELL=/bin/bash drivers/media/v4l2-core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/media/v4l2-core/v4l2-subdev.c: In function 'v4l2_subdev_init_finalize':
>> drivers/media/v4l2-core/v4l2-subdev.c:951:17: error: implicit declaration of function '__v4l2_subdev_state_alloc' [-Werror=implicit-function-declaration]
     951 |         state = __v4l2_subdev_state_alloc(sd);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/v4l2-core/v4l2-subdev.c:951:15: warning: assignment to 'struct v4l2_subdev_state *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     951 |         state = __v4l2_subdev_state_alloc(sd);
         |               ^
   drivers/media/v4l2-core/v4l2-subdev.c: In function 'v4l2_subdev_cleanup':
   drivers/media/v4l2-core/v4l2-subdev.c:963:9: error: implicit declaration of function '__v4l2_subdev_state_free' [-Werror=implicit-function-declaration]
     963 |         __v4l2_subdev_state_free(sd->active_state);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/__v4l2_subdev_state_alloc +951 drivers/media/v4l2-core/v4l2-subdev.c

   946	
   947	int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
   948	{
   949		struct v4l2_subdev_state *state;
   950	
 > 951		state = __v4l2_subdev_state_alloc(sd);
   952		if (IS_ERR(state))
   953			return PTR_ERR(state);
   954	
   955		sd->active_state = state;
   956	
   957		return 0;
   958	}
   959	EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
   960	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Dec 17, 2021 at 03:50:18PM +0200, Tomi Valkeinen wrote:
> 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>
> ---
>  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 fe49c86a9b02..de160140d63b 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
> +
> +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);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index e52bf508c75b..eddf72768e10 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.

There are very few drivers that access the fields directly, how about a
patch on top to address this ?

>   */
>  struct v4l2_subdev_pad_config {
>  	struct v4l2_mbus_framefmt try_fmt;
> @@ -898,6 +901,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.
> @@ -929,6 +935,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;
>  };
>  
>  
> @@ -1217,4 +1236,43 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
>  void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  			      const struct v4l2_event *ev);
>  
> +/**
> + * 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 needed by the subdevice

s/needed/allocated/

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

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

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

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

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

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

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

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

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

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

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index de160140d63b..779d7ae2262d 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -353,6 +353,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);
@@ -360,8 +398,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;
@@ -484,7 +525,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: {
@@ -495,7 +536,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: {
@@ -509,7 +550,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;
 
@@ -531,7 +572,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;
 
@@ -542,7 +583,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);
 	}
 
@@ -550,7 +591,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);
 	}
 
@@ -575,7 +616,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);
 	}
 
@@ -584,7 +625,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: {
@@ -595,7 +636,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: {
@@ -829,10 +870,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 v2 4/6] media: subdev: add subdev state locking

[Tomi Valkeinen]

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

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

Add a mutex to the struct v4l2_subdev_state, along with a few helper
functions for locking/unlocking.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 5aae01456c04..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,
@@ -263,7 +264,11 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 	u32 width, height;
 	int ret;
 
-	sd_state = __v4l2_subdev_state_alloc(sd);
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	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 869cadc1468d..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;
@@ -675,7 +676,12 @@ 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_state_alloc(&entity->subdev);
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	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 779d7ae2262d..5322329d396f 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);
 
@@ -914,7 +915,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;
@@ -923,6 +926,8 @@ 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->entity.num_pads) {
 		state->pads = kvmalloc_array(sd->entity.num_pads,
 					     sizeof(*state->pads),
@@ -954,6 +959,8 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
 	if (!state)
 		return;
 
+	mutex_destroy(&state->lock);
+
 	kvfree(state->pads);
 	kfree(state);
 }
@@ -988,11 +995,12 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
 
-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);
 
@@ -1000,7 +1008,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)
 {
@@ -1008,3 +1016,23 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
 	sd->active_state = NULL;
 }
 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);
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index b01f19e0f332..b366b56df15c 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;
@@ -507,7 +508,12 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 	if (!subdev)
 		return -ENODEV;
 
-	sd_state = __v4l2_subdev_state_alloc(subdev);
+	/*
+	 * FIXME: Drop this call, drivers are not supposed to use
+	 * __v4l2_subdev_state_alloc().
+	 */
+	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 eddf72768e10..867e19ef80bd 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -658,6 +658,7 @@ struct v4l2_subdev_pad_config {
 /**
  * struct v4l2_subdev_state - Used for storing subdev state information.
  *
+ * @lock: mutex for the state
  * @pads: &struct v4l2_subdev_pad_config array
  *
  * This structure only needs to be passed to the pad op if the 'which' field
@@ -665,6 +666,8 @@ 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 v4l2_subdev_pad_config *pads;
 };
 
@@ -1157,10 +1160,14 @@ 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.
  */
-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
@@ -1250,7 +1257,16 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
  *
  * 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 needed by the subdevice
@@ -1275,4 +1291,34 @@ v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
 	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
-- 
2.25.1

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

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Dec 17, 2021 at 03:50:20PM +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.
> 
> Add a mutex to the struct v4l2_subdev_state, along with a few helper
> functions for locking/unlocking.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  7 ++-
>  drivers/media/platform/vsp1/vsp1_entity.c   |  8 +++-
>  drivers/media/v4l2-core/v4l2-subdev.c       | 38 +++++++++++++---
>  drivers/staging/media/tegra-video/vi.c      |  8 +++-
>  include/media/v4l2-subdev.h                 | 50 ++++++++++++++++++++-
>  5 files changed, 101 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 5aae01456c04..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,
> @@ -263,7 +264,11 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>  	u32 width, height;
>  	int ret;
>  
> -	sd_state = __v4l2_subdev_state_alloc(sd);
> +	/*
> +	 * FIXME: Drop this call, drivers are not supposed to use
> +	 * __v4l2_subdev_state_alloc().
> +	 */
> +	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 869cadc1468d..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;
> @@ -675,7 +676,12 @@ 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_state_alloc(&entity->subdev);
> +	/*
> +	 * FIXME: Drop this call, drivers are not supposed to use
> +	 * __v4l2_subdev_state_alloc().
> +	 */

Feel free to remind me to fix this once the series lands :-)

> +	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 779d7ae2262d..5322329d396f 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);
>  
> @@ -914,7 +915,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;
> @@ -923,6 +926,8 @@ 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->entity.num_pads) {
>  		state->pads = kvmalloc_array(sd->entity.num_pads,
>  					     sizeof(*state->pads),
> @@ -954,6 +959,8 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
>  	if (!state)
>  		return;
>  
> +	mutex_destroy(&state->lock);
> +
>  	kvfree(state->pads);
>  	kfree(state);
>  }
> @@ -988,11 +995,12 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
>  
> -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);
>  
> @@ -1000,7 +1008,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)
>  {
> @@ -1008,3 +1016,23 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
>  	sd->active_state = NULL;
>  }
>  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);
> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
> index b01f19e0f332..b366b56df15c 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;
> @@ -507,7 +508,12 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
>  	if (!subdev)
>  		return -ENODEV;
>  
> -	sd_state = __v4l2_subdev_state_alloc(subdev);
> +	/*
> +	 * FIXME: Drop this call, drivers are not supposed to use
> +	 * __v4l2_subdev_state_alloc().
> +	 */
> +	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 eddf72768e10..867e19ef80bd 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -658,6 +658,7 @@ struct v4l2_subdev_pad_config {
>  /**
>   * struct v4l2_subdev_state - Used for storing subdev state information.
>   *
> + * @lock: mutex for the state
>   * @pads: &struct v4l2_subdev_pad_config array
>   *
>   * This structure only needs to be passed to the pad op if the 'which' field
> @@ -665,6 +666,8 @@ 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 v4l2_subdev_pad_config *pads;
>  };
>  
> @@ -1157,10 +1160,14 @@ 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.
>   */
> -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
> @@ -1250,7 +1257,16 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>   *
>   * 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 needed by the subdevice
> @@ -1275,4 +1291,34 @@ v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
>  	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

-- 
Regards,

Laurent Pinchart

[PATCH v2 5/6] 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 867e19ef80bd..d6c7db1d37e4 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1321,4 +1321,40 @@ 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)
+		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 = state ? state : sd->active_state;
+
+	v4l2_subdev_lock_state(state);
+
+	return state;
+}
+
 #endif
-- 
2.25.1

Re: [PATCH v2 5/6] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Sakari Ailus]

Moi,

On Fri, Dec 17, 2021 at 03:50:21PM +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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 867e19ef80bd..d6c7db1d37e4 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1321,4 +1321,40 @@ 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)
> +		dev_notice_once(sd->dev,

I thought about this a little more.

This currently warns once when there's something that calls the function
with NULL state.

How about going a bit further, moving this to a macro so all instances will
be covered, and then using WARN_ON_ONCE() so there would be a clear
indication where the warning comes from?

> +			"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 = state ? state : sd->active_state;
> +
> +	v4l2_subdev_lock_state(state);
> +
> +	return state;
> +}
> +
>  #endif

-- 
Kind regards.

Sakari Ailus

Re: [PATCH v2 5/6] media: subdev: Add v4l2_subdev_lock_and_return_state()

[Sakari Ailus]

On Mon, Dec 20, 2021 at 02:51:32PM +0200, Sakari Ailus wrote:
> Moi,
> 
> On Fri, Dec 17, 2021 at 03:50:21PM +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>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 867e19ef80bd..d6c7db1d37e4 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -1321,4 +1321,40 @@ 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)
> > +		dev_notice_once(sd->dev,
> 
> I thought about this a little more.
> 
> This currently warns once when there's something that calls the function
> with NULL state.
> 
> How about going a bit further, moving this to a macro so all instances will
> be covered, and then using WARN_ON_ONCE() so there would be a clear
> indication where the warning comes from?

Actually --- please ignore this. The function is already static inline in
the header and prints the device name. That should be enough to point out
the culprit.

-- 
Sakari Ailus

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

[Tomi Valkeinen]

Add documentation about centrally managed subdev state.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
index 08ea2673b19e..18b00bd3d6d4 100644
--- a/Documentation/driver-api/media/v4l2-subdev.rst
+++ b/Documentation/driver-api/media/v4l2-subdev.rst
@@ -518,6 +518,63 @@ 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 e.g. as an array of
+struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
+and composition rectangles.
+
+In addition to the active configuration, each subdev file-handle has an array of
+struct v4l2_subdev_pad_config, managed by 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 associated with the sub-device itself as part of
+the :c:type:`v4l2_subdev` structure, while the core associates to each open
+file-handle a try state, which contains the configuration valid in the
+file-handle context only.
+
+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 acquired resources before unregistering the sub-device.
+The core automatically initializes a state for each open file-handle where to
+store the try configurations and releases them at file-handle closing time.
+
+V4L2 sub-device operations that use both the :ref:`ACTIVE and TRY formats
+<v4l2-subdev-format-whence>` receive the correct state to operate on as a
+'state' parameter. The sub-device driver can access and modify the
+configuration stored in the provided state after having locked it by calling
+:c:func:`v4l2_subdev_lock_state()`. The driver must then call
+:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
+
+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 will pass the correct try- or active-state to the
+subdevice operations, device drivers might call operations on other
+subdevices by using :c:func:`v4l2_subdev_call()` kAPI and pass NULL as the
+state. This is only a problem for subdev drivers which use the
+centrally managed active-state and are used in media pipelines with older
+subdev drivers. In these cases the called subdev ops must also handle the NULL
+case. This can be easily managed by the use of
+:c:func:`v4l2_subdev_lock_and_return_state()` helper.
+
+: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.
+
 V4L2 sub-device functions and data structures
 ---------------------------------------------
 
-- 
2.25.1

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

[Hans Verkuil]

On 17/12/2021 14:50, 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>
> ---
>  .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> index 08ea2673b19e..18b00bd3d6d4 100644
> --- a/Documentation/driver-api/media/v4l2-subdev.rst
> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> @@ -518,6 +518,63 @@ 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 e.g. as an array of

e.g. as -> as e.g.

> +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
> +and composition rectangles.
> +
> +In addition to the active configuration, each subdev file-handle has an array of
> +struct v4l2_subdev_pad_config, managed by V4L2 core, which contains the try

by V4L2 -> by the V4L2

> +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 associated with the sub-device itself as part of
> +the :c:type:`v4l2_subdev` structure, while the core associates to each open
> +file-handle a try state, which contains the configuration valid in the
> +file-handle context only.
> +
> +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 acquired resources before unregistering the sub-device.
> +The core automatically initializes a state for each open file-handle where to
> +store the try configurations and releases them at file-handle closing time.
> +
> +V4L2 sub-device operations that use both the :ref:`ACTIVE and TRY formats
> +<v4l2-subdev-format-whence>` receive the correct state to operate on as a
> +'state' parameter. The sub-device driver can access and modify the
> +configuration stored in the provided state after having locked it by calling
> +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
> +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
> +
> +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 will pass the correct try- or active-state to the
> +subdevice operations, device drivers might call operations on other
> +subdevices by using :c:func:`v4l2_subdev_call()` kAPI and pass NULL as the
> +state. This is only a problem for subdev drivers which use the
> +centrally managed active-state and are used in media pipelines with older
> +subdev drivers. In these cases the called subdev ops must also handle the NULL
> +case. This can be easily managed by the use of
> +:c:func:`v4l2_subdev_lock_and_return_state()` helper.
> +
> +: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.
> +
>  V4L2 sub-device functions and data structures
>  ---------------------------------------------
>  
> 

With those tiny changes:

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

Regards,

	Hans

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

[Sakari Ailus]

Moi,

Thanks for the update.

On Fri, Dec 17, 2021 at 03:50:22PM +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>
> ---
>  .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> index 08ea2673b19e..18b00bd3d6d4 100644
> --- a/Documentation/driver-api/media/v4l2-subdev.rst
> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> @@ -518,6 +518,63 @@ 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 e.g. as an array of
> +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
> +and composition rectangles.
> +
> +In addition to the active configuration, each subdev file-handle has an array of
> +struct v4l2_subdev_pad_config, managed by 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 associated with the sub-device itself as part of
> +the :c:type:`v4l2_subdev` structure, while the core associates to each open
> +file-handle a try state, which contains the configuration valid in the
> +file-handle context only.
> +
> +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 acquired resources before unregistering the sub-device.
> +The core automatically initializes a state for each open file-handle where to
> +store the try configurations and releases them at file-handle closing time.
> +
> +V4L2 sub-device operations that use both the :ref:`ACTIVE and TRY formats
> +<v4l2-subdev-format-whence>` receive the correct state to operate on as a
> +'state' parameter. The sub-device driver can access and modify the
> +configuration stored in the provided state after having locked it by calling
> +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
> +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
> +
> +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.

Have you thought how this will interact with controls?

Generally active state information exists for a device in struct
v4l2_subdev_state as well as the device's control handler as control
values. Controls have dependencies to other state information (active and
try).

Until now, drivers have been responsible for serialising access to this
state on their own, mostly using a single mutex. Controls require a mutex
as well, but it's the same mutex independently of whether a driver is
dealing with active or try subdev state.

In other words, if the above is assumed, when you're dealing with try state
that has dependencies to controls, you have to hold both that try state's
mutex as well as the control handler's mutex.

> +
> +While the V4L2 core will pass the correct try- or active-state to the
> +subdevice operations, device drivers might call operations on other
> +subdevices by using :c:func:`v4l2_subdev_call()` kAPI and pass NULL as the
> +state. This is only a problem for subdev drivers which use the
> +centrally managed active-state and are used in media pipelines with older
> +subdev drivers. In these cases the called subdev ops must also handle the NULL
> +case. This can be easily managed by the use of
> +:c:func:`v4l2_subdev_lock_and_return_state()` helper.
> +
> +: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.
> +
>  V4L2 sub-device functions and data structures
>  ---------------------------------------------
>  

-- 
Terveisin,

Sakari Ailus

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

[Laurent Pinchart]

Moi Sakari,

On Mon, Dec 20, 2021 at 07:56:31PM +0200, Sakari Ailus wrote:
> Moi,
> 
> Thanks for the update.

Kiitos arvostelusta.

> On Fri, Dec 17, 2021 at 03:50:22PM +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>
> > ---
> >  .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> > index 08ea2673b19e..18b00bd3d6d4 100644
> > --- a/Documentation/driver-api/media/v4l2-subdev.rst
> > +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> > @@ -518,6 +518,63 @@ 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 e.g. as an array of
> > +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
> > +and composition rectangles.

I think we usually write

s/cropping/crop/
s/composition/compose/

> > +
> > +In addition to the active configuration, each subdev file-handle has an array of

s/file-handle/file handle/ (through the whole patch)

> > +struct v4l2_subdev_pad_config, managed by 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 associated with the sub-device itself as part of

s/associated with/stored in/

> > +the :c:type:`v4l2_subdev` structure, while the core associates to each open
> > +file-handle a try state, which contains the configuration valid in the
> > +file-handle context only.

I'd write

[...] 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 acquired resources before unregistering the sub-device.

s/acquired/allocated/

> > +The core automatically initializes a state for each open file-handle where to

s/initializes/allocates and initializes/
s/where to/to/

> > +store the try configurations and releases them at file-handle closing time.

s/releases it at file-handle closing time/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 as a

s/as a/through the/

> > +'state' parameter. The sub-device driver can access and modify the
> > +configuration stored in the provided state after having locked it by calling

s/locked it/locked the state/

> > +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
> > +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
> > +
> > +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.
> 
> Have you thought how this will interact with controls?
> 
> Generally active state information exists for a device in struct
> v4l2_subdev_state as well as the device's control handler as control
> values. Controls have dependencies to other state information (active and
> try).
> 
> Until now, drivers have been responsible for serialising access to this
> state on their own, mostly using a single mutex. Controls require a mutex
> as well, but it's the same mutex independently of whether a driver is
> dealing with active or try subdev state.
> 
> In other words, if the above is assumed, when you're dealing with try state
> that has dependencies to controls, you have to hold both that try state's
> mutex as well as the control handler's mutex.

Going forward, I think we should store the controls in the subdev state.
That will require a uAPI extension to pass a `which` parameter to the
control ioctls, and deprecated the control TRY ioctl on subdevs.
Interactions between controls and pad formats will be easier to test, as
applications will be able to set controls in the TRY state, interacting
with the TRY formats. We will also need to rework the control handler
operations to split .s_ctrl() in two, with one function to adjust a
control value and one function to apply it.

In the meantime, I think we'll need to acquire both locks, or possibly
use the active state lock as the control handler lock.

> > +
> > +While the V4L2 core will pass the correct try- or active-state to the

s/try-/try/
s/active-/active /

> > +subdevice operations, device drivers might call operations on other
> > +subdevices by using :c:func:`v4l2_subdev_call()` kAPI and pass NULL as the
> > +state. This is only a problem for subdev drivers which use the
> > +centrally managed active-state and are used in media pipelines with older
> > +subdev drivers. In these cases the called subdev ops must also handle the NULL
> > +case. This can be easily managed by the use of
> > +:c:func:`v4l2_subdev_lock_and_return_state()` helper.

This doesn't emphasize strongly enough that this pattern should be
phased out. You could write it as follows.

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.

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

> > +
> > +: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.
> > +
> >  V4L2 sub-device functions and data structures
> >  ---------------------------------------------
> >  

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Tue, Dec 21, 2021 at 10:36:43AM +0200, Laurent Pinchart wrote:
> Moi Sakari,
> 
> On Mon, Dec 20, 2021 at 07:56:31PM +0200, Sakari Ailus wrote:
> > Moi,
> > 
> > Thanks for the update.
> 
> Kiitos arvostelusta.
> 
> > On Fri, Dec 17, 2021 at 03:50:22PM +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>
> > > ---
> > >  .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
> > >  1 file changed, 57 insertions(+)
> > > 
> > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> > > index 08ea2673b19e..18b00bd3d6d4 100644
> > > --- a/Documentation/driver-api/media/v4l2-subdev.rst
> > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> > > @@ -518,6 +518,63 @@ 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 e.g. as an array of
> > > +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
> > > +and composition rectangles.
> 
> I think we usually write
> 
> s/cropping/crop/
> s/composition/compose/
> 
> > > +
> > > +In addition to the active configuration, each subdev file-handle has an array of
> 
> s/file-handle/file handle/ (through the whole patch)
> 
> > > +struct v4l2_subdev_pad_config, managed by 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 associated with the sub-device itself as part of
> 
> s/associated with/stored in/
> 
> > > +the :c:type:`v4l2_subdev` structure, while the core associates to each open
> > > +file-handle a try state, which contains the configuration valid in the
> > > +file-handle context only.
> 
> I'd write
> 
> [...] 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 acquired resources before unregistering the sub-device.
> 
> s/acquired/allocated/
> 
> > > +The core automatically initializes a state for each open file-handle where to
> 
> s/initializes/allocates and initializes/
> s/where to/to/
> 
> > > +store the try configurations and releases them at file-handle closing time.
> 
> s/releases it at file-handle closing time/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 as a
> 
> s/as a/through the/
> 
> > > +'state' parameter. The sub-device driver can access and modify the
> > > +configuration stored in the provided state after having locked it by calling
> 
> s/locked it/locked the state/
> 
> > > +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
> > > +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
> > > +
> > > +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.
> > 
> > Have you thought how this will interact with controls?
> > 
> > Generally active state information exists for a device in struct
> > v4l2_subdev_state as well as the device's control handler as control
> > values. Controls have dependencies to other state information (active and
> > try).
> > 
> > Until now, drivers have been responsible for serialising access to this
> > state on their own, mostly using a single mutex. Controls require a mutex
> > as well, but it's the same mutex independently of whether a driver is
> > dealing with active or try subdev state.
> > 
> > In other words, if the above is assumed, when you're dealing with try state
> > that has dependencies to controls, you have to hold both that try state's
> > mutex as well as the control handler's mutex.
> 
> Going forward, I think we should store the controls in the subdev state.
> That will require a uAPI extension to pass a `which` parameter to the
> control ioctls, and deprecated the control TRY ioctl on subdevs.
> Interactions between controls and pad formats will be easier to test, as
> applications will be able to set controls in the TRY state, interacting
> with the TRY formats. We will also need to rework the control handler
> operations to split .s_ctrl() in two, with one function to adjust a
> control value and one function to apply it.
> 
> In the meantime, I think we'll need to acquire both locks, or possibly
> use the active state lock as the control handler lock.

Note that also trying controls requires locking the control handler,
meaning that the control handler's mutex may not be the same as the active
state mutex (unless access also to try state is serialised using the same
mutex).

What I'm saying is that to make this better usable with controls, changes
will be needed somewhere as the locking scheme is a poor match with that of
controls currently. Just saying the mutexes are acquired in a certain
order and pushing the problem to drivers is not a great solution.

-- 
Sakari Ailus

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

[Laurent Pinchart]

Hi Sakari,

On Tue, Dec 21, 2021 at 12:27:26PM +0200, Sakari Ailus wrote:
> On Tue, Dec 21, 2021 at 10:36:43AM +0200, Laurent Pinchart wrote:
> > On Mon, Dec 20, 2021 at 07:56:31PM +0200, Sakari Ailus wrote:
> > > Moi,
> > > 
> > > Thanks for the update.
> > 
> > Kiitos arvostelusta.
> > 
> > > On Fri, Dec 17, 2021 at 03:50:22PM +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>
> > > > ---
> > > >  .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
> > > >  1 file changed, 57 insertions(+)
> > > > 
> > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> > > > index 08ea2673b19e..18b00bd3d6d4 100644
> > > > --- a/Documentation/driver-api/media/v4l2-subdev.rst
> > > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> > > > @@ -518,6 +518,63 @@ 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 e.g. as an array of
> > > > +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
> > > > +and composition rectangles.
> > 
> > I think we usually write
> > 
> > s/cropping/crop/
> > s/composition/compose/
> > 
> > > > +
> > > > +In addition to the active configuration, each subdev file-handle has an array of
> > 
> > s/file-handle/file handle/ (through the whole patch)
> > 
> > > > +struct v4l2_subdev_pad_config, managed by 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 associated with the sub-device itself as part of
> > 
> > s/associated with/stored in/
> > 
> > > > +the :c:type:`v4l2_subdev` structure, while the core associates to each open
> > > > +file-handle a try state, which contains the configuration valid in the
> > > > +file-handle context only.
> > 
> > I'd write
> > 
> > [...] 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 acquired resources before unregistering the sub-device.
> > 
> > s/acquired/allocated/
> > 
> > > > +The core automatically initializes a state for each open file-handle where to
> > 
> > s/initializes/allocates and initializes/
> > s/where to/to/
> > 
> > > > +store the try configurations and releases them at file-handle closing time.
> > 
> > s/releases it at file-handle closing time/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 as a
> > 
> > s/as a/through the/
> > 
> > > > +'state' parameter. The sub-device driver can access and modify the
> > > > +configuration stored in the provided state after having locked it by calling
> > 
> > s/locked it/locked the state/
> > 
> > > > +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
> > > > +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
> > > > +
> > > > +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.
> > > 
> > > Have you thought how this will interact with controls?
> > > 
> > > Generally active state information exists for a device in struct
> > > v4l2_subdev_state as well as the device's control handler as control
> > > values. Controls have dependencies to other state information (active and
> > > try).
> > > 
> > > Until now, drivers have been responsible for serialising access to this
> > > state on their own, mostly using a single mutex. Controls require a mutex
> > > as well, but it's the same mutex independently of whether a driver is
> > > dealing with active or try subdev state.
> > > 
> > > In other words, if the above is assumed, when you're dealing with try state
> > > that has dependencies to controls, you have to hold both that try state's
> > > mutex as well as the control handler's mutex.
> > 
> > Going forward, I think we should store the controls in the subdev state.
> > That will require a uAPI extension to pass a `which` parameter to the
> > control ioctls, and deprecated the control TRY ioctl on subdevs.
> > Interactions between controls and pad formats will be easier to test, as
> > applications will be able to set controls in the TRY state, interacting
> > with the TRY formats. We will also need to rework the control handler
> > operations to split .s_ctrl() in two, with one function to adjust a
> > control value and one function to apply it.
> > 
> > In the meantime, I think we'll need to acquire both locks, or possibly
> > use the active state lock as the control handler lock.
> 
> Note that also trying controls requires locking the control handler,
> meaning that the control handler's mutex may not be the same as the active
> state mutex (unless access also to try state is serialised using the same
> mutex).
> 
> What I'm saying is that to make this better usable with controls, changes
> will be needed somewhere as the locking scheme is a poor match with that of
> controls currently. Just saying the mutexes are acquired in a certain
> order and pushing the problem to drivers is not a great solution.

Could you maybe provide an example of existing subdev driver code that
showcases this issue ? I'm not sure we really understand each other
here.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Tue, Dec 21, 2021 at 12:33:46PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Dec 21, 2021 at 12:27:26PM +0200, Sakari Ailus wrote:
> > On Tue, Dec 21, 2021 at 10:36:43AM +0200, Laurent Pinchart wrote:
> > > On Mon, Dec 20, 2021 at 07:56:31PM +0200, Sakari Ailus wrote:
> > > > Moi,
> > > > 
> > > > Thanks for the update.
> > > 
> > > Kiitos arvostelusta.
> > > 
> > > > On Fri, Dec 17, 2021 at 03:50:22PM +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>
> > > > > ---
> > > > >  .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
> > > > >  1 file changed, 57 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> > > > > index 08ea2673b19e..18b00bd3d6d4 100644
> > > > > --- a/Documentation/driver-api/media/v4l2-subdev.rst
> > > > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> > > > > @@ -518,6 +518,63 @@ 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 e.g. as an array of
> > > > > +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
> > > > > +and composition rectangles.
> > > 
> > > I think we usually write
> > > 
> > > s/cropping/crop/
> > > s/composition/compose/
> > > 
> > > > > +
> > > > > +In addition to the active configuration, each subdev file-handle has an array of
> > > 
> > > s/file-handle/file handle/ (through the whole patch)
> > > 
> > > > > +struct v4l2_subdev_pad_config, managed by 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 associated with the sub-device itself as part of
> > > 
> > > s/associated with/stored in/
> > > 
> > > > > +the :c:type:`v4l2_subdev` structure, while the core associates to each open
> > > > > +file-handle a try state, which contains the configuration valid in the
> > > > > +file-handle context only.
> > > 
> > > I'd write
> > > 
> > > [...] 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 acquired resources before unregistering the sub-device.
> > > 
> > > s/acquired/allocated/
> > > 
> > > > > +The core automatically initializes a state for each open file-handle where to
> > > 
> > > s/initializes/allocates and initializes/
> > > s/where to/to/
> > > 
> > > > > +store the try configurations and releases them at file-handle closing time.
> > > 
> > > s/releases it at file-handle closing time/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 as a
> > > 
> > > s/as a/through the/
> > > 
> > > > > +'state' parameter. The sub-device driver can access and modify the
> > > > > +configuration stored in the provided state after having locked it by calling
> > > 
> > > s/locked it/locked the state/
> > > 
> > > > > +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
> > > > > +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
> > > > > +
> > > > > +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.
> > > > 
> > > > Have you thought how this will interact with controls?
> > > > 
> > > > Generally active state information exists for a device in struct
> > > > v4l2_subdev_state as well as the device's control handler as control
> > > > values. Controls have dependencies to other state information (active and
> > > > try).
> > > > 
> > > > Until now, drivers have been responsible for serialising access to this
> > > > state on their own, mostly using a single mutex. Controls require a mutex
> > > > as well, but it's the same mutex independently of whether a driver is
> > > > dealing with active or try subdev state.
> > > > 
> > > > In other words, if the above is assumed, when you're dealing with try state
> > > > that has dependencies to controls, you have to hold both that try state's
> > > > mutex as well as the control handler's mutex.
> > > 
> > > Going forward, I think we should store the controls in the subdev state.
> > > That will require a uAPI extension to pass a `which` parameter to the
> > > control ioctls, and deprecated the control TRY ioctl on subdevs.
> > > Interactions between controls and pad formats will be easier to test, as
> > > applications will be able to set controls in the TRY state, interacting
> > > with the TRY formats. We will also need to rework the control handler
> > > operations to split .s_ctrl() in two, with one function to adjust a
> > > control value and one function to apply it.
> > > 
> > > In the meantime, I think we'll need to acquire both locks, or possibly
> > > use the active state lock as the control handler lock.
> > 
> > Note that also trying controls requires locking the control handler,
> > meaning that the control handler's mutex may not be the same as the active
> > state mutex (unless access also to try state is serialised using the same
> > mutex).
> > 
> > What I'm saying is that to make this better usable with controls, changes
> > will be needed somewhere as the locking scheme is a poor match with that of
> > controls currently. Just saying the mutexes are acquired in a certain
> > order and pushing the problem to drivers is not a great solution.
> 
> Could you maybe provide an example of existing subdev driver code that
> showcases this issue ? I'm not sure we really understand each other
> here.

Whenever you're dealing with both controls and something in the state. Also
you've got a problem if the sensor driver does I²C writes to more than
8-bit registers in 8-bit chunks and relies on hardware caching some values
before the entire register is updated.

For instance, in the CCS driver, computing the PLL tree configuration
requires state (subdev format and selection rectangles) as well as control
values as input from multiple sub-devices. I suppose this is the case with
many sensor drivers --- I just know CCS best.

The current implementation uses a single mutex for all controls and
subdevs.

-- 
Kind regards,

Sakari Ailus

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

[Laurent Pinchart]

Hi Sakari,

On Tue, Dec 21, 2021 at 01:10:08PM +0200, Sakari Ailus wrote:
> On Tue, Dec 21, 2021 at 12:33:46PM +0200, Laurent Pinchart wrote:
> > On Tue, Dec 21, 2021 at 12:27:26PM +0200, Sakari Ailus wrote:
> > > On Tue, Dec 21, 2021 at 10:36:43AM +0200, Laurent Pinchart wrote:
> > > > On Mon, Dec 20, 2021 at 07:56:31PM +0200, Sakari Ailus wrote:
> > > > > Moi,
> > > > > 
> > > > > Thanks for the update.
> > > > 
> > > > Kiitos arvostelusta.
> > > > 
> > > > > On Fri, Dec 17, 2021 at 03:50:22PM +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>
> > > > > > ---
> > > > > >  .../driver-api/media/v4l2-subdev.rst          | 57 +++++++++++++++++++
> > > > > >  1 file changed, 57 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> > > > > > index 08ea2673b19e..18b00bd3d6d4 100644
> > > > > > --- a/Documentation/driver-api/media/v4l2-subdev.rst
> > > > > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> > > > > > @@ -518,6 +518,63 @@ 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 e.g. as an array of
> > > > > > +struct v4l2_mbus_framefmt, one entry for each pad, and similarly for cropping
> > > > > > +and composition rectangles.
> > > > 
> > > > I think we usually write
> > > > 
> > > > s/cropping/crop/
> > > > s/composition/compose/
> > > > 
> > > > > > +
> > > > > > +In addition to the active configuration, each subdev file-handle has an array of
> > > > 
> > > > s/file-handle/file handle/ (through the whole patch)
> > > > 
> > > > > > +struct v4l2_subdev_pad_config, managed by 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 associated with the sub-device itself as part of
> > > > 
> > > > s/associated with/stored in/
> > > > 
> > > > > > +the :c:type:`v4l2_subdev` structure, while the core associates to each open
> > > > > > +file-handle a try state, which contains the configuration valid in the
> > > > > > +file-handle context only.
> > > > 
> > > > I'd write
> > > > 
> > > > [...] 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 acquired resources before unregistering the sub-device.
> > > > 
> > > > s/acquired/allocated/
> > > > 
> > > > > > +The core automatically initializes a state for each open file-handle where to
> > > > 
> > > > s/initializes/allocates and initializes/
> > > > s/where to/to/
> > > > 
> > > > > > +store the try configurations and releases them at file-handle closing time.
> > > > 
> > > > s/releases it at file-handle closing time/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 as a
> > > > 
> > > > s/as a/through the/
> > > > 
> > > > > > +'state' parameter. The sub-device driver can access and modify the
> > > > > > +configuration stored in the provided state after having locked it by calling
> > > > 
> > > > s/locked it/locked the state/
> > > > 
> > > > > > +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
> > > > > > +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
> > > > > > +
> > > > > > +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.
> > > > > 
> > > > > Have you thought how this will interact with controls?
> > > > > 
> > > > > Generally active state information exists for a device in struct
> > > > > v4l2_subdev_state as well as the device's control handler as control
> > > > > values. Controls have dependencies to other state information (active and
> > > > > try).
> > > > > 
> > > > > Until now, drivers have been responsible for serialising access to this
> > > > > state on their own, mostly using a single mutex. Controls require a mutex
> > > > > as well, but it's the same mutex independently of whether a driver is
> > > > > dealing with active or try subdev state.
> > > > > 
> > > > > In other words, if the above is assumed, when you're dealing with try state
> > > > > that has dependencies to controls, you have to hold both that try state's
> > > > > mutex as well as the control handler's mutex.
> > > > 
> > > > Going forward, I think we should store the controls in the subdev state.
> > > > That will require a uAPI extension to pass a `which` parameter to the
> > > > control ioctls, and deprecated the control TRY ioctl on subdevs.
> > > > Interactions between controls and pad formats will be easier to test, as
> > > > applications will be able to set controls in the TRY state, interacting
> > > > with the TRY formats. We will also need to rework the control handler
> > > > operations to split .s_ctrl() in two, with one function to adjust a
> > > > control value and one function to apply it.
> > > > 
> > > > In the meantime, I think we'll need to acquire both locks, or possibly
> > > > use the active state lock as the control handler lock.
> > > 
> > > Note that also trying controls requires locking the control handler,
> > > meaning that the control handler's mutex may not be the same as the active
> > > state mutex (unless access also to try state is serialised using the same
> > > mutex).
> > > 
> > > What I'm saying is that to make this better usable with controls, changes
> > > will be needed somewhere as the locking scheme is a poor match with that of
> > > controls currently. Just saying the mutexes are acquired in a certain
> > > order and pushing the problem to drivers is not a great solution.
> > 
> > Could you maybe provide an example of existing subdev driver code that
> > showcases this issue ? I'm not sure we really understand each other
> > here.
> 
> Whenever you're dealing with both controls and something in the state. Also
> you've got a problem if the sensor driver does I²C writes to more than
> 8-bit registers in 8-bit chunks and relies on hardware caching some values
> before the entire register is updated.
> 
> For instance, in the CCS driver, computing the PLL tree configuration
> requires state (subdev format and selection rectangles) as well as control
> values as input from multiple sub-devices. I suppose this is the case with
> many sensor drivers --- I just know CCS best.
> 
> The current implementation uses a single mutex for all controls and
> subdevs.

For a single subdev, that could be done by setting
v4l2_ctrl_handler.lock to &v4l2_subdev.active_state->lock.

If we need to access the state of multiple subdevs I think we'll need to
switch to ww_mutex. That will require moving lock handling in the V4L2
core, I don't trust drivers to handle the ww_mutex lock failures and the
necessary rewind and retry correctly.

This will require quite a bit of work, which I think will need to be
done step by step.

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

On 22/12/2021 02:00, Laurent Pinchart wrote:

>>>>>>> +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
>>>>>>> +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
>>>>>>> +
>>>>>>> +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.
>>>>>>
>>>>>> Have you thought how this will interact with controls?
>>>>>>
>>>>>> Generally active state information exists for a device in struct
>>>>>> v4l2_subdev_state as well as the device's control handler as control
>>>>>> values. Controls have dependencies to other state information (active and
>>>>>> try).
>>>>>>
>>>>>> Until now, drivers have been responsible for serialising access to this
>>>>>> state on their own, mostly using a single mutex. Controls require a mutex
>>>>>> as well, but it's the same mutex independently of whether a driver is
>>>>>> dealing with active or try subdev state.
>>>>>>
>>>>>> In other words, if the above is assumed, when you're dealing with try state
>>>>>> that has dependencies to controls, you have to hold both that try state's
>>>>>> mutex as well as the control handler's mutex.
>>>>>
>>>>> Going forward, I think we should store the controls in the subdev state.
>>>>> That will require a uAPI extension to pass a `which` parameter to the
>>>>> control ioctls, and deprecated the control TRY ioctl on subdevs.
>>>>> Interactions between controls and pad formats will be easier to test, as
>>>>> applications will be able to set controls in the TRY state, interacting
>>>>> with the TRY formats. We will also need to rework the control handler
>>>>> operations to split .s_ctrl() in two, with one function to adjust a
>>>>> control value and one function to apply it.
>>>>>
>>>>> In the meantime, I think we'll need to acquire both locks, or possibly
>>>>> use the active state lock as the control handler lock.
>>>>
>>>> Note that also trying controls requires locking the control handler,
>>>> meaning that the control handler's mutex may not be the same as the active
>>>> state mutex (unless access also to try state is serialised using the same
>>>> mutex).
>>>>
>>>> What I'm saying is that to make this better usable with controls, changes
>>>> will be needed somewhere as the locking scheme is a poor match with that of
>>>> controls currently. Just saying the mutexes are acquired in a certain
>>>> order and pushing the problem to drivers is not a great solution.
>>>
>>> Could you maybe provide an example of existing subdev driver code that
>>> showcases this issue ? I'm not sure we really understand each other
>>> here.
>>
>> Whenever you're dealing with both controls and something in the state. Also
>> you've got a problem if the sensor driver does I²C writes to more than
>> 8-bit registers in 8-bit chunks and relies on hardware caching some values
>> before the entire register is updated.
>>
>> For instance, in the CCS driver, computing the PLL tree configuration
>> requires state (subdev format and selection rectangles) as well as control
>> values as input from multiple sub-devices. I suppose this is the case with
>> many sensor drivers --- I just know CCS best.
>>
>> The current implementation uses a single mutex for all controls and
>> subdevs.
> 
> For a single subdev, that could be done by setting
> v4l2_ctrl_handler.lock to &v4l2_subdev.active_state->lock.

That would only cover the active state, not the try state.

I made a test change by changing the subdev state to have 'struct mutex 
_lock' and 'struct mutex *lock', similar to the struct 
v4l2_ctrl_handler. The driver could then, in its init_cfg(), set the 
state->lock to either v4l2_ctrl_handler.lock or a lock in the driver's 
private data.

This kind of lock sharing makes me a bit uncomfortable (although the 
controls are already allowing this, and a driver private mutex can be 
set as the ctrl lock): if I call v4l2_subdev_lock_active_state(), I 
don't expect the controls to be locked too.

Then again, if I think about this as the subdev state really containing 
three parts: 1) the subdev active & try states, 2) controls, 3) driver 
private data, then it kind of makes sense. In the long run we could move 
towards combining these pieces together, and thus cleaning up the state 
management and locking.

  Tomi

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

[Laurent Pinchart]

Hi Tomi,

On Mon, Feb 07, 2022 at 11:19:23AM +0200, Tomi Valkeinen wrote:
> On 22/12/2021 02:00, Laurent Pinchart wrote:
> 
> >>>>>>> +:c:func:`v4l2_subdev_lock_state()`. The driver must then call
> >>>>>>> +:c:func:`v4l2_subdev_unlock_state()` to unlock the state when done.
> >>>>>>> +
> >>>>>>> +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.
> >>>>>>
> >>>>>> Have you thought how this will interact with controls?
> >>>>>>
> >>>>>> Generally active state information exists for a device in struct
> >>>>>> v4l2_subdev_state as well as the device's control handler as control
> >>>>>> values. Controls have dependencies to other state information (active and
> >>>>>> try).
> >>>>>>
> >>>>>> Until now, drivers have been responsible for serialising access to this
> >>>>>> state on their own, mostly using a single mutex. Controls require a mutex
> >>>>>> as well, but it's the same mutex independently of whether a driver is
> >>>>>> dealing with active or try subdev state.
> >>>>>>
> >>>>>> In other words, if the above is assumed, when you're dealing with try state
> >>>>>> that has dependencies to controls, you have to hold both that try state's
> >>>>>> mutex as well as the control handler's mutex.
> >>>>>
> >>>>> Going forward, I think we should store the controls in the subdev state.
> >>>>> That will require a uAPI extension to pass a `which` parameter to the
> >>>>> control ioctls, and deprecated the control TRY ioctl on subdevs.
> >>>>> Interactions between controls and pad formats will be easier to test, as
> >>>>> applications will be able to set controls in the TRY state, interacting
> >>>>> with the TRY formats. We will also need to rework the control handler
> >>>>> operations to split .s_ctrl() in two, with one function to adjust a
> >>>>> control value and one function to apply it.
> >>>>>
> >>>>> In the meantime, I think we'll need to acquire both locks, or possibly
> >>>>> use the active state lock as the control handler lock.
> >>>>
> >>>> Note that also trying controls requires locking the control handler,
> >>>> meaning that the control handler's mutex may not be the same as the active
> >>>> state mutex (unless access also to try state is serialised using the same
> >>>> mutex).
> >>>>
> >>>> What I'm saying is that to make this better usable with controls, changes
> >>>> will be needed somewhere as the locking scheme is a poor match with that of
> >>>> controls currently. Just saying the mutexes are acquired in a certain
> >>>> order and pushing the problem to drivers is not a great solution.
> >>>
> >>> Could you maybe provide an example of existing subdev driver code that
> >>> showcases this issue ? I'm not sure we really understand each other
> >>> here.
> >>
> >> Whenever you're dealing with both controls and something in the state. Also
> >> you've got a problem if the sensor driver does I²C writes to more than
> >> 8-bit registers in 8-bit chunks and relies on hardware caching some values
> >> before the entire register is updated.
> >>
> >> For instance, in the CCS driver, computing the PLL tree configuration
> >> requires state (subdev format and selection rectangles) as well as control
> >> values as input from multiple sub-devices. I suppose this is the case with
> >> many sensor drivers --- I just know CCS best.
> >>
> >> The current implementation uses a single mutex for all controls and
> >> subdevs.
> > 
> > For a single subdev, that could be done by setting
> > v4l2_ctrl_handler.lock to &v4l2_subdev.active_state->lock.
> 
> That would only cover the active state, not the try state.

That's right, but semantics of the try operation for formats and
controls are inherently different, so it's impossible to get it
completely right in any case. To really solve this, we need to move
control storage to the v4l2_subdev_state structure, and extend the
subdev control ioctls with a which field. That's out of scope for this
series.

> I made a test change by changing the subdev state to have 'struct mutex 
> _lock' and 'struct mutex *lock', similar to the struct 
> v4l2_ctrl_handler. The driver could then, in its init_cfg(), set the 
> state->lock to either v4l2_ctrl_handler.lock or a lock in the driver's 
> private data.
> 
> This kind of lock sharing makes me a bit uncomfortable (although the 
> controls are already allowing this, and a driver private mutex can be 
> set as the ctrl lock): if I call v4l2_subdev_lock_active_state(), I 
> don't expect the controls to be locked too.
> 
> Then again, if I think about this as the subdev state really containing 
> three parts: 1) the subdev active & try states, 2) controls, 3) driver 
> private data, then it kind of makes sense. In the long run we could move 
> towards combining these pieces together, and thus cleaning up the state 
> management and locking.

I'm fine with this approach as a temporary hack.

-- 
Regards,

Laurent Pinchart