Re: [PATCH 08/11] media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Am Donnerstag, 2. Februar 2023, 18:42:43 CET schrieb Dave Stevenson:
> On Thu, 2 Feb 2023 at 16:04, Dave Stevenson
> 
> <dave.stevenson@raspberrypi.com> wrote:
> > Hi Alexander
> > 
> > On Thu, 2 Feb 2023 at 15:40, Alexander Stein
> > 
> > <alexander.stein@ew.tq-group.com> wrote:
> > > Hi Dave,
> > > 
> > > thanks for working on this.
> > > 
> > > Am Dienstag, 31. Januar 2023, 20:20:13 CET schrieb Dave Stevenson:
> > > > The driver exposed V4L2_CID_HBLANK as a read only control to allow
> > > > for exposure calculations and determination of the frame rate.
> > > > 
> > > > Convert to a read/write control so that the frame rate can be
> > > > controlled.
> > > > V4L2_CID_VBLANK also sets the limits for the exposure control,
> > > > therefore exposure ranges have to be updated when vblank changes.
> > > > 
> > > > This also updates the default VMAX in 720p mode from 1125 to
> > > > 750 to achieve 60fps and follow the datasheet.
> > > > 
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > > 
> > > >  drivers/media/i2c/imx290.c | 57
> > > >  ++++++++++++++++++++++++++++++--------
> > > >  1 file changed, 45 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 9006be6e5e7c..3413d83369ba 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -46,6 +46,7 @@
> > > > 
> > > >  #define IMX290_BLKLEVEL
> > > 
> > > IMX290_REG_16BIT(0x300a)
> > > 
> > > >  #define IMX290_GAIN
> > > 
> > > IMX290_REG_8BIT(0x3014)
> > > 
> > > >  #define IMX290_VMAX
> > > 
> > > IMX290_REG_24BIT(0x3018)
> > > 
> > > > +#define IMX290_VMAX_MAX                                      0x3ffff
> > > > 
> > > >  #define IMX290_HMAX
> > > 
> > > IMX290_REG_16BIT(0x301c)
> > > 
> > > >  #define IMX290_HMAX_MAX                                      0xffff
> > > >  #define IMX290_SHS1
> > > 
> > > IMX290_REG_24BIT(0x3020)
> > > 
> > > > @@ -106,8 +107,6 @@
> > > > 
> > > >  #define IMX290_PGCTRL_THRU                           BIT(1)
> > > >  #define IMX290_PGCTRL_MODE(n)                                ((n) <<
> > > 
> > > 4)
> > > 
> > > > -#define IMX290_VMAX_DEFAULT                          1125
> > > > -
> > > > 
> > > >  #define IMX290_PIXEL_RATE                            148500000
> > > >  /*
> > > >  
> > > >   * The IMX290 pixel array is organized as follows:
> > > > @@ -169,6 +168,7 @@ struct imx290_mode {
> > > > 
> > > >       u32 width;
> > > >       u32 height;
> > > >       u32 hmax_min;
> > > > 
> > > > +     u32 vmax_min;
> > > > 
> > > >       u8 link_freq_index;
> > > >       
> > > >       const struct imx290_regval *data;
> > > > 
> > > > @@ -206,6 +206,7 @@ struct imx290 {
> > > > 
> > > >       struct v4l2_ctrl *link_freq;
> > > >       struct v4l2_ctrl *hblank;
> > > >       struct v4l2_ctrl *vblank;
> > > > 
> > > > +     struct v4l2_ctrl *exposure;
> > > > 
> > > >  };
> > > >  
> > > >  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> > > > 
> > > > @@ -219,7 +220,6 @@ static inline struct imx290 *to_imx290(struct
> > > > v4l2_subdev *_sd)
> > > > 
> > > >  static const struct imx290_regval imx290_global_init_settings[] = {
> > > >  
> > > >       { IMX290_CTRL_07, IMX290_WINMODE_1080P },
> > > > 
> > > > -     { IMX290_VMAX, IMX290_VMAX_DEFAULT },
> > > > 
> > > >       { IMX290_EXTCK_FREQ, 0x2520 },
> > > >       { IMX290_WINWV_OB, 12 },
> > > >       { IMX290_WINPH, 0 },
> > > > 
> > > > @@ -412,6 +412,7 @@ static const struct imx290_mode
> > > > imx290_modes_2lanes[] =
> > > > { .width = 1920,
> > > > 
> > > >               .height = 1080,
> > > >               .hmax_min = 2200,
> > > > 
> > > > +             .vmax_min = 1125,
> > > > 
> > > >               .link_freq_index = FREQ_INDEX_1080P,
> > > >               .data = imx290_1080p_settings,
> > > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > > 
> > > > @@ -420,6 +421,7 @@ static const struct imx290_mode
> > > > imx290_modes_2lanes[] =
> > > > { .width = 1280,
> > > > 
> > > >               .height = 720,
> > > >               .hmax_min = 3300,
> > > > 
> > > > +             .vmax_min = 750,
> > > > 
> > > >               .link_freq_index = FREQ_INDEX_720P,
> > > >               .data = imx290_720p_settings,
> > > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > > 
> > > > @@ -431,6 +433,7 @@ static const struct imx290_mode
> > > > imx290_modes_4lanes[] =
> > > > { .width = 1920,
> > > > 
> > > >               .height = 1080,
> > > >               .hmax_min = 2200,
> > > > 
> > > > +             .vmax_min = 1125,
> > > > 
> > > >               .link_freq_index = FREQ_INDEX_1080P,
> > > >               .data = imx290_1080p_settings,
> > > >               .data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > > 
> > > > @@ -439,6 +442,7 @@ static const struct imx290_mode
> > > > imx290_modes_4lanes[] =
> > > > { .width = 1280,
> > > > 
> > > >               .height = 720,
> > > >               .hmax_min = 3300,
> > > > 
> > > > +             .vmax_min = 750,
> > > > 
> > > >               .link_freq_index = FREQ_INDEX_720P,
> > > >               .data = imx290_720p_settings,
> > > >               .data_size = ARRAY_SIZE(imx290_720p_settings),
> > > > 
> > > > @@ -645,7 +649,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > 
> > > >                                            struct imx290, ctrls);
> > > >       
> > > >       const struct v4l2_mbus_framefmt *format;
> > > >       struct v4l2_subdev_state *state;
> > > > 
> > > > -     int ret = 0;
> > > > +     int ret = 0, vmax;
> > > > 
> > > >       /*
> > > >       
> > > >        * Return immediately for controls that don't need to be applied
> > > >        to
> > > 
> > > the
> > > 
> > > > @@ -654,6 +658,18 @@ static int imx290_set_ctrl(struct v4l2_ctrl
> > > > *ctrl)
> > > > 
> > > >       if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
> > > >       
> > > >               return 0;
> > > > 
> > > > +     if (ctrl->id == V4L2_CID_VBLANK) {
> > > > +             u32 vmax = ctrl->val + imx290->current_mode->height;
> > > > +
> > > > +             /*
> > > > +              * Changing vblank changes the allowed range for
> > > > exposure.
> > > > +              * We don't supply the current exposure as default here
> > > > as
> > > 
> > > it
> > > 
> > > > +              * may lie outside the new range. We will reset it just
> > > 
> > > below.
> > > 
> > > > +              */
> > > > +             __v4l2_ctrl_modify_range(imx290->exposure,
> > > > +                                      1, vmax - 2, 1, vmax - 2);
> > > > +     }
> > > > +
> > > > 
> > > >       /* V4L2 controls values will be applied only when power is
> > > >       already
> > > 
> > > up */
> > > 
> > > >       if (!pm_runtime_get_if_in_use(imx290->dev))
> > > >       
> > > >               return 0;
> > > > 
> > > > @@ -666,9 +682,23 @@ static int imx290_set_ctrl(struct v4l2_ctrl
> > > > *ctrl)
> > > > 
> > > >               ret = imx290_write(imx290, IMX290_GAIN, ctrl->val,
> > > >               NULL);
> > > >               break;
> > > > 
> > > > +     case V4L2_CID_VBLANK:
> > > > +             ret = imx290_write(imx290, IMX290_VMAX,
> > > > +                                ctrl->val + imx290->current_mode-
> > > >
> > > >height,
> > > >
> > > > +                                NULL);
> > > > +             /*
> > > > +              * Due to the way that exposure is programmed in this
> > > 
> > > sensor in
> > > 
> > > > +              * relation to VMAX, we have to reprogramme it whenever
> > > 
> > > VMAX is
> > > 
> > > > +              * changed.
> > > > +              * Update ctrl so that the V4L2_CID_EXPOSURE case can
> > > 
> > > refer to
> > > 
> > > > +              * it.
> > > > +              */
> > > > +             ctrl = imx290->exposure;
> > > > +             fallthrough;
> > > > 
> > > >       case V4L2_CID_EXPOSURE:
> > > > +             vmax = imx290->vblank->val +
> > > > imx290->current_mode->height;
> > > > 
> > > >               ret = imx290_write(imx290, IMX290_SHS1,
> > > > 
> > > > -                                IMX290_VMAX_DEFAULT - ctrl->val -
> > > 
> > > 1, NULL);
> > > 
> > > > +                                vmax - ctrl->val - 1, NULL);
> > > > 
> > > >               break;
> > > >       
> > > >       case V4L2_CID_TEST_PATTERN:
> > > > @@ -725,13 +755,15 @@ static void imx290_ctrl_update(struct imx290
> > > > *imx290,
> > > > 
> > > >  {
> > > >  
> > > >       unsigned int hblank_min = mode->hmax_min - mode->width;
> > > >       unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > > > 
> > > > -     unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> > > > +     unsigned int vblank_min = mode->vmax_min - mode->height;
> > > > +     unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> > > > 
> > > >       __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> > > >       
> > > >       __v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max,
> > > >       1,
> > > >       
> > > >                                hblank_min);
> > > > 
> > > > -     __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1,
> > > > vblank);
> > > > +     __v4l2_ctrl_modify_range(imx290->vblank, vblank_min, vblank_max,
> > > > 1,
> > > > +                              vblank_min);
> > > 
> > > You are missing setting the range for V4L2_CID_EXPOSURE. So it stays at
> > > 1
> > > resulting in a black image.
> > 
> > You're right.
> > I'm always testing with libcamera which will set V4L2_CID_VBLANK to
> > configure the frame rate, and that will update the exposure range.
> 
> Actually I'm going to backtrack somewhat, although I may have
> misunderstood your comment.
> 
> imx290_subdev_init calls imx290_ctrl_update. VBLANK was created with
> min/max/def all set to 1. The update sets a range of 45 to 261063, so
> the old value is now invalid and the new default is adopted. That
> means set_ctrl is called for VBLANK, and that triggers the exposure
> range to be updated. There is no need to update the exposure range in
> imx290_ctrl_update on that basis.

