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.
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.
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.
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);
> >
> >
> >
> >
