| 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: <176474407271.1636185.12086100281205720721@freya>
Date: Wed, 03 Dec 2025 12:11:12 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Jacopo Mondi <jacopo.mondi@...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 Jacopo
Quoting Jacopo Mondi (2025-12-01 20:45:27)
> 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 ?
I think if someone looks at the git blame years down the line, it might
help that the commit description states explicitly that register value is
the opposite of the control value, and it was an informed choice.
>
> >
> > 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
Ah this is unintentional, we don't mirror by default. All control values
get applied when sensor wakes up on .s_stream, and BIT(1) here gets written
as our default control value is hflip=0 (which is intended).
But I agree this is confusing, so I will fix the register table value to
match the control value in next revision.
>
> > {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));
>
Indeed, will do.
> > + }
> > +
> > + 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 ?
>
We don't mirror by default, I will fix the register table instead to avoid
confusion.
> Nits apart
> Reviewed-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
>
Thank you for the review!
Jai
> 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