| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <gla47pqyt6aan7fzr4eizm5ftyoc5s5u3dyh5u2fqbig7h2n6o@3lernf2jpswf>
Date: Mon, 1 Dec 2025 16:15:27 +0100
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Jai Luthra <jai.luthra@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Dave Stevenson <dave.stevenson@...pberrypi.com>, Jacopo Mondi <jacopo@...ndi.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Naushir Patuck <naush@...pberrypi.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Kieran Bingham <kieran.bingham@...asonboard.com>, David Plowman <david.plowman@...pberrypi.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>, Peter Robinson <pbrobinson@...il.com>,
Stefan Wahren <wahrenst@....net>, "Ivan T. Ivanov" <iivanov@...e.de>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>
Subject: Re: [PATCH v2 09/16] media: i2c: ov5647: Support HFLIP and VFLIP
Hi Jai
On Tue, Nov 18, 2025 at 05:33:02PM +0530, Jai Luthra wrote:
> From: David Plowman <david.plowman@...pberrypi.com>
>
> Add missing controls for horizontal and vertical flipping.
>
> The sensor readout mirrors in the horizontal direction by default (if
> 0x3821[1] = 0) which can make things unnecessarily difficult for
> applications. The register table prior to this commit was setting that
> bit explicitly, to achieve a normally oriented image.
>
> Now that we have userspace controls for HFLIP, we keep the convention
> and report the non-mirrored image (with 0x3821[1] = 1) as
> horizontal_flip=0, and vice versa.
I would drop this last part. This patch makes thing work "as
expected", HFLIP=1 -> mirror, HFLIP=0 -> non mirror
The fact we invert the control value to get the right register value
might just be confusing to read here ?
>
> Signed-off-by: David Plowman <david.plowman@...pberrypi.com>
> Co-developed-by: Jai Luthra <jai.luthra@...asonboard.com>
> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> ---
> drivers/media/i2c/ov5647.c | 86 ++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 79 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 5009fd8c05a64d7e06f66f8f75f0a881cd0b95c1..0343583692ab9bcca1a07d874a707ac6093a9035 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -55,6 +55,8 @@
> #define OV5647_REG_GAIN_LO 0x350b
> #define OV5647_REG_VTS_HI 0x380e
> #define OV5647_REG_VTS_LO 0x380f
> +#define OV5647_REG_TIMING_TC_V 0x3820
> +#define OV5647_REG_TIMING_TC_H 0x3821
> #define OV5647_REG_FRAME_OFF_NUMBER 0x4202
> #define OV5647_REG_MIPI_CTRL00 0x4800
> #define OV5647_REG_MIPI_CTRL14 0x4814
> @@ -120,6 +122,8 @@ struct ov5647 {
> struct v4l2_ctrl *hblank;
> struct v4l2_ctrl *vblank;
> struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *hflip;
> + struct v4l2_ctrl *vflip;
> };
>
> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> @@ -161,7 +165,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> {0x3036, 0x69},
> {0x303c, 0x11},
> {0x3106, 0xf5},
> - {0x3821, 0x06},
> + {0x3821, 0x00},
> {0x3820, 0x00},
> {0x3827, 0xec},
> {0x370c, 0x03},
> @@ -250,7 +254,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> {0x3036, 0x62},
> {0x303c, 0x11},
> {0x3106, 0xf5},
> - {0x3821, 0x06},
> + {0x3821, 0x00},
> {0x3820, 0x00},
> {0x3827, 0xec},
> {0x370c, 0x03},
> @@ -414,7 +418,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> {0x4800, 0x24},
> {0x3503, 0x03},
> {0x3820, 0x41},
> - {0x3821, 0x07},
> + {0x3821, 0x01},
> {0x350a, 0x00},
> {0x350b, 0x10},
> {0x3500, 0x00},
> @@ -430,7 +434,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> {0x3035, 0x11},
> {0x3036, 0x46},
> {0x303c, 0x11},
> - {0x3821, 0x07},
> + {0x3821, 0x01},
So we now mirror by default (HFLIP=1). See below at controls
initialization
> {0x3820, 0x41},
> {0x370c, 0x03},
> {0x3612, 0x59},
> @@ -956,6 +960,26 @@ static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
> .s_stream = ov5647_s_stream,
> };
>
> +/*
> + * This function returns the mbus code for the current settings of the HFLIP
> + * and VFLIP controls.
> + */
> +static u32 ov5647_get_mbus_code(struct v4l2_subdev *sd)
> +{
> + struct ov5647 *sensor = to_sensor(sd);
> + /* The control values are only 0 or 1. */
> + int index = sensor->hflip->val | (sensor->vflip->val << 1);
> +
> + static const u32 codes[4] = {
> + MEDIA_BUS_FMT_SGBRG10_1X10,
> + MEDIA_BUS_FMT_SBGGR10_1X10,
> + MEDIA_BUS_FMT_SRGGB10_1X10,
> + MEDIA_BUS_FMT_SGRBG10_1X10
> + };
> +
> + return codes[index];
> +}
> +
> static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_mbus_code_enum *code)
> @@ -963,7 +987,7 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> if (code->index > 0)
> return -EINVAL;
>
> - code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> + code->code = ov5647_get_mbus_code(sd);
>
> return 0;
> }
> @@ -974,7 +998,7 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd,
> {
> const struct v4l2_mbus_framefmt *fmt;
>
> - if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10 ||
> + if (fse->code != ov5647_get_mbus_code(sd) ||
> fse->index >= ARRAY_SIZE(ov5647_modes))
> return -EINVAL;
>
> @@ -1007,6 +1031,8 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
> }
>
> *fmt = *sensor_format;
> + /* The code we pass back must reflect the current h/vflips. */
> + fmt->code = ov5647_get_mbus_code(sd);
> mutex_unlock(&sensor->lock);
>
> return 0;
> @@ -1054,6 +1080,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> exposure_def);
> }
> *fmt = mode->format;
> + /* The code we pass back must reflect the current h/vflips. */
> + fmt->code = ov5647_get_mbus_code(sd);
> mutex_unlock(&sensor->lock);
>
> return 0;
> @@ -1229,6 +1257,36 @@ static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
> return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
> }
>
> +static int ov5647_s_flip(struct v4l2_subdev *sd, u16 reg, u32 ctrl_val)
> +{
> + int ret;
> + u8 reg_val;
nit: invert the declaration order
> +
> + /*
> + * TIMING TC REG20 (Vertical) and REG21 (Horizontal):
> + * - [2]: ISP mirror/flip
> + * - [1]: Sensor mirror/flip
> + *
> + * We only use sensor flip.
> + *
> + * Using ISP flip retains the BGGR pattern at the cost of changing the
> + * pixel array readout. This affects the selection rectangles in ways
> + * that are not very well documented, and would be tougher to deal with
> + * for applications compared to reading a different bayer pattern.
Nice you reported this
> + */
> + ret = ov5647_read(sd, reg, ®_val);
> + if (ret == 0) {
isn't it easier:
if (ret)
return ret;
> + if (ctrl_val)
> + reg_val |= BIT(1);
> + else
> + reg_val &= ~BIT(1);
> +
> + ret = ov5647_write(sd, reg, reg_val);
return ov5647_write(sd, reg, val ? reg_val | BIT(1)
: reg_val &= ~BIT(1));
> + }
> +
> + return ret;
> +}
> +
> static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov5647 *sensor = container_of(ctrl->handler,
> @@ -1291,6 +1349,14 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> /* Read-only, but we adjust it based on mode. */
> break;
>
> + case V4L2_CID_HFLIP:
> + /* There's an in-built hflip in the sensor, so account for that here. */
> + ov5647_s_flip(sd, OV5647_REG_TIMING_TC_H, !ctrl->val);
> + break;
> + case V4L2_CID_VFLIP:
> + ov5647_s_flip(sd, OV5647_REG_TIMING_TC_V, ctrl->val);
> + break;
> +
> default:
> dev_info(&client->dev,
> "Control (id:0x%x, val:0x%x) not supported\n",
> @@ -1324,7 +1390,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)
> int hblank, exposure_max, exposure_def;
> struct device *dev = &client->dev;
>
> - v4l2_ctrl_handler_init(&sensor->ctrls, 11);
> + v4l2_ctrl_handler_init(&sensor->ctrls, 13);
>
> v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> @@ -1373,6 +1439,12 @@ static int ov5647_init_controls(struct ov5647 *sensor)
> ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> 0, 0, ov5647_test_pattern_menu);
>
> + sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
if now we mirror by default, should you initialize the control value
to 1 ?
Nits apart
Reviewed-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Thanks
j
> +
> + sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> +
> v4l2_fwnode_device_parse(dev, &props);
>
> v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
>
> --
> 2.51.1
>
Powered by blists - more mailing lists