[PATCH v2 0/3] media: i2c: ov5693: Fix mode changes, flips and autosuspend

[PATCH v2 0/3] media: i2c: ov5693: Fix mode changes, flips and autosuspend

[Marco Ziliani]

Three fixes for the ov5693, found while getting the front camera of the
Microsoft Surface Pro 5 (IPU3) to work, and since tested on IPU6 and
IPU4P Surfaces as well.

Patch 1 programs the mode registers on every stream start instead of
on runtime PM resume, so that a format change also reaches a sensor
that stays powered between streams. This is the ov5693 counterpart of
commit 4e1446ac169a ("media: i2c: ov8865: Program the sensor on stream
start").

Patch 2 keeps the reported SBGGR10 order when a single flip is enabled
at full resolution: each flip moves the Bayer phase by one column, so
the output window is shifted by one column when hflip XOR vflip.

Patch 3 drops the usage count at stream stop with
pm_runtime_put_autosuspend(), so that the autosuspend delay counts from
stream stop rather than from the last resume.

Patch 1 has been public and in testing in the linux-surface tree since
2026-10-23 [2]. Simon Kruse's active state conversion [1] made the same
change as part of a larger rework; he has since dropped it from his
patch and will rebase the conversion on top of this series.

Patches 1 and 2 were developed in the linux-surface tree and tested
there on a Surface Pro 5 (IPU3), a Surface Pro 7 (IPU4P) and a Surface
Pro 7+ and Pro 9 (IPU6); patch 3 follows a report by Dario Manresa in
the same thread [2]. The three patches as posted here have also been
run on a Surface Pro 7 (IPU4P) and a Surface Pro 9 (IPU6) there.

[1] https://lore.kernel.org/linux-media/20261027074933.21921-1-simon0x+git@proton.me/
[2] https://github.com/linux-surface/kernel/pull/181

Signed-off-by: Marco Ziliani <rylos78@gmail.com>
---
Changes in v2:
- Patch 1: move ov5693_mode_configure() from ov5693_sensor_init() to
  ov5693_s_stream() rather than calling it in both, and shorten the
  commit message (Dan)
- Patch 1: Tested-by tags not carried over, since the code changed
- Patch 2: unchanged; whether to shift the window or to use
  V4L2_CTRL_FLAG_MODIFY_LAYOUT is still being discussed on v1, happy
  to switch if that is preferred (Dan)
- Patches 2 and 3: collected Reviewed-by and Tested-by tags
- Retested the series on the Surface Pro 5 (IPU3)
- Link to v1: https://patch.msgid.link/20261027-ov5693-fixes-v1-0-3c8ab1ae104d@gmail.com

---
Marco Ziliani (3):
      media: i2c: ov5693: Program the mode registers on stream start
      media: i2c: ov5693: Keep the Bayer order when flipping at full resolution
      media: i2c: ov5693: Use autosuspend when stopping the stream

 drivers/media/i2c/ov5693.c | 76 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 62 insertions(+), 14 deletions(-)
---
base-commit: d7e7bca7169873b46e857c025c10f3336c1258d8
change-id: 20261027-ov5693-fixes-8ec29752893a

Best regards,
--  
Marco Ziliani <rylos78@gmail.com>

[PATCH v2 1/3] media: i2c: ov5693: Program the mode registers on stream start

[Marco Ziliani]

ov5693_mode_configure() is only called from the runtime PM resume path,
through ov5693_sensor_init(). If the sensor stays powered between two
streams with different modes, the settings for the second mode are never
written to the hardware.

Move the call from ov5693_sensor_init() to ov5693_s_stream(), before the
controls are applied, so that the mode is written on every stream start.

Fixes: 89aef879cb53 ("media: i2c: Add support for ov5693 sensor")
Link: https://github.com/linux-surface/kernel/pull/181
Signed-off-by: Marco Ziliani <rylos78@gmail.com>
---
 drivers/media/i2c/ov5693.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 56599f9ca..d09095c3e 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -663,11 +663,6 @@ static int ov5693_sensor_init(struct ov5693_device *ov5693)
 		return dev_err_probe(ov5693->dev, ret,
 				     "global settings error\n");
 
-	ret = ov5693_mode_configure(ov5693);
-	if (ret)
-		return dev_err_probe(ov5693->dev, ret,
-				     "mode configure error\n");
-
 	ret = ov5693_enable_streaming(ov5693, false);
 	if (ret)
 		dev_err(ov5693->dev, "stop streaming error\n");
@@ -1000,6 +995,19 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable)
 			return ret;
 
 		mutex_lock(&ov5693->lock);
