Re: [PATCH] media: i2c: ov13858: add horizontal and vertical flip controls

[Sakari Ailus]

Hi Dave, others,

On Mon, Sep 21, 2026 at 11:57:07AM +0100, Dave Stevenson wrote:
> Hi Sergey and Kieran
> 
> On Mon, 21 Sept 2026 at 10:02, Kieran Bingham
> <kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
> >
> > Quoting Sergey Lebedev (2026-09-21 09:26:14)
> > > The driver programs OV13858_REG_FORMAT1 (0x3820) from its mode tables and
> > > never exposes the readout direction, so a module mounted rotated cannot be
> > > corrected.
> > >
> > > The Microsoft Surface Pro 11 for Business (Intel) mounts this sensor upside
> > > down, and ipu-bridge now says so - b238116ccd4b ("media: ipu-bridge: Add
> > > upside-down quirk for Surface Pro 11"). libcamera reads that rotation and
> > > tries to compensate with sensor flips, finds neither control, and falls
> > > back to Rot0, so the quirk on its own names a rotation nothing can undo.
> > >
> > > ov13b10 has the same two controls, but it is a different part and its bit
> > > assignments do not carry over. There is no public datasheet for this one,
> > > so these were found by experiment on a single sample: single bits written
> > > over i2c mid-stream, each captured frame correlated against the flipped
> > > baseline. Of every bit in 0x3820 through 0x3823 exactly two move the
> > > image: 0x3820 BIT(4) set flips vertically, and BIT(3) cleared mirrors
> > > horizontally. 0x3821, where the mirror sits on several other OmniVision
> > > parts, has no effect here.
> 
> Intel's ipu6 driver for ov13858 confirms this -
> https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/ov13858_intel.c
> 
> > > Verified through the controls against a static scene, as correlation with
> > > the flipped reference and, as a control, with the unflipped one:
> > >
> > >   vflip  +0.994 / +0.629    hflip  +0.973 / -0.141    both  +0.975 / -0.178
> >
> > What do these numbers mean ?
> >
> > Flips are 100% flips. They're not 90% flipped... or 90% correlated to
> > something which might be flipped.
> >
> >
> > > The mirror bit is active low and every mode table already sets it, so the
> > > defaults write back what the mode list just wrote.
> > > __v4l2_ctrl_handler_setup() runs after that list and before MODE_SELECT,
> > > so the read-modify-write here sees the value the mode just programmed.
> > >
> > > The Bayer order at the output does not change with either flip: per-channel
> > > means over the four states agree to 0.2 counts in 70, and all four frames
> >
> > What does this mean ? (what's 0.2?)
> >
> > > demosaic correctly against one fixed pattern. Unlike imx219 and imx258,
> > > whose flips select a different media bus code, these controls therefore do
> > > not need V4L2_CTRL_FLAG_MODIFY_LAYOUT.
> 
> Just as a note, if the driver supported get_selection then the crop
> should move by one pixel in the relevant direction with the flips.
> 
> Moving the crop to preserve the Bayer order isn't that unusual in
> sensors now (most of the Sony Starvis and Starvis2 sensors do this, as
> do some OnSemi sensors I'm aware of), so it's not really worth stating
> in the commit text.

It'd still be better to do this by using set_selection() to select the crop
rectangle, which is a bit awkward before we have the common raw sensor
model patches merged.

On the other hand, if this is all the sensor supports, there's little we
can do about it I guess.

-- 
Regards,

Sakari Ailus