This is how it should be and in imx290_set_ctrl is the bug. There is a 'switch 
(ctrl->id)' for fast return on ctrl which don't need writes to the device. But 
this changed on your series. Removing the fast return for both VBLANK and 
HBLANK I get proper settings for exposure.

> It is missed when changing mode as the current VBLANK value is still
> within range, therefore it doesn't need to change, and doesn't update
> the exposure range. I'll fix that.

But the default VBLANK of 1 is not in range of the minimum of 45, so there is 
definitely an update.

> Are you relying on the initial exposure to be some particular value?
> Is the initial value for any setting actually guaranteed in the spec?
> If your application is expecting max exposure by default, then that
> sounds very fragile and not portable to other sensors, but it can be
> fixed up.

I'm not relying on any specific value, everything is configured/controlled by 
libcamera.

Best regards,
Alexander

> Do note that switching to 1280x720 mode will limit the exposure range
> to 1-763. Switching back to 1920x1080 mode will increase the range
> again, but as the current value is still in range the exposure setting
> will not be changed. What controls get reset when changing mode is
> undefined, but AIUI it should update the ranges and leave everything
> else alone, which is what is done here. Some drivers pepper
> v4l2_ctrl_s_ctrl calls around the place, but what value to adopt seems
> random.
> 
>   Dave
> 
> >   Dave
> >   
> > > Best regards,
> > > Alexander
> > > 
> > > >  }
> > > >  
> > > >  static int imx290_ctrl_init(struct imx290 *imx290)
> > > > 
> > > > @@ -761,9 +793,12 @@ static int imx290_ctrl_init(struct imx290
> > > > *imx290)
> > > > 
> > > >       v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > > >       
> > > >                         V4L2_CID_ANALOGUE_GAIN, 0, 100, 1, 0);
> > > > 
> > > > -     v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > > > -                       V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2,
> > > 
> > > 1,
> > > 
> > > > -                       IMX290_VMAX_DEFAULT - 2);
> > > > +     /*
> > > > +      * Correct range will be determined through imx290_ctrl_update
> > > 
> > > setting
> > > 
> > > > +      * V4L2_CID_VBLANK.
> > > > +      */
> > > > +     imx290->exposure = v4l2_ctrl_new_std(&imx290->ctrls,
> > > 
> > > &imx290_ctrl_ops,
> > > 
> > > > +                                          V4L2_CID_EXPOSURE, 1,
> > > 
> > > 1, 1, 1);
> > > 
> > > >       /*
> > > >       
> > > >        * Set the link frequency, pixel rate, horizontal blanking and
> > > 
> > > vertical
> > > 
> > > > @@ -795,8 +830,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > > > 
> > > >       imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls,
> > > >       &imx290_ctrl_ops,
> > > >       
> > > >                                          V4L2_CID_VBLANK, 1, 1, 1,
> > > 
> > > 1);
> > > 
> > > > -     if (imx290->vblank)
> > > > -             imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > 
> > > >       v4l2_ctrl_new_fwnode_properties(&imx290->ctrls,
> > > >       &imx290_ctrl_ops,
> > > >       
> > > >                                       &props);