Am Donnerstag, 2. Februar 2023, 18:42:43 CET schrieb Dave Stevenson:
> On Thu, 2 Feb 2023 at 16:04, Dave Stevenson
>
> <dave.stevenson@xxxxxxxxxxxxxxx> wrote:
> > Hi Alexander
> >
> > On Thu, 2 Feb 2023 at 15:40, Alexander Stein
> >
> > <alexander.stein@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > >
> > > > 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);
