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

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

[Marco Ziliani via B4 Relay]

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. They are
only written on runtime PM resume, so a format change on a sensor that
stays powered never reaches the chip. 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], posted this
morning, makes the same change as part of a larger rework. Patch 1 is
kept here as a small standalone fix with a Fixes: tag, so that it can
be applied and backported on its own; should the rework land first, I
will rebase patches 2 and 3 on top of it.

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>
---
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 | 71 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 62 insertions(+), 9 deletions(-)
---
base-commit: d7e7bca7169873b46e857c025c10f3336c1258d8
change-id: 20261027-ov5693-fixes-8ec29752893a

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

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

[Marco Ziliani via B4 Relay]

From: Marco Ziliani <rylos78@gmail.com>

The mode registers (readout window, binning and subsampling) are only
written from the runtime PM resume handler, and ov5693_set_fmt() merely
stores the requested mode. If the sensor stays powered between two
streams, the new mode never reaches the chip: the sensor keeps
streaming the previous mode while the CSI-2 receiver expects the newly
negotiated format.

This happens whenever the format is changed while the sensor is still
powered. On a Surface Pro 7+ (IPU6), with the sensor held powered
between two streams, OUTPUT_SIZE_X read back over I2C showed the sensor
stuck in the mode it was powered up in, and 0 of 16 streams that asked
for a different mode produced a frame. With this change, 32 of 32
streams completed and the register followed the requested mode. A
Surface Pro 9 (IPU6) gave the same result: without the change
OUTPUT_SIZE stayed at 2592x1944 in all 32 streams of 16 such switches,
with it the register matched the requested size in all 32. On a
Surface Pro 5 (IPU3), switching between 2592x1944 and a 1920x1080 crop
the same way, OUTPUT_SIZE_X stayed at 2592 and none of the 16 streams
in the new size produced a frame; with this change, 32 of 32 streams
gave 10 of 10 frames at the requested size.

Program the mode on every stream start, before the controls, so that
the mode registers don't clobber them. This matches what commit
4e1446ac169a ("media: i2c: ov8865: Program the sensor on stream start")
does for the ov8865.

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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 56599f9ca..579aa55b7 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1000,6 +1000,19 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable)
 			return ret;
 
 		mutex_lock(&ov5693->lock);
