* [PATCH v2 0/2] media: i2c: Surface Pro 7+ camera flip fixes
@ 2026-07-29 15:41 Jakob Berg Jespersen
2026-07-29 15:41 ` [PATCH v2 1/2] media: i2c: ov8865: fix horizontal flip control polarity Jakob Berg Jespersen
2026-07-29 15:41 ` [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase Jakob Berg Jespersen
0 siblings, 2 replies; 7+ messages in thread
From: Jakob Berg Jespersen @ 2026-07-29 15:41 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab,
Kévin L'hôpital, Paul Kocialkowski, Daniel Scally,
Jean-Michel Hautbois
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Daniel Scally,
Bryan O'Donoghue, Tooraj Taraz, Joseph V. Lavigne, stable,
Hans de Goede, Jakob Berg Jespersen, Fernando Rimoli
This series fixes an inverted horizontal-flip control on the two
OmniVision sensors of the Microsoft Surface Pro 7+ (front ov5693 and
rear ov8865). Both sensors are mounted with a 180 degree rotation (ACPI
SSDB rotation 180), so libcamera requests HFLIP=1+VFLIP=1 to correct the
mount and, with the inverted control, gets a mirrored image instead. The
ov5693 patch additionally compensates the Bayer phase, which shifts one
column when the flip bits are cleared.
The inverted-HFLIP behaviour has been confirmed on three platforms: the
Surface Pro 7+ (here), a Surface Pro 9 (Fernando Rimoli), and a Surface
Go (Hans de Goede).
1/2 media: i2c: ov8865: fix horizontal flip control polarity
2/2 media: i2c: ov5693: fix horizontal flip polarity and Bayer phase
Note: bringing these cameras up on the Surface Pro 7+ needs two
prerequisites this series does not itself depend on -- it concerns only
image orientation and colour. The ov5693 front camera streams on IPU6
only with a MIPI clock-lane-gate write being added by Fernando Rimoli
[1]; and the ov8865 rear camera powers on only with an int3472 POWER1
regulator mapping I posted separately [2].
I discovered these issues independently, though the motivation to work
on the Surface cameras came from the linux-surface community. I'd like
to credit two parallel efforts there on the same hardware family: Tooraj
Taraz observed the inverted feed while adding Surface Pro 9 camera
support out of tree [3], and Joseph V. Lavigne posted an RFC enabling
the ov8865 power supply on the Surface Pro 7+ itself [4]. I have not
drawn on their code here.
Thanks to Sakari, Hans, Bryan and Fernando for the review, testing and
suggestions on v1.
[1] https://lore.kernel.org/linux-media/20260720163819.104130-1-fernandorimoli11@gmail.com/
[2] https://lore.kernel.org/platform-driver-x86/20260729-sp7plus-int3472-v2-1-cdfaf97ac3ad@berg.pm/
[3] https://github.com/linux-surface/linux-surface/pull/1867
[4] https://github.com/linux-surface/linux-surface/pull/2201
---
Changes in v2:
- Simplified both commit messages and the code comments (Sakari, Hans).
- ov5693: dropped the "like the ov8865" cross-reference; made the
message self-contained (Hans).
- Added Fixes: and Cc: stable to both patches (Bryan).
- Picked up Reviewed-by (Hans, both) and Tested-by (Fernando, ov5693).
- Dropped the RFC prefix.
- Retested on the Surface Pro 7+ (7.1.5): both cameras correctly
oriented; ov5693 colours correct (Bayer compensation).
- Link to v1: https://patch.msgid.link/20260720-sp7plus-ov-flips-v1-0-5f345b0673de@berg.pm
To: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>
To: Kévin L'hôpital <kevin.lhopital@bootlin.com>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Daniel Scally <dan.scally@ideasonboard.com>
To: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Daniel Scally <djrscally@gmail.com>
Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Tooraj Taraz <tooraj.taraz@yahoo.com>
Cc: Joseph V. Lavigne <jlavig88@gmail.com>
---
Jakob Berg Jespersen (2):
media: i2c: ov8865: fix horizontal flip control polarity
media: i2c: ov5693: fix horizontal flip polarity and Bayer phase
drivers/media/i2c/ov5693.c | 12 +++++++++++-
drivers/media/i2c/ov8865.c | 3 ++-
2 files changed, 13 insertions(+), 2 deletions(-)
---
base-commit: a52e6f7923c17a672135b485ffd96fbd72f46267
change-id: 20260720-sp7plus-ov-flips-b6cc645236b6
Best regards,
--
Jakob Berg Jespersen <dev@berg.pm>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] media: i2c: ov8865: fix horizontal flip control polarity
2026-07-29 15:41 [PATCH v2 0/2] media: i2c: Surface Pro 7+ camera flip fixes Jakob Berg Jespersen
@ 2026-07-29 15:41 ` Jakob Berg Jespersen
2026-08-28 23:18 ` D. Manresa
2026-07-29 15:41 ` [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase Jakob Berg Jespersen
1 sibling, 1 reply; 7+ messages in thread
From: Jakob Berg Jespersen @ 2026-07-29 15:41 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab,
Kévin L'hôpital, Paul Kocialkowski, Daniel Scally,
Jean-Michel Hautbois
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Daniel Scally,
Bryan O'Donoghue, Tooraj Taraz, Joseph V. Lavigne, stable,
Hans de Goede, Jakob Berg Jespersen
The sensor's native readout is horizontally mirrored and the FORMAT2
FLIP_HORZ bits (reg 0x3821) un-mirror it. The driver sets those bits for
V4L2_CID_HFLIP=1, so the control is inverted: HFLIP=1 yields the
un-mirrored image and HFLIP=0 the mirrored one, the opposite of what is
requested.
Invert the polarity so HFLIP=0 yields the unflipped image.
Found on the Surface Pro 7+ rear camera (180 degree mount), where
libcamera's HFLIP=1+VFLIP=1 correction otherwise produces a mirrored
image.
Fixes: 11c0d8fdccc5 ("media: i2c: Add support for the OV8865 image sensor")
Cc: stable@vger.kernel.org
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Jakob Berg Jespersen <dev@berg.pm>
---
drivers/media/i2c/ov8865.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index c6d53c3d55ca..ccda33839246 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2204,8 +2204,9 @@ static int ov8865_flip_horz_configure(struct ov8865_sensor *sensor, bool enable)
u8 bits = OV8865_FORMAT2_FLIP_HORZ_ISP_EN |
OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN;
+ /* HFLIP is inverted on this sensor: the FLIP_HORZ bits un-mirror the readout. */
return ov8865_update_bits(sensor, OV8865_FORMAT2_REG, bits,
- enable ? bits : 0);
+ enable ? 0 : bits);
}
/* Test Pattern */
--
2.55.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase
2026-07-29 15:41 [PATCH v2 0/2] media: i2c: Surface Pro 7+ camera flip fixes Jakob Berg Jespersen
2026-07-29 15:41 ` [PATCH v2 1/2] media: i2c: ov8865: fix horizontal flip control polarity Jakob Berg Jespersen
@ 2026-07-29 15:41 ` Jakob Berg Jespersen
2026-08-28 23:18 ` D. Manresa
1 sibling, 1 reply; 7+ messages in thread
From: Jakob Berg Jespersen @ 2026-07-29 15:41 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab,
Kévin L'hôpital, Paul Kocialkowski, Daniel Scally,
Jean-Michel Hautbois
Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, Daniel Scally,
Bryan O'Donoghue, Tooraj Taraz, Joseph V. Lavigne, stable,
Fernando Rimoli, Hans de Goede, Jakob Berg Jespersen
The sensor's native readout is horizontally mirrored and the FORMAT2
FLIP_HORZ bits (reg 0x3821) un-mirror it; the init table sets them by
default (0x3821 = 0x1e). The driver sets those bits for V4L2_CID_HFLIP=1,
so the control is inverted: HFLIP=1 yields the un-mirrored image and
HFLIP=0 the mirrored one. Invert the polarity so HFLIP=0 yields the
unflipped image.
Clearing the bits also shifts the Bayer phase one column off the fixed
SBGGR10 mbus code the driver reports, giving wrong colors in the HFLIP=1
state. Compensate by offsetting the output window (reg 0x3810) one column
in that state; the crop has a spare column since crop end = crop start +
width (inclusive).
Found on the Surface Pro 7+ front camera (180 degree mount); geometry and
colors verified correct in both flip states.
Fixes: 89aef879cb53 ("media: i2c: Add support for ov5693 sensor")
Cc: stable@vger.kernel.org
Tested-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Jakob Berg Jespersen <dev@berg.pm>
---
drivers/media/i2c/ov5693.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 4cc796bbee92..4b016822d561 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -375,8 +375,18 @@ static int ov5693_flip_horz_configure(struct ov5693_device *ov5693,
OV5693_FORMAT2_FLIP_HORZ_SENSOR_EN;
int ret;
+ /* HFLIP is inverted on this sensor: the FLIP_HORZ bits un-mirror the readout. */
ret = cci_update_bits(ov5693->regmap, OV5693_FORMAT2_REG, bits,
- enable ? bits : 0, NULL);
+ enable ? 0 : bits, NULL);
+ if (ret)
+ return ret;
+
+ /*
+ * Clearing the bits shifts the Bayer phase one column off the
+ * reported SBGGR10; offset the output window to compensate.
+ */
+ ret = cci_write(ov5693->regmap, OV5693_OFFSET_START_X_REG,
+ enable ? 1 : 0, NULL);
if (ret)
return ret;
--
2.55.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] media: i2c: ov8865: fix horizontal flip control polarity
2026-07-29 15:41 ` [PATCH v2 1/2] media: i2c: ov8865: fix horizontal flip control polarity Jakob Berg Jespersen
@ 2026-08-28 23:18 ` D. Manresa
0 siblings, 0 replies; 7+ messages in thread
From: D. Manresa @ 2026-08-28 23:18 UTC (permalink / raw)
To: Jakob Berg Jespersen
Cc: Sakari Ailus, Mauro Carvalho Chehab,
Kévin L'hôpital, Paul Kocialkowski, Daniel Scally,
Jean-Michel Hautbois, Hans de Goede, linux-media, linux-kernel,
stable, D . Manresa
On Wed, 29 Jul 2026, Jakob Berg Jespersen wrote:
> The sensor's native readout is horizontally mirrored and the FORMAT2
> FLIP_HORZ bits (reg 0x3821) un-mirror it. The driver sets those bits for
> V4L2_CID_HFLIP=1, so the control is inverted: HFLIP=1 yields the
> un-mirrored image and HFLIP=0 the mirrored one, the opposite of what is
> requested.
Tested on the rear camera of a Surface Pro 7+ (OV8865 behind an IPU6,
patch applied on v6.19 sources, module built out of tree and loaded on a
linux-surface 6.19.8 kernel). Verified from raw captures at 3264x2448 and
1632x1224:
- HFLIP now toggles the horizontal mirror as requested (verified against a
fixed scene landmark and by column-profile reversal of the raw frames);
- the Bayer order stays BGGR, matching the reported SBGGR10, in every flip
state I could produce (HFLIP/VFLIP 0/0, 1/0, 0/1, 1/1, both readout
sizes) - so no phase compensation is needed on this sensor and existing
colour tunings are unaffected;
- with the fix, libcamera's HFLIP=1 + VFLIP=1 correction for the 180
degree SSDB mount now yields a true 180 degree rotation: the raw frame in
that state is an exact point rotation of the HFLIP=0/VFLIP=0 frame,
where before the series the same request produced the horizontally
mirrored image.
Tested-by: D. Manresa <dmanresa@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase
2026-07-29 15:41 ` [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase Jakob Berg Jespersen
@ 2026-08-28 23:18 ` D. Manresa
2026-08-29 13:21 ` Jakob Berg Jespersen
0 siblings, 1 reply; 7+ messages in thread
From: D. Manresa @ 2026-08-28 23:18 UTC (permalink / raw)
To: Jakob Berg Jespersen
Cc: Sakari Ailus, Mauro Carvalho Chehab,
Kévin L'hôpital, Paul Kocialkowski, Daniel Scally,
Jean-Michel Hautbois, Hans de Goede, Fernando Rimoli, linux-media,
linux-kernel, stable
On Wed, 29 Jul 2026, Jakob Berg Jespersen wrote:
> The sensor's native readout is horizontally mirrored and the FORMAT2
> FLIP_HORZ bits (reg 0x3821) un-mirror it; the init table sets them by
> default (0x3821 = 0x1e). The driver sets those bits for V4L2_CID_HFLIP=1,
> so the control is inverted: HFLIP=1 yields the un-mirrored image and
> HFLIP=0 the mirrored one. Invert the polarity so HFLIP=0 yields the
> unflipped image.
Tested on the front camera of a Surface Pro 7+ (OV5693 behind an IPU6,
patch applied on v6.19 sources, loaded on a linux-surface 6.19.8 kernel),
Bayer phases read from raw 2592x1944 captures. Partial results - the
polarity fix checks out, the 0x3810 compensation appears to sit on the
wrong flip state on my unit:
1) The polarity inversion is correct. Independent confirmation: the
register tables inside the vendor Windows driver (ov5693.sys from the
Surface MSI) keep the 0x3821 FLIP_HORZ bits set in every mode
(0x3821 = 0x1e/0x1f), and Windows delivers the un-mirrored image; with
your patch HFLIP=0 keeps them set, as the init table intends. HFLIP
also toggles the mirror geometry correctly in both directions here.
2) The 0x3810 write does what the commit message says in the relative
sense: the two flip states come out with the SAME Bayer phase, so
toggling HFLIP no longer changes the colours.
3) However, on my unit BOTH states then decode as GBRG, one column off the
reported SBGGR10. I measured the four register combinations
independently (phase identified from raw frames by green-pair
statistics and confirmed by demosaicing a known-colour scene under each
hypothesis):
FLIP_HORZ bits set + 0x3810=0 (your HFLIP=0): GBRG
FLIP_HORZ bits cleared + 0x3810=1 (your HFLIP=1): GBRG
FLIP_HORZ bits cleared + 0x3810=0: BGGR (clean)
FLIP_HORZ bits set + 0x3810=1: does not stream
i.e. here it is the un-mirrored readout (bits set) that carries the
one-column phase shift, and the mirrored readout that is SBGGR-clean -
the opposite of what the patch compensates. The last row is why the
compensation cannot simply be moved to the other state: with the
FLIP_HORZ bits set the sensor refuses to stream with a one-column
window offset (perpetual "Frame sync error" on the IPU6 CSI-2
receiver, reproduced across repeated attempts); shifting the crop
window by one column instead should work, but I have not tested that.
4) For completeness: in the 2x2-binned readout the Surface uses for
video (a downstream patch of mine, not in mainline), the mirror does
not move the Bayer phase at all - same behaviour I measured on the
OV8865 - so there the 0x3810 parity must stay constant across flip
states.
Since you and Fernando verified colours correct in both flip states on
your units, and point 3 is exactly the opposite assignment, maybe the
two of us are not decoding the same thing - or the modules differ. Could
you double-check the absolute phase at HFLIP=0 on your unit from a raw
capture of a known-colour scene (not through an ISP that may be
auto-correcting, and not with the sensor's test pattern - on my unit the
colour-bar generator is inserted after the flip/window stage and shows
the same order and phase in every flip state, so it cannot see this)?
Happy to test a v3. For patch 1/2 of this series everything checks out
on my unit; sent a Tested-by there separately.
D. Manresa <dmanresa@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase
2026-08-28 23:18 ` D. Manresa
@ 2026-08-29 13:21 ` Jakob Berg Jespersen
2026-09-03 12:57 ` D. Manresa
0 siblings, 1 reply; 7+ messages in thread
From: Jakob Berg Jespersen @ 2026-08-29 13:21 UTC (permalink / raw)
To: D. Manresa
Cc: Sakari Ailus, Mauro Carvalho Chehab,
Kévin L'hôpital, Paul Kocialkowski, Daniel Scally,
Jean-Michel Hautbois, Hans de Goede, Fernando Rimoli, linux-media,
linux-kernel, stable
Thanks for the careful measurement. You are right, and 2/2 should not be
applied. It took me a while to figure out what I had missed before I
could reproduce your table on my own SP7+. I had not been careful to
track vertical flip. My original validation went through a libcamera
preview rather than raw frames, and libcamera leaves VFLIP set on this
machine, so I had only ever looked at the one state in which the patch
happens to be correct.
So at VFLIP=0 the compensation sits on the wrong flip state.
2/2 takes a default that was color-correct and leaves it one column off
in both flip states. This 2/2 patch should be dropped.
1/2 is unaffected and can be applied.
I have collected detailed information from my testing here:
Raw SBGGR10 frames off the IPU6 CSI-2 receiver (/dev/video32), no ISP in
the path, no test pattern, a red card as the main subject. Full
resolution 2592x1944. Crop start X (0x3800/0x3801) reads 0x0010.
The FLIP_HORZ bits and the ISP offset were written over i2c while
streaming. They have to be: ov5693_mode_configure() writes
OFFSET_START_X = 0 at every stream start.
Phase identified by green-pair statistics plus a demosaic check. As a
control I repeated the sweep with a blue object and the opposite
assumption about which channel dominates. Every label came out the same,
and the two scenes put blue at the same site. For VFLIP=1 I also
rendered the frame under both candidate patterns and looked at it, which
is unambiguous.
RESULTS, VERTICAL FLIP OFF
My unit gives exactly your four rows:
FLIP_HORZ set + 0x3810/11 = 0 (patch's HFLIP=0): GBRG
FLIP_HORZ cleared + 0x3810/11 = 1 (patch's HFLIP=1): GBRG
FLIP_HORZ cleared + 0x3810/11 = 0: BGGR (clean)
FLIP_HORZ set + 0x3810/11 = 1: does not stream
(IPU6 "csi2-4 error: Frame sync error")
So the compensation is on the wrong flip state, as you found. The state
it would have to move to is the one that will not stream.
This is visible without trusting my analysis. Render both frames as
BGGR, which is what the driver tells userspace to do: the pre-patch
default (bits cleared) looks natural, the patched default (bits set) has
a magenta cast. Red and blue swapped, which is what a one-column phase
error does.
RESULTS, VERTICAL FLIP ON
My earlier testing was not done at VFLIP=0, and that is why it disagreed
with yours. V4L2 flip controls persist on the subdev between openers,
and libcamera leaves vertical_flip=1 here for the 180 degree SSDB mount.
My raw captures inherited it. With VFLIP=1 the same four combinations
invert:
FLIP_HORZ set + offset 0: BGGR (clean)
FLIP_HORZ cleared + offset 0: GBRG
FLIP_HORZ cleared + offset 1: BGGR (clean)
FLIP_HORZ set + offset 1: does not stream
So the absolute phase depends on both flips, and no compensation keyed
on HFLIP alone can be right in both vertical states. VFLIP=1 is the
state my patch was written for without my realizing it, and the state
libcamera uses here. That is why it looked correct to me and to
Fernando.
One oddity I cannot explain: the vertical flip moves the phase by a
column, not a row. This does not depend on my red/blue naming. Under the
opposite naming the same transition reads GRBG -> RGGB, also a column
shift, and neither is the row shift a plain vertical mirror would give.
I checked that the vertical flip really does mirror vertically by
rendering both frames. I report it as measured; I have no mechanism to
offer for it.
WHAT 2/2 ACTUALLY DOES
Per vertical state:
VFLIP=0: before the patch HFLIP=0 -> BGGR (clean), HFLIP=1 -> GBRG
after the patch HFLIP=0 -> GBRG, HFLIP=1 -> GBRG
VFLIP=1: before the patch HFLIP=0 -> GBRG, HFLIP=1 -> BGGR
after the patch HFLIP=0 -> BGGR (clean), HFLIP=1 -> BGGR
At VFLIP=1 the patch is an improvement. At VFLIP=0, the driver's
default, it takes a state that was color-correct and leaves it one
column off in both flip states. That is a regression.
BINNING
Your point 4. At 1296x972 (SUB_INC_X = 0x31, HBIN_EN set) my unit only
streams in the mirrored state. With the FLIP_HORZ bits set it gives
frame sync errors at both offsets, so I cannot test whether the mirror
alone moves the phase when binned. In the state that does stream, the
offset still moves it:
FLIP_HORZ cleared + offset 0: BGGR (clean)
FLIP_HORZ cleared + offset 1: GBRG
Same conclusion as yours by another route: with the patch applied,
HFLIP=1 in a binned mode writes offset 1 and turns a color-correct
readout into a one-column-off one. The compensation should not fire
there, and as posted it cannot tell.
WHERE THIS LEAVES THE SERIES
- 1/2 (ov8865) is unaffected: it only inverts the HFLIP polarity and
contains no Bayer compensation. I repeated your check on that sensor
here. Raw frames off the CSI-2 receiver in all four HFLIP/VFLIP
combinations decode correctly as BGGR, the code the driver reports.
That agrees with your result, so 1/2 can be applied as it stands.
- Please drop 2/2. The window-offset approach cannot express what the
hardware does: The phase is a function of both flips, and the register
combination the fix would need in one state does not stream. It
carries Cc: stable, so I would rather it be dropped explicitly than
left pending while I rework it.
- I am thinking to have another go on this patch and maybe send the
ov5693 work as a new series, would the following be a good approach?
1) Report the media bus code as a function of both flip controls,
with V4L2_CTRL_FLAG_MODIFY_LAYOUT on them, as imx219 does. This
is a fix on its own: the driver hardcodes SBGGR10 today and is
already wrong at VFLIP=0 with HFLIP=1, before any patch of mine.
2) Invert the HFLIP polarity, which is safe once (1) is in.
In that order neither patch leaves the driver worse than it found it.
The polarity fix must not go first: on its own it moves the VFLIP=0
default from color-correct to one column off.
Hans, Fernando: Your Reviewed-by and Tested-by were for the offset
approach, which is gone, so I will not carry them to the new series.
Sakari, Dan: Two things I would rather ask than assume. Do you want
the code table done this way? And should patch (1) carry Fixes?
It fixes a real pre-existing bug, but it also changes what
the driver reports to userspace.
Thanks again for catching this. It would have gone to stable otherwise.
Jakob
On Saturday, August 29th, 2026 at 01:18, D. Manresa <dmanresa@gmail.com> wrote:
> On Wed, 29 Jul 2026, Jakob Berg Jespersen wrote:
> > The sensor's native readout is horizontally mirrored and the FORMAT2
> > FLIP_HORZ bits (reg 0x3821) un-mirror it; the init table sets them by
> > default (0x3821 = 0x1e). The driver sets those bits for V4L2_CID_HFLIP=1,
> > so the control is inverted: HFLIP=1 yields the un-mirrored image and
> > HFLIP=0 the mirrored one. Invert the polarity so HFLIP=0 yields the
> > unflipped image.
>
> Tested on the front camera of a Surface Pro 7+ (OV5693 behind an IPU6,
> patch applied on v6.19 sources, loaded on a linux-surface 6.19.8 kernel),
> Bayer phases read from raw 2592x1944 captures. Partial results - the
> polarity fix checks out, the 0x3810 compensation appears to sit on the
> wrong flip state on my unit:
>
> 1) The polarity inversion is correct. Independent confirmation: the
> register tables inside the vendor Windows driver (ov5693.sys from the
> Surface MSI) keep the 0x3821 FLIP_HORZ bits set in every mode
> (0x3821 = 0x1e/0x1f), and Windows delivers the un-mirrored image; with
> your patch HFLIP=0 keeps them set, as the init table intends. HFLIP
> also toggles the mirror geometry correctly in both directions here.
>
> 2) The 0x3810 write does what the commit message says in the relative
> sense: the two flip states come out with the SAME Bayer phase, so
> toggling HFLIP no longer changes the colours.
>
> 3) However, on my unit BOTH states then decode as GBRG, one column off the
> reported SBGGR10. I measured the four register combinations
> independently (phase identified from raw frames by green-pair
> statistics and confirmed by demosaicing a known-colour scene under each
> hypothesis):
>
> FLIP_HORZ bits set + 0x3810=0 (your HFLIP=0): GBRG
> FLIP_HORZ bits cleared + 0x3810=1 (your HFLIP=1): GBRG
> FLIP_HORZ bits cleared + 0x3810=0: BGGR (clean)
> FLIP_HORZ bits set + 0x3810=1: does not stream
>
> i.e. here it is the un-mirrored readout (bits set) that carries the
> one-column phase shift, and the mirrored readout that is SBGGR-clean -
> the opposite of what the patch compensates. The last row is why the
> compensation cannot simply be moved to the other state: with the
> FLIP_HORZ bits set the sensor refuses to stream with a one-column
> window offset (perpetual "Frame sync error" on the IPU6 CSI-2
> receiver, reproduced across repeated attempts); shifting the crop
> window by one column instead should work, but I have not tested that.
>
> 4) For completeness: in the 2x2-binned readout the Surface uses for
> video (a downstream patch of mine, not in mainline), the mirror does
> not move the Bayer phase at all - same behaviour I measured on the
> OV8865 - so there the 0x3810 parity must stay constant across flip
> states.
>
> Since you and Fernando verified colours correct in both flip states on
> your units, and point 3 is exactly the opposite assignment, maybe the
> two of us are not decoding the same thing - or the modules differ. Could
> you double-check the absolute phase at HFLIP=0 on your unit from a raw
> capture of a known-colour scene (not through an ISP that may be
> auto-correcting, and not with the sensor's test pattern - on my unit the
> colour-bar generator is inserted after the flip/window stage and shows
> the same order and phase in every flip state, so it cannot see this)?
>
> Happy to test a v3. For patch 1/2 of this series everything checks out
> on my unit; sent a Tested-by there separately.
>
> D. Manresa <dmanresa@gmail.com>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase
2026-08-29 13:21 ` Jakob Berg Jespersen
@ 2026-09-03 12:57 ` D. Manresa
0 siblings, 0 replies; 7+ messages in thread
From: D. Manresa @ 2026-09-03 12:57 UTC (permalink / raw)
To: Jakob Berg Jespersen
Cc: Sakari Ailus, Mauro Carvalho Chehab, Kevin Lhopital,
Paul Kocialkowski, Daniel Scally, Jean-Michel Hautbois,
Hans de Goede, Fernando Rimoli, linux-media, linux-kernel, stable,
D . Manresa
[Resent to the list at Jakob's request: the original, sent 29 Aug, reached
him alone - a mail tooling error on my side that dropped the Cc headers on
several replies that week, since fixed. Content unchanged. Jakob: yes, it
was always meant for the record - thanks for checking before quoting it.]
On Sat, 29 Aug 2026, Jakob Berg Jespersen wrote:
> One oddity I cannot explain: the vertical flip moves the phase by a
> column, not a row. [...] I report it as measured; I have no mechanism to
> offer for it.
I think I have the mechanism. All eight of your full-resolution states
reproduce here exactly - same phases, same two states that will not stream -
so this is a second unit agreeing with you, and then some extra measurements
that localise the column shift to one register bit.
Same setup as before: OV5693 at 2-0036 behind an IPU6, raw SBGGR10 off the
CSI-2 receiver, no ISP, no test pattern, registers written over i2c while
streaming, 40-frame captures.
1) The VFLIP output is a *pure* vertical mirror. Correlating each state
against the reference frame and against its h-flipped, v-flipped and
180-rotated copies (on 2x2-block means, so the metric is phase-blind)
gives, for 0x3820=0x42 with the FLIP_HORZ bits set: vflip 0.996 against
hflip 0.182. Nothing horizontal is being mirrored, so the column shift is
not a hidden mirror.
2) The row phase does not move, and that part is correct behaviour. At
VFLIP=0 the phase is GBRG, so row 0 is G B G B; at VFLIP=1 it is BGGR, so
row 0 is B G B G. Both are the blue row: the sensor's one-row
Bayer-preserving compensation is present and works. Only the column phase
moves.
3) The sensor does not expose the shift. Reading 0x3800..0x3821 back in every
state: CROP_START_X = 0x0010, CROP_START_Y = 0x0006, CROP_END_X = 0x0a30,
CROP_END_Y = 0x079e, OFFSET_Y = 0, unchanged throughout.
4) Splitting 0x3820 into its two bits is what localises it. The driver writes
FLIP_VERT_ISP_EN (bit 6) and FLIP_VERT_SENSOR_EN (bit 1) together; taken
apart, at 0x3821=0x1e, offset 0:
0x3820 = 0x40 (ISP bit alone): no flip at all, phase unchanged (GBRG),
image registers at (0,0) against the
reference. A complete no-op on its own.
0x3820 = 0x02 (sensor bit alone): flips the picture (vflip corr 0.997),
and does NOT move the column phase -
but the colour path then delivers R and
B at the noise floor: plane means
18.6 / 18.4, std 8.5, against 175 / 173
and std 251 for the two green planes.
Unusable alone.
0x3820 = 0x42 (both): flips, R and B are restored, and the
column phase has moved (BGGR).
So the ISP-side flip bit is the one that both rescues R/B and carries the
spurious column shift. It behaves like the internal pipeline being told the
mosaic moved, and moving it one column too far.
5) It really is a one-column window move, and it composes with one as an XOR.
Calibrated against a known one-column change, at 2592x1944:
FLIP_HORZ set (0x3821=0x1e), OFFSET_START_X=0:
CROP_START_X 16: VFLIP=0 GBRG VFLIP=1 BGGR
CROP_START_X 15: VFLIP=0 BGGR VFLIP=1 GBRG
FLIP_HORZ cleared (0x3821=0x18), CROP_START_X=16:
OFFSET_START_X 0: VFLIP=0 BGGR VFLIP=1 GBRG
OFFSET_START_X 1: VFLIP=0 GBRG VFLIP=1 BGGR
Pixel registration agrees: a per-colour-class-normalised, high-passed,
15-frame-averaged correlation gives an envelope centre of -1.15 px for a
deliberate CROP_START_X 16->15 move, -0.90 px for VFLIP, and 0.00 px for
the self-comparison. Same signature, same magnitude, same sign.
Two by-products that bear on how a v3 could be written:
- Your two non-streaming states are a plain window overrun, not a flip
interaction: CROP_END_X - CROP_START_X = 2608 - 16 = 2592, exactly the
output width, so OFFSET_START_X=1 runs off the end. With CROP_START_X=15,
the state 0x3820=0x42 / 0x3821=0x1e / OFFSET_START_X=1 streams cleanly here
(40 frames) and decodes BGGR. Symmetrically, CROP_START_X=17 with
OFFSET_START_X=0 stalls. So the state you needed is reachable after all -
through the crop window rather than the ISP offset.
- With the FLIP_HORZ bits cleared, CROP_START_X has no effect on the phase at
all (the window appears to be anchored at CROP_END_X in the mirrored
readout); there, OFFSET_START_X is the knob that works. With the bits set it
is the other way round. Any compensation therefore has to pick its register
according to the horizontal state.
Measured combinations that give a clean BGGR at 2592x1944 on this unit:
VFLIP=0, 0x3821=0x1e, CROP_START_X=15, OFFSET_START_X=0
VFLIP=0, 0x3821=0x18, CROP_START_X=16, OFFSET_START_X=0
VFLIP=1, 0x3821=0x1e, CROP_START_X=16, OFFSET_START_X=0
VFLIP=1, 0x3821=0x18, CROP_START_X=16, OFFSET_START_X=1
On your plan: reporting the media bus code as a function of both flip
controls, with V4L2_CTRL_FLAG_MODIFY_LAYOUT, and inverting the polarity only
after that, sounds right to me, and matches what the numbers above say - the
phase is a function of both flips, so no HFLIP-keyed compensation can be
correct in both vertical states. It also has the advantage of not needing the
sensor to do anything it does not want to do. If you would rather compensate
than report, the tables above say it is possible, but it needs both flips as
inputs and a different register in each horizontal state, which is a lot of
machinery next to just telling userspace the truth.
For whatever it is worth downstream: my out-of-tree binned mode is unaffected
either way, since it keys its window offset on the mode rather than on the
flip controls.
Happy to test a v3, and to re-run any of the above on request - the harness is
a loop of i2c writes plus raw captures, so extra states are cheap.
D. Manresa <dmanresa@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-09-03 12:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-29 15:41 [PATCH v2 0/2] media: i2c: Surface Pro 7+ camera flip fixes Jakob Berg Jespersen
2026-07-29 15:41 ` [PATCH v2 1/2] media: i2c: ov8865: fix horizontal flip control polarity Jakob Berg Jespersen
2026-08-28 23:18 ` D. Manresa
2026-07-29 15:41 ` [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase Jakob Berg Jespersen
2026-08-28 23:18 ` D. Manresa
2026-08-29 13:21 ` Jakob Berg Jespersen
2026-09-03 12:57 ` D. Manresa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox