Re: [PATCH 11/16] media: i2c: ov9282: Add HFLIP and VFLIP support

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

Hi Jacopo

cc Sakari for policy view.

On Thu, 6 Oct 2022 at 10:38, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:04PM +0100, Dave Stevenson wrote:
> > Adds support for V4L2_CID_HFLIP and V4L2_CID_VFLIP to allow
> > flipping the image.
> >
> > The driver previously enabled H & V flips in the register table,
> > therefore the controls default to the same settings to avoid
> > changing the behaviour.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 52 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 49 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 5ddef6e2b3ac..12cbe401fd78 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -46,6 +46,10 @@
> >  /* Group hold register */
> >  #define OV9282_REG_HOLD              0x3308
> >
> > +#define OV9282_REG_TIMING_FORMAT_1   0x3820
> > +#define OV9282_REG_TIMING_FORMAT_2   0x3821
> > +#define OV9282_FLIP_BIT                      BIT(2)
> > +
>
> Can we remove them from the list of common registers or do those
> registers contains other settings which might get lost ?

They also contain the binning information, so generally best to keep
them in the mode list. (640x400 mode added later makes use of
binning).

> >  #define OV9282_REG_MIPI_CTRL00       0x4800
> >  #define OV9282_GATED_CLOCK   BIT(5)
> >
> > @@ -440,6 +444,38 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> >       return ret;
> >  }
> >
> > +static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value)
> > +{
> > +     u32 current_val;
> > +     int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
> > +                               &current_val);
> > +     if (!ret) {
> > +             if (value)
> > +                     current_val |= OV9282_FLIP_BIT;
> > +             else
> > +                     current_val &= ~OV9282_FLIP_BIT;
> > +             return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
> > +                                     current_val);
> > +     }
> > +     return ret;
>
> Or simply
>
>         int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
>                         &current_val);
>         if (ret)
>                 return ret;
>
>         if (value)
>                 current_val |= OV9282_FLIP_BIT;
>         else
>                 current_val &= ~OV9282_FLIP_BIT;
>
>         return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
>                                 current_val);
>

Done

> > +}
> > +
> > +static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
> > +{
> > +     u32 current_val;
> > +     int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
> > +                               &current_val);
> > +     if (!ret) {
> > +             if (value)
> > +                     current_val |= OV9282_FLIP_BIT;
> > +             else
> > +                     current_val &= ~OV9282_FLIP_BIT;
> > +             return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
> > +                                     current_val);
> > +     }
> > +     return ret;
> > +}
> > +
> >  /**
> >   * ov9282_set_ctrl() - Set subdevice control
> >   * @ctrl: pointer to v4l2_ctrl structure
> > @@ -484,7 +520,6 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >       switch (ctrl->id) {
> >       case V4L2_CID_EXPOSURE:
> > -
>
> Unrelated and possibly part of the previous patch ?
>
> >               exposure = ctrl->val;
> >               analog_gain = ov9282->again_ctrl->val;
> >
> > @@ -493,12 +528,17 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >               ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
> >
> > -
>
> same here
>
> >               break;
> >       case V4L2_CID_VBLANK:
> >               lpfr = ov9282->vblank + ov9282->cur_mode->height;
> >               ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
> >               break;
> > +     case V4L2_CID_HFLIP:
> > +             ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val);
> > +             break;
> > +     case V4L2_CID_VFLIP:
> > +             ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
> > +             break;
> >       default:
> >               dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >               ret = -EINVAL;
> > @@ -996,7 +1036,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >       u32 lpfr;
> >       int ret;
> >
> > -     ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> > +     ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> >       if (ret)
> >               return ret;
> >
> > @@ -1030,6 +1070,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >                                               mode->vblank_max,
> >                                               1, mode->vblank);
> >
> > +     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_VFLIP,
> > +                       0, 1, 1, 1);
> > +
> > +     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_HFLIP,
> > +                       0, 1, 1, 1);
>
> By default 0x3820 and 0x3821 are initialized to 0x3c and 0x84 which
> have BIT(2) set, so the image is "flipped" by default to compensate
> the sensor mounting orientation. For users though the image appears
> "not flipped", and if they have to set the control to 0 to flip it it
> would feel a bit weird. Should the logic be inverted ? ie setting the
> FLIP controls value to 1 results in BIT(2) being cleared ?

This comes back to my comment at the Dublin Media Summit of wanting
fully featured drivers instead of product specific ones.

Drivers have been accepted with no flip controls and some form of
inversion buried in the registers. I'll acknowledge that restrictions
on datasheets makes it almost impossible for reviewers to pick up on
this, but we could push for flip controls to be mandatory in future if
the sensor supports it.
Looking at imx258 we again have a hardcoded rotation of 180degrees[1],
and it won't even probe if a "rotation" property isn't set to 180 [2].
It seems to be almost normal for the company who submitted both imx258
and ov9282 drivers that their sensors are inverted.

I guess the question really comes down to how HFLIP and VFLIP should
be defined. Are they relative to the platform, or relative to the
sensor vendor's definition? If relative to the platform, then do we
merge in V4L2_CID_CAMERA_SENSOR_ROTATION somehow?
With Bayer sensors it gets even more confusing as the flips typically
change the Bayer order, so someone referencing the datasheet will
start scratching their head over why they're getting BGGR when they
haven't asked for flips despite the datasheet saying the sensor is
RGGB.

Perhaps a more general discussion on how to handle these cases of
adding HFLIP and VFLIP is warranted.

If necessary then I'll drop this patch for now, but some path for
adding these features needs to be formulated. OV7251 is next on my
list to send patches, and then IMX258, so the subject will come up
again very soon.

  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L968
[2] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L1283

> > +
> >       /* Read only controls */
> >       v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> >                         OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> > --
> > 2.34.1
> >