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

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

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