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

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

[Tomi Valkeinen]

Hi,

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

- Doc improvements
- Allow state->lock to be set by the driver (similarly to v4l2_ctrl_handler)
- Rename fields in 'struct v4l2_subdev_pad_config' and drop the try_ prefix.
- Add v4l2_subdev_get_locked_active_state(), which calls lockdep_assert_locked() and returns the state.
- Changed v4l2_subdev_get_active_state() to call lockdep_assert_not_locked()

The idea with the v4l2_subdev_get_active_state /
v4l2_subdev_get_locked_active_state change is to have a lockdep_assert
called. Roughly I think there are two cases where the
v4l2_subdev_get_active_state could be called:

- With the intention of just passing it forward to another subdev, in
  which case the state must _not_ be locked. Here
  v4l2_subdev_get_active_state() can be called.

- With the intention of using the state in a case where the state is
  known to be already locked. Here v4l2_subdev_get_locked_active_state()
  can be called.

The state->lock change hopefully solves Sakari's concerns about the
locking between controls and state.

 Tomi

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

 .../driver-api/media/v4l2-subdev.rst          |  60 ++++++
 drivers/media/i2c/adv7183.c                   |   2 +-
 drivers/media/i2c/imx274.c                    |  12 +-
 drivers/media/i2c/mt9m001.c                   |   2 +-
 drivers/media/i2c/mt9m111.c                   |   2 +-
 drivers/media/i2c/mt9t112.c                   |   2 +-
 drivers/media/i2c/mt9v011.c                   |   2 +-
 drivers/media/i2c/mt9v111.c                   |   4 +-
 drivers/media/i2c/ov2640.c                    |   2 +-
 drivers/media/i2c/ov6650.c                    |  18 +-
 drivers/media/i2c/ov772x.c                    |   2 +-
 drivers/media/i2c/ov9640.c                    |   2 +-
 drivers/media/i2c/rj54n1cb0c.c                |   2 +-
 drivers/media/i2c/saa6752hs.c                 |   2 +-
 drivers/media/i2c/sr030pc30.c                 |   2 +-
 drivers/media/i2c/tw9910.c                    |   2 +-
 drivers/media/i2c/vs6624.c                    |   2 +-
 drivers/media/platform/atmel/atmel-isc-base.c |   8 +-
 drivers/media/platform/atmel/atmel-isi.c      |   8 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c   |   9 +-
 drivers/media/platform/vsp1/vsp1_entity.c     |  10 +-
 drivers/media/v4l2-core/v4l2-subdev.c         | 126 +++++++++--
 drivers/staging/media/tegra-video/vi.c        |  10 +-
 include/media/v4l2-subdev.h                   | 201 ++++++++++++++++--
 24 files changed, 415 insertions(+), 77 deletions(-)

-- 
2.25.1

[PATCH v3 1/7] media: subdev: rename subdev-state alloc & free

[Tomi Valkeinen]

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

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  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 d1f43f465c22..dc16406ff7dc 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 v3 2/7] media: subdev: add active state to struct v4l2_subdev

[Tomi Valkeinen]

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 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..09648adc97fe 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -915,6 +915,27 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
 }
 EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
 
+int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
+{
+	struct v4l2_subdev_state *state;
+
+	state = __v4l2_subdev_state_alloc(sd);
+	if (IS_ERR(state))
+		return PTR_ERR(state);
+
+	sd->active_state = state;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
+
+void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
+{
+	__v4l2_subdev_state_free(sd->active_state);
+	sd->active_state = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index e52bf508c75b..f481fafe8e4b 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;
 };
 
 
@@ -1150,6 +1169,45 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
  */
 void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
 
+/**
+ * v4l2_subdev_init_finalize() - Finalizes the initialization of the subdevice
+ * @sd: The subdev
+ *
+ * This function finalizes the initialization of the subdev, including
+ * allocation of the active state for the subdev.
+ *
+ * This function must be called by the subdev drivers that use the centralized
+ * active state, after the subdev struct has been initialized and
+ * media_entity_pads_init() has been called, but before registering the
+ * subdev.
+ *
+ * The user must call v4l2_subdev_cleanup() when the subdev is being removed.
+ */
+int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
+
+/**
+ * v4l2_subdev_cleanup() - Releases the resources allocated by the subdevice
+ * @sd: The subdevice
+ *
+ * This function will release the resources allocated in
+ * v4l2_subdev_init_finalize.
+ */
+void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
+
+/**
+ * v4l2_subdev_get_active_state() - Returns the active subdev state for
+ *				    subdevice
+ * @sd: The subdevice
+ *
+ * Returns the active state for the subdevice, or NULL if the subdev does not
+ * support active state.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
+{
+	return sd->active_state;
+}
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 /**
-- 
2.25.1

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

[Tomi Valkeinen]

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

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

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

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

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

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

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

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 09648adc97fe..32598d20fe35 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 v3 4/7] media: subdev: add subdev state locking

[Tomi Valkeinen]

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

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

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       | 39 ++++++++--
 drivers/staging/media/tegra-video/vi.c      |  8 +-
 include/media/v4l2-subdev.h                 | 81 ++++++++++++++++++++-
 5 files changed, 132 insertions(+), 11 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 32598d20fe35..a30e156d5303 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,9 @@ 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);
+	state->lock = &state->_lock;
+
 	if (sd->entity.num_pads) {
 		state->pads = kvmalloc_array(sd->entity.num_pads,
 					     sizeof(*state->pads),
@@ -954,16 +960,19 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
 	if (!state)
 		return;
 
+	mutex_destroy(&state->_lock);
+
 	kvfree(state->pads);
 	kfree(state);
 }
 EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
 
-int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
+int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
+				struct lock_class_key *key)
 {
 	struct v4l2_subdev_state *state;
 
-	state = __v4l2_subdev_state_alloc(sd);
+	state = __v4l2_subdev_state_alloc(sd, name, key);
 	if (IS_ERR(state))
 		return PTR_ERR(state);
 
@@ -971,7 +980,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)
 {
@@ -980,6 +989,26 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
 
+struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd)
+{
+	mutex_lock(sd->active_state->lock);
+
+	return sd->active_state;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_lock_active_state);
+
+void v4l2_subdev_lock_state(struct v4l2_subdev_state *state)
+{
+	mutex_lock(state->lock);
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_lock_state);
+
+void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
+{
+	mutex_unlock(state->lock);
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index dc16406ff7dc..8e184aa4c252 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -491,6 +491,7 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
 				      struct v4l2_pix_format *pix)
 {
 	const struct tegra_video_format *fmtinfo;
+	static struct lock_class_key key;
 	struct v4l2_subdev *subdev;
 	struct v4l2_subdev_format fmt;
 	struct v4l2_subdev_state *sd_state;
@@ -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 f481fafe8e4b..e9aa38dc6489 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -658,6 +658,8 @@ struct v4l2_subdev_pad_config {
 /**
  * struct v4l2_subdev_state - Used for storing subdev state information.
  *
+ * @_lock: default for 'lock'
+ * @lock: mutex for the state. May be replaced by the user.
  * @pads: &struct v4l2_subdev_pad_config array
  *
  * This structure only needs to be passed to the pad op if the 'which' field
@@ -665,6 +667,9 @@ struct v4l2_subdev_pad_config {
  * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
  */
 struct v4l2_subdev_state {
+	/* lock for the struct v4l2_subdev_state fields */
+	struct mutex _lock;
+	struct mutex *lock;
 	struct v4l2_subdev_pad_config *pads;
 };
 
@@ -1157,10 +1162,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
@@ -1183,7 +1192,16 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
  *
  * The user must call v4l2_subdev_cleanup() when the subdev is being removed.
  */
-int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
+#define v4l2_subdev_init_finalize(sd)                                          \
+	({                                                                     \
+		static struct lock_class_key __key;                            \
+		const char *name = KBUILD_BASENAME                             \
+			":" __stringify(__LINE__) ":sd->active_state->lock";   \
+		__v4l2_subdev_init_finalize(sd, name, &__key);                 \
+	})
+
+int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
+				struct lock_class_key *key);
 
 /**
  * v4l2_subdev_cleanup() - Releases the resources allocated by the subdevice
@@ -1200,14 +1218,71 @@ void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
  * @sd: The subdevice
  *
  * Returns the active state for the subdevice, or NULL if the subdev does not
- * support active state.
+ * support active state. If the state is not NULL, calls
+ * lockdep_assert_not_held() to issue a warning if the state is locked.
+ *
+ * This function is to be used e.g. when getting the active state for the sole
+ * purpose of passing it forward, without accessing the state fields.
  */
 static inline struct v4l2_subdev_state *
 v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
 {
+	if (sd->active_state)
+		lockdep_assert_not_held(sd->active_state->lock);
+	return sd->active_state;
+}
+
+/**
+ * v4l2_subdev_get_locked_active_state() - Checks that the active subdev state
+ *					   is locked and returns it
+ *
+ * @sd: The subdevice
+ *
+ * Returns the active state for the subdevice, or NULL if the subdev does not
+ * support active state. If the state is not NULL, calls lockdep_assert_held()
+ * to issue a warning if the state is not locked.
+ *
+ * This function is to be used when the caller knows that the active state is
+ * already locked.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_get_locked_active_state(struct v4l2_subdev *sd)
+{
+	if (sd->active_state)
+		lockdep_assert_held(sd->active_state->lock);
 	return sd->active_state;
 }
 
+/**
+ * v4l2_subdev_lock_active_state() - Locks and returns the active subdev state
+ *				     for the subdevice
+ * @sd: The subdevice
+ *
+ * Returns the locked active state for the subdevice, or NULL if the subdev
+ * does not support active state.
+ *
+ * The state must be unlocked with v4l2_subdev_unlock_state() after use.
+ */
+struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
+
+/**
+ * v4l2_subdev_lock_state() - Locks the subdev state
+ * @state: The subdevice state
+ *
+ * Locks the given subdev state.
+ *
+ * The state must be unlocked with v4l2_subdev_unlock_state() after use.
+ */
+void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
+
+/**
+ * v4l2_subdev_unlock_state() - Unlocks the subdev state
+ * @state: The subdevice state
+ *
+ * Unlocks the given subdev state.
+ */
+void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
+
 #endif /* CONFIG_MEDIA_CONTROLLER */
 
 /**
-- 
2.25.1

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

[Tomi Valkeinen]

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

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

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

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

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
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 e9aa38dc6489..16497b8fc875 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1283,6 +1283,42 @@ 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 /* CONFIG_MEDIA_CONTROLLER */
 
 /**
-- 
2.25.1

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

[Tomi Valkeinen]

Add documentation about centrally managed subdev state.

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

diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
index 08ea2673b19e..6b14cc97afa3 100644
--- a/Documentation/driver-api/media/v4l2-subdev.rst
+++ b/Documentation/driver-api/media/v4l2-subdev.rst
@@ -518,6 +518,66 @@ The :c:func:`v4l2_i2c_new_subdev` function will call
 :c:type:`i2c_board_info` structure using the ``client_type`` and the
 ``addr`` to fill it.
 
+Centrally managed subdev active state
+-------------------------------------
+
+Traditionally V4L2 subdev drivers maintained internal state for the active
+device configuration. This is often implemented as e.g. an array of struct
+v4l2_mbus_framefmt, one entry for each pad, and similarly for crop and compose
+rectangles.
+
+In addition to the active configuration, each subdev file handle has an array of
+struct v4l2_subdev_pad_config, managed by the V4L2 core, which contains the try
+configuration.
+
+To simplify the subdev drivers the V4L2 subdev API now optionally supports a
+centrally managed active configuration represented by
+:c:type:`v4l2_subdev_state`. One instance of state, which contains the active
+device configuration, is stored in the sub-device itself as part of
+the :c:type:`v4l2_subdev` structure, while the core associates a try state to
+each open file handle, to store the try configuration related to that file
+handle.
+
+Sub-device drivers can opt-in and use state to manage their active configuration
+by initializing the subdevice state with a call to v4l2_subdev_init_finalize()
+before registering the sub-device. They must also call v4l2_subdev_cleanup()
+to release all the allocated resources before unregistering the sub-device.
+The core automatically allocates and initializes a state for each open file
+handle to store the try configurations and frees it when closing the file
+handle.
+
+V4L2 sub-device operations that use both the :ref:`ACTIVE and TRY formats
+<v4l2-subdev-format-whence>` receive the correct state to operate on through the
+'state' parameter. The sub-device driver can access and modify the configuration
+ stored in the provided state after having locked the state 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 passes the correct try or active state to the
+subdevice operations, some existing device drivers pass a NULL state
+when calling operations with :c:func:`v4l2_subdev_call()`. This legacy
+construct causes issues with subdevice drivers that let the V4L2 core
+manage the active state, as they expect to receive the appropriate state
+as a parameter. To help the conversion of subdevice drivers to a managed
+active state without having to convert all callers at the same time, the
+:c:func:`v4l2_subdev_lock_and_return_state()` helper function can be
+used by subdevice drivers to retrieve the active state if a NULL state
+is passed to the subdevice operation.
+
+:c:func:`v4l2_subdev_lock_and_return_state()` should only be used when porting
+an existing driver to the new state management when it cannot be guaranteed
+that the current callers will pass the state properly. The function prints a
+notice when the passed state is NULL to encourage the porting of the callers
+to the new state management.
+
 V4L2 sub-device functions and data structures
 ---------------------------------------------
 
-- 
2.25.1

[PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[Tomi Valkeinen]

struct v4l2_subdev_pad_config used to be relevant only for
V4L2_SUBDEV_FORMAT_TRY configuration, and thus the fields of the struct
were named, e.g. try_fmt.

This struct is now used in struct v4l2_subdev_state, and thus can be
used in both try and active cases. Thus rename the fields and drop the
"try_" prefix.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/adv7183.c                   |  2 +-
 drivers/media/i2c/imx274.c                    | 12 ++++-----
 drivers/media/i2c/mt9m001.c                   |  2 +-
 drivers/media/i2c/mt9m111.c                   |  2 +-
 drivers/media/i2c/mt9t112.c                   |  2 +-
 drivers/media/i2c/mt9v011.c                   |  2 +-
 drivers/media/i2c/mt9v111.c                   |  4 +--
 drivers/media/i2c/ov2640.c                    |  2 +-
 drivers/media/i2c/ov6650.c                    | 18 ++++++-------
 drivers/media/i2c/ov772x.c                    |  2 +-
 drivers/media/i2c/ov9640.c                    |  2 +-
 drivers/media/i2c/rj54n1cb0c.c                |  2 +-
 drivers/media/i2c/saa6752hs.c                 |  2 +-
 drivers/media/i2c/sr030pc30.c                 |  2 +-
 drivers/media/i2c/tw9910.c                    |  2 +-
 drivers/media/i2c/vs6624.c                    |  2 +-
 drivers/media/platform/atmel/atmel-isc-base.c |  8 +++---
 drivers/media/platform/atmel/atmel-isi.c      |  8 +++---
 include/media/v4l2-subdev.h                   | 26 +++++++++----------
 19 files changed, 50 insertions(+), 52 deletions(-)

diff --git a/drivers/media/i2c/adv7183.c b/drivers/media/i2c/adv7183.c
index 92cafdea3f1f..9e590abf88e1 100644
--- a/drivers/media/i2c/adv7183.c
+++ b/drivers/media/i2c/adv7183.c
@@ -443,7 +443,7 @@ static int adv7183_set_fmt(struct v4l2_subdev *sd,
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		decoder->fmt = *fmt;
 	else
-		sd_state->pads->try_fmt = *fmt;
+		sd_state->pads->fmt = *fmt;
 	return 0;
 }
 
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2aa15b9c23cc..c94c24358931 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1020,8 +1020,8 @@ static int __imx274_change_compose(struct stimx274 *imx274,
 	int best_goodness = INT_MIN;
 
 	if (which == V4L2_SUBDEV_FORMAT_TRY) {
-		cur_crop = &sd_state->pads->try_crop;
-		tgt_fmt = &sd_state->pads->try_fmt;
+		cur_crop = &sd_state->pads->crop;
+		tgt_fmt = &sd_state->pads->fmt;
 	} else {
 		cur_crop = &imx274->crop;
 		tgt_fmt = &imx274->format;
@@ -1114,7 +1114,7 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 	 */
 	fmt->field = V4L2_FIELD_NONE;
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
-		sd_state->pads->try_fmt = *fmt;
+		sd_state->pads->fmt = *fmt;
 	else
 		imx274->format = *fmt;
 
@@ -1145,8 +1145,8 @@ static int imx274_get_selection(struct v4l2_subdev *sd,
 	}
 
 	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
