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

[Kieran Bingham]

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.
> 
> 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.


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.



> +
> +       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;
>         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);
> +
>         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)
> 
>