[PATCH v3 0/2] media: i2c: vd55g1: Two control fixes

[PATCH v3 0/2] media: i2c: vd55g1: Two control fixes

[Fernando Rimoli]

1/2 marks the auto exposure lock and bias controls inactive rather than
grabbed in manual exposure mode. It is unchanged; Sakari's question on
it is answered on v2.

2/2 replaces v2's hand-written event ops, as Sakari asked: it
initialises the controls before v4l2_subdev_init_finalize(), which
enables control events. The same order fixes a locking bug the driver
has had since it was added: the state lock was set before the control
handler lock existed, so the two were separate mutexes. Moving the
finalise alone deadlocks the first stream start, because
vd55g1_grab_ctrls() then takes the control handler lock a second time,
so 2/2 also switches it to __v4l2_ctrl_grab(). 2/2 does not depend on
1/2.

Because of the locking bug, 2/2 carries Fixes and Cc: stable, unlike v2.
Benjamin, drop them if you would rather not have it backported.

Changes since v2:
 - 2/2: new approach, see above (Sakari)

Testing. I do not have a VD55G1, so this was tested on the VD55G0 of a
Surface Pro 9 (IPU6) through an out-of-tree driver derived from vd55g1.
It is identical in the code 2/2 touches, apart from one more
format-dependent control (pixel rate). A test-only print in s_ctrl()
compared the two locks:

 - before 2/2, VIDIOC_S_CTRL ran s_ctrl() with only the control lock
   held, and setting the active format and stream on ran it with only
   the state lock held;
 - with 2/2 they are one lock, held in all three cases;
 - without the __v4l2_ctrl_grab() change, the first stream start hung in
   vd55g1_grab_ctrls(), blocked on the mutex it already held.

The controls at probe and after a format change match the unpatched
driver, apart from the default of that driver's pixel rate control,
which my port of 2/2 left at its placeholder. The flip and test pattern
controls are grabbed while streaming and released after. R8 644x604, R10
and R10_CSI2P 640x480 streamed 90 of 90 frames, and five start/stop
cycles were clean. v4l2-compliance on the subdev gives the same results
as with v2, in both exposure modes, and control events pass. The kernel
here has no lockdep, hence the print.

v2: https://lore.kernel.org/linux-media/20261101195006.220657-1-fernandorimoli11@gmail.com/
v1: https://lore.kernel.org/linux-media/20261025154031.27083-1-fernandorimoli11@gmail.com/

Fernando Rimoli (2):
  media: i2c: vd55g1: Mark the AE lock and bias controls inactive in
    manual mode
  media: i2c: vd55g1: Use the control handler lock for the subdev state

 drivers/media/i2c/vd55g1.c | 65 ++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

[PATCH v3 1/2] media: i2c: vd55g1: Mark the AE lock and bias controls inactive in manual mode

[Fernando Rimoli]

In manual exposure mode the auto exposure lock and bias controls have
no effect, and the driver marks them grabbed. The grabbed flag means a
control is temporarily locked, for instance while streaming, so
v4l2-compliance fails on it while the sensor is idle:

  fail: v4l2-test-controls.cpp(134): GRABBED flag set
  test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL
  test VIDIOC_QUERYCTRL: FAIL

A control that does not apply to the current configuration is what the
inactive flag is for. Use v4l2_ctrl_activate() instead.

Setting the controls in manual mode now stores the value rather than
returning -EBUSY. That has no effect on the sensor:
vd55g1_lock_exposure() writes only in auto mode, and the bias sets the
auto exposure target, which the sensor uses only once auto exposure
resumes.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 drivers/media/i2c/vd55g1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index e3939bb..69ffb3a 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -1503,8 +1503,8 @@ static int vd55g1_s_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_EXPOSURE_AUTO:
 		is_auto = (ctrl->val == V4L2_EXPOSURE_AUTO);
-		__v4l2_ctrl_grab(sensor->ae_lock_ctrl, !is_auto);
-		__v4l2_ctrl_grab(sensor->ae_bias_ctrl, !is_auto);
+		v4l2_ctrl_activate(sensor->ae_lock_ctrl, is_auto);
+		v4l2_ctrl_activate(sensor->ae_bias_ctrl, is_auto);
 		break;
 	case V4L2_CID_HDR_SENSOR_MODE:
 		/* Discriminate if the userspace changed the control value */
-- 
2.43.0

[PATCH v3 2/2] media: i2c: vd55g1: Use the control handler lock for the subdev state

[Fernando Rimoli]

vd55g1_subdev_init() sets sd.state_lock to ctrl_handler.lock before the
control handler is initialised, while the pointer is still NULL, so the
subdev state gets a mutex of its own. vd55g1_s_ctrl() under
VIDIOC_S_CTRL, and __v4l2_ctrl_modify_range() and
__v4l2_ctrl_handler_setup() under set_fmt and enable_streams, then run
without the lock they assert, and format and control changes can race.

Initialise the controls first and finalise last. The blanking ranges
depend on the format, so set them, and the default frame length, once
the active state exists. vd55g1_grab_ctrls() now runs with the lock
held, so use __v4l2_ctrl_grab().

With the control handler set, v4l2_subdev_init_finalize() also enables
control events, which v4l2-compliance reported missing.