+
+		/*
+		 * The mode registers are otherwise only written on runtime PM
+		 * resume, so a format change on a sensor that stays powered
+		 * would never reach the chip. Program the mode first, then the
+		 * controls, so 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

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

[Marco Ziliani via B4 Relay]

From: Marco Ziliani <rylos78@gmail.com>

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
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 579aa55b7..ca0968498 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 3/3] media: i2c: ov5693: Use autosuspend when stopping the stream

[Marco Ziliani via B4 Relay]

From: Marco Ziliani <rylos78@gmail.com>

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
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 ca0968498..ef5e7285c 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1070,7 +1070,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

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

[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 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:

- Patch 1: with the sensor held powered between streams, OUTPUT_SIZE
  (read over I2C during 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 binned streams gave no image.
- Patch 2: raw 2592x1944 frames are BGGR at all four hflip/vflip
  settings. Unpatched, the order is GBRG with exactly one flip on.
- Patch 3: with autosuspend_delay_ms at 3000, the sensor suspended
  3.4-3.7 s after stream stop (the delay plus the 25% timer slack),
  after both 1 s and 5 s streams. Unpatched, 1-2 ms after a 5 s stream.

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>

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

[Tony Garcia]

Tested on a Surface Pro 7 (Ice Lake). Its front OV5693 sits behind an
IPU4P, which I run with ruslanbay's IPU4P-on-IPU6 RFC series on
linux-surface 7.3.0-rc7. The three patches were applied to the 7.3
ov5693.c (they apply cleanly). Raw capture from the ISYS video node with
v4l2-ctl, frames saved and checked buffer by buffer (bytesused and pixel
data), no libcamera.

The patches tested were the ones posted in the linux-surface/kernel#181
thread. The code in this v1 is identical: applying each patch here and
each tested one to the same ov5693.c gives the same tree after every
patch.

Patch 1 (mode registers on stream start): with the sensor held powered
(power/control=on), 8 full->binned and 8 binned->full switches:
OUTPUT_SIZE_X (0x3808) read back over I2C during each stream matched the
requested width 16 of 16 times, and all 16 streams delivered frames. Out
of window (sensor runtime-suspended before each stream), 5 + 5 switches:
10 of 10. The unpatched driver, same in-window test: 0x3808 stayed at
2592 after each switch to 1296x972, and none of the 6 streams after the
first switch produced a frame.

Patch 2 (Bayer order with flips): 2592x1944 at all four hflip/vflip
settings, the sensor reported SBGGR10 and the 2x2 sub-plane test found
the green pair on the BGGR/RGGB diagonal in all four, with the same
blue/red plane order each time. Comparing image profiles against
hflip=0/vflip=0 confirmed each setting really mirrored the image. 45 of
45 buffers at each setting had full bytesused and non-constant data.

Patch 3 (autosuspend at stream stop): after a 60-frame stream, the
sensor stayed active for 1.2 s and was suspended at the 1.3 s sample
(100 ms sampling, 3 of 3 runs, 1 s autosuspend delay). With the old
pm_runtime_put() (my installed 7.3 driver, whose stop path is stock) it
was already suspended at the first sample (3 of 3).

Also 1296x972 binned: 10 module loads x 2 captures, 900 of 900 buffers
with full bytesused and real image data, no CSI-2 errors.

The tests were scripted and run on my Surface Pro 7 by an AI coding
assistant (Claude) at my direction, and it drafted this summary.

Tested-by: Tony Garcia <tony@loafecho.com>

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

[Dan Scally]

Morning Marco

On 27/10/2026 20:29, Marco Ziliani via B4 Relay wrote:
> From: Marco Ziliani <rylos78@gmail.com>
> 
> The mode registers (readout window, binning and subsampling) are only
> written from the runtime PM resume handler, and ov5693_set_fmt() merely
> stores the requested mode. If the sensor stays powered between two
> streams, the new mode never reaches the chip: the sensor keeps
> streaming the previous mode while the CSI-2 receiver expects the newly
> negotiated format.
> 
> This happens whenever the format is changed while the sensor is still
> powered. On a Surface Pro 7+ (IPU6), with the sensor held powered
> between two streams, OUTPUT_SIZE_X read back over I2C showed the sensor
> stuck in the mode it was powered up in, and 0 of 16 streams that asked
> for a different mode produced a frame. With this change, 32 of 32
> streams completed and the register followed the requested mode. A
> Surface Pro 9 (IPU6) gave the same result: without the change
> OUTPUT_SIZE stayed at 2592x1944 in all 32 streams of 16 such switches,
> with it the register matched the requested size in all 32. On a
> Surface Pro 5 (IPU3), switching between 2592x1944 and a 1920x1080 crop
> the same way, OUTPUT_SIZE_X stayed at 2592 and none of the 16 streams
> in the new size produced a frame; with this change, 32 of 32 streams
> gave 10 of 10 frames at the requested size.
> 
> Program the mode on every stream start, before the controls, so that
> the mode registers don't clobber them. This matches what commit
> 4e1446ac169a ("media: i2c: ov8865: Program the sensor on stream start")
> does for the ov8865.

This commit message is very long, most of this detail is not necessary:

"ov5693_mode_configure() is only called in the driver's PM .runtime_resume() hook. If the sensor 
remains powered between two streams with different modes, the settings for the second mode are never 
written to hardware.

Move the call to ov5693_mode_configure() to ov5693_s_stream() to ensure it's written every time."
> 
> 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 | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 56599f9ca..579aa55b7 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1000,6 +1000,19 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable)
>   			return ret;
>   
>   		mutex_lock(&ov5693->lock);
> +
> +		/*
> +		 * The mode registers are otherwise only written on runtime PM
> +		 * resume, so a format change on a sensor that stays powered
> +		 * would never reach the chip. Program the mode first, then the
> +		 * controls, so the mode registers don't clobber them.
> +		 */
> +		ret = ov5693_mode_configure(ov5693);
> +		if (ret) {
> +			mutex_unlock(&ov5693->lock);
> +			goto err_power_down;
> +		}

