[PATCH 0/2] media: i2c: vd55g1: Two v4l2-compliance fixes for controls

[PATCH 0/2] media: i2c: vd55g1: Two v4l2-compliance fixes for controls

[Fernando Rimoli]

Two control fixes for failures v4l2-compliance reports on the sensor
subdev.

1/2: In manual exposure mode the driver marks the auto exposure lock and
bias controls grabbed. Outside streaming that fails the QUERYCTRL and
QUERY_EXT_CTRL tests, and because v4l2-compliance switches the exposure
mode while it walks the controls, the S_CTRL tests also fail with -EBUSY
when it starts from auto mode. The controls do not apply in manual mode,
which is what the inactive flag is for.

2/2: Control events, as most sensor drivers have.

How these were tested, since it is not the obvious way: I do not have a
VD55G1. I have the VD55G0 of a Surface Pro 9 (Intel IPU6), which runs an
out-of-tree driver derived from vd55g1 through Peter Marshall's VD55G0
support. In the code these patches touch, the exposure control handling
and the subdev ops, it is identical to vd55g1 in media next. I applied
the same two changes there. With v4l2-compliance 1.26.1 on the subdev:

  before:      41/44 in both exposure modes
  after 1/2:   QUERYCTRL, QUERY_EXT_CTRL and S_CTRL pass, controls show
               "inactive" rather than "grabbed" in manual mode, and the
               bias can be set there
  after both:  44/44 in both exposure modes

Getting from 43 to 44 also needed a fix to that driver's own flash
duty-cycle cap, which vd55g1 does not have (it is not upstream), so I
mention it only so the numbers add up. Streaming is unchanged, 150 of
150 frames at 57.60 fps before and after.

Both patches build without warnings at W=1 against v7.3-rc1, the same as
the unpatched file, and apply to media next as of d7e7bca71698. The file
there has not changed since 4a069a763e8a.

Fernando Rimoli (2):
  media: i2c: vd55g1: Mark the AE lock and bias controls inactive in
    manual mode
  media: i2c: vd55g1: Support control events

 drivers/media/i2c/vd55g1.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

base-commit: d7e7bca7169873b46e857c025c10f3336c1258d8
-- 
2.43.0

[PATCH 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.

Fixes: e56616d7b23c ("media: i2c: Add driver for ST VD55G1 camera sensor")
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 2/2] media: i2c: vd55g1: Support control events

[Fernando Rimoli]

The sensor subdev exposes controls but does not implement the control
event operations, so VIDIOC_SUBSCRIBE_EVENT fails and v4l2-compliance
reports:

  fail: v4l2-test-controls.cpp(1108): subscribe event for control 'User Controls' failed
  test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL

Add the standard control event subdev operations and set
V4L2_SUBDEV_FL_HAS_EVENTS.

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

diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
index 69ffb3a..162c2d5 100644
--- a/drivers/media/i2c/vd55g1.c
+++ b/drivers/media/i2c/vd55g1.c
@@ -1447,7 +1447,13 @@ static const struct v4l2_subdev_video_ops vd55g1_video_ops = {
 	.s_stream = v4l2_subdev_s_stream_helper,
 };
 
+static const struct v4l2_subdev_core_ops vd55g1_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_ops vd55g1_subdev_ops = {
+	.core = &vd55g1_core_ops,
 	.video = &vd55g1_video_ops,
 	.pad = &vd55g1_pad_ops,
 };
@@ -1918,7 +1924,8 @@ static int vd55g1_subdev_init(struct vd55g1 *sensor)
 	int ret;
 
 	/* Init sub device */
-	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			    V4L2_SUBDEV_FL_HAS_EVENTS;
 	sensor->sd.internal_ops = &vd55g1_internal_ops;
 
 	/* Init source pad */
-- 
2.43.0