Re: [PATCH 06/13] media: i2c: ov5647: Support HFLIP and VFLIP

[Jai Luthra <jai.luthra@ideasonboard.com>]

Hi Jacopo,

Thanks a lot for the review.

Quoting Jacopo Mondi (2025-11-02 16:20:36)
> Hi Jai
> 
> On Tue, Oct 28, 2025 at 12:57:17PM +0530, Jai Luthra wrote:
> > From: David Plowman <david.plowman@raspberrypi.com>
> >
> > Add missing controls for horizontal and vertical flipping.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.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 ?

Indeed! That was a great find, I am now able to set BIT(2) to get flips
without needing to modify the pixelarray layout.

Will do that in v2.

> 
> 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, &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 ?
> 

Yes, this particular sensor flips the image horizontally if 0x3820 = 0x00,
resulting in a mirrored image.

But now that userspace can control flips, I am a bit confused on how to
report this in the controls.

> > +
> >       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 ?
> 

My preference is to let the driver toggle the value for HFLIP before
writing to the sensor, as it is done currently in this patch.

This makes this sensor behave like others, where you get a normal
non-mirrored image when you set HFLIP = 0, and mirrored if HFLIP = 1.

The alternative is to keep the driver transparent by reporting the register
values as-is in the control. In that case, we should make horizontal_flip =
1 the default value for the control. But I'm not aware if any userspace
applications will override the default value resulting in bad experience
for the users.

WDYT?

Thanks,
    Jai

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