Fixes: e56616d7b23c ("media: i2c: Add driver for ST VD55G1 camera sensor")
Cc: stable@vger.kernel.org
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 drivers/media/i2c/vd55g1.c | 61 +++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 69ffb3a..9f737ea 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -1113,10 +1113,10 @@ static int vd55g1_ro_ctrls_setup(struct vd55g1 *sensor, struct v4l2_rect *crop)
 static void vd55g1_grab_ctrls(struct vd55g1 *sensor, bool enable)
 {
 	/* These settings cannot change during stream */
-	v4l2_ctrl_grab(sensor->hflip_ctrl, enable);
-	v4l2_ctrl_grab(sensor->vflip_ctrl, enable);
-	v4l2_ctrl_grab(sensor->patgen_ctrl, enable);
-	v4l2_ctrl_grab(sensor->hdr_ctrl, enable);
+	__v4l2_ctrl_grab(sensor->hflip_ctrl, enable);
+	__v4l2_ctrl_grab(sensor->vflip_ctrl, enable);
+	__v4l2_ctrl_grab(sensor->patgen_ctrl, enable);
+	__v4l2_ctrl_grab(sensor->hdr_ctrl, enable);
 }
 
 static int vd55g1_enable_streams(struct v4l2_subdev *sd,
@@ -1580,14 +1580,6 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
 	struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler;
 	struct v4l2_ctrl *ctrl;
 	struct v4l2_fwnode_device_properties fwnode_props;
-	struct vd55g1_vblank_limits vblank;
-	unsigned int hblank;
-	struct v4l2_subdev_state *state =
-		v4l2_subdev_lock_and_get_active_state(&sensor->sd);
-	struct v4l2_rect *crop =
-		v4l2_subdev_state_get_crop(state, 0);
-	struct v4l2_mbus_framefmt *format =
-		v4l2_subdev_state_get_format(state, 0);
 	int ret;
 
 	v4l2_ctrl_handler_init(hdl, 16);
@@ -1645,15 +1637,13 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
 					     V4L2_CID_HDR_SENSOR_MODE,
 					     ARRAY_SIZE(vd55g1_hdr_menu) - 1, 0,
 					     VD55G1_NO_HDR, vd55g1_hdr_menu);
-	hblank = vd55g1_get_hblank_min(sensor, format, crop);
+	/* Placeholder ranges, set from the format in vd55g1_subdev_init() */
 	sensor->hblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
-						hblank, hblank, 1, hblank);
+						1, 1, 1, 1);
 	if (sensor->hblank_ctrl)
 		sensor->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	vd55g1_get_vblank_limits(sensor, crop, &vblank);
 	sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
-						vblank.min, vblank.max,
-						1, vblank.def);
+						1, 1, 1, 1);
 
 	/* Additional controls based on device tree properties */
 	if (sensor->ext_leds_mask) {
@@ -1673,12 +1663,11 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)
 		goto free_ctrls;
 
 	sensor->sd.ctrl_handler = hdl;
-	goto unlock_state;
+
+	return 0;
 
 free_ctrls:
 	v4l2_ctrl_handler_free(hdl);
-unlock_state:
-	v4l2_subdev_unlock_state(state);
 	return ret;
 }
 
@@ -1915,6 +1904,9 @@ static int vd55g1_parse_dt(struct vd55g1 *sensor)
 
 static int vd55g1_subdev_init(struct vd55g1 *sensor)
 {
+	struct v4l2_subdev_state *state;
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
 	int ret;
 
 	/* Init sub device */
@@ -1930,6 +1922,14 @@ static int vd55g1_subdev_init(struct vd55g1 *sensor)
 		return ret;
 	}
 
+	ret = vd55g1_init_ctrls(sensor);
+	if (ret) {
+		dev_err(sensor->dev, "Controls initialization failed %d\n",
+			ret);
+		goto err_media;
+	}
+
+	/* The state shares the control handler lock, so finalize last */
 	sensor->sd.state_lock = sensor->ctrl_handler.lock;
 	ret = v4l2_subdev_init_finalize(&sensor->sd);
 	if (ret) {
@@ -1937,19 +1937,24 @@ static int vd55g1_subdev_init(struct vd55g1 *sensor)
 		goto err_ctrls;
 	}
 
-	/*
-	 * Initialize controls after v4l2_subdev_init_finalize() to make sure
-	 * active state is set
-	 */
-	ret = vd55g1_init_ctrls(sensor);
+	/* Set the format dependent ranges and the default frame length */
+	state = v4l2_subdev_lock_and_get_active_state(&sensor->sd);
+	format = v4l2_subdev_state_get_format(state, 0);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	ret = vd55g1_new_format_change_controls(sensor, format, crop);
+	if (!ret)
+		ret = __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl,
+					 sensor->vblank_ctrl->default_value);
+	v4l2_subdev_unlock_state(state);
 	if (ret) {
-		dev_err(sensor->dev, "Controls initialization failed %d\n",
-			ret);
-		goto err_media;
+		dev_err(sensor->dev, "Controls update failed %d\n", ret);
+		goto err_state;
 	}
 
 	return 0;
 
+err_state:
+	v4l2_subdev_cleanup(&sensor->sd);
 err_ctrls:
 	v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
 
-- 
2.43.0