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

[PATCH v2 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 marks the auto exposure lock and bias controls inactive
rather than grabbed in manual exposure mode, and 2/2 adds control
events.

Changes since v1:
 - 1/2: dropped the Fixes tag (Benjamin). No code changes.

Testing is as in v1. I do not have a VD55G1, so the changes were tested
on the VD55G0 of a Surface Pro 9 (IPU6) through an out-of-tree driver
derived from vd55g1, identical in the code these patches touch. With
them, the QUERYCTRL, QUERY_EXT_CTRL, S_CTRL and control event tests
pass on the subdev, where they failed before.

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: Support control events

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

base-commit: 27285b099fbc5cb2ae3aef20f3bbd7b3c88b3005
-- 
2.43.0

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

Re: [PATCH v2 2/2] media: i2c: vd55g1: Support control events

[Sakari Ailus]

Hi Fernando,

Thanks for the set.

On Sun, Nov 01, 2026 at 07:50:05PM +0000, Fernando Rimoli wrote:
> 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,
> +};

Instead of doing this, can you make v4l2_subdev_init_finalize() the last
step of the sub-device initialisation? It enables events if the control
handler has been initialised for the sub-device.

-- 
Regards,

Sakari Ailus

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

[Sakari Ailus]

Hi Fernando, Hans,

On Sun, Nov 01, 2026 at 07:50:04PM +0000, Fernando Rimoli wrote:
> 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

Hans: v4l-utils commit 0a4d4accea726 adds a check for this but the commit
message doesn't explain why. I don't think grabbing a control is a bug as
such; it's normal that if exposure is set to auto, you can't access the
manual exposure controls.

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

-- 
Regards,

Sakari Ailus

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

[Fernando Rimoli]

Hi Sakari,

On Mon, Nov 02, 2026 at 02:08:40PM +0200, Sakari Ailus wrote:
> I don't think grabbing a control is a bug as such; it's normal that if
> exposure is set to auto, you can't access the manual exposure controls.

For that case the framework itself uses inactive rather than grabbed:
v4l2_ctrl_auto_cluster() sets V4L2_CTRL_FLAG_INACTIVE on the manual
controls while the cluster is in auto, and vd55g1 already uses it for
its exposure cluster. The AE lock and bias are the mirror image,
auto-only controls in manual mode, so this patch makes them follow the
same rule.

That also matches the flag descriptions in vidioc-queryctrl.rst: grabbed
is "temporarily unchangeable, for example because another application
took over control of the respective resource", inactive is "not
applicable to the current configuration".

vd56g3 has the same two __v4l2_ctrl_grab() calls. If this goes in, I can
send the same change for it.

Fernando

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

[Sakari Ailus]

Hi Fernando,

On Tue, Nov 03, 2026 at 10:49:58AM +0000, Fernando Rimoli wrote:
> Hi Sakari,
> 
> On Mon, Nov 02, 2026 at 02:08:40PM +0200, Sakari Ailus wrote:
> > I don't think grabbing a control is a bug as such; it's normal that if
> > exposure is set to auto, you can't access the manual exposure controls.
> 
> For that case the framework itself uses inactive rather than grabbed:
> v4l2_ctrl_auto_cluster() sets V4L2_CTRL_FLAG_INACTIVE on the manual
> controls while the cluster is in auto, and vd55g1 already uses it for
> its exposure cluster. The AE lock and bias are the mirror image,
> auto-only controls in manual mode, so this patch makes them follow the
> same rule.
> 
> That also matches the flag descriptions in vidioc-queryctrl.rst: grabbed
> is "temporarily unchangeable, for example because another application
> took over control of the respective resource", inactive is "not
> applicable to the current configuration".
> 
> vd56g3 has the same two __v4l2_ctrl_grab() calls. If this goes in, I can
> send the same change for it.

Right, indeed that sounds reasonable. The drivers that grab controls today
do it for the duration of streaming so the use case is a little bit
different.

-- 
Regards,

Sakari Ailus