| Message ID | 20230131192016.3476937-9-dave.stevenson@raspberrypi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | imx290: Minor fixes, support for alternate INCK, and more ctrls | expand |
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. 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);
Hi Dave, Thank you for the patch. On Tue, Jan 31, 2023 at 07:20:13PM +0000, Dave Stevenson wrote: > The driver exposed V4L2_CID_HBLANK as a read only control to allow s/HBLANK/VBLANK/ > 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. I would have split this to a separate patch. > 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; You could skip this and use imx290->exposure below instead. I see pros and cons, up to you. > + 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; The relationship between blanking and exposure is painful to handle :-( Simplifying it is probably my number one wish for a good V4L2 raw sensor helper. Just a bit of ranting, not a candidate for this series of course. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > 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); > } > > 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);
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. 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); > > > >
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. 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); > > > > > > > >
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);
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); } 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);
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(-)