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

[Dave Stevenson]

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 sounds like ... you've found the right bits to handle flips. That's
> great.
>
> But please just use a human to verify, and skip the almost maybe
> probablys.
>
> A flip is either flipped or it isn't. A 95% correlation that something
> might be flipped, probably, maybe ... isn't quite the same as "Oh look  I
> can see it's mirrored and I'm 100% certain of this because I'm a real
> person and I can tell when I look at something from left to right or
> right to left."
>
> If we know this flips correctly - lets say so clearly.
>
> > Signed-off-by: Sergey Lebedev <lsa.uz@xxxxx>
> > ---
> >  drivers/media/i2c/ov13858.c | 44 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
> > index de2b79a9a0..1bf21ffbc7 100644
> > --- a/drivers/media/i2c/ov13858.c
> > +++ b/drivers/media/i2c/ov13858.c
> > @@ -76,6 +76,15 @@
> >  #define OV13858_DGTL_GAIN_DEFAULT      1024    /* Default gain = 1 X */
> >  #define OV13858_DGTL_GAIN_STEP         1       /* Each step = 1/1024 */
> >
> > +/*
> > + * Readout direction. Neither flip changes the Bayer order at the output, so
> > + * no window offset compensation is needed. The mirror bit is active low: the
> > + * value the mode tables program already has it set.
> > + */
> > +#define OV13858_REG_FORMAT1            0x3820
> > +#define OV13858_FORMAT1_VFLIP          BIT(4)
> > +#define OV13858_FORMAT1_HFLIP_N                BIT(3)
> > +
> >  /* Test Pattern Control */
> >  #define OV13858_REG_TEST_PATTERN       0x4503
> >  #define OV13858_TEST_PATTERN_ENABLE    BIT(7)
> > @@ -1042,6 +1051,8 @@ struct ov13858 {
> >         struct v4l2_ctrl *vblank;
> >         struct v4l2_ctrl *hblank;
> >         struct v4l2_ctrl *exposure;
> > +       struct v4l2_ctrl *hflip;
> > +       struct v4l2_ctrl *vflip;
> >
> >         /* Current mode */
> >         const struct ov13858_mode *cur_mode;
> > @@ -1208,6 +1219,30 @@ static int ov13858_enable_test_pattern(struct ov13858 *ov13858, u32 pattern)
> >                                  OV13858_REG_VALUE_08BIT, val);
> >  }
> >
> > +static int ov13858_update_flips(struct ov13858 *ov13858)
> > +{
> > +       u32 val;
> > +       int ret;
> > +
> > +       ret = ov13858_read_reg(ov13858, OV13858_REG_FORMAT1,
> > +                              OV13858_REG_VALUE_08BIT, &val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (ov13858->vflip->val)
> > +               val |= OV13858_FORMAT1_VFLIP;
> > +       else
> > +               val &= ~OV13858_FORMAT1_VFLIP;
> > +
> > +       if (ov13858->hflip->val)
> > +               val &= ~OV13858_FORMAT1_HFLIP_N;
> > +       else
> > +               val |= OV13858_FORMAT1_HFLIP_N;
>
> Why is VFLIP set to flip, and HFLIP set to not flip. This sounds very
> odd to me.

It's not uncommon with Omnivision sensors. OV5647 is the same.

> > +
> > +       return ov13858_write_reg(ov13858, OV13858_REG_FORMAT1,
> > +                                OV13858_REG_VALUE_08BIT, val);
> > +}
> > +
> >  static int ov13858_set_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >         struct ov13858 *ov13858 = container_of(ctrl->handler,
> > @@ -1254,6 +1289,10 @@ static int ov13858_set_ctrl(struct v4l2_ctrl *ctrl)
> >                                         ov13858->cur_mode->height
> >                                           + ctrl->val);
> >                 break;
> > +       case V4L2_CID_HFLIP:
> > +       case V4L2_CID_VFLIP:
> > +               ret = ov13858_update_flips(ov13858);
> > +               break;

One thought which may not be answerable without a datasheet. Many
sensors will produce invalid frame/frames if flips are changed whilst
streaming, and others may lock up. Do you need to grab/release the
control at set_stream?

> >         case V4L2_CID_TEST_PATTERN:
> >                 ret = ov13858_enable_test_pattern(ov13858, ctrl->val);
> >                 break;
> > @@ -1619,6 +1658,11 @@ static int ov13858_init_controls(struct ov13858 *ov13858)
> >                           OV13858_DGTL_GAIN_MIN, OV13858_DGTL_GAIN_MAX,
> >                           OV13858_DGTL_GAIN_STEP, OV13858_DGTL_GAIN_DEFAULT);
> >
> > +       ov13858->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops,
> > +                                          V4L2_CID_HFLIP, 0, 1, 1, 0);
> > +       ov13858->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &ov13858_ctrl_ops,
> > +                                          V4L2_CID_VFLIP, 0, 1, 1, 0);
> > +

Use a control cluster so that you only call the s_ctrl function once
even if both are present in a S_EXT_CTRLS call? See imx290 or imx214.

  Dave

> >         v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &ov13858_ctrl_ops,
> >                                      V4L2_CID_TEST_PATTERN,
> >                                      ARRAY_SIZE(ov13858_test_pattern_menu) - 1,
> > --
> > 2.54.0 (Apple Git-157)
> >
> >
>