The same function call should come out of ov5693_sensor_init() then, or it'll be written twice on a 
stream on from an unpowered state.

Thanks
Dan
> +
>   		ret = __v4l2_ctrl_handler_setup(&ov5693->ctrls.handler);
>   		if (ret) {
>   			mutex_unlock(&ov5693->lock);
> 

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

[Dan Scally]

Morning

On 27/10/2026 20:29, Marco Ziliani via B4 Relay wrote:
> From: Marco Ziliani <rylos78@gmail.com>
> 
> 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
> 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 ca0968498..ef5e7285c 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1070,7 +1070,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);

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   
>   	return 0;
>   err_power_down:
> 

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

[Simon Kruse]

Hi Marco,

On Tue, Oct 27, 2026 at 09:29:44PM +0100, Marco Ziliani wrote:
> [PATCH 0/3] media: i2c: ov5693: Fix mode changes, flips and autosuspend

Thanks for Cc'ing me. Your 1/3 is the same change as the mode move in my
active-state conversion [1], and it stands better on its own, so I will
drop that part from my patch.

Unless anyone objects, I will rebase v2 of the conversion on top of
your whole series rather than the other way around. The conversion
then also moves your flip/Bayer window helpers from ov5693->mode to the
active state. v2 will keep the driver mutex as the state lock, as Dan
asked, and say in the cover letter that it depends on your series.

I will run your series on a Surface Pro 9 and reply with the result.

[1] https://lore.kernel.org/all/20261027074933.21921-1-simon0x+git@proton.me/

Regards,
Simon

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

[Dan Scally]

On 27/10/2026 20:29, Marco Ziliani via B4 Relay wrote:
> From: Marco Ziliani <rylos78@gmail.com>
> 
> 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)

Hm. But what's the right approach? To allow flips without changing the media bus format code by 
offsetting the output rectangle, or to have flips change the media bus format that the driver 
reports and flag the flip controls with V4L2_CTRL_FLAG_MODIFY_LAYOUT. At the moment that would mean 
that you couldn't set flips whilst streaming though, which is suboptimal.


; 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
> 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 579aa55b7..ca0968498 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,
> 

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

[Marco Ziliani]

Hi Simon,

On Wed, Oct 28, 2026 at 08:29:32AM +0000, Simon Kruse wrote:
> Unless anyone objects, I will rebase v2 of the conversion on top of
> your whole series rather than the other way around.

Thanks, that works for me. I'll post v2 with Dan's comments on patch 1
addressed (ov5693_mode_configure() moves out of ov5693_sensor_init()),
so please base the conversion on that one.

Regards,
Marco

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

[Marco Ziliani]

Hi Dan,

On 28/10/2026 09:31, Dan Scally wrote:
> Hm. But what's the right approach? To allow flips without changing the
> media bus format code by offsetting the output rectangle, or to have
> flips change the media bus format that the driver reports and flag the
> flip controls with V4L2_CTRL_FLAG_MODIFY_LAYOUT. At the moment that
> would mean that you couldn't set flips whilst streaming though, which
> is suboptimal.

I went for the offset because it keeps the flips usable while
streaming and keeps the format the driver has always reported, so
nothing changes for existing users. Other drivers do the same: ov13b10
and ov01a10 shift the window offset by one pixel when flipping to keep
the Bayer order, and in imx290 the sensor itself moves the readout by
one pixel for the same reason.

The cost is one extra column of the pixel array, which is why the crop
end is widened by two columns, clamped to the array. The reported
format, crop rectangle and size don't change, and the binned modes are
left alone since the flips don't move their Bayer phase.

If you and Sakari would rather have the MODIFY_LAYOUT approach, I can
switch to it in v2.

Regards,
Marco