+
+		/*
+		 * Program the mode on every stream start, so that a format
+		 * change also reaches a sensor that stayed powered since the
+		 * previous stream, and before the controls so that the mode
+		 * registers don't clobber them.
+		 */
+		ret = ov5693_mode_configure(ov5693);
+		if (ret) {
+			mutex_unlock(&ov5693->lock);
+			goto err_power_down;
+		}
+
 		ret = __v4l2_ctrl_handler_setup(&ov5693->ctrls.handler);
 		if (ret) {
 			mutex_unlock(&ov5693->lock);

-- 
2.56.0

Re: [PATCH v2 1/3] media: i2c: ov5693: Program the mode registers on stream start

[Chris Simpson]

Tested on a Surface Pro 9 (Intel Alder Lake-U, i7-1255U; IPU6, front
OV5693) in a 7.3-based kernel, with the v2 series applied to an
ov5693.c that matches the base-commit (d7e7bca71698) apart from the
pad ops' client_info argument, against the same file unpatched, using
raw captures from the ISYS video node:

- With the sensor held powered between streams, OUTPUT_SIZE (read
  over I2C during and after each stream) matched the requested size
  in 32 of 32 streams requesting 2592x1944 or 1296x972, and every
  stream delivered image data. Unpatched, it stayed at 2592x1944 in
  all 32 and the 1296x972 streams gave no frames.
- With each stream started from runtime suspend, the path v2
  changed, OUTPUT_SIZE matched the requested size in 12 of 12
  streams (as it did unpatched), all with complete image data.

The test scripts, the analysis of their output and this summary were
prepared with an AI coding assistant (Claude); I ran the tests on my
Surface Pro 9.

Tested-by: Chris Simpson <chris@nilk.net>

[PATCH v2 2/3] media: i2c: ov5693: Keep the Bayer order when flipping at full resolution

[Marco Ziliani]

In the full (unbinned) readout each of the horizontal and vertical
flips moves the Bayer phase by one column. With exactly one flip
enabled the CSI-2 receiver gets GBRG instead of the SBGGR10 the driver
reports, which gives a magenta and green image; with both enabled the
two shifts cancel out.

The crop start is only honoured in steps of two pixels, so compensate
by shifting the output window by one column when hflip XOR vflip, and
widen the crop by two columns, within the pixel array, so that the
window still fits. Reprogram the window when either flip changes. The
binned readout is left unchanged.

With this change, raw 2592x1944 frames come out BGGR at all four
hflip/vflip settings on the Surface Pro 5 (IPU3); the same change in
the linux-surface tree gave BGGR at all four settings on a Surface
Pro 7 (IPU4P) and a Surface Pro 9 (IPU6). Without it, the order is only
right with both flips off or both on.

Fixes: 89aef879cb53 ("media: i2c: Add support for ov5693 sensor")
Link: https://github.com/linux-surface/kernel/pull/181
Tested-by: Chris Simpson <chris@nilk.net>
Tested-by: Tony Garcia <tony@loafecho.com>
Signed-off-by: Marco Ziliani <rylos78@gmail.com>
---
 drivers/media/i2c/ov5693.c | 56 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index d09095c3e..9fedc087e 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -361,6 +361,49 @@ static const u8 ov5693_test_pattern_bits[] = {
 
 /* V4L2 Controls Functions */
 
+/*
+ * In the full (unbinned) readout each flip moves the Bayer phase by one
+ * column, so with exactly one of them enabled the CSI-2 receiver gets GBRG
+ * instead of the SBGGR10 the driver reports; with both enabled the two shifts
+ * cancel out. The crop start is only honoured in steps of two pixels, so
+ * compensate by shifting the output window by one column instead, and widen
+ * the crop so that the window still fits.
+ */
+static bool ov5693_bayer_shift(struct ov5693_device *ov5693)
+{
+	const struct ov5693_mode *mode = &ov5693->mode;
+	bool hflip, vflip;
+
+	if (mode->binning_x || mode->binning_y)
+		return false;
+
+	hflip = ov5693->ctrls.hflip && ov5693->ctrls.hflip->val;
+	vflip = ov5693->ctrls.vflip && ov5693->ctrls.vflip->val;
+
+	return hflip ^ vflip;
+}
+
+static int ov5693_window_x_configure(struct ov5693_device *ov5693)
+{
+	const struct ov5693_mode *mode = &ov5693->mode;
+	bool shift = ov5693_bayer_shift(ov5693);
+	u32 crop_end = mode->crop.left + mode->crop.width;
+	int ret = 0;
+
+	/* Widen the crop for the shifted window, within the pixel array */
+	if (shift)
+		crop_end = min(crop_end + 2,
+			       max_t(u32, crop_end, OV5693_NATIVE_WIDTH - 1));
+
+	/* Offset X */
+	cci_write(ov5693->regmap, OV5693_OFFSET_START_X_REG, shift, &ret);
+
+	/* Crop End X */
+	cci_write(ov5693->regmap, OV5693_CROP_END_X_REG, crop_end, &ret);
+
+	return ret;
+}
+
 static int ov5693_flip_vert_configure(struct ov5693_device *ov5693,
 				      bool enable)
 {
@@ -373,7 +416,7 @@ static int ov5693_flip_vert_configure(struct ov5693_device *ov5693,
 	if (ret)
 		return ret;
 
-	return 0;
+	return ov5693_window_x_configure(ov5693);
 }
 
 static int ov5693_flip_horz_configure(struct ov5693_device *ov5693,
@@ -388,7 +431,7 @@ static int ov5693_flip_horz_configure(struct ov5693_device *ov5693,
 	if (ret)
 		return ret;
 
-	return 0;
+	return ov5693_window_x_configure(ov5693);
 }
 
 static int ov5693_get_exposure(struct ov5693_device *ov5693, s32 *value)
@@ -566,16 +609,13 @@ static int ov5693_mode_configure(struct ov5693_device *ov5693)
 	cci_write(ov5693->regmap, OV5693_CROP_START_X_REG, mode->crop.left,
 		  &ret);
 
-	/* Offset X */
-	cci_write(ov5693->regmap, OV5693_OFFSET_START_X_REG, 0, &ret);
-
 	/* Output Size X */
 	cci_write(ov5693->regmap, OV5693_OUTPUT_SIZE_X_REG, mode->format.width,
 		  &ret);
 
-	/* Crop End X */
-	cci_write(ov5693->regmap, OV5693_CROP_END_X_REG,
-		  mode->crop.left + mode->crop.width, &ret);
+	/* Offset X and Crop End X follow the flips, see ov5693_bayer_shift() */
+	if (!ret)
+		ret = ov5693_window_x_configure(ov5693);
 
 	/* Horizontal Total Size */
 	cci_write(ov5693->regmap, OV5693_TIMING_HTS_REG, OV5693_FIXED_PPL,

-- 
2.56.0

[PATCH v2 3/3] media: i2c: ov5693: Use autosuspend when stopping the stream

[Marco Ziliani]

The driver enables runtime PM autosuspend with a 1 second delay at
probe, but drops the usage count at stream stop with pm_runtime_put(),
which doesn't update the device's last busy time. The autosuspend
delay is then counted from the runtime resume at stream start rather
than from stream stop, so after any stream longer than the delay the
sensor is powered off right away.

Use pm_runtime_put_autosuspend(), which marks the device busy, so that
the delay counts from stream stop as intended, and a following stream
started within it doesn't power the sensor up from scratch again. With
the mode now programmed on every stream start, keeping the sensor
powered between streams no longer leaves it in a stale mode.

On a Surface Pro 9 (IPU6) with autosuspend_delay_ms set to 3000, the
sensor suspended 1-2 ms after a 5 s stream stopped; with this change,
3.7 s after it, i.e. the delay plus the autosuspend timer slack. On a
Surface Pro 5 (IPU3) with the default 1 s delay, the sensor suspended
as soon as a 2 s stream stopped; with this change, 1.1 s after it.

Fixes: 89aef879cb53 ("media: i2c: Add support for ov5693 sensor")
Reported-by: D. Manresa <dmanresa@gmail.com>
Closes: https://github.com/linux-surface/kernel/pull/181#issuecomment-6436016335
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Tested-by: Chris Simpson <chris@nilk.net>
Tested-by: Tony Garcia <tony@loafecho.com>
Signed-off-by: Marco Ziliani <rylos78@gmail.com>
---
 drivers/media/i2c/ov5693.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 9fedc087e..086f9df07 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1065,7 +1065,7 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable)
 		goto err_power_down;
 
 	if (!enable)
-		pm_runtime_put(ov5693->dev);
+		pm_runtime_put_autosuspend(ov5693->dev);
 
 	return 0;
 err_power_down:

-- 
2.56.0