Re: [PATCH v3 2/3] media: i2c: Add Sony IMX681 sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardware exposure register becomes out of sync with V4L2 state when VBLANK is changed.
- [Medium] Independent toggling of group hold register defeats atomic parameter updates.
--

commit 7db2a0f5a0754b78ccc388a5ff14535efd86e6ff
Author: Andre Gilerson <andre.gilerson@gmail.com>

media: i2c: Add Sony IMX681 sensor driver

This commit adds a new V4L2 I2C sensor driver for the Sony IMX681 camera,
which is used on the Surface Pro 11 for Business. It implements standard
functionality including register initialization, the streams API, and
runtime PM.

> diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
> new file mode 100644
> index 0000000000000..eba792ca18ef4
> --- /dev/null
> +++ b/drivers/media/i2c/imx681.c
[ ... ]
> +static int imx681_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct imx681 *imx681 = container_of(ctrl->handler, struct imx681,
> +					     ctrl_handler);
> +	s64 exposure_max;
> +	int pm_status;
> +	int ret = 0;
> +
> +	/* Update exposure max when VBLANK changes (even when not streaming) */
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		exposure_max = IMX681_HEIGHT + ctrl->val - IMX681_EXPOSURE_OFFSET;
> +		__v4l2_ctrl_modify_range(imx681->exposure,
> +					 IMX681_EXPOSURE_MIN, exposure_max,
> +					 1, IMX681_EXPOSURE_DEFAULT);
> +	}
> +
> +	/*
> +	 * 1 with a reference taken, 0 if the device is not active, or -EINVAL
> +	 * if runtime PM is unavailable. Only the 0 means there is nothing to
> +	 * do: without runtime PM the sensor is powered from probe and never
> +	 * suspended, so the write still has to go out - but no reference was
> +	 * taken then, and none may be dropped.
> +	 */
> +	pm_status = pm_runtime_get_if_active(imx681->dev);
> +	if (!pm_status)
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_VBLANK:
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
> +		cci_write(imx681->cci, IMX681_REG_FRAME_LENGTH,
> +			  IMX681_HEIGHT + ctrl->val, &ret);
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);

[Severity: Medium]
Does this leave the hardware exposure register out of sync with the V4L2
state?

When VBLANK is modified, the exposure control range is clamped earlier in
this function. However, if the old exposure value is clamped to a new, smaller
maximum, the newly clamped exposure value is never written to the hardware
because VBLANK and EXPOSURE are not clustered. This could leave the sensor's
hardware exposure register with an out-of-bounds value.

> +		dev_dbg(imx681->dev, "set frame_length: %d, ret=%d\n",
> +			IMX681_HEIGHT + ctrl->val, ret);
> +		break;
> +
> +	case V4L2_CID_EXPOSURE:
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
> +		cci_write(imx681->cci, IMX681_REG_EXPOSURE, ctrl->val, &ret);
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);

[Severity: Medium]
Does this independent toggling of the group hold register defeat its purpose?

Group hold is intended to defer register updates until a frame boundary to
ensure multiple settings apply atomically. By toggling it independently for
each V4L2 control, simultaneous user updates to VBLANK, EXPOSURE, and GAIN
will take effect individually and may split across different frames,
potentially causing temporary visual tearing or artifacts.

Would it be better to cluster these controls and use a single group hold block
when they are updated together?

> +		dev_dbg(imx681->dev, "set exposure: %d, ret=%d\n",
> +			ctrl->val, ret);
> +		break;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909203717.90605-1-lsa.uz@pm.me?part=2