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

Sergey Lebedev posted 1 patch 3 days, 11 hours ago
There is a newer version of this series
drivers/media/i2c/ov13858.c | 44 +++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
[PATCH] media: i2c: ov13858: add horizontal and vertical flip controls
Posted by Sergey Lebedev 3 days, 11 hours ago
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

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

Signed-off-by: Sergey Lebedev <lsa.uz@pm.me>
---
 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;
+
+	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)
Re: [PATCH] media: i2c: ov13858: add horizontal and vertical flip controls
Posted by Kieran Bingham 3 days, 11 hours ago
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@pm.me>
> ---
>  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)
> 
>
Re: [PATCH] media: i2c: ov13858: add horizontal and vertical flip controls
Posted by Dave Stevenson 3 days, 9 hours ago
Hi Sergey and Kieran

On Mon, 21 Sept 2026 at 10:02, Kieran Bingham
<kieran.bingham@ideasonboard.com> 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@pm.me>
> > ---
> >  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)
> >
> >
>
Re: [PATCH] media: i2c: ov13858: add horizontal and vertical flip controls
Posted by Sakari Ailus 3 days, 8 hours ago
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@ideasonboard.com> 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
Re: [PATCH] media: i2c: ov13858: add horizontal and vertical flip controls
Posted by Sergey Lebedev 3 days, 7 hours ago
v2 is posted, as its own thread:

  https://lore.kernel.org/all/20260921124444.79396-1-lsa.uz@pm.me/

Sakari - it does not do the set_selection() part, and the changelog says
why at length. The short of it: ov13858 has no get_selection or
set_selection at all, so there is no crop rectangle for the flips to
move, and ov13b10 writes its window offsets register-side without
exposing one either. Glad to add selection support as its own patch if
you would like it here.

Kieran, Dave - everything you both raised is in v2.

Sergey
Re: [PATCH] media: i2c: ov13858: add horizontal and vertical flip controls
Posted by Sergey Lebedev 3 days, 10 hours ago
Kieran,

You are right on all four. The bits are fine; the message was not.

1. The numbers are not an angle - nothing there is partly flipped.
   They are how the bits were found: every bit of 0x3820 through
   0x3823 written one at a time and ranked by correlation against a
   flipped reference, there being no public datasheet. "0.2" is a
   spread of 0.2 counts on a mean level of 70, from the same exercise.
   I included the working in case anyone later wanted more than the
   result; a commit message was the wrong place for it. v2 gives the
   result, and the method is yours for the asking.

2. I did check it myself, by eye, and that never reached the message.
   The four states are one contact sheet, which I have sent you
   directly rather than here because it is a 101 KB attachment:

     <EViHAe8VXLnuCLNW2WvrH9wZvJzSwWcAsaS16-dxkYEdCDlpHnQyQpkcfcEGYIIQ1jRO0ChI_aIXf-HNYdroAOlCbEg-BXux6yw76GGfA94=@pm.me>

   Unflipped is upside down, which is what userspace gets today;
   hflip+vflip is the one the right way up. Say the word and it goes to
   anyone else here.

3. The mirror bit is active low, and ov13b10 does the same:
   ov13b10_set_ctrl_hflip() clears BIT(3) of 0x3820 and
   ov13b10_set_ctrl_vflip() sets BIT(4) | BIT(5) of it. v2 says so at
   that line, rather than only in the defines at the top of the file.

Which makes this, in my own message, untrue:

> ov13b10 has the same two controls, but it is a different part and its
> bit assignments do not carry over.

They carry over exactly. BIT(5) differs only in where the mode tables
leave it - 0x88 and 0x8b there, 0xa8/0xab/0xac here - leaving this part
only BIT(4) to move. The real difference is the other way round:
ov13b10 shifts H_WIN_OFFSET and V_WIN_OFFSET on each flip to reverse
the Bayer order, and this one does neither. v2 says why, from the
picture rather than a statistic.

Thanks for the fast read.

Sergey