Re: [PATCH v2 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase

[Jakob Berg Jespersen]

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@xxxxxxxxx> 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@xxxxxxxxx>
>