diff mbox series

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

Message ID 20230131192016.3476937-9-dave.stevenson@raspberrypi.com (mailing list archive)
State Superseded
Headers
Series imx290: Minor fixes, support for alternate INCK, and more ctrls |

Commit Message

Dave Stevenson Jan. 31, 2023, 7:20 p.m. UTC 
  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(-)

Comments

Alexander Stein Feb. 2, 2023, 3:40 p.m. UTC | #1 
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);
Laurent Pinchart Feb. 2, 2023, 4:01 p.m. UTC | #2 
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);
Dave Stevenson Feb. 2, 2023, 4:04 p.m. UTC | #3 
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);
>
>
>
>
Dave Stevenson Feb. 2, 2023, 5:42 p.m. UTC | #4 
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);
> >
> >
> >
> >
Alexander Stein Feb. 3, 2023, 7:16 a.m. UTC | #5 
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 mbox series

Patch

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