| 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: <u5wdva6kk5n6q5myj3wuy4bxqhaqyhk2ngentrj4p3v5447y2a@3kujjl3nnwea>
Date: Sun, 2 Nov 2025 11:50:36 +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>, linux-media@...r.kernel.org, linux-kernel@...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>
Subject: Re: [PATCH 06/13] media: i2c: ov5647: Support HFLIP and VFLIP
Hi Jai
On Tue, Oct 28, 2025 at 12:57:17PM +0530, Jai Luthra wrote:
> From: David Plowman <david.plowman@...pberrypi.com>
>
> Add missing controls for horizontal and vertical flipping.
>
> Signed-off-by: David Plowman <david.plowman@...pberrypi.com>
> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> ---
> drivers/media/i2c/ov5647.c | 77 ++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 71 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 977b878b0d4b8cd5f39f510ebd8b33c9163f7da2..a33e2d8edc114d302e830639cb7cb161f16a6208 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -54,6 +54,8 @@
> #define OV5647_REG_GAIN_LO 0x350b
> #define OV5647_REG_VTS_HI 0x380e
> #define OV5647_REG_VTS_LO 0x380f
> +#define OV5647_REG_VFLIP 0x3820
> +#define OV5647_REG_HFLIP 0x3821
> #define OV5647_REG_FRAME_OFF_NUMBER 0x4202
> #define OV5647_REG_MIPI_CTRL00 0x4800
> #define OV5647_REG_MIPI_CTRL14 0x4814
> @@ -109,6 +111,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)
> @@ -150,7 +154,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
> {0x3036, 0x69},
> {0x303c, 0x11},
> {0x3106, 0xf5},
> - {0x3821, 0x06},
> + {0x3821, 0x00},
> {0x3820, 0x00},
That's interesting, as the datasheet says that by default
3820 = 0x40
3821 = 0x00
And
- BIT[2] = flip ISP
- BIT[1] = flip SNR
The implementation of ov5647_s_flip() toggles BIT(1) and ignores
BIT(2) while the modes definition have both (BIT(2) | BIT(1)) set
More interestingly the datasheet says:
In flip mode, the OV5647 does not need additional settings because the
ISP block will auto-detect whether the pixel is in the red line or
blue line and make the necessary adjustments
Might this suggest that if we flip using BIT(2) we don't need to
change the bayer pattern ordering ?
Now, I admit I'm not sure what are the ISP functions on the ov5647 and
this patch is super-duper-tested as it comes from the RPi BSP, so if
you don't have answers to the above questions, I'm fine with this
patch!
> {0x3827, 0xec},
> {0x370c, 0x03},
> @@ -239,7 +243,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
> {0x3036, 0x62},
> {0x303c, 0x11},
> {0x3106, 0xf5},
> - {0x3821, 0x06},
> + {0x3821, 0x00},
> {0x3820, 0x00},
> {0x3827, 0xec},
> {0x370c, 0x03},
> @@ -403,7 +407,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
> {0x4800, 0x24},
> {0x3503, 0x03},
> {0x3820, 0x41},
> - {0x3821, 0x07},
> + {0x3821, 0x01},
> {0x350a, 0x00},
> {0x350b, 0x10},
> {0x3500, 0x00},
> @@ -419,7 +423,7 @@ static struct regval_list ov5647_640x480_10bpp[] = {
> {0x3035, 0x11},
> {0x3036, 0x46},
> {0x303c, 0x11},
> - {0x3821, 0x07},
> + {0x3821, 0x01},
> {0x3820, 0x41},
> {0x370c, 0x03},
> {0x3612, 0x59},
> @@ -935,6 +939,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)
> @@ -942,7 +966,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;
> }
> @@ -953,7 +977,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;
>
> @@ -986,6 +1010,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;
> @@ -1033,6 +1059,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;
> @@ -1208,6 +1236,25 @@ 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;
> +
> + /* Set or clear bit 1 and leave everything else alone. */
> + ret = ov5647_read(sd, reg, ®_val);
> + if (ret == 0) {
> + if (ctrl_val)
> + reg_val |= 2;
> + else
> + reg_val &= ~2;
> +
> + ret = ov5647_write(sd, reg, reg_val);
> + }
> +
> + return ret;
> +}
> +
> static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov5647 *sensor = container_of(ctrl->handler,
> @@ -1270,6 +1317,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_HFLIP, !ctrl->val);
> + break;
> + case V4L2_CID_VFLIP:
> + ov5647_s_flip(sd, OV5647_REG_VFLIP, ctrl->val);
> + break;
The modes definition used to set
0x3820 = 0x00
0x3821 = 0x06
Is this the built-in hflip ?
Or does it mean that setting the registers value to 1 'disabled' flips ?
> +
> default:
> dev_info(&client->dev,
> "Control (id:0x%x, val:0x%x) not supported\n",
> @@ -1341,6 +1396,16 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> 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 (sensor->hflip)
> + sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
I wonder if hflip is enabled by default we shouldn't register the
control with default value of 1 ?
> +
> + sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + if (sensor->vflip)
> + sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> v4l2_fwnode_device_parse(dev, &props);
>
> v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
>
> --
> 2.51.0
>
Powered by blists - more mailing lists