[PATCH v3] media: i2c: ov13858: add horizontal and vertical flip controls
From: Sergey Lebedev
Date: Sat Sep 26 2026 - 04:03:19 EST
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 - commit 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 with the quirk alone the picture stays upside down in any
application that does not rotate it itself.
In 0x3820, BIT(4) set flips vertically and BIT(3) cleared mirrors - the
mirror is active low, which is ordinary for the family. There is no public
datasheet, so the bits were found by experiment: every bit of 0x3820
through 0x3823 written singly mid-stream, and exactly two move the image.
Two existing drivers agree. Intel's out-of-tree ov13858 declares 0x3820 as
a bitfield with "hflip: 0 enable, 1 disable" at BIT(3) and "vflip: 0
disable, 1 enable" at BIT(4), and writes !ctrl->val for the mirror;
ov13b10 clears BIT(3) and sets BIT(4) of the same register in tree.
Both controls were checked by eye, one static scene in all four states:
unflipped the image is upside down, vflip alone stands it up, hflip alone
mirrors it, and the two together give the view this machine needs. The
Bayer order at the output is the same in all four, so unlike imx219 and
imx258, whose flips select a different media bus code, these controls do
not need V4L2_CTRL_FLAG_MODIFY_LAYOUT.
Every mode table already sets the mirror bit, 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 two are clustered, so a combined S_EXT_CTRLS costs one register
read-modify-write rather than two, and grabbed while streaming: nothing
here documents a mid-stream flip as safe, and imx290 and imx219 grab
theirs because on those parts it is not.
Signed-off-by: Sergey Lebedev <lsa.uz@xxxxx>
Tested-by: German Pablo Lindo <germanpapulindez@xxxxxxxxx>
---
v3, commit message only:
- Cites b238116ccd4b in the "commit <sha> (...)" form checkpatch asks
for; the Media CI flagged v2 for it.
- v2 said the quirk on its own "names a rotation nothing can undo".
Snapshot and Firefox do undo it - they rotate the picture themselves -
and qcam, which does not, shows it upside down. The sentence now says
that.
- Tested-by from German Pablo Lindo, on a second Surface Pro 11: qcam
upright with this patch, Snapshot and Firefox unchanged.
The code is v2's, byte for byte. Sakari's set_selection() point is
answered in v2's notes, and nothing here changes it.
v2: https://lore.kernel.org/all/20260921124444.79396-1-lsa.uz@xxxxx/
v1: https://lore.kernel.org/all/20260921082609.30830-1-lsa.uz@xxxxx/
drivers/media/i2c/ov13858.c | 52 +++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
index de2b79a9a0e..d870d2edb4f 100644
--- a/drivers/media/i2c/ov13858.c
+++ b/drivers/media/i2c/ov13858.c
@@ -76,6 +76,14 @@
#define OV13858_DGTL_GAIN_DEFAULT 1024 /* Default gain = 1 X */
#define OV13858_DGTL_GAIN_STEP 1 /* Each step = 1/1024 */
+/*
+ * Readout direction. The mirror bit is active low, and the value every mode
+ * table programs 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 +1050,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 +1218,31 @@ 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;
+
+ /* The mirror bit is active low, as it is on ov13b10. */
+ 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;
@@ -1481,6 +1520,13 @@ static int ov13858_set_stream(struct v4l2_subdev *sd, int enable)
pm_runtime_put(ov13858->dev);
}
+ /*
+ * Do not let the flips change while streaming. ov13858->mutex is the
+ * control handler's own lock and is held here, hence the __ form.
+ */
+ __v4l2_ctrl_grab(ov13858->hflip, enable);
+ __v4l2_ctrl_grab(ov13858->vflip, enable);
+
mutex_unlock(&ov13858->mutex);
return ret;
@@ -1619,6 +1665,12 @@ 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_cluster(2, &ov13858->hflip);
+
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)