-		src_crop = &sd_state->pads->try_crop;
-		src_fmt = &sd_state->pads->try_fmt;
+		src_crop = &sd_state->pads->crop;
+		src_fmt = &sd_state->pads->fmt;
 	} else {
 		src_crop = &imx274->crop;
 		src_fmt = &imx274->format;
@@ -1217,7 +1217,7 @@ static int imx274_set_selection_crop(struct stimx274 *imx274,
 	sel->r = new_crop;
 
 	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
-		tgt_crop = &sd_state->pads->try_crop;
+		tgt_crop = &sd_state->pads->crop;
 	else
 		tgt_crop = &imx274->crop;
 
diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index c9f0bd997ea7..4cf1334efdfe 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -411,7 +411,7 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return mt9m001_s_fmt(sd, fmt, mf);
-	sd_state->pads->try_fmt = *mf;
+	sd_state->pads->fmt = *mf;
 	return 0;
 }
 
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 91a44359bcd3..495731f11006 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -678,7 +678,7 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
 	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		sd_state->pads->try_fmt = *mf;
+		sd_state->pads->fmt = *mf;
 		return 0;
 	}
 
diff --git a/drivers/media/i2c/mt9t112.c b/drivers/media/i2c/mt9t112.c
index 8d2e3caa9b28..4457b030d1a8 100644
--- a/drivers/media/i2c/mt9t112.c
+++ b/drivers/media/i2c/mt9t112.c
@@ -982,7 +982,7 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return mt9t112_s_fmt(sd, mf);
-	sd_state->pads->try_fmt = *mf;
+	sd_state->pads->fmt = *mf;
 
 	return 0;
 }
diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
index 7699e64e1127..c02a51f2a327 100644
--- a/drivers/media/i2c/mt9v011.c
+++ b/drivers/media/i2c/mt9v011.c
@@ -358,7 +358,7 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
 
 		set_res(sd);
 	} else {
-		sd_state->pads->try_fmt = *fmt;
+		sd_state->pads->fmt = *fmt;
 	}
 
 	return 0;
diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
index 2dc4a0f24ce8..eed8e5a8dcdd 100644
--- a/drivers/media/i2c/mt9v111.c
+++ b/drivers/media/i2c/mt9v111.c
@@ -800,7 +800,7 @@ static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format(
 #if IS_ENABLED(CONFIG_VIDEO_V4L2_SUBDEV_API)
 		return v4l2_subdev_get_try_format(&mt9v111->sd, sd_state, pad);
 #else
-		return &sd_state->pads->try_fmt;
+		return &sd_state->pads->fmt;
 #endif
 	case V4L2_SUBDEV_FORMAT_ACTIVE:
 		return &mt9v111->fmt;
@@ -957,7 +957,7 @@ static int mt9v111_set_format(struct v4l2_subdev *subdev,
 static int mt9v111_init_cfg(struct v4l2_subdev *subdev,
 			    struct v4l2_subdev_state *sd_state)
 {
-	sd_state->pads->try_fmt = mt9v111_def_fmt;
+	sd_state->pads->fmt = mt9v111_def_fmt;
 
 	return 0;
 }
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 4b75da55b260..8c576fe1e203 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -996,7 +996,7 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
 		/* select format */
 		priv->cfmt_code = mf->code;
 	} else {
-		sd_state->pads->try_fmt = *mf;
+		sd_state->pads->fmt = *mf;
 	}
 out:
 	mutex_unlock(&priv->lock);
diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index f67412150b16..2ba4402c5e96 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -550,9 +550,9 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
 
 	/* update media bus format code and frame size */
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		mf->width = sd_state->pads->try_fmt.width;
-		mf->height = sd_state->pads->try_fmt.height;
-		mf->code = sd_state->pads->try_fmt.code;
+		mf->width = sd_state->pads->fmt.width;
+		mf->height = sd_state->pads->fmt.height;
+		mf->code = sd_state->pads->fmt.code;
 
 	} else {
 		mf->width = priv->rect.width >> priv->half_scale;
@@ -701,15 +701,15 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		/* store media bus format code and frame size in pad config */
-		sd_state->pads->try_fmt.width = mf->width;
-		sd_state->pads->try_fmt.height = mf->height;
-		sd_state->pads->try_fmt.code = mf->code;
+		sd_state->pads->fmt.width = mf->width;
+		sd_state->pads->fmt.height = mf->height;
+		sd_state->pads->fmt.code = mf->code;
 
 		/* return default mbus frame format updated with pad config */
 		*mf = ov6650_def_fmt;
-		mf->width = sd_state->pads->try_fmt.width;
-		mf->height = sd_state->pads->try_fmt.height;
-		mf->code = sd_state->pads->try_fmt.code;
+		mf->width = sd_state->pads->fmt.width;
+		mf->height = sd_state->pads->fmt.height;
+		mf->code = sd_state->pads->fmt.code;
 
 	} else {
 		/* apply new media bus format code and frame size */
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 78602a2f70b0..6ff5961b87b2 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1222,7 +1222,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 	mf->xfer_func = V4L2_XFER_FUNC_DEFAULT;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		sd_state->pads->try_fmt = *mf;
+		sd_state->pads->fmt = *mf;
 		return 0;
 	}
 
diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
index 0bab8c2cf160..f4b23183b8b1 100644
--- a/drivers/media/i2c/ov9640.c
+++ b/drivers/media/i2c/ov9640.c
@@ -547,7 +547,7 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return ov9640_s_fmt(sd, mf);
 
-	sd_state->pads->try_fmt = *mf;
+	sd_state->pads->fmt = *mf;
 
 	return 0;
 }
diff --git a/drivers/media/i2c/rj54n1cb0c.c b/drivers/media/i2c/rj54n1cb0c.c
index 2e4018c26912..867e4d7339f2 100644
--- a/drivers/media/i2c/rj54n1cb0c.c
+++ b/drivers/media/i2c/rj54n1cb0c.c
@@ -1009,7 +1009,7 @@ static int rj54n1_set_fmt(struct v4l2_subdev *sd,
 			      &mf->height, 84, RJ54N1_MAX_HEIGHT, align, 0);
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		sd_state->pads->try_fmt = *mf;
+		sd_state->pads->fmt = *mf;
 		return 0;
 	}
 
diff --git a/drivers/media/i2c/saa6752hs.c b/drivers/media/i2c/saa6752hs.c
index a7f043cad149..74c10fba2af0 100644
--- a/drivers/media/i2c/saa6752hs.c
+++ b/drivers/media/i2c/saa6752hs.c
@@ -595,7 +595,7 @@ static int saa6752hs_set_fmt(struct v4l2_subdev *sd,
 	f->colorspace = V4L2_COLORSPACE_SMPTE170M;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		sd_state->pads->try_fmt = *f;
+		sd_state->pads->fmt = *f;
 		return 0;
 	}
 
diff --git a/drivers/media/i2c/sr030pc30.c b/drivers/media/i2c/sr030pc30.c
index 19c0252df2f1..f0ccdf1f887d 100644
--- a/drivers/media/i2c/sr030pc30.c
+++ b/drivers/media/i2c/sr030pc30.c
@@ -541,7 +541,7 @@ static int sr030pc30_set_fmt(struct v4l2_subdev *sd,
 
 	fmt = try_fmt(sd, mf);
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		sd_state->pads->try_fmt = *mf;
+		sd_state->pads->fmt = *mf;
 		return 0;
 	}
 
diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
index 09f5b3986928..96f6cbc663f0 100644
--- a/drivers/media/i2c/tw9910.c
+++ b/drivers/media/i2c/tw9910.c
@@ -829,7 +829,7 @@ static int tw9910_set_fmt(struct v4l2_subdev *sd,
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return tw9910_s_fmt(sd, mf);
 
-	sd_state->pads->try_fmt = *mf;
+	sd_state->pads->fmt = *mf;
 
 	return 0;
 }
diff --git a/drivers/media/i2c/vs6624.c b/drivers/media/i2c/vs6624.c
index 29003dec6f2d..72a422693bc0 100644
--- a/drivers/media/i2c/vs6624.c
+++ b/drivers/media/i2c/vs6624.c
@@ -587,7 +587,7 @@ static int vs6624_set_fmt(struct v4l2_subdev *sd,
 	fmt->colorspace = vs6624_formats[index].colorspace;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		sd_state->pads->try_fmt = *fmt;
+		sd_state->pads->fmt = *fmt;
 		return 0;
 	}
 
diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index db15770d5b88..aa90aa55a128 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -835,11 +835,11 @@ static void isc_try_fse(struct isc_device *isc,
 	 * just use the maximum ISC can receive.
 	 */
 	if (ret) {
-		sd_state->pads->try_crop.width = isc->max_width;
-		sd_state->pads->try_crop.height = isc->max_height;
+		sd_state->pads->crop.width = isc->max_width;
+		sd_state->pads->crop.height = isc->max_height;
 	} else {
-		sd_state->pads->try_crop.width = fse.max_width;
-		sd_state->pads->try_crop.height = fse.max_height;
+		sd_state->pads->crop.width = fse.max_width;
+		sd_state->pads->crop.height = fse.max_height;
 	}
 }
 
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index 4d15814e4481..5806de277bdc 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -572,11 +572,11 @@ static void isi_try_fse(struct atmel_isi *isi, const struct isi_format *isi_fmt,
 	 * just use the maximum ISI can receive.
 	 */
 	if (ret) {
-		sd_state->pads->try_crop.width = MAX_SUPPORT_WIDTH;
-		sd_state->pads->try_crop.height = MAX_SUPPORT_HEIGHT;
+		sd_state->pads->crop.width = MAX_SUPPORT_WIDTH;
+		sd_state->pads->crop.height = MAX_SUPPORT_HEIGHT;
 	} else {
-		sd_state->pads->try_crop.width = fse.max_width;
-		sd_state->pads->try_crop.height = fse.max_height;
+		sd_state->pads->crop.width = fse.max_width;
+		sd_state->pads->crop.height = fse.max_height;
 	}
 }
 
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 16497b8fc875..e7c5617820ab 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -638,21 +638,19 @@ struct v4l2_subdev_ir_ops {
 /**
  * struct v4l2_subdev_pad_config - Used for storing subdev pad information.
  *
- * @try_fmt: &struct v4l2_mbus_framefmt
- * @try_crop: &struct v4l2_rect to be used for crop
- * @try_compose: &struct v4l2_rect to be used for compose
+ * @fmt: &struct v4l2_mbus_framefmt
+ * @crop: &struct v4l2_rect to be used for crop
+ * @compose: &struct v4l2_rect to be used for compose
  *
  * 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;
-	struct v4l2_rect try_crop;
-	struct v4l2_rect try_compose;
+	struct v4l2_mbus_framefmt fmt;
+	struct v4l2_rect crop;
+	struct v4l2_rect compose;
 };
 
 /**
@@ -1008,7 +1006,7 @@ struct v4l2_subdev_fh {
 
 /**
  * v4l2_subdev_get_try_format - ancillary routine to call
- *	&struct v4l2_subdev_pad_config->try_fmt
+ *	&struct v4l2_subdev_pad_config->fmt
  *
  * @sd: pointer to &struct v4l2_subdev
  * @state: pointer to &struct v4l2_subdev_state
@@ -1021,12 +1019,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
 {
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
-	return &state->pads[pad].try_fmt;
+	return &state->pads[pad].fmt;
 }
 
 /**
  * v4l2_subdev_get_try_crop - ancillary routine to call
- *	&struct v4l2_subdev_pad_config->try_crop
+ *	&struct v4l2_subdev_pad_config->crop
  *
  * @sd: pointer to &struct v4l2_subdev
  * @state: pointer to &struct v4l2_subdev_state.
@@ -1039,12 +1037,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
 {
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
-	return &state->pads[pad].try_crop;
+	return &state->pads[pad].crop;
 }
 
 /**
  * v4l2_subdev_get_try_compose - ancillary routine to call
- *	&struct v4l2_subdev_pad_config->try_compose
+ *	&struct v4l2_subdev_pad_config->compose
  *
  * @sd: pointer to &struct v4l2_subdev
  * @state: pointer to &struct v4l2_subdev_state.
@@ -1057,7 +1055,7 @@ v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
 {
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
-	return &state->pads[pad].try_compose;
+	return &state->pads[pad].compose;
 }
 
 #endif
-- 
2.25.1

Re: [PATCH v3 1/7] media: subdev: rename subdev-state alloc & free

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Feb 07, 2022 at 06:11:01PM +0200, Tomi Valkeinen wrote:
> 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 d1f43f465c22..dc16406ff7dc 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.

While at it, you could add a sentence to indicate that the function
should not be called by drivers. Same for __v4l2_subdev_state_free().

>   */
> -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 */
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/7] media: subdev: add active state to struct v4l2_subdev

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Feb 07, 2022 at 06:11:02PM +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>

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

> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++
>  include/media/v4l2-subdev.h           | 58 +++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index fe49c86a9b02..09648adc97fe 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -915,6 +915,27 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
>  }
>  EXPORT_SYMBOL_GPL(__v4l2_subdev_state_free);
>  
> +int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
> +{
> +	struct v4l2_subdev_state *state;
> +
> +	state = __v4l2_subdev_state_alloc(sd);
> +	if (IS_ERR(state))
> +		return PTR_ERR(state);
> +
> +	sd->active_state = state;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
> +
> +void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
> +{
> +	__v4l2_subdev_state_free(sd->active_state);
> +	sd->active_state = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index e52bf508c75b..f481fafe8e4b 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;
>  };
>  
>  
> @@ -1150,6 +1169,45 @@ struct v4l2_subdev_state *__v4l2_subdev_state_alloc(struct v4l2_subdev *sd);
>   */
>  void __v4l2_subdev_state_free(struct v4l2_subdev_state *state);
>  
> +/**
> + * v4l2_subdev_init_finalize() - Finalizes the initialization of the subdevice
> + * @sd: The subdev
> + *
> + * This function finalizes the initialization of the subdev, including
> + * allocation of the active state for the subdev.
> + *
> + * This function must be called by the subdev drivers that use the centralized
> + * active state, after the subdev struct has been initialized and
> + * media_entity_pads_init() has been called, but before registering the
> + * subdev.
> + *
> + * The user must call v4l2_subdev_cleanup() when the subdev is being removed.
> + */
> +int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
> +
> +/**
> + * v4l2_subdev_cleanup() - Releases the resources allocated by the subdevice
> + * @sd: The subdevice
> + *
> + * This function will release the resources allocated in
> + * v4l2_subdev_init_finalize.
> + */
> +void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
> +
> +/**
> + * v4l2_subdev_get_active_state() - Returns the active subdev state for
> + *				    subdevice
> + * @sd: The subdevice
> + *
> + * Returns the active state for the subdevice, or NULL if the subdev does not
> + * support active state.
> + */
> +static inline struct v4l2_subdev_state *
> +v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
> +{
> +	return sd->active_state;
> +}
> +
>  #endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/7] v4l: subdev active state

[Laurent Pinchart]

Hi Tomi,

On Mon, Feb 07, 2022 at 06:11:00PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> This is v3 of the subdev active state series. Changes since v2:
> 
> - Doc improvements
> - Allow state->lock to be set by the driver (similarly to v4l2_ctrl_handler)

While I think we need better in the longer term, this seems like a
reasonable compromise to land this series and continue building on top.

> - Rename fields in 'struct v4l2_subdev_pad_config' and drop the try_ prefix.
> - Add v4l2_subdev_get_locked_active_state(), which calls lockdep_assert_locked() and returns the state.
> - Changed v4l2_subdev_get_active_state() to call lockdep_assert_not_locked()
> 
> The idea with the v4l2_subdev_get_active_state /
> v4l2_subdev_get_locked_active_state change is to have a lockdep_assert
> called. Roughly I think there are two cases where the
> v4l2_subdev_get_active_state could be called:
> 
> - With the intention of just passing it forward to another subdev, in
>   which case the state must _not_ be locked. Here
>   v4l2_subdev_get_active_state() can be called.
> 
> - With the intention of using the state in a case where the state is
>   known to be already locked. Here v4l2_subdev_get_locked_active_state()
>   can be called.

I'm not sure how this will work out, but it seems fine to me to start
with.

> The state->lock change hopefully solves Sakari's concerns about the
> locking between controls and state.
> 
>  Tomi
> 
> Tomi Valkeinen (7):
>   media: subdev: rename subdev-state alloc & free
>   media: subdev: add active state to struct v4l2_subdev
>   media: subdev: pass also the active state to subdevs from ioctls
>   media: subdev: add subdev state locking
>   media: subdev: Add v4l2_subdev_lock_and_return_state()
>   media: Documentation: add documentation about subdev state
>   media: subdev: rename v4l2_subdev_pad_config.try_* fields
> 
>  .../driver-api/media/v4l2-subdev.rst          |  60 ++++++
>  drivers/media/i2c/adv7183.c                   |   2 +-
>  drivers/media/i2c/imx274.c                    |  12 +-
>  drivers/media/i2c/mt9m001.c                   |   2 +-
>  drivers/media/i2c/mt9m111.c                   |   2 +-
>  drivers/media/i2c/mt9t112.c                   |   2 +-
>  drivers/media/i2c/mt9v011.c                   |   2 +-
>  drivers/media/i2c/mt9v111.c                   |   4 +-
>  drivers/media/i2c/ov2640.c                    |   2 +-
>  drivers/media/i2c/ov6650.c                    |  18 +-
>  drivers/media/i2c/ov772x.c                    |   2 +-
>  drivers/media/i2c/ov9640.c                    |   2 +-
>  drivers/media/i2c/rj54n1cb0c.c                |   2 +-
>  drivers/media/i2c/saa6752hs.c                 |   2 +-
>  drivers/media/i2c/sr030pc30.c                 |   2 +-
>  drivers/media/i2c/tw9910.c                    |   2 +-
>  drivers/media/i2c/vs6624.c                    |   2 +-
>  drivers/media/platform/atmel/atmel-isc-base.c |   8 +-
>  drivers/media/platform/atmel/atmel-isi.c      |   8 +-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c   |   9 +-
>  drivers/media/platform/vsp1/vsp1_entity.c     |  10 +-
>  drivers/media/v4l2-core/v4l2-subdev.c         | 126 +++++++++--
>  drivers/staging/media/tegra-video/vi.c        |  10 +-
>  include/media/v4l2-subdev.h                   | 201 ++++++++++++++++--
>  24 files changed, 415 insertions(+), 77 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Feb 07, 2022 at 06:11:07PM +0200, Tomi Valkeinen wrote:
> struct v4l2_subdev_pad_config used to be relevant only for
> V4L2_SUBDEV_FORMAT_TRY configuration, and thus the fields of the struct
> were named, e.g. try_fmt.
> 
> This struct is now used in struct v4l2_subdev_state, and thus can be
> used in both try and active cases. Thus rename the fields and drop the
> "try_" prefix.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/i2c/adv7183.c                   |  2 +-
>  drivers/media/i2c/imx274.c                    | 12 ++++-----
>  drivers/media/i2c/mt9m001.c                   |  2 +-
>  drivers/media/i2c/mt9m111.c                   |  2 +-
>  drivers/media/i2c/mt9t112.c                   |  2 +-
>  drivers/media/i2c/mt9v011.c                   |  2 +-
>  drivers/media/i2c/mt9v111.c                   |  4 +--
>  drivers/media/i2c/ov2640.c                    |  2 +-
>  drivers/media/i2c/ov6650.c                    | 18 ++++++-------
>  drivers/media/i2c/ov772x.c                    |  2 +-
>  drivers/media/i2c/ov9640.c                    |  2 +-
>  drivers/media/i2c/rj54n1cb0c.c                |  2 +-
>  drivers/media/i2c/saa6752hs.c                 |  2 +-
>  drivers/media/i2c/sr030pc30.c                 |  2 +-
>  drivers/media/i2c/tw9910.c                    |  2 +-
>  drivers/media/i2c/vs6624.c                    |  2 +-
>  drivers/media/platform/atmel/atmel-isc-base.c |  8 +++---
>  drivers/media/platform/atmel/atmel-isi.c      |  8 +++---

Would it be possible to move those drivers to use the
v4l2_subdev_get_try_*() helpers instead of accessing the fields directly
? It doesn't necessarily need to be done as part of this series, but
after removing the try_ prefix from the fields it will be more difficult
to identify the offending drivers.

In a separate patch (not part of this series) we should also rename the
v4l2_subdev_get_try_*() functions to v4l2_subdev_state_get_*().

>  include/media/v4l2-subdev.h                   | 26 +++++++++----------
>  19 files changed, 50 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7183.c b/drivers/media/i2c/adv7183.c
> index 92cafdea3f1f..9e590abf88e1 100644
> --- a/drivers/media/i2c/adv7183.c
> +++ b/drivers/media/i2c/adv7183.c
> @@ -443,7 +443,7 @@ static int adv7183_set_fmt(struct v4l2_subdev *sd,
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		decoder->fmt = *fmt;
>  	else
> -		sd_state->pads->try_fmt = *fmt;
> +		sd_state->pads->fmt = *fmt;
>  	return 0;
>  }
>  
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 2aa15b9c23cc..c94c24358931 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -1020,8 +1020,8 @@ static int __imx274_change_compose(struct stimx274 *imx274,
>  	int best_goodness = INT_MIN;
>  
>  	if (which == V4L2_SUBDEV_FORMAT_TRY) {
> -		cur_crop = &sd_state->pads->try_crop;
> -		tgt_fmt = &sd_state->pads->try_fmt;
> +		cur_crop = &sd_state->pads->crop;
> +		tgt_fmt = &sd_state->pads->fmt;
>  	} else {
>  		cur_crop = &imx274->crop;
>  		tgt_fmt = &imx274->format;
> @@ -1114,7 +1114,7 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>  	 */
>  	fmt->field = V4L2_FIELD_NONE;
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> -		sd_state->pads->try_fmt = *fmt;
> +		sd_state->pads->fmt = *fmt;
>  	else
>  		imx274->format = *fmt;
>  
> @@ -1145,8 +1145,8 @@ static int imx274_get_selection(struct v4l2_subdev *sd,
>  	}
>  
>  	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		src_crop = &sd_state->pads->try_crop;
> -		src_fmt = &sd_state->pads->try_fmt;
> +		src_crop = &sd_state->pads->crop;
> +		src_fmt = &sd_state->pads->fmt;
>  	} else {
>  		src_crop = &imx274->crop;
>  		src_fmt = &imx274->format;
> @@ -1217,7 +1217,7 @@ static int imx274_set_selection_crop(struct stimx274 *imx274,
>  	sel->r = new_crop;
>  
>  	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> -		tgt_crop = &sd_state->pads->try_crop;
> +		tgt_crop = &sd_state->pads->crop;
>  	else
>  		tgt_crop = &imx274->crop;
>  
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index c9f0bd997ea7..4cf1334efdfe 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -411,7 +411,7 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return mt9m001_s_fmt(sd, fmt, mf);
> -	sd_state->pads->try_fmt = *mf;
> +	sd_state->pads->fmt = *mf;
>  	return 0;
>  }
>  
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 91a44359bcd3..495731f11006 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -678,7 +678,7 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>  	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/mt9t112.c b/drivers/media/i2c/mt9t112.c
> index 8d2e3caa9b28..4457b030d1a8 100644
> --- a/drivers/media/i2c/mt9t112.c
> +++ b/drivers/media/i2c/mt9t112.c
> @@ -982,7 +982,7 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return mt9t112_s_fmt(sd, mf);
> -	sd_state->pads->try_fmt = *mf;
> +	sd_state->pads->fmt = *mf;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
> index 7699e64e1127..c02a51f2a327 100644
> --- a/drivers/media/i2c/mt9v011.c
> +++ b/drivers/media/i2c/mt9v011.c
> @@ -358,7 +358,7 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
>  
>  		set_res(sd);
>  	} else {
> -		sd_state->pads->try_fmt = *fmt;
> +		sd_state->pads->fmt = *fmt;
>  	}
>  
>  	return 0;
> diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
> index 2dc4a0f24ce8..eed8e5a8dcdd 100644
> --- a/drivers/media/i2c/mt9v111.c
> +++ b/drivers/media/i2c/mt9v111.c
> @@ -800,7 +800,7 @@ static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format(
>  #if IS_ENABLED(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  		return v4l2_subdev_get_try_format(&mt9v111->sd, sd_state, pad);
>  #else
> -		return &sd_state->pads->try_fmt;
> +		return &sd_state->pads->fmt;
>  #endif
>  	case V4L2_SUBDEV_FORMAT_ACTIVE:
>  		return &mt9v111->fmt;
> @@ -957,7 +957,7 @@ static int mt9v111_set_format(struct v4l2_subdev *subdev,
>  static int mt9v111_init_cfg(struct v4l2_subdev *subdev,
>  			    struct v4l2_subdev_state *sd_state)
>  {
> -	sd_state->pads->try_fmt = mt9v111_def_fmt;
> +	sd_state->pads->fmt = mt9v111_def_fmt;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 4b75da55b260..8c576fe1e203 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -996,7 +996,7 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>  		/* select format */
>  		priv->cfmt_code = mf->code;
>  	} else {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  	}
>  out:
>  	mutex_unlock(&priv->lock);
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index f67412150b16..2ba4402c5e96 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -550,9 +550,9 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
>  
>  	/* update media bus format code and frame size */
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		mf->width = sd_state->pads->try_fmt.width;
> -		mf->height = sd_state->pads->try_fmt.height;
> -		mf->code = sd_state->pads->try_fmt.code;
> +		mf->width = sd_state->pads->fmt.width;
> +		mf->height = sd_state->pads->fmt.height;
> +		mf->code = sd_state->pads->fmt.code;
>  
>  	} else {
>  		mf->width = priv->rect.width >> priv->half_scale;
> @@ -701,15 +701,15 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>  		/* store media bus format code and frame size in pad config */
> -		sd_state->pads->try_fmt.width = mf->width;
> -		sd_state->pads->try_fmt.height = mf->height;
> -		sd_state->pads->try_fmt.code = mf->code;
> +		sd_state->pads->fmt.width = mf->width;
> +		sd_state->pads->fmt.height = mf->height;
> +		sd_state->pads->fmt.code = mf->code;
>  
>  		/* return default mbus frame format updated with pad config */
>  		*mf = ov6650_def_fmt;
> -		mf->width = sd_state->pads->try_fmt.width;
> -		mf->height = sd_state->pads->try_fmt.height;
> -		mf->code = sd_state->pads->try_fmt.code;
> +		mf->width = sd_state->pads->fmt.width;
> +		mf->height = sd_state->pads->fmt.height;
> +		mf->code = sd_state->pads->fmt.code;
>  
>  	} else {
>  		/* apply new media bus format code and frame size */
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 78602a2f70b0..6ff5961b87b2 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1222,7 +1222,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  	mf->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
> index 0bab8c2cf160..f4b23183b8b1 100644
> --- a/drivers/media/i2c/ov9640.c
> +++ b/drivers/media/i2c/ov9640.c
> @@ -547,7 +547,7 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return ov9640_s_fmt(sd, mf);
>  
> -	sd_state->pads->try_fmt = *mf;
> +	sd_state->pads->fmt = *mf;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/rj54n1cb0c.c b/drivers/media/i2c/rj54n1cb0c.c
> index 2e4018c26912..867e4d7339f2 100644
> --- a/drivers/media/i2c/rj54n1cb0c.c
> +++ b/drivers/media/i2c/rj54n1cb0c.c
> @@ -1009,7 +1009,7 @@ static int rj54n1_set_fmt(struct v4l2_subdev *sd,
>  			      &mf->height, 84, RJ54N1_MAX_HEIGHT, align, 0);
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/saa6752hs.c b/drivers/media/i2c/saa6752hs.c
> index a7f043cad149..74c10fba2af0 100644
> --- a/drivers/media/i2c/saa6752hs.c
> +++ b/drivers/media/i2c/saa6752hs.c
> @@ -595,7 +595,7 @@ static int saa6752hs_set_fmt(struct v4l2_subdev *sd,
>  	f->colorspace = V4L2_COLORSPACE_SMPTE170M;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *f;
> +		sd_state->pads->fmt = *f;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/sr030pc30.c b/drivers/media/i2c/sr030pc30.c
> index 19c0252df2f1..f0ccdf1f887d 100644
> --- a/drivers/media/i2c/sr030pc30.c
> +++ b/drivers/media/i2c/sr030pc30.c
> @@ -541,7 +541,7 @@ static int sr030pc30_set_fmt(struct v4l2_subdev *sd,
>  
>  	fmt = try_fmt(sd, mf);
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
> index 09f5b3986928..96f6cbc663f0 100644
> --- a/drivers/media/i2c/tw9910.c
> +++ b/drivers/media/i2c/tw9910.c
> @@ -829,7 +829,7 @@ static int tw9910_set_fmt(struct v4l2_subdev *sd,
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return tw9910_s_fmt(sd, mf);
>  
> -	sd_state->pads->try_fmt = *mf;
> +	sd_state->pads->fmt = *mf;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/vs6624.c b/drivers/media/i2c/vs6624.c
> index 29003dec6f2d..72a422693bc0 100644
> --- a/drivers/media/i2c/vs6624.c
> +++ b/drivers/media/i2c/vs6624.c
> @@ -587,7 +587,7 @@ static int vs6624_set_fmt(struct v4l2_subdev *sd,
>  	fmt->colorspace = vs6624_formats[index].colorspace;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *fmt;
> +		sd_state->pads->fmt = *fmt;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index db15770d5b88..aa90aa55a128 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -835,11 +835,11 @@ static void isc_try_fse(struct isc_device *isc,
>  	 * just use the maximum ISC can receive.
>  	 */
>  	if (ret) {
> -		sd_state->pads->try_crop.width = isc->max_width;
> -		sd_state->pads->try_crop.height = isc->max_height;
> +		sd_state->pads->crop.width = isc->max_width;
> +		sd_state->pads->crop.height = isc->max_height;
>  	} else {
> -		sd_state->pads->try_crop.width = fse.max_width;
> -		sd_state->pads->try_crop.height = fse.max_height;
> +		sd_state->pads->crop.width = fse.max_width;
> +		sd_state->pads->crop.height = fse.max_height;
>  	}
>  }
>  
> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> index 4d15814e4481..5806de277bdc 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -572,11 +572,11 @@ static void isi_try_fse(struct atmel_isi *isi, const struct isi_format *isi_fmt,
>  	 * just use the maximum ISI can receive.
>  	 */
>  	if (ret) {
> -		sd_state->pads->try_crop.width = MAX_SUPPORT_WIDTH;
> -		sd_state->pads->try_crop.height = MAX_SUPPORT_HEIGHT;
> +		sd_state->pads->crop.width = MAX_SUPPORT_WIDTH;
> +		sd_state->pads->crop.height = MAX_SUPPORT_HEIGHT;
>  	} else {
> -		sd_state->pads->try_crop.width = fse.max_width;
> -		sd_state->pads->try_crop.height = fse.max_height;
> +		sd_state->pads->crop.width = fse.max_width;
> +		sd_state->pads->crop.height = fse.max_height;
>  	}
>  }
>  
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 16497b8fc875..e7c5617820ab 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -638,21 +638,19 @@ struct v4l2_subdev_ir_ops {
>  /**
>   * struct v4l2_subdev_pad_config - Used for storing subdev pad information.
>   *
> - * @try_fmt: &struct v4l2_mbus_framefmt
> - * @try_crop: &struct v4l2_rect to be used for crop
> - * @try_compose: &struct v4l2_rect to be used for compose
> + * @fmt: &struct v4l2_mbus_framefmt
> + * @crop: &struct v4l2_rect to be used for crop
> + * @compose: &struct v4l2_rect to be used for compose
>   *
>   * 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.

You could drop this paragraph while at it, v4l2_subdev_pad_config isn't
passed to subdev operations anymore.

>   *
> - * 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;
> -	struct v4l2_rect try_crop;
> -	struct v4l2_rect try_compose;
> +	struct v4l2_mbus_framefmt fmt;
> +	struct v4l2_rect crop;
> +	struct v4l2_rect compose;
>  };
>  
>  /**
> @@ -1008,7 +1006,7 @@ struct v4l2_subdev_fh {
>  
>  /**
>   * v4l2_subdev_get_try_format - ancillary routine to call
> - *	&struct v4l2_subdev_pad_config->try_fmt
> + *	&struct v4l2_subdev_pad_config->fmt
>   *
>   * @sd: pointer to &struct v4l2_subdev
>   * @state: pointer to &struct v4l2_subdev_state
> @@ -1021,12 +1019,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
>  {
>  	if (WARN_ON(pad >= sd->entity.num_pads))
>  		pad = 0;
> -	return &state->pads[pad].try_fmt;
> +	return &state->pads[pad].fmt;
>  }
>  
>  /**
>   * v4l2_subdev_get_try_crop - ancillary routine to call
> - *	&struct v4l2_subdev_pad_config->try_crop
> + *	&struct v4l2_subdev_pad_config->crop
>   *
>   * @sd: pointer to &struct v4l2_subdev
>   * @state: pointer to &struct v4l2_subdev_state.
> @@ -1039,12 +1037,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
>  {
>  	if (WARN_ON(pad >= sd->entity.num_pads))
>  		pad = 0;
> -	return &state->pads[pad].try_crop;
> +	return &state->pads[pad].crop;
>  }
>  
>  /**
>   * v4l2_subdev_get_try_compose - ancillary routine to call
> - *	&struct v4l2_subdev_pad_config->try_compose
> + *	&struct v4l2_subdev_pad_config->compose
>   *
>   * @sd: pointer to &struct v4l2_subdev
>   * @state: pointer to &struct v4l2_subdev_state.
> @@ -1057,7 +1055,7 @@ v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
>  {
>  	if (WARN_ON(pad >= sd->entity.num_pads))
>  		pad = 0;
> -	return &state->pads[pad].try_compose;
> +	return &state->pads[pad].compose;
>  }
>  
>  #endif

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Feb 07, 2022 at 06:11:06PM +0200, Tomi Valkeinen wrote:
> Add documentation about centrally managed subdev state.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

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

> ---
>  .../driver-api/media/v4l2-subdev.rst          | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
> index 08ea2673b19e..6b14cc97afa3 100644
> --- a/Documentation/driver-api/media/v4l2-subdev.rst
> +++ b/Documentation/driver-api/media/v4l2-subdev.rst
> @@ -518,6 +518,66 @@ The :c:func:`v4l2_i2c_new_subdev` function will call
>  :c:type:`i2c_board_info` structure using the ``client_type`` and the
>  ``addr`` to fill it.
>  
> +Centrally managed subdev active state
> +-------------------------------------
> +
> +Traditionally V4L2 subdev drivers maintained internal state for the active
> +device configuration. This is often implemented as e.g. an array of struct
> +v4l2_mbus_framefmt, one entry for each pad, and similarly for crop and compose
> +rectangles.
> +
> +In addition to the active configuration, each subdev file handle has an array of
> +struct v4l2_subdev_pad_config, managed by the V4L2 core, which contains the try
> +configuration.
> +
> +To simplify the subdev drivers the V4L2 subdev API now optionally supports a
> +centrally managed active configuration represented by
> +:c:type:`v4l2_subdev_state`. One instance of state, which contains the active
> +device configuration, is stored in the sub-device itself as part of
> +the :c:type:`v4l2_subdev` structure, while the core associates a try state to
> +each open file handle, to store the try configuration related to that file
> +handle.
> +
> +Sub-device drivers can opt-in and use state to manage their active configuration
> +by initializing the subdevice state with a call to v4l2_subdev_init_finalize()
> +before registering the sub-device. They must also call v4l2_subdev_cleanup()
> +to release all the allocated resources before unregistering the sub-device.
> +The core automatically allocates and initializes a state for each open file
> +handle to store the try configurations and frees it when closing the file
> +handle.
> +
> +V4L2 sub-device operations that use both the :ref:`ACTIVE and TRY formats
> +<v4l2-subdev-format-whence>` receive the correct state to operate on through the
> +'state' parameter. The sub-device driver can access and modify the configuration
> + stored in the provided state after having locked the state 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 passes the correct try or active state to the
> +subdevice operations, some existing device drivers pass a NULL state
> +when calling operations with :c:func:`v4l2_subdev_call()`. This legacy
> +construct causes issues with subdevice drivers that let the V4L2 core
> +manage the active state, as they expect to receive the appropriate state
> +as a parameter. To help the conversion of subdevice drivers to a managed
> +active state without having to convert all callers at the same time, the
> +:c:func:`v4l2_subdev_lock_and_return_state()` helper function can be
> +used by subdevice drivers to retrieve the active state if a NULL state
> +is passed to the subdevice operation.
> +
> +:c:func:`v4l2_subdev_lock_and_return_state()` should only be used when porting
> +an existing driver to the new state management when it cannot be guaranteed
> +that the current callers will pass the state properly. The function prints a
> +notice when the passed state is NULL to encourage the porting of the callers
> +to the new state management.
> +
>  V4L2 sub-device functions and data structures
>  ---------------------------------------------
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

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.17-rc3]
[cannot apply to next-20220207]
[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/20220208-002350
base:   git://linuxtv.org/media_tree.git master
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20220208/202202081223.gzH6LYK9-lkp@intel.com/config)
compiler: arm-linux-gnueabi-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/8283dea08111c6a813e9340d735c158df3fcbe5f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tomi-Valkeinen/v4l-subdev-active-state/20220208-002350
        git checkout 8283dea08111c6a813e9340d735c158df3fcbe5f
        # 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=arm SHELL=/bin/bash drivers/staging/

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/staging/media/tegra-video/vi.c: In function '__tegra_channel_try_format':
>> drivers/staging/media/tegra-video/vi.c:544:39: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     544 |                         sd_state->pads->try_crop.width = 0;
         |                                       ^~
   drivers/staging/media/tegra-video/vi.c:545:39: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     545 |                         sd_state->pads->try_crop.height = 0;
         |                                       ^~
   drivers/staging/media/tegra-video/vi.c:552:39: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     552 |                         sd_state->pads->try_crop.width = sdsel.r.width;
         |                                       ^~
   drivers/staging/media/tegra-video/vi.c:553:39: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     553 |                         sd_state->pads->try_crop.height = sdsel.r.height;
         |                                       ^~
   drivers/staging/media/tegra-video/vi.c:556:31: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     556 |                 sd_state->pads->try_crop.width = fse.max_width;
         |                               ^~
   drivers/staging/media/tegra-video/vi.c:557:31: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     557 |                 sd_state->pads->try_crop.height = fse.max_height;
         |                               ^~


vim +544 drivers/staging/media/tegra-video/vi.c

3d8a97eabef088 Sowjanya Komatineni 2020-05-04  489  
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  490  static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  491  				      struct v4l2_pix_format *pix)
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  492  {
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  493  	const struct tegra_video_format *fmtinfo;
d156949c999153 Tomi Valkeinen      2022-02-07  494  	static struct lock_class_key key;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  495  	struct v4l2_subdev *subdev;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  496  	struct v4l2_subdev_format fmt;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  497  	struct v4l2_subdev_state *sd_state;
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  498  	struct v4l2_subdev_frame_size_enum fse = {
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  499  		.which = V4L2_SUBDEV_FORMAT_TRY,
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  500  	};
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  501  	struct v4l2_subdev_selection sdsel = {
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  502  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  503  		.target = V4L2_SEL_TGT_CROP_BOUNDS,
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  504  	};
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  505  	int ret;
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  506  
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  507  	subdev = tegra_channel_get_remote_source_subdev(chan);
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  508  	if (!subdev)
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  509  		return -ENODEV;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  510  
d156949c999153 Tomi Valkeinen      2022-02-07  511  	/*
d156949c999153 Tomi Valkeinen      2022-02-07  512  	 * FIXME: Drop this call, drivers are not supposed to use
d156949c999153 Tomi Valkeinen      2022-02-07  513  	 * __v4l2_subdev_state_alloc().
d156949c999153 Tomi Valkeinen      2022-02-07  514  	 */
d156949c999153 Tomi Valkeinen      2022-02-07  515  	sd_state = __v4l2_subdev_state_alloc(subdev, "tegra:state->lock",
d156949c999153 Tomi Valkeinen      2022-02-07  516  					     &key);
ba7a93e507f883 Dan Carpenter       2021-06-22  517  	if (IS_ERR(sd_state))
ba7a93e507f883 Dan Carpenter       2021-06-22  518  		return PTR_ERR(sd_state);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  519  	/*
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  520  	 * Retrieve the format information and if requested format isn't
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  521  	 * supported, keep the current format.
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  522  	 */
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  523  	fmtinfo = tegra_get_format_by_fourcc(chan->vi, pix->pixelformat);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  524  	if (!fmtinfo) {
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  525  		pix->pixelformat = chan->format.pixelformat;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  526  		pix->colorspace = chan->format.colorspace;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  527  		fmtinfo = tegra_get_format_by_fourcc(chan->vi,
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  528  						     pix->pixelformat);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  529  	}
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  530  
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  531  	pix->field = V4L2_FIELD_NONE;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  532  	fmt.which = V4L2_SUBDEV_FORMAT_TRY;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  533  	fmt.pad = 0;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  534  	v4l2_fill_mbus_format(&fmt.format, pix, fmtinfo->code);
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  535  
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  536  	/*
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  537  	 * Attempt to obtain the format size from subdev.
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  538  	 * If not available, try to get crop boundary from subdev.
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  539  	 */
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  540  	fse.code = fmtinfo->code;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  541  	ret = v4l2_subdev_call(subdev, pad, enum_frame_size, sd_state, &fse);
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  542  	if (ret) {
56f64b82356b74 Sowjanya Komatineni 2020-12-11  543  		if (!v4l2_subdev_has_op(subdev, pad, get_selection)) {
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10 @544  			sd_state->pads->try_crop.width = 0;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  545  			sd_state->pads->try_crop.height = 0;
56f64b82356b74 Sowjanya Komatineni 2020-12-11  546  		} else {
56f64b82356b74 Sowjanya Komatineni 2020-12-11  547  			ret = v4l2_subdev_call(subdev, pad, get_selection,
56f64b82356b74 Sowjanya Komatineni 2020-12-11  548  					       NULL, &sdsel);
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  549  			if (ret)
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  550  				return -EINVAL;
56f64b82356b74 Sowjanya Komatineni 2020-12-11  551  
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  552  			sd_state->pads->try_crop.width = sdsel.r.width;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  553  			sd_state->pads->try_crop.height = sdsel.r.height;
56f64b82356b74 Sowjanya Komatineni 2020-12-11  554  		}
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  555  	} else {
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  556  		sd_state->pads->try_crop.width = fse.max_width;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  557  		sd_state->pads->try_crop.height = fse.max_height;
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  558  	}
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  559  
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  560  	ret = v4l2_subdev_call(subdev, pad, set_fmt, sd_state, &fmt);
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  561  	if (ret < 0)
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  562  		return ret;
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  563  
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  564  	v4l2_fill_pix_format(pix, &fmt.format);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  565  	tegra_channel_fmt_align(chan, pix, fmtinfo->bpp);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  566  
a702cbb5ddd2f7 Tomi Valkeinen      2022-02-07  567  	__v4l2_subdev_state_free(sd_state);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  568  
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  569  	return 0;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  570  }
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  571  

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

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10010 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.17-rc3]
[cannot apply to next-20220207]
[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/20220208-002350
base:   git://linuxtv.org/media_tree.git master
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20220208/202202081223.gzH6LYK9-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-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/8283dea08111c6a813e9340d735c158df3fcbe5f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tomi-Valkeinen/v4l-subdev-active-state/20220208-002350
        git checkout 8283dea08111c6a813e9340d735c158df3fcbe5f
        # 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=arm SHELL=/bin/bash drivers/staging/

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/staging/media/tegra-video/vi.c: In function '__tegra_channel_try_format':
>> drivers/staging/media/tegra-video/vi.c:544:39: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     544 |                         sd_state->pads->try_crop.width = 0;
         |                                       ^~
   drivers/staging/media/tegra-video/vi.c:545:39: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     545 |                         sd_state->pads->try_crop.height = 0;
         |                                       ^~
   drivers/staging/media/tegra-video/vi.c:552:39: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     552 |                         sd_state->pads->try_crop.width = sdsel.r.width;
         |                                       ^~
   drivers/staging/media/tegra-video/vi.c:553:39: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     553 |                         sd_state->pads->try_crop.height = sdsel.r.height;
         |                                       ^~
   drivers/staging/media/tegra-video/vi.c:556:31: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     556 |                 sd_state->pads->try_crop.width = fse.max_width;
         |                               ^~
   drivers/staging/media/tegra-video/vi.c:557:31: error: 'struct v4l2_subdev_pad_config' has no member named 'try_crop'
     557 |                 sd_state->pads->try_crop.height = fse.max_height;
         |                               ^~


vim +544 drivers/staging/media/tegra-video/vi.c

3d8a97eabef088 Sowjanya Komatineni 2020-05-04  489  
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  490  static int __tegra_channel_try_format(struct tegra_vi_channel *chan,
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  491  				      struct v4l2_pix_format *pix)
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  492  {
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  493  	const struct tegra_video_format *fmtinfo;
d156949c999153 Tomi Valkeinen      2022-02-07  494  	static struct lock_class_key key;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  495  	struct v4l2_subdev *subdev;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  496  	struct v4l2_subdev_format fmt;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  497  	struct v4l2_subdev_state *sd_state;
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  498  	struct v4l2_subdev_frame_size_enum fse = {
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  499  		.which = V4L2_SUBDEV_FORMAT_TRY,
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  500  	};
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  501  	struct v4l2_subdev_selection sdsel = {
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  502  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  503  		.target = V4L2_SEL_TGT_CROP_BOUNDS,
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  504  	};
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  505  	int ret;
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  506  
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  507  	subdev = tegra_channel_get_remote_source_subdev(chan);
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  508  	if (!subdev)
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  509  		return -ENODEV;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  510  
d156949c999153 Tomi Valkeinen      2022-02-07  511  	/*
d156949c999153 Tomi Valkeinen      2022-02-07  512  	 * FIXME: Drop this call, drivers are not supposed to use
d156949c999153 Tomi Valkeinen      2022-02-07  513  	 * __v4l2_subdev_state_alloc().
d156949c999153 Tomi Valkeinen      2022-02-07  514  	 */
d156949c999153 Tomi Valkeinen      2022-02-07  515  	sd_state = __v4l2_subdev_state_alloc(subdev, "tegra:state->lock",
d156949c999153 Tomi Valkeinen      2022-02-07  516  					     &key);
ba7a93e507f883 Dan Carpenter       2021-06-22  517  	if (IS_ERR(sd_state))
ba7a93e507f883 Dan Carpenter       2021-06-22  518  		return PTR_ERR(sd_state);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  519  	/*
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  520  	 * Retrieve the format information and if requested format isn't
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  521  	 * supported, keep the current format.
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  522  	 */
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  523  	fmtinfo = tegra_get_format_by_fourcc(chan->vi, pix->pixelformat);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  524  	if (!fmtinfo) {
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  525  		pix->pixelformat = chan->format.pixelformat;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  526  		pix->colorspace = chan->format.colorspace;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  527  		fmtinfo = tegra_get_format_by_fourcc(chan->vi,
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  528  						     pix->pixelformat);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  529  	}
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  530  
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  531  	pix->field = V4L2_FIELD_NONE;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  532  	fmt.which = V4L2_SUBDEV_FORMAT_TRY;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  533  	fmt.pad = 0;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  534  	v4l2_fill_mbus_format(&fmt.format, pix, fmtinfo->code);
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  535  
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  536  	/*
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  537  	 * Attempt to obtain the format size from subdev.
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  538  	 * If not available, try to get crop boundary from subdev.
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  539  	 */
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  540  	fse.code = fmtinfo->code;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  541  	ret = v4l2_subdev_call(subdev, pad, enum_frame_size, sd_state, &fse);
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  542  	if (ret) {
56f64b82356b74 Sowjanya Komatineni 2020-12-11  543  		if (!v4l2_subdev_has_op(subdev, pad, get_selection)) {
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10 @544  			sd_state->pads->try_crop.width = 0;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  545  			sd_state->pads->try_crop.height = 0;
56f64b82356b74 Sowjanya Komatineni 2020-12-11  546  		} else {
56f64b82356b74 Sowjanya Komatineni 2020-12-11  547  			ret = v4l2_subdev_call(subdev, pad, get_selection,
56f64b82356b74 Sowjanya Komatineni 2020-12-11  548  					       NULL, &sdsel);
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  549  			if (ret)
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  550  				return -EINVAL;
56f64b82356b74 Sowjanya Komatineni 2020-12-11  551  
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  552  			sd_state->pads->try_crop.width = sdsel.r.width;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  553  			sd_state->pads->try_crop.height = sdsel.r.height;
56f64b82356b74 Sowjanya Komatineni 2020-12-11  554  		}
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  555  	} else {
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  556  		sd_state->pads->try_crop.width = fse.max_width;
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  557  		sd_state->pads->try_crop.height = fse.max_height;
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  558  	}
bdcad5ce6dde6e Sowjanya Komatineni 2020-08-12  559  
0d346d2a6f54f0 Tomi Valkeinen      2021-06-10  560  	ret = v4l2_subdev_call(subdev, pad, set_fmt, sd_state, &fmt);
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  561  	if (ret < 0)
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  562  		return ret;
1ebaeb09830f36 Sowjanya Komatineni 2020-08-12  563  
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  564  	v4l2_fill_pix_format(pix, &fmt.format);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  565  	tegra_channel_fmt_align(chan, pix, fmtinfo->bpp);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  566  
a702cbb5ddd2f7 Tomi Valkeinen      2022-02-07  567  	__v4l2_subdev_state_free(sd_state);
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  568  
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  569  	return 0;
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  570  }
3d8a97eabef088 Sowjanya Komatineni 2020-05-04  571  

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

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[Tomi Valkeinen]

On 08/02/2022 04:40, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Feb 07, 2022 at 06:11:07PM +0200, Tomi Valkeinen wrote:
>> struct v4l2_subdev_pad_config used to be relevant only for
>> V4L2_SUBDEV_FORMAT_TRY configuration, and thus the fields of the struct
>> were named, e.g. try_fmt.
>>
>> This struct is now used in struct v4l2_subdev_state, and thus can be
>> used in both try and active cases. Thus rename the fields and drop the
>> "try_" prefix.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/i2c/adv7183.c                   |  2 +-
>>   drivers/media/i2c/imx274.c                    | 12 ++++-----
>>   drivers/media/i2c/mt9m001.c                   |  2 +-
>>   drivers/media/i2c/mt9m111.c                   |  2 +-
>>   drivers/media/i2c/mt9t112.c                   |  2 +-
>>   drivers/media/i2c/mt9v011.c                   |  2 +-
>>   drivers/media/i2c/mt9v111.c                   |  4 +--
>>   drivers/media/i2c/ov2640.c                    |  2 +-
>>   drivers/media/i2c/ov6650.c                    | 18 ++++++-------
>>   drivers/media/i2c/ov772x.c                    |  2 +-
>>   drivers/media/i2c/ov9640.c                    |  2 +-
>>   drivers/media/i2c/rj54n1cb0c.c                |  2 +-
>>   drivers/media/i2c/saa6752hs.c                 |  2 +-
>>   drivers/media/i2c/sr030pc30.c                 |  2 +-
>>   drivers/media/i2c/tw9910.c                    |  2 +-
>>   drivers/media/i2c/vs6624.c                    |  2 +-
>>   drivers/media/platform/atmel/atmel-isc-base.c |  8 +++---
>>   drivers/media/platform/atmel/atmel-isi.c      |  8 +++---
> 
> Would it be possible to move those drivers to use the
> v4l2_subdev_get_try_*() helpers instead of accessing the fields directly
> ? It doesn't necessarily need to be done as part of this series, but
> after removing the try_ prefix from the fields it will be more difficult
> to identify the offending drivers.
> 
> In a separate patch (not part of this series) we should also rename the
> v4l2_subdev_get_try_*() functions to v4l2_subdev_state_get_*().

I _knew_ I should not have added this patch, as these only lead to 
"could you also..." ;).

Yes, I think it makes sense to use the helpers. And kernel test robot 
tells me that I forgot about the staging/

I think I will just drop this one, and go with the helper version 
directly. Or rather, first change drivers to use the helper, and then 
change the fields.

  Tomi

Re: [PATCH v3 0/7] v4l: subdev active state

[Tomi Valkeinen]

On 08/02/2022 04:33, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Feb 07, 2022 at 06:11:00PM +0200, Tomi Valkeinen wrote:
>> Hi,
>>
>> This is v3 of the subdev active state series. Changes since v2:
>>
>> - Doc improvements
>> - Allow state->lock to be set by the driver (similarly to v4l2_ctrl_handler)
> 
> While I think we need better in the longer term, this seems like a
> reasonable compromise to land this series and continue building on top.
> 
>> - Rename fields in 'struct v4l2_subdev_pad_config' and drop the try_ prefix.
>> - Add v4l2_subdev_get_locked_active_state(), which calls lockdep_assert_locked() and returns the state.
>> - Changed v4l2_subdev_get_active_state() to call lockdep_assert_not_locked()
>>
>> The idea with the v4l2_subdev_get_active_state /
>> v4l2_subdev_get_locked_active_state change is to have a lockdep_assert
>> called. Roughly I think there are two cases where the
>> v4l2_subdev_get_active_state could be called:
>>
>> - With the intention of just passing it forward to another subdev, in
>>    which case the state must _not_ be locked. Here
>>    v4l2_subdev_get_active_state() can be called.
>>
>> - With the intention of using the state in a case where the state is
>>    known to be already locked. Here v4l2_subdev_get_locked_active_state()
>>    can be called.
> 
> I'm not sure how this will work out, but it seems fine to me to start
> with.

So, a bit longer explanation:

After I added the state->lock change, and used it in a driver so that I 
share the ctrl and the state locks, I had a piece of code in a control 
handler function which needs the state. I had this in the function:

	state = v4l2_subdev_lock_active_state()
	...
	v4l2_subdev_unlock_state(state);

After sharing the lock with ctrls, I had to change it to:

	state = v4l2_subdev_get_active_state()

And my immediate thought was, is it really locked? So I added a 
lockdep_assert. Then I similarly added lockdep_assert in a few other 
drivers, after which I thought that this needs a helper. Then I realized 
that all the old places where v4l2_subdev_get_active_state() is used 
require that the state is _not_locked, so I added 
lockdep_assert_not_locked().

So, when you need the state, you must use one of:

- v4l2_subdev_lock_active_state() (lock the state and get it)
- v4l2_subdev_get_active_state() (get the currently unlocked state)
- v4l2_subdev_get_locked_active_state() (get the currently locked state)

which cover the possible cases and can implicitly do lockdep_assert. The 
first and last functions sound a bit similar, though, but I'm not sure 
how to name them better.

  Tomi

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[Hans Verkuil]

Hi Tomi,

On 2/7/22 17:11, Tomi Valkeinen wrote:
> struct v4l2_subdev_pad_config used to be relevant only for
> V4L2_SUBDEV_FORMAT_TRY configuration, and thus the fields of the struct
> were named, e.g. try_fmt.
> 
> This struct is now used in struct v4l2_subdev_state, and thus can be
> used in both try and active cases. Thus rename the fields and drop the
> "try_" prefix.

This is fine and necessary, but...

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/i2c/adv7183.c                   |  2 +-
>  drivers/media/i2c/imx274.c                    | 12 ++++-----
>  drivers/media/i2c/mt9m001.c                   |  2 +-
>  drivers/media/i2c/mt9m111.c                   |  2 +-
>  drivers/media/i2c/mt9t112.c                   |  2 +-
>  drivers/media/i2c/mt9v011.c                   |  2 +-
>  drivers/media/i2c/mt9v111.c                   |  4 +--
>  drivers/media/i2c/ov2640.c                    |  2 +-
>  drivers/media/i2c/ov6650.c                    | 18 ++++++-------
>  drivers/media/i2c/ov772x.c                    |  2 +-
>  drivers/media/i2c/ov9640.c                    |  2 +-
>  drivers/media/i2c/rj54n1cb0c.c                |  2 +-
>  drivers/media/i2c/saa6752hs.c                 |  2 +-
>  drivers/media/i2c/sr030pc30.c                 |  2 +-
>  drivers/media/i2c/tw9910.c                    |  2 +-
>  drivers/media/i2c/vs6624.c                    |  2 +-
>  drivers/media/platform/atmel/atmel-isc-base.c |  8 +++---
>  drivers/media/platform/atmel/atmel-isi.c      |  8 +++---
>  include/media/v4l2-subdev.h                   | 26 +++++++++----------
>  19 files changed, 50 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7183.c b/drivers/media/i2c/adv7183.c
> index 92cafdea3f1f..9e590abf88e1 100644
> --- a/drivers/media/i2c/adv7183.c
> +++ b/drivers/media/i2c/adv7183.c
> @@ -443,7 +443,7 @@ static int adv7183_set_fmt(struct v4l2_subdev *sd,
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		decoder->fmt = *fmt;
>  	else
> -		sd_state->pads->try_fmt = *fmt;
> +		sd_state->pads->fmt = *fmt;

...now this isn't as clear as it used to be, since there is no longer any
indication that sd_state->pads refers to a 'try' state.

It would help if sd_state would be renamed to try_state, then it is clear
again.

The same would have to be done in the subdev.h callback prototypes
(i.e. rename state to try_state).

Calling it try_state is also consistent with the new sd->active_state, so
I think that would make a lot of sense.

Changing these names in the drivers is of course more work, but it would
make it a lot more readable.

Regards,

	Hans

>  	return 0;
>  }
>  
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 2aa15b9c23cc..c94c24358931 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -1020,8 +1020,8 @@ static int __imx274_change_compose(struct stimx274 *imx274,
>  	int best_goodness = INT_MIN;
>  
>  	if (which == V4L2_SUBDEV_FORMAT_TRY) {
> -		cur_crop = &sd_state->pads->try_crop;
> -		tgt_fmt = &sd_state->pads->try_fmt;
> +		cur_crop = &sd_state->pads->crop;
> +		tgt_fmt = &sd_state->pads->fmt;
>  	} else {
>  		cur_crop = &imx274->crop;
>  		tgt_fmt = &imx274->format;
> @@ -1114,7 +1114,7 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>  	 */
>  	fmt->field = V4L2_FIELD_NONE;
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> -		sd_state->pads->try_fmt = *fmt;
> +		sd_state->pads->fmt = *fmt;
>  	else
>  		imx274->format = *fmt;
>  
> @@ -1145,8 +1145,8 @@ static int imx274_get_selection(struct v4l2_subdev *sd,
>  	}
>  
>  	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		src_crop = &sd_state->pads->try_crop;
> -		src_fmt = &sd_state->pads->try_fmt;
> +		src_crop = &sd_state->pads->crop;
> +		src_fmt = &sd_state->pads->fmt;
>  	} else {
>  		src_crop = &imx274->crop;
>  		src_fmt = &imx274->format;
> @@ -1217,7 +1217,7 @@ static int imx274_set_selection_crop(struct stimx274 *imx274,
>  	sel->r = new_crop;
>  
>  	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> -		tgt_crop = &sd_state->pads->try_crop;
> +		tgt_crop = &sd_state->pads->crop;
>  	else
>  		tgt_crop = &imx274->crop;
>  
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index c9f0bd997ea7..4cf1334efdfe 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -411,7 +411,7 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return mt9m001_s_fmt(sd, fmt, mf);
> -	sd_state->pads->try_fmt = *mf;
> +	sd_state->pads->fmt = *mf;
>  	return 0;
>  }
>  
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 91a44359bcd3..495731f11006 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -678,7 +678,7 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>  	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/mt9t112.c b/drivers/media/i2c/mt9t112.c
> index 8d2e3caa9b28..4457b030d1a8 100644
> --- a/drivers/media/i2c/mt9t112.c
> +++ b/drivers/media/i2c/mt9t112.c
> @@ -982,7 +982,7 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return mt9t112_s_fmt(sd, mf);
> -	sd_state->pads->try_fmt = *mf;
> +	sd_state->pads->fmt = *mf;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
> index 7699e64e1127..c02a51f2a327 100644
> --- a/drivers/media/i2c/mt9v011.c
> +++ b/drivers/media/i2c/mt9v011.c
> @@ -358,7 +358,7 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
>  
>  		set_res(sd);
>  	} else {
> -		sd_state->pads->try_fmt = *fmt;
> +		sd_state->pads->fmt = *fmt;
>  	}
>  
>  	return 0;
> diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
> index 2dc4a0f24ce8..eed8e5a8dcdd 100644
> --- a/drivers/media/i2c/mt9v111.c
> +++ b/drivers/media/i2c/mt9v111.c
> @@ -800,7 +800,7 @@ static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format(
>  #if IS_ENABLED(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  		return v4l2_subdev_get_try_format(&mt9v111->sd, sd_state, pad);
>  #else
> -		return &sd_state->pads->try_fmt;
> +		return &sd_state->pads->fmt;
>  #endif
>  	case V4L2_SUBDEV_FORMAT_ACTIVE:
>  		return &mt9v111->fmt;
> @@ -957,7 +957,7 @@ static int mt9v111_set_format(struct v4l2_subdev *subdev,
>  static int mt9v111_init_cfg(struct v4l2_subdev *subdev,
>  			    struct v4l2_subdev_state *sd_state)
>  {
> -	sd_state->pads->try_fmt = mt9v111_def_fmt;
> +	sd_state->pads->fmt = mt9v111_def_fmt;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 4b75da55b260..8c576fe1e203 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -996,7 +996,7 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>  		/* select format */
>  		priv->cfmt_code = mf->code;
>  	} else {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  	}
>  out:
>  	mutex_unlock(&priv->lock);
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index f67412150b16..2ba4402c5e96 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -550,9 +550,9 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
>  
>  	/* update media bus format code and frame size */
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		mf->width = sd_state->pads->try_fmt.width;
> -		mf->height = sd_state->pads->try_fmt.height;
> -		mf->code = sd_state->pads->try_fmt.code;
> +		mf->width = sd_state->pads->fmt.width;
> +		mf->height = sd_state->pads->fmt.height;
> +		mf->code = sd_state->pads->fmt.code;
>  
>  	} else {
>  		mf->width = priv->rect.width >> priv->half_scale;
> @@ -701,15 +701,15 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>  		/* store media bus format code and frame size in pad config */
> -		sd_state->pads->try_fmt.width = mf->width;
> -		sd_state->pads->try_fmt.height = mf->height;
> -		sd_state->pads->try_fmt.code = mf->code;
> +		sd_state->pads->fmt.width = mf->width;
> +		sd_state->pads->fmt.height = mf->height;
> +		sd_state->pads->fmt.code = mf->code;
>  
>  		/* return default mbus frame format updated with pad config */
>  		*mf = ov6650_def_fmt;
> -		mf->width = sd_state->pads->try_fmt.width;
> -		mf->height = sd_state->pads->try_fmt.height;
> -		mf->code = sd_state->pads->try_fmt.code;
> +		mf->width = sd_state->pads->fmt.width;
> +		mf->height = sd_state->pads->fmt.height;
> +		mf->code = sd_state->pads->fmt.code;
>  
>  	} else {
>  		/* apply new media bus format code and frame size */
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 78602a2f70b0..6ff5961b87b2 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1222,7 +1222,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  	mf->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
> index 0bab8c2cf160..f4b23183b8b1 100644
> --- a/drivers/media/i2c/ov9640.c
> +++ b/drivers/media/i2c/ov9640.c
> @@ -547,7 +547,7 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return ov9640_s_fmt(sd, mf);
>  
> -	sd_state->pads->try_fmt = *mf;
> +	sd_state->pads->fmt = *mf;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/rj54n1cb0c.c b/drivers/media/i2c/rj54n1cb0c.c
> index 2e4018c26912..867e4d7339f2 100644
> --- a/drivers/media/i2c/rj54n1cb0c.c
> +++ b/drivers/media/i2c/rj54n1cb0c.c
> @@ -1009,7 +1009,7 @@ static int rj54n1_set_fmt(struct v4l2_subdev *sd,
>  			      &mf->height, 84, RJ54N1_MAX_HEIGHT, align, 0);
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/saa6752hs.c b/drivers/media/i2c/saa6752hs.c
> index a7f043cad149..74c10fba2af0 100644
> --- a/drivers/media/i2c/saa6752hs.c
> +++ b/drivers/media/i2c/saa6752hs.c
> @@ -595,7 +595,7 @@ static int saa6752hs_set_fmt(struct v4l2_subdev *sd,
>  	f->colorspace = V4L2_COLORSPACE_SMPTE170M;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *f;
> +		sd_state->pads->fmt = *f;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/sr030pc30.c b/drivers/media/i2c/sr030pc30.c
> index 19c0252df2f1..f0ccdf1f887d 100644
> --- a/drivers/media/i2c/sr030pc30.c
> +++ b/drivers/media/i2c/sr030pc30.c
> @@ -541,7 +541,7 @@ static int sr030pc30_set_fmt(struct v4l2_subdev *sd,
>  
>  	fmt = try_fmt(sd, mf);
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *mf;
> +		sd_state->pads->fmt = *mf;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
> index 09f5b3986928..96f6cbc663f0 100644
> --- a/drivers/media/i2c/tw9910.c
> +++ b/drivers/media/i2c/tw9910.c
> @@ -829,7 +829,7 @@ static int tw9910_set_fmt(struct v4l2_subdev *sd,
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return tw9910_s_fmt(sd, mf);
>  
> -	sd_state->pads->try_fmt = *mf;
> +	sd_state->pads->fmt = *mf;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/i2c/vs6624.c b/drivers/media/i2c/vs6624.c
> index 29003dec6f2d..72a422693bc0 100644
> --- a/drivers/media/i2c/vs6624.c
> +++ b/drivers/media/i2c/vs6624.c
> @@ -587,7 +587,7 @@ static int vs6624_set_fmt(struct v4l2_subdev *sd,
>  	fmt->colorspace = vs6624_formats[index].colorspace;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		sd_state->pads->try_fmt = *fmt;
> +		sd_state->pads->fmt = *fmt;
>  		return 0;
>  	}
>  
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index db15770d5b88..aa90aa55a128 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -835,11 +835,11 @@ static void isc_try_fse(struct isc_device *isc,
>  	 * just use the maximum ISC can receive.
>  	 */
>  	if (ret) {
> -		sd_state->pads->try_crop.width = isc->max_width;
> -		sd_state->pads->try_crop.height = isc->max_height;
> +		sd_state->pads->crop.width = isc->max_width;
> +		sd_state->pads->crop.height = isc->max_height;
>  	} else {
> -		sd_state->pads->try_crop.width = fse.max_width;
> -		sd_state->pads->try_crop.height = fse.max_height;
> +		sd_state->pads->crop.width = fse.max_width;
> +		sd_state->pads->crop.height = fse.max_height;
>  	}
>  }
>  
> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> index 4d15814e4481..5806de277bdc 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -572,11 +572,11 @@ static void isi_try_fse(struct atmel_isi *isi, const struct isi_format *isi_fmt,
>  	 * just use the maximum ISI can receive.
>  	 */
>  	if (ret) {
> -		sd_state->pads->try_crop.width = MAX_SUPPORT_WIDTH;
> -		sd_state->pads->try_crop.height = MAX_SUPPORT_HEIGHT;
> +		sd_state->pads->crop.width = MAX_SUPPORT_WIDTH;
> +		sd_state->pads->crop.height = MAX_SUPPORT_HEIGHT;
>  	} else {
> -		sd_state->pads->try_crop.width = fse.max_width;
> -		sd_state->pads->try_crop.height = fse.max_height;
> +		sd_state->pads->crop.width = fse.max_width;
> +		sd_state->pads->crop.height = fse.max_height;
>  	}
>  }
>  
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 16497b8fc875..e7c5617820ab 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -638,21 +638,19 @@ struct v4l2_subdev_ir_ops {
>  /**
>   * struct v4l2_subdev_pad_config - Used for storing subdev pad information.
>   *
> - * @try_fmt: &struct v4l2_mbus_framefmt
> - * @try_crop: &struct v4l2_rect to be used for crop
> - * @try_compose: &struct v4l2_rect to be used for compose
> + * @fmt: &struct v4l2_mbus_framefmt
> + * @crop: &struct v4l2_rect to be used for crop
> + * @compose: &struct v4l2_rect to be used for compose
>   *
>   * 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;
> -	struct v4l2_rect try_crop;
> -	struct v4l2_rect try_compose;
> +	struct v4l2_mbus_framefmt fmt;
> +	struct v4l2_rect crop;
> +	struct v4l2_rect compose;
>  };
>  
>  /**
> @@ -1008,7 +1006,7 @@ struct v4l2_subdev_fh {
>  
>  /**
>   * v4l2_subdev_get_try_format - ancillary routine to call
> - *	&struct v4l2_subdev_pad_config->try_fmt
> + *	&struct v4l2_subdev_pad_config->fmt
>   *
>   * @sd: pointer to &struct v4l2_subdev
>   * @state: pointer to &struct v4l2_subdev_state
> @@ -1021,12 +1019,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
>  {
>  	if (WARN_ON(pad >= sd->entity.num_pads))
>  		pad = 0;
> -	return &state->pads[pad].try_fmt;
> +	return &state->pads[pad].fmt;
>  }
>  
>  /**
>   * v4l2_subdev_get_try_crop - ancillary routine to call
> - *	&struct v4l2_subdev_pad_config->try_crop
> + *	&struct v4l2_subdev_pad_config->crop
>   *
>   * @sd: pointer to &struct v4l2_subdev
>   * @state: pointer to &struct v4l2_subdev_state.
> @@ -1039,12 +1037,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
>  {
>  	if (WARN_ON(pad >= sd->entity.num_pads))
>  		pad = 0;
> -	return &state->pads[pad].try_crop;
> +	return &state->pads[pad].crop;
>  }
>  
>  /**
>   * v4l2_subdev_get_try_compose - ancillary routine to call
> - *	&struct v4l2_subdev_pad_config->try_compose
> + *	&struct v4l2_subdev_pad_config->compose
>   *
>   * @sd: pointer to &struct v4l2_subdev
>   * @state: pointer to &struct v4l2_subdev_state.
> @@ -1057,7 +1055,7 @@ v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
>  {
>  	if (WARN_ON(pad >= sd->entity.num_pads))
>  		pad = 0;
> -	return &state->pads[pad].try_compose;
> +	return &state->pads[pad].compose;
>  }
>  
>  #endif

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[Laurent Pinchart]

Hi Hans,

On Tue, Feb 08, 2022 at 02:59:31PM +0100, Hans Verkuil wrote:
> On 2/7/22 17:11, Tomi Valkeinen wrote:
> > struct v4l2_subdev_pad_config used to be relevant only for
> > V4L2_SUBDEV_FORMAT_TRY configuration, and thus the fields of the struct
> > were named, e.g. try_fmt.
> > 
> > This struct is now used in struct v4l2_subdev_state, and thus can be
> > used in both try and active cases. Thus rename the fields and drop the
> > "try_" prefix.
> 
> This is fine and necessary, but...
> 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  drivers/media/i2c/adv7183.c                   |  2 +-
> >  drivers/media/i2c/imx274.c                    | 12 ++++-----
> >  drivers/media/i2c/mt9m001.c                   |  2 +-
> >  drivers/media/i2c/mt9m111.c                   |  2 +-
> >  drivers/media/i2c/mt9t112.c                   |  2 +-
> >  drivers/media/i2c/mt9v011.c                   |  2 +-
> >  drivers/media/i2c/mt9v111.c                   |  4 +--
> >  drivers/media/i2c/ov2640.c                    |  2 +-
> >  drivers/media/i2c/ov6650.c                    | 18 ++++++-------
> >  drivers/media/i2c/ov772x.c                    |  2 +-
> >  drivers/media/i2c/ov9640.c                    |  2 +-
> >  drivers/media/i2c/rj54n1cb0c.c                |  2 +-
> >  drivers/media/i2c/saa6752hs.c                 |  2 +-
> >  drivers/media/i2c/sr030pc30.c                 |  2 +-
> >  drivers/media/i2c/tw9910.c                    |  2 +-
> >  drivers/media/i2c/vs6624.c                    |  2 +-
> >  drivers/media/platform/atmel/atmel-isc-base.c |  8 +++---
> >  drivers/media/platform/atmel/atmel-isi.c      |  8 +++---
> >  include/media/v4l2-subdev.h                   | 26 +++++++++----------
> >  19 files changed, 50 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv7183.c b/drivers/media/i2c/adv7183.c
> > index 92cafdea3f1f..9e590abf88e1 100644
> > --- a/drivers/media/i2c/adv7183.c
> > +++ b/drivers/media/i2c/adv7183.c
> > @@ -443,7 +443,7 @@ static int adv7183_set_fmt(struct v4l2_subdev *sd,
> >  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> >  		decoder->fmt = *fmt;
> >  	else
> > -		sd_state->pads->try_fmt = *fmt;
> > +		sd_state->pads->fmt = *fmt;
> 
> ...now this isn't as clear as it used to be, since there is no longer any
> indication that sd_state->pads refers to a 'try' state.
> 
> It would help if sd_state would be renamed to try_state, then it is clear
> again.
> 
> The same would have to be done in the subdev.h callback prototypes
> (i.e. rename state to try_state).
> 
> Calling it try_state is also consistent with the new sd->active_state, so
> I think that would make a lot of sense.

With the active state series, the state passed to .set_fmt() can be the
try or active state, depending on the which value. Existing drivers will
continue to operate as expected, and will receive a NULL state when
which is ACTIVE. Drivers that call v4l2_subdev_init_finalize() will get
the active state in that case.

We could rename the arguments in the driver touched by this patch, but
I'd rather modify them to use the v4l2_subdev_get_try_format() helper
instead of accessing the try_fmt field directly. We'll then have to
discuss this again when we'll rename v4l2_subdev_get_try_format() to
v4l2_subdev_state_get_format() :-)

> Changing these names in the drivers is of course more work, but it would
> make it a lot more readable.
> 
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > index 2aa15b9c23cc..c94c24358931 100644
> > --- a/drivers/media/i2c/imx274.c
> > +++ b/drivers/media/i2c/imx274.c
> > @@ -1020,8 +1020,8 @@ static int __imx274_change_compose(struct stimx274 *imx274,
> >  	int best_goodness = INT_MIN;
> >  
> >  	if (which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		cur_crop = &sd_state->pads->try_crop;
> > -		tgt_fmt = &sd_state->pads->try_fmt;
> > +		cur_crop = &sd_state->pads->crop;
> > +		tgt_fmt = &sd_state->pads->fmt;
> >  	} else {
> >  		cur_crop = &imx274->crop;
> >  		tgt_fmt = &imx274->format;
> > @@ -1114,7 +1114,7 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
> >  	 */
> >  	fmt->field = V4L2_FIELD_NONE;
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> > -		sd_state->pads->try_fmt = *fmt;
> > +		sd_state->pads->fmt = *fmt;
> >  	else
> >  		imx274->format = *fmt;
> >  
> > @@ -1145,8 +1145,8 @@ static int imx274_get_selection(struct v4l2_subdev *sd,
> >  	}
> >  
> >  	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		src_crop = &sd_state->pads->try_crop;
> > -		src_fmt = &sd_state->pads->try_fmt;
> > +		src_crop = &sd_state->pads->crop;
> > +		src_fmt = &sd_state->pads->fmt;
> >  	} else {
> >  		src_crop = &imx274->crop;
> >  		src_fmt = &imx274->format;
> > @@ -1217,7 +1217,7 @@ static int imx274_set_selection_crop(struct stimx274 *imx274,
> >  	sel->r = new_crop;
> >  
> >  	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> > -		tgt_crop = &sd_state->pads->try_crop;
> > +		tgt_crop = &sd_state->pads->crop;
> >  	else
> >  		tgt_crop = &imx274->crop;
> >  
> > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > index c9f0bd997ea7..4cf1334efdfe 100644
> > --- a/drivers/media/i2c/mt9m001.c
> > +++ b/drivers/media/i2c/mt9m001.c
> > @@ -411,7 +411,7 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> >  		return mt9m001_s_fmt(sd, fmt, mf);
> > -	sd_state->pads->try_fmt = *mf;
> > +	sd_state->pads->fmt = *mf;
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 91a44359bcd3..495731f11006 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -678,7 +678,7 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> >  	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		sd_state->pads->try_fmt = *mf;
> > +		sd_state->pads->fmt = *mf;
> >  		return 0;
> >  	}
> >  
> > diff --git a/drivers/media/i2c/mt9t112.c b/drivers/media/i2c/mt9t112.c
> > index 8d2e3caa9b28..4457b030d1a8 100644
> > --- a/drivers/media/i2c/mt9t112.c
> > +++ b/drivers/media/i2c/mt9t112.c
> > @@ -982,7 +982,7 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> >  		return mt9t112_s_fmt(sd, mf);
> > -	sd_state->pads->try_fmt = *mf;
> > +	sd_state->pads->fmt = *mf;
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
> > index 7699e64e1127..c02a51f2a327 100644
> > --- a/drivers/media/i2c/mt9v011.c
> > +++ b/drivers/media/i2c/mt9v011.c
> > @@ -358,7 +358,7 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
> >  
> >  		set_res(sd);
> >  	} else {
> > -		sd_state->pads->try_fmt = *fmt;
> > +		sd_state->pads->fmt = *fmt;
> >  	}
> >  
> >  	return 0;
> > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
> > index 2dc4a0f24ce8..eed8e5a8dcdd 100644
> > --- a/drivers/media/i2c/mt9v111.c
> > +++ b/drivers/media/i2c/mt9v111.c
> > @@ -800,7 +800,7 @@ static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format(
> >  #if IS_ENABLED(CONFIG_VIDEO_V4L2_SUBDEV_API)
> >  		return v4l2_subdev_get_try_format(&mt9v111->sd, sd_state, pad);
> >  #else
> > -		return &sd_state->pads->try_fmt;
> > +		return &sd_state->pads->fmt;
> >  #endif
> >  	case V4L2_SUBDEV_FORMAT_ACTIVE:
> >  		return &mt9v111->fmt;
> > @@ -957,7 +957,7 @@ static int mt9v111_set_format(struct v4l2_subdev *subdev,
> >  static int mt9v111_init_cfg(struct v4l2_subdev *subdev,
> >  			    struct v4l2_subdev_state *sd_state)
> >  {
> > -	sd_state->pads->try_fmt = mt9v111_def_fmt;
> > +	sd_state->pads->fmt = mt9v111_def_fmt;
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > index 4b75da55b260..8c576fe1e203 100644
> > --- a/drivers/media/i2c/ov2640.c
> > +++ b/drivers/media/i2c/ov2640.c
> > @@ -996,7 +996,7 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
> >  		/* select format */
> >  		priv->cfmt_code = mf->code;
> >  	} else {
> > -		sd_state->pads->try_fmt = *mf;
> > +		sd_state->pads->fmt = *mf;
> >  	}
> >  out:
> >  	mutex_unlock(&priv->lock);
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index f67412150b16..2ba4402c5e96 100644
> > --- a/drivers/media/i2c/ov6650.c
> > +++ b/drivers/media/i2c/ov6650.c
> > @@ -550,9 +550,9 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
> >  
> >  	/* update media bus format code and frame size */
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		mf->width = sd_state->pads->try_fmt.width;
> > -		mf->height = sd_state->pads->try_fmt.height;
> > -		mf->code = sd_state->pads->try_fmt.code;
> > +		mf->width = sd_state->pads->fmt.width;
> > +		mf->height = sd_state->pads->fmt.height;
> > +		mf->code = sd_state->pads->fmt.code;
> >  
> >  	} else {
> >  		mf->width = priv->rect.width >> priv->half_scale;
> > @@ -701,15 +701,15 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> >  		/* store media bus format code and frame size in pad config */
> > -		sd_state->pads->try_fmt.width = mf->width;
> > -		sd_state->pads->try_fmt.height = mf->height;
> > -		sd_state->pads->try_fmt.code = mf->code;
> > +		sd_state->pads->fmt.width = mf->width;
> > +		sd_state->pads->fmt.height = mf->height;
> > +		sd_state->pads->fmt.code = mf->code;
> >  
> >  		/* return default mbus frame format updated with pad config */
> >  		*mf = ov6650_def_fmt;
> > -		mf->width = sd_state->pads->try_fmt.width;
> > -		mf->height = sd_state->pads->try_fmt.height;
> > -		mf->code = sd_state->pads->try_fmt.code;
> > +		mf->width = sd_state->pads->fmt.width;
> > +		mf->height = sd_state->pads->fmt.height;
> > +		mf->code = sd_state->pads->fmt.code;
> >  
> >  	} else {
> >  		/* apply new media bus format code and frame size */
> > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > index 78602a2f70b0..6ff5961b87b2 100644
> > --- a/drivers/media/i2c/ov772x.c
> > +++ b/drivers/media/i2c/ov772x.c
> > @@ -1222,7 +1222,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
> >  	mf->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		sd_state->pads->try_fmt = *mf;
> > +		sd_state->pads->fmt = *mf;
> >  		return 0;
> >  	}
> >  
> > diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
> > index 0bab8c2cf160..f4b23183b8b1 100644
> > --- a/drivers/media/i2c/ov9640.c
> > +++ b/drivers/media/i2c/ov9640.c
> > @@ -547,7 +547,7 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
> >  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> >  		return ov9640_s_fmt(sd, mf);
> >  
> > -	sd_state->pads->try_fmt = *mf;
> > +	sd_state->pads->fmt = *mf;
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/media/i2c/rj54n1cb0c.c b/drivers/media/i2c/rj54n1cb0c.c
> > index 2e4018c26912..867e4d7339f2 100644
> > --- a/drivers/media/i2c/rj54n1cb0c.c
> > +++ b/drivers/media/i2c/rj54n1cb0c.c
> > @@ -1009,7 +1009,7 @@ static int rj54n1_set_fmt(struct v4l2_subdev *sd,
> >  			      &mf->height, 84, RJ54N1_MAX_HEIGHT, align, 0);
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		sd_state->pads->try_fmt = *mf;
> > +		sd_state->pads->fmt = *mf;
> >  		return 0;
> >  	}
> >  
> > diff --git a/drivers/media/i2c/saa6752hs.c b/drivers/media/i2c/saa6752hs.c
> > index a7f043cad149..74c10fba2af0 100644
> > --- a/drivers/media/i2c/saa6752hs.c
> > +++ b/drivers/media/i2c/saa6752hs.c
> > @@ -595,7 +595,7 @@ static int saa6752hs_set_fmt(struct v4l2_subdev *sd,
> >  	f->colorspace = V4L2_COLORSPACE_SMPTE170M;
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		sd_state->pads->try_fmt = *f;
> > +		sd_state->pads->fmt = *f;
> >  		return 0;
> >  	}
> >  
> > diff --git a/drivers/media/i2c/sr030pc30.c b/drivers/media/i2c/sr030pc30.c
> > index 19c0252df2f1..f0ccdf1f887d 100644
> > --- a/drivers/media/i2c/sr030pc30.c
> > +++ b/drivers/media/i2c/sr030pc30.c
> > @@ -541,7 +541,7 @@ static int sr030pc30_set_fmt(struct v4l2_subdev *sd,
> >  
> >  	fmt = try_fmt(sd, mf);
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		sd_state->pads->try_fmt = *mf;
> > +		sd_state->pads->fmt = *mf;
> >  		return 0;
> >  	}
> >  
> > diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
> > index 09f5b3986928..96f6cbc663f0 100644
> > --- a/drivers/media/i2c/tw9910.c
> > +++ b/drivers/media/i2c/tw9910.c
> > @@ -829,7 +829,7 @@ static int tw9910_set_fmt(struct v4l2_subdev *sd,
> >  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> >  		return tw9910_s_fmt(sd, mf);
> >  
> > -	sd_state->pads->try_fmt = *mf;
> > +	sd_state->pads->fmt = *mf;
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/media/i2c/vs6624.c b/drivers/media/i2c/vs6624.c
> > index 29003dec6f2d..72a422693bc0 100644
> > --- a/drivers/media/i2c/vs6624.c
> > +++ b/drivers/media/i2c/vs6624.c
> > @@ -587,7 +587,7 @@ static int vs6624_set_fmt(struct v4l2_subdev *sd,
> >  	fmt->colorspace = vs6624_formats[index].colorspace;
> >  
> >  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		sd_state->pads->try_fmt = *fmt;
> > +		sd_state->pads->fmt = *fmt;
> >  		return 0;
> >  	}
> >  
> > diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> > index db15770d5b88..aa90aa55a128 100644
> > --- a/drivers/media/platform/atmel/atmel-isc-base.c
> > +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> > @@ -835,11 +835,11 @@ static void isc_try_fse(struct isc_device *isc,
> >  	 * just use the maximum ISC can receive.
> >  	 */
> >  	if (ret) {
> > -		sd_state->pads->try_crop.width = isc->max_width;
> > -		sd_state->pads->try_crop.height = isc->max_height;
> > +		sd_state->pads->crop.width = isc->max_width;
> > +		sd_state->pads->crop.height = isc->max_height;
> >  	} else {
> > -		sd_state->pads->try_crop.width = fse.max_width;
> > -		sd_state->pads->try_crop.height = fse.max_height;
> > +		sd_state->pads->crop.width = fse.max_width;
> > +		sd_state->pads->crop.height = fse.max_height;
> >  	}
> >  }
> >  
> > diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> > index 4d15814e4481..5806de277bdc 100644
> > --- a/drivers/media/platform/atmel/atmel-isi.c
> > +++ b/drivers/media/platform/atmel/atmel-isi.c
> > @@ -572,11 +572,11 @@ static void isi_try_fse(struct atmel_isi *isi, const struct isi_format *isi_fmt,
> >  	 * just use the maximum ISI can receive.
> >  	 */
> >  	if (ret) {
> > -		sd_state->pads->try_crop.width = MAX_SUPPORT_WIDTH;
> > -		sd_state->pads->try_crop.height = MAX_SUPPORT_HEIGHT;
> > +		sd_state->pads->crop.width = MAX_SUPPORT_WIDTH;
> > +		sd_state->pads->crop.height = MAX_SUPPORT_HEIGHT;
> >  	} else {
> > -		sd_state->pads->try_crop.width = fse.max_width;
> > -		sd_state->pads->try_crop.height = fse.max_height;
> > +		sd_state->pads->crop.width = fse.max_width;
> > +		sd_state->pads->crop.height = fse.max_height;
> >  	}
> >  }
> >  
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 16497b8fc875..e7c5617820ab 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -638,21 +638,19 @@ struct v4l2_subdev_ir_ops {
> >  /**
> >   * struct v4l2_subdev_pad_config - Used for storing subdev pad information.
> >   *
> > - * @try_fmt: &struct v4l2_mbus_framefmt
> > - * @try_crop: &struct v4l2_rect to be used for crop
> > - * @try_compose: &struct v4l2_rect to be used for compose
> > + * @fmt: &struct v4l2_mbus_framefmt
> > + * @crop: &struct v4l2_rect to be used for crop
> > + * @compose: &struct v4l2_rect to be used for compose
> >   *
> >   * 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;
> > -	struct v4l2_rect try_crop;
> > -	struct v4l2_rect try_compose;
> > +	struct v4l2_mbus_framefmt fmt;
> > +	struct v4l2_rect crop;
> > +	struct v4l2_rect compose;
> >  };
> >  
> >  /**
> > @@ -1008,7 +1006,7 @@ struct v4l2_subdev_fh {
> >  
> >  /**
> >   * v4l2_subdev_get_try_format - ancillary routine to call
> > - *	&struct v4l2_subdev_pad_config->try_fmt
> > + *	&struct v4l2_subdev_pad_config->fmt
> >   *
> >   * @sd: pointer to &struct v4l2_subdev
> >   * @state: pointer to &struct v4l2_subdev_state
> > @@ -1021,12 +1019,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> >  {
> >  	if (WARN_ON(pad >= sd->entity.num_pads))
> >  		pad = 0;
> > -	return &state->pads[pad].try_fmt;
> > +	return &state->pads[pad].fmt;
> >  }
> >  
> >  /**
> >   * v4l2_subdev_get_try_crop - ancillary routine to call
> > - *	&struct v4l2_subdev_pad_config->try_crop
> > + *	&struct v4l2_subdev_pad_config->crop
> >   *
> >   * @sd: pointer to &struct v4l2_subdev
> >   * @state: pointer to &struct v4l2_subdev_state.
> > @@ -1039,12 +1037,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> >  {
> >  	if (WARN_ON(pad >= sd->entity.num_pads))
> >  		pad = 0;
> > -	return &state->pads[pad].try_crop;
> > +	return &state->pads[pad].crop;
> >  }
> >  
> >  /**
> >   * v4l2_subdev_get_try_compose - ancillary routine to call
> > - *	&struct v4l2_subdev_pad_config->try_compose
> > + *	&struct v4l2_subdev_pad_config->compose
> >   *
> >   * @sd: pointer to &struct v4l2_subdev
> >   * @state: pointer to &struct v4l2_subdev_state.
> > @@ -1057,7 +1055,7 @@ v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> >  {
> >  	if (WARN_ON(pad >= sd->entity.num_pads))
> >  		pad = 0;
> > -	return &state->pads[pad].try_compose;
> > +	return &state->pads[pad].compose;
> >  }
> >  
> >  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[Hans Verkuil]

Hi Laurent,

On 2/8/22 15:05, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tue, Feb 08, 2022 at 02:59:31PM +0100, Hans Verkuil wrote:
>> On 2/7/22 17:11, Tomi Valkeinen wrote:
>>> struct v4l2_subdev_pad_config used to be relevant only for
>>> V4L2_SUBDEV_FORMAT_TRY configuration, and thus the fields of the struct
>>> were named, e.g. try_fmt.
>>>
>>> This struct is now used in struct v4l2_subdev_state, and thus can be
>>> used in both try and active cases. Thus rename the fields and drop the
>>> "try_" prefix.
>>
>> This is fine and necessary, but...
>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>  drivers/media/i2c/adv7183.c                   |  2 +-
>>>  drivers/media/i2c/imx274.c                    | 12 ++++-----
>>>  drivers/media/i2c/mt9m001.c                   |  2 +-
>>>  drivers/media/i2c/mt9m111.c                   |  2 +-
>>>  drivers/media/i2c/mt9t112.c                   |  2 +-
>>>  drivers/media/i2c/mt9v011.c                   |  2 +-
>>>  drivers/media/i2c/mt9v111.c                   |  4 +--
>>>  drivers/media/i2c/ov2640.c                    |  2 +-
>>>  drivers/media/i2c/ov6650.c                    | 18 ++++++-------
>>>  drivers/media/i2c/ov772x.c                    |  2 +-
>>>  drivers/media/i2c/ov9640.c                    |  2 +-
>>>  drivers/media/i2c/rj54n1cb0c.c                |  2 +-
>>>  drivers/media/i2c/saa6752hs.c                 |  2 +-
>>>  drivers/media/i2c/sr030pc30.c                 |  2 +-
>>>  drivers/media/i2c/tw9910.c                    |  2 +-
>>>  drivers/media/i2c/vs6624.c                    |  2 +-
>>>  drivers/media/platform/atmel/atmel-isc-base.c |  8 +++---
>>>  drivers/media/platform/atmel/atmel-isi.c      |  8 +++---
>>>  include/media/v4l2-subdev.h                   | 26 +++++++++----------
>>>  19 files changed, 50 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/adv7183.c b/drivers/media/i2c/adv7183.c
>>> index 92cafdea3f1f..9e590abf88e1 100644
>>> --- a/drivers/media/i2c/adv7183.c
>>> +++ b/drivers/media/i2c/adv7183.c
>>> @@ -443,7 +443,7 @@ static int adv7183_set_fmt(struct v4l2_subdev *sd,
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>>>  		decoder->fmt = *fmt;
>>>  	else
>>> -		sd_state->pads->try_fmt = *fmt;
>>> +		sd_state->pads->fmt = *fmt;
>>
>> ...now this isn't as clear as it used to be, since there is no longer any
>> indication that sd_state->pads refers to a 'try' state.
>>
>> It would help if sd_state would be renamed to try_state, then it is clear
>> again.
>>
>> The same would have to be done in the subdev.h callback prototypes
>> (i.e. rename state to try_state).
>>
>> Calling it try_state is also consistent with the new sd->active_state, so
>> I think that would make a lot of sense.
> 
> With the active state series, the state passed to .set_fmt() can be the
> try or active state, depending on the which value. Existing drivers will
> continue to operate as expected, and will receive a NULL state when
> which is ACTIVE. Drivers that call v4l2_subdev_init_finalize() will get
> the active state in that case.
> 
> We could rename the arguments in the driver touched by this patch, but
> I'd rather modify them to use the v4l2_subdev_get_try_format() helper
> instead of accessing the try_fmt field directly. We'll then have to
> discuss this again when we'll rename v4l2_subdev_get_try_format() to
> v4l2_subdev_state_get_format() :-)

You are absolutely correct. I should have refeshed my memory of this series
a bit better.

Ignore my comments, and just add:

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

And BTW, I *do* think v4l2_subdev_get_try_format() et al should be renamed to
v4l2_subdev_state_get_format() et al. It makes a lot of sense. But that's a
separate patch, I think.

Regards,

	Hans

> 
>> Changing these names in the drivers is of course more work, but it would
>> make it a lot more readable.
>>
>>>  	return 0;
>>>  }
>>>  
>>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>>> index 2aa15b9c23cc..c94c24358931 100644
>>> --- a/drivers/media/i2c/imx274.c
>>> +++ b/drivers/media/i2c/imx274.c
>>> @@ -1020,8 +1020,8 @@ static int __imx274_change_compose(struct stimx274 *imx274,
>>>  	int best_goodness = INT_MIN;
>>>  
>>>  	if (which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		cur_crop = &sd_state->pads->try_crop;
>>> -		tgt_fmt = &sd_state->pads->try_fmt;
>>> +		cur_crop = &sd_state->pads->crop;
>>> +		tgt_fmt = &sd_state->pads->fmt;
>>>  	} else {
>>>  		cur_crop = &imx274->crop;
>>>  		tgt_fmt = &imx274->format;
>>> @@ -1114,7 +1114,7 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>>>  	 */
>>>  	fmt->field = V4L2_FIELD_NONE;
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>> -		sd_state->pads->try_fmt = *fmt;
>>> +		sd_state->pads->fmt = *fmt;
>>>  	else
>>>  		imx274->format = *fmt;
>>>  
>>> @@ -1145,8 +1145,8 @@ static int imx274_get_selection(struct v4l2_subdev *sd,
>>>  	}
>>>  
>>>  	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		src_crop = &sd_state->pads->try_crop;
>>> -		src_fmt = &sd_state->pads->try_fmt;
>>> +		src_crop = &sd_state->pads->crop;
>>> +		src_fmt = &sd_state->pads->fmt;
>>>  	} else {
>>>  		src_crop = &imx274->crop;
>>>  		src_fmt = &imx274->format;
>>> @@ -1217,7 +1217,7 @@ static int imx274_set_selection_crop(struct stimx274 *imx274,
>>>  	sel->r = new_crop;
>>>  
>>>  	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
>>> -		tgt_crop = &sd_state->pads->try_crop;
>>> +		tgt_crop = &sd_state->pads->crop;
>>>  	else
>>>  		tgt_crop = &imx274->crop;
>>>  
>>> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
>>> index c9f0bd997ea7..4cf1334efdfe 100644
>>> --- a/drivers/media/i2c/mt9m001.c
>>> +++ b/drivers/media/i2c/mt9m001.c
>>> @@ -411,7 +411,7 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
>>>  
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>>>  		return mt9m001_s_fmt(sd, fmt, mf);
>>> -	sd_state->pads->try_fmt = *mf;
>>> +	sd_state->pads->fmt = *mf;
>>>  	return 0;
>>>  }
>>>  
>>> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
>>> index 91a44359bcd3..495731f11006 100644
>>> --- a/drivers/media/i2c/mt9m111.c
>>> +++ b/drivers/media/i2c/mt9m111.c
>>> @@ -678,7 +678,7 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>>>  	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
>>>  
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		sd_state->pads->try_fmt = *mf;
>>> +		sd_state->pads->fmt = *mf;
>>>  		return 0;
>>>  	}
>>>  
>>> diff --git a/drivers/media/i2c/mt9t112.c b/drivers/media/i2c/mt9t112.c
>>> index 8d2e3caa9b28..4457b030d1a8 100644
>>> --- a/drivers/media/i2c/mt9t112.c
>>> +++ b/drivers/media/i2c/mt9t112.c
>>> @@ -982,7 +982,7 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
>>>  
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>>>  		return mt9t112_s_fmt(sd, mf);
>>> -	sd_state->pads->try_fmt = *mf;
>>> +	sd_state->pads->fmt = *mf;
>>>  
>>>  	return 0;
>>>  }
>>> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
>>> index 7699e64e1127..c02a51f2a327 100644
>>> --- a/drivers/media/i2c/mt9v011.c
>>> +++ b/drivers/media/i2c/mt9v011.c
>>> @@ -358,7 +358,7 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
>>>  
>>>  		set_res(sd);
>>>  	} else {
>>> -		sd_state->pads->try_fmt = *fmt;
>>> +		sd_state->pads->fmt = *fmt;
>>>  	}
>>>  
>>>  	return 0;
>>> diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c
>>> index 2dc4a0f24ce8..eed8e5a8dcdd 100644
>>> --- a/drivers/media/i2c/mt9v111.c
>>> +++ b/drivers/media/i2c/mt9v111.c
>>> @@ -800,7 +800,7 @@ static struct v4l2_mbus_framefmt *__mt9v111_get_pad_format(
>>>  #if IS_ENABLED(CONFIG_VIDEO_V4L2_SUBDEV_API)
>>>  		return v4l2_subdev_get_try_format(&mt9v111->sd, sd_state, pad);
>>>  #else
>>> -		return &sd_state->pads->try_fmt;
>>> +		return &sd_state->pads->fmt;
>>>  #endif
>>>  	case V4L2_SUBDEV_FORMAT_ACTIVE:
>>>  		return &mt9v111->fmt;
>>> @@ -957,7 +957,7 @@ static int mt9v111_set_format(struct v4l2_subdev *subdev,
>>>  static int mt9v111_init_cfg(struct v4l2_subdev *subdev,
>>>  			    struct v4l2_subdev_state *sd_state)
>>>  {
>>> -	sd_state->pads->try_fmt = mt9v111_def_fmt;
>>> +	sd_state->pads->fmt = mt9v111_def_fmt;
>>>  
>>>  	return 0;
>>>  }
>>> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
>>> index 4b75da55b260..8c576fe1e203 100644
>>> --- a/drivers/media/i2c/ov2640.c
>>> +++ b/drivers/media/i2c/ov2640.c
>>> @@ -996,7 +996,7 @@ static int ov2640_set_fmt(struct v4l2_subdev *sd,
>>>  		/* select format */
>>>  		priv->cfmt_code = mf->code;
>>>  	} else {
>>> -		sd_state->pads->try_fmt = *mf;
>>> +		sd_state->pads->fmt = *mf;
>>>  	}
>>>  out:
>>>  	mutex_unlock(&priv->lock);
>>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
>>> index f67412150b16..2ba4402c5e96 100644
>>> --- a/drivers/media/i2c/ov6650.c
>>> +++ b/drivers/media/i2c/ov6650.c
>>> @@ -550,9 +550,9 @@ static int ov6650_get_fmt(struct v4l2_subdev *sd,
>>>  
>>>  	/* update media bus format code and frame size */
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		mf->width = sd_state->pads->try_fmt.width;
>>> -		mf->height = sd_state->pads->try_fmt.height;
>>> -		mf->code = sd_state->pads->try_fmt.code;
>>> +		mf->width = sd_state->pads->fmt.width;
>>> +		mf->height = sd_state->pads->fmt.height;
>>> +		mf->code = sd_state->pads->fmt.code;
>>>  
>>>  	} else {
>>>  		mf->width = priv->rect.width >> priv->half_scale;
>>> @@ -701,15 +701,15 @@ static int ov6650_set_fmt(struct v4l2_subdev *sd,
>>>  
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>>  		/* store media bus format code and frame size in pad config */
>>> -		sd_state->pads->try_fmt.width = mf->width;
>>> -		sd_state->pads->try_fmt.height = mf->height;
>>> -		sd_state->pads->try_fmt.code = mf->code;
>>> +		sd_state->pads->fmt.width = mf->width;
>>> +		sd_state->pads->fmt.height = mf->height;
>>> +		sd_state->pads->fmt.code = mf->code;
>>>  
>>>  		/* return default mbus frame format updated with pad config */
>>>  		*mf = ov6650_def_fmt;
>>> -		mf->width = sd_state->pads->try_fmt.width;
>>> -		mf->height = sd_state->pads->try_fmt.height;
>>> -		mf->code = sd_state->pads->try_fmt.code;
>>> +		mf->width = sd_state->pads->fmt.width;
>>> +		mf->height = sd_state->pads->fmt.height;
>>> +		mf->code = sd_state->pads->fmt.code;
>>>  
>>>  	} else {
>>>  		/* apply new media bus format code and frame size */
>>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>>> index 78602a2f70b0..6ff5961b87b2 100644
>>> --- a/drivers/media/i2c/ov772x.c
>>> +++ b/drivers/media/i2c/ov772x.c
>>> @@ -1222,7 +1222,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>>>  	mf->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>  
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		sd_state->pads->try_fmt = *mf;
>>> +		sd_state->pads->fmt = *mf;
>>>  		return 0;
>>>  	}
>>>  
>>> diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
>>> index 0bab8c2cf160..f4b23183b8b1 100644
>>> --- a/drivers/media/i2c/ov9640.c
>>> +++ b/drivers/media/i2c/ov9640.c
>>> @@ -547,7 +547,7 @@ static int ov9640_set_fmt(struct v4l2_subdev *sd,
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>>>  		return ov9640_s_fmt(sd, mf);
>>>  
>>> -	sd_state->pads->try_fmt = *mf;
>>> +	sd_state->pads->fmt = *mf;
>>>  
>>>  	return 0;
>>>  }
>>> diff --git a/drivers/media/i2c/rj54n1cb0c.c b/drivers/media/i2c/rj54n1cb0c.c
>>> index 2e4018c26912..867e4d7339f2 100644
>>> --- a/drivers/media/i2c/rj54n1cb0c.c
>>> +++ b/drivers/media/i2c/rj54n1cb0c.c
>>> @@ -1009,7 +1009,7 @@ static int rj54n1_set_fmt(struct v4l2_subdev *sd,
>>>  			      &mf->height, 84, RJ54N1_MAX_HEIGHT, align, 0);
>>>  
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		sd_state->pads->try_fmt = *mf;
>>> +		sd_state->pads->fmt = *mf;
>>>  		return 0;
>>>  	}
>>>  
>>> diff --git a/drivers/media/i2c/saa6752hs.c b/drivers/media/i2c/saa6752hs.c
>>> index a7f043cad149..74c10fba2af0 100644
>>> --- a/drivers/media/i2c/saa6752hs.c
>>> +++ b/drivers/media/i2c/saa6752hs.c
>>> @@ -595,7 +595,7 @@ static int saa6752hs_set_fmt(struct v4l2_subdev *sd,
>>>  	f->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>  
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		sd_state->pads->try_fmt = *f;
>>> +		sd_state->pads->fmt = *f;
>>>  		return 0;
>>>  	}
>>>  
>>> diff --git a/drivers/media/i2c/sr030pc30.c b/drivers/media/i2c/sr030pc30.c
>>> index 19c0252df2f1..f0ccdf1f887d 100644
>>> --- a/drivers/media/i2c/sr030pc30.c
>>> +++ b/drivers/media/i2c/sr030pc30.c
>>> @@ -541,7 +541,7 @@ static int sr030pc30_set_fmt(struct v4l2_subdev *sd,
>>>  
>>>  	fmt = try_fmt(sd, mf);
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		sd_state->pads->try_fmt = *mf;
>>> +		sd_state->pads->fmt = *mf;
>>>  		return 0;
>>>  	}
>>>  
>>> diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
>>> index 09f5b3986928..96f6cbc663f0 100644
>>> --- a/drivers/media/i2c/tw9910.c
>>> +++ b/drivers/media/i2c/tw9910.c
>>> @@ -829,7 +829,7 @@ static int tw9910_set_fmt(struct v4l2_subdev *sd,
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>>>  		return tw9910_s_fmt(sd, mf);
>>>  
>>> -	sd_state->pads->try_fmt = *mf;
>>> +	sd_state->pads->fmt = *mf;
>>>  
>>>  	return 0;
>>>  }
>>> diff --git a/drivers/media/i2c/vs6624.c b/drivers/media/i2c/vs6624.c
>>> index 29003dec6f2d..72a422693bc0 100644
>>> --- a/drivers/media/i2c/vs6624.c
>>> +++ b/drivers/media/i2c/vs6624.c
>>> @@ -587,7 +587,7 @@ static int vs6624_set_fmt(struct v4l2_subdev *sd,
>>>  	fmt->colorspace = vs6624_formats[index].colorspace;
>>>  
>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		sd_state->pads->try_fmt = *fmt;
>>> +		sd_state->pads->fmt = *fmt;
>>>  		return 0;
>>>  	}
>>>  
>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>> index db15770d5b88..aa90aa55a128 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>> @@ -835,11 +835,11 @@ static void isc_try_fse(struct isc_device *isc,
>>>  	 * just use the maximum ISC can receive.
>>>  	 */
>>>  	if (ret) {
>>> -		sd_state->pads->try_crop.width = isc->max_width;
>>> -		sd_state->pads->try_crop.height = isc->max_height;
>>> +		sd_state->pads->crop.width = isc->max_width;
>>> +		sd_state->pads->crop.height = isc->max_height;
>>>  	} else {
>>> -		sd_state->pads->try_crop.width = fse.max_width;
>>> -		sd_state->pads->try_crop.height = fse.max_height;
>>> +		sd_state->pads->crop.width = fse.max_width;
>>> +		sd_state->pads->crop.height = fse.max_height;
>>>  	}
>>>  }
>>>  
>>> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
>>> index 4d15814e4481..5806de277bdc 100644
>>> --- a/drivers/media/platform/atmel/atmel-isi.c
>>> +++ b/drivers/media/platform/atmel/atmel-isi.c
>>> @@ -572,11 +572,11 @@ static void isi_try_fse(struct atmel_isi *isi, const struct isi_format *isi_fmt,
>>>  	 * just use the maximum ISI can receive.
>>>  	 */
>>>  	if (ret) {
>>> -		sd_state->pads->try_crop.width = MAX_SUPPORT_WIDTH;
>>> -		sd_state->pads->try_crop.height = MAX_SUPPORT_HEIGHT;
>>> +		sd_state->pads->crop.width = MAX_SUPPORT_WIDTH;
>>> +		sd_state->pads->crop.height = MAX_SUPPORT_HEIGHT;
>>>  	} else {
>>> -		sd_state->pads->try_crop.width = fse.max_width;
>>> -		sd_state->pads->try_crop.height = fse.max_height;
>>> +		sd_state->pads->crop.width = fse.max_width;
>>> +		sd_state->pads->crop.height = fse.max_height;
>>>  	}
>>>  }
>>>  
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index 16497b8fc875..e7c5617820ab 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -638,21 +638,19 @@ struct v4l2_subdev_ir_ops {
>>>  /**
>>>   * struct v4l2_subdev_pad_config - Used for storing subdev pad information.
>>>   *
>>> - * @try_fmt: &struct v4l2_mbus_framefmt
>>> - * @try_crop: &struct v4l2_rect to be used for crop
>>> - * @try_compose: &struct v4l2_rect to be used for compose
>>> + * @fmt: &struct v4l2_mbus_framefmt
>>> + * @crop: &struct v4l2_rect to be used for crop
>>> + * @compose: &struct v4l2_rect to be used for compose
>>>   *
>>>   * 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;
>>> -	struct v4l2_rect try_crop;
>>> -	struct v4l2_rect try_compose;
>>> +	struct v4l2_mbus_framefmt fmt;
>>> +	struct v4l2_rect crop;
>>> +	struct v4l2_rect compose;
>>>  };
>>>  
>>>  /**
>>> @@ -1008,7 +1006,7 @@ struct v4l2_subdev_fh {
>>>  
>>>  /**
>>>   * v4l2_subdev_get_try_format - ancillary routine to call
>>> - *	&struct v4l2_subdev_pad_config->try_fmt
>>> + *	&struct v4l2_subdev_pad_config->fmt
>>>   *
>>>   * @sd: pointer to &struct v4l2_subdev
>>>   * @state: pointer to &struct v4l2_subdev_state
>>> @@ -1021,12 +1019,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
>>>  {
>>>  	if (WARN_ON(pad >= sd->entity.num_pads))
>>>  		pad = 0;
>>> -	return &state->pads[pad].try_fmt;
>>> +	return &state->pads[pad].fmt;
>>>  }
>>>  
>>>  /**
>>>   * v4l2_subdev_get_try_crop - ancillary routine to call
>>> - *	&struct v4l2_subdev_pad_config->try_crop
>>> + *	&struct v4l2_subdev_pad_config->crop
>>>   *
>>>   * @sd: pointer to &struct v4l2_subdev
>>>   * @state: pointer to &struct v4l2_subdev_state.
>>> @@ -1039,12 +1037,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
>>>  {
>>>  	if (WARN_ON(pad >= sd->entity.num_pads))
>>>  		pad = 0;
>>> -	return &state->pads[pad].try_crop;
>>> +	return &state->pads[pad].crop;
>>>  }
>>>  
>>>  /**
>>>   * v4l2_subdev_get_try_compose - ancillary routine to call
>>> - *	&struct v4l2_subdev_pad_config->try_compose
>>> + *	&struct v4l2_subdev_pad_config->compose
>>>   *
>>>   * @sd: pointer to &struct v4l2_subdev
>>>   * @state: pointer to &struct v4l2_subdev_state.
>>> @@ -1057,7 +1055,7 @@ v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
>>>  {
>>>  	if (WARN_ON(pad >= sd->entity.num_pads))
>>>  		pad = 0;
>>> -	return &state->pads[pad].try_compose;
>>> +	return &state->pads[pad].compose;
>>>  }
>>>  
>>>  #endif
> 

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[kernel test robot]

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.17-rc3]
[cannot apply to next-20220208]
[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/20220208-002350
base:   git://linuxtv.org/media_tree.git master
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220209/202202090347.nNP3NUjy-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/8283dea08111c6a813e9340d735c158df3fcbe5f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tomi-Valkeinen/v4l-subdev-active-state/20220208-002350
        git checkout 8283dea08111c6a813e9340d735c158df3fcbe5f
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

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/staging/media/atomisp/pci/atomisp_file.c: In function 'file_input_set_fmt':
>> drivers/staging/media/atomisp/pci/atomisp_file.c:116:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     116 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/pci/atomisp_tpg.c: In function 'tpg_set_fmt':
>> drivers/staging/media/atomisp/pci/atomisp_tpg.c:50:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
      50 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c: In function 'mt9m114_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c:757:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     757 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-gc2235.c: In function 'gc2235_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-gc2235.c:735:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     735 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-ov2722.c: In function 'ov2722_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-ov2722.c:840:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     840 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-ov2680.c: In function 'ov2680_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-ov2680.c:857:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     857 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-gc0310.c: In function 'gc0310_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-gc0310.c:933:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     933 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c: In function 'ov5693_set_fmt':
>> drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c:1611:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
    1611 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~


vim +116 drivers/staging/media/atomisp/pci/atomisp_file.c

ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  105  
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  106  static int file_input_set_fmt(struct v4l2_subdev *sd,
0d346d2a6f54f0 drivers/staging/media/atomisp/pci/atomisp_file.c          Tomi Valkeinen        2021-06-10  107  			      struct v4l2_subdev_state *sd_state,
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  108  			      struct v4l2_subdev_format *format)
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  109  {
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  110  	struct v4l2_mbus_framefmt *fmt = &format->format;
bdfe0beb95eebc drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  111  
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  112  	if (format->pad)
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  113  		return -EINVAL;
0d346d2a6f54f0 drivers/staging/media/atomisp/pci/atomisp_file.c          Tomi Valkeinen        2021-06-10  114  	file_input_get_fmt(sd, sd_state, format);
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  115  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
0d346d2a6f54f0 drivers/staging/media/atomisp/pci/atomisp_file.c          Tomi Valkeinen        2021-06-10 @116  		sd_state->pads->try_fmt = *fmt;
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  117  	return 0;
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  118  }
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  119  

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

Re: [PATCH v3 7/7] media: subdev: rename v4l2_subdev_pad_config.try_* fields

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5991 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.17-rc3]
[cannot apply to next-20220208]
[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/20220208-002350
base:   git://linuxtv.org/media_tree.git master
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220209/202202090347.nNP3NUjy-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/8283dea08111c6a813e9340d735c158df3fcbe5f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tomi-Valkeinen/v4l-subdev-active-state/20220208-002350
        git checkout 8283dea08111c6a813e9340d735c158df3fcbe5f
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

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/staging/media/atomisp/pci/atomisp_file.c: In function 'file_input_set_fmt':
>> drivers/staging/media/atomisp/pci/atomisp_file.c:116:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     116 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/pci/atomisp_tpg.c: In function 'tpg_set_fmt':
>> drivers/staging/media/atomisp/pci/atomisp_tpg.c:50:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
      50 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c: In function 'mt9m114_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c:757:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     757 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-gc2235.c: In function 'gc2235_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-gc2235.c:735:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     735 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-ov2722.c: In function 'ov2722_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-ov2722.c:840:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     840 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-ov2680.c: In function 'ov2680_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-ov2680.c:857:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     857 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/atomisp-gc0310.c: In function 'gc0310_set_fmt':
>> drivers/staging/media/atomisp/i2c/atomisp-gc0310.c:933:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
     933 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~
--
   drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c: In function 'ov5693_set_fmt':
>> drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c:1611:17: error: 'struct v4l2_subdev_pad_config' has no member named 'try_fmt'
    1611 |   sd_state->pads->try_fmt = *fmt;
         |                 ^~


vim +116 drivers/staging/media/atomisp/pci/atomisp_file.c

ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  105  
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  106  static int file_input_set_fmt(struct v4l2_subdev *sd,
0d346d2a6f54f0 drivers/staging/media/atomisp/pci/atomisp_file.c          Tomi Valkeinen        2021-06-10  107  			      struct v4l2_subdev_state *sd_state,
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  108  			      struct v4l2_subdev_format *format)
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  109  {
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  110  	struct v4l2_mbus_framefmt *fmt = &format->format;
bdfe0beb95eebc drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  111  
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  112  	if (format->pad)
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  113  		return -EINVAL;
0d346d2a6f54f0 drivers/staging/media/atomisp/pci/atomisp_file.c          Tomi Valkeinen        2021-06-10  114  	file_input_get_fmt(sd, sd_state, format);
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  115  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
0d346d2a6f54f0 drivers/staging/media/atomisp/pci/atomisp_file.c          Tomi Valkeinen        2021-06-10 @116  		sd_state->pads->try_fmt = *fmt;
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  117  	return 0;
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  118  }
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/atomisp_file.c Mauro Carvalho Chehab 2020-04-19  119  

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