Re: [PATCH v3 11/17] media: i2c: imx290: Use V4L2 subdev active state

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

Hi Laurent,

thanks for the update.

Am Montag, 16. Januar 2023, 15:44:48 CET schrieb Laurent Pinchart:
> Use the V4L2 subdev active state API to store the active format. This
> simplifies the driver not only by dropping the imx290 current_format
> field, but it also allows dropping the imx290 lock, replaced with the
> state lock.
> 
> The lock check in imx290_ctrl_update() can be dropped as
> imx290_set_fmt() can't be called anywmore with which set to ACTIVE
> before controls are initialized.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

LGTM.
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
> Changes since v2:
> 
> - Test for V4L2_CTRL_FLAG_READ_ONLY
> 
> Changes since v1:
> 
> - Remove lock check from imx290_ctrl_update()
> - Drop unrelated change
> - Fix error handling in imx290_subdev_init()
> - Handle errors from v4l2_subdev_init_finalize()
> - Return immediately from imx290_set_ctrl() for read-only controls
> - Call v4l2_subdev_cleanup()
> ---
>  drivers/media/i2c/imx290.c | 159 ++++++++++++++++---------------------
>  1 file changed, 70 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index c1d18ec51e41..74b9a7110709 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -177,12 +177,12 @@ struct imx290 {
>  	struct clk *xclk;
>  	struct regmap *regmap;
>  	u8 nlanes;
> -	u8 bpp;
> 
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
> -	struct v4l2_mbus_framefmt current_format;
> +
>  	const struct imx290_mode *current_mode;
> +	u8 bpp;
> 
>  	struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
>  	struct gpio_desc *rst_gpio;
> @@ -192,8 +192,6 @@ struct imx290 {
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *vblank;
> -
> -	struct mutex lock;
>  };
> 
>  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> @@ -524,13 +522,14 @@ static int imx290_set_black_level(struct imx290
> *imx290, black_level >> (16 - imx290->bpp), err);
>  }
> 
> -static int imx290_write_current_format(struct imx290 *imx290)
> +static int imx290_setup_format(struct imx290 *imx290,
> +			       const struct v4l2_mbus_framefmt *format)
>  {
>  	const struct imx290_regval *regs;
>  	unsigned int num_regs;
>  	int ret;
> 
> -	switch (imx290->current_format.code) {
> +	switch (format->code) {
>  	case MEDIA_BUS_FMT_SRGGB10_1X10:
>  		regs = imx290_10bit_settings;
>  		num_regs = ARRAY_SIZE(imx290_10bit_settings);
> @@ -561,12 +560,26 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct imx290 *imx290 = container_of(ctrl->handler,
>  					     struct imx290, ctrls);
> +	const struct v4l2_mbus_framefmt *format;
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
> 
> +	/*
> +	 * Return immediately for controls that don't need to be applied to 
the
> +	 * device. Those controls are modified in imx290_ctrl_update(), 
which
> +	 * is called at probe time before runtime PM is initialized, so we
> +	 * can't proceed to the pm_runtime_get_if_in_use() call below.
> +	 */
> +	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
> +		return 0;
> +
>  	/* V4L2 controls values will be applied only when power is already 
up */
>  	if (!pm_runtime_get_if_in_use(imx290->dev))
>  		return 0;
> 
> +	state = v4l2_subdev_get_locked_active_state(&imx290->sd);
> +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_ANALOGUE_GAIN:
>  		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
> @@ -592,6 +605,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  					       &ret);
>  		}
>  		break;
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -625,13 +639,6 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>  	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode-
>link_freq_index];
>  	u64 pixel_rate;
> 
> -	/*
> -	 * This function may be called from imx290_set_fmt() before controls
> -	 * get created by imx290_ctrl_init(). Return immediately in that 
case.
> -	 */
> -	if (!imx290->ctrls.lock)
> -		return;
> -
>  	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
>  	pixel_rate = link_freq * 2 * imx290->nlanes;
>  	do_div(pixel_rate, imx290->bpp);
> @@ -653,7 +660,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  		return ret;
> 
>  	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
> -	imx290->ctrls.lock = &imx290->lock;
> 
>  	/*
>  	 * The sensor has an analog gain and a digital gain, both controlled
> @@ -718,10 +724,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  		return ret;
>  	}
> 
> -	mutex_lock(imx290->ctrls.lock);
> -	imx290_ctrl_update(imx290, imx290->current_mode);
> -	mutex_unlock(imx290->ctrls.lock);
> -
>  	return 0;
>  }
> 
> @@ -730,8 +732,10 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>   */
> 
>  /* Start streaming */
> -static int imx290_start_streaming(struct imx290 *imx290)
> +static int imx290_start_streaming(struct imx290 *imx290,
> +				  struct v4l2_subdev_state *state)
>  {
> +	const struct v4l2_mbus_framefmt *format;
>  	int ret;
> 
>  	/* Set init register settings */
> @@ -744,7 +748,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
> }
> 
>  	/* Apply the register values related to current frame format */
> -	ret = imx290_write_current_format(imx290);
> +	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
> +	ret = imx290_setup_format(imx290, format);
>  	if (ret < 0) {
>  		dev_err(imx290->dev, "Could not set frame format\n");
>  		return ret;
> @@ -764,7 +769,7 @@ static int imx290_start_streaming(struct imx290 *imx290)
> return ret;
> 
>  	/* Apply customized values from user */
> -	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> +	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
>  	if (ret) {
>  		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
>  		return ret;
> @@ -793,39 +798,32 @@ static int imx290_stop_streaming(struct imx290
> *imx290) static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct imx290 *imx290 = to_imx290(sd);
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
> 
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +
>  	if (enable) {
>  		ret = pm_runtime_resume_and_get(imx290->dev);
>  		if (ret < 0)
> -			goto unlock_and_return;
> +			goto unlock;
> 
> -		ret = imx290_start_streaming(imx290);
> +		ret = imx290_start_streaming(imx290, state);
>  		if (ret) {
>  			dev_err(imx290->dev, "Start stream failed\n");
>  			pm_runtime_put(imx290->dev);
> -			goto unlock_and_return;
> +			goto unlock;
>  		}
>  	} else {
>  		imx290_stop_streaming(imx290);
>  		pm_runtime_put(imx290->dev);
>  	}
> 
> -unlock_and_return:
> -
> +unlock:
> +	v4l2_subdev_unlock_state(state);
>  	return ret;
>  }
> 
> -static struct v4l2_mbus_framefmt *
> -imx290_get_pad_format(struct imx290 *imx290, struct v4l2_subdev_state
> *state, -		      u32 which)
> -{
> -	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return &imx290->current_format;
> -	else
> -		return v4l2_subdev_get_try_format(&imx290->sd, state, 0);
> -}
> -
>  static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum 
*code)
> @@ -860,23 +858,6 @@ static int imx290_enum_frame_size(struct v4l2_subdev
> *sd, return 0;
>  }
> 
> -static int imx290_get_fmt(struct v4l2_subdev *sd,
> -			  struct v4l2_subdev_state *sd_state,
> -			  struct v4l2_subdev_format *fmt)
> -{
> -	struct imx290 *imx290 = to_imx290(sd);
> -	struct v4l2_mbus_framefmt *framefmt;
> -
> -	mutex_lock(&imx290->lock);
> -
> -	framefmt = imx290_get_pad_format(imx290, sd_state, fmt->which);
> -	fmt->format = *framefmt;
> -
> -	mutex_unlock(&imx290->lock);
> -
> -	return 0;
> -}
> -
>  static int imx290_set_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_state *sd_state,
>  			  struct v4l2_subdev_format *fmt)
> @@ -886,8 +867,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *format;
>  	unsigned int i;
> 
> -	mutex_lock(&imx290->lock);
> -
>  	mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
>  				      imx290_modes_num(imx290), width, 
height,
>  				      fmt->format.width, fmt-
>format.height);
> @@ -905,7 +884,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  	fmt->format.code = imx290_formats[i].code;
>  	fmt->format.field = V4L2_FIELD_NONE;
> 
> -	format = imx290_get_pad_format(imx290, sd_state, fmt->which);
> +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> 
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		imx290->current_mode = mode;
> @@ -916,8 +895,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
> 
>  	*format = fmt->format;
> 
> -	mutex_unlock(&imx290->lock);
> -
>  	return 0;
>  }
> 
> @@ -925,14 +902,11 @@ static int imx290_get_selection(struct v4l2_subdev
> *sd, struct v4l2_subdev_state *sd_state,
>  				struct v4l2_subdev_selection *sel)
>  {
> -	struct imx290 *imx290 = to_imx290(sd);
>  	struct v4l2_mbus_framefmt *format;
> 
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP: {
> -		format = imx290_get_pad_format(imx290, sd_state, sel-
>which);
> -
> -		mutex_lock(&imx290->lock);
> +		format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> 
>  		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
>  			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - 
format->height) / 2;
> @@ -941,7 +915,6 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
>  		sel->r.width = format->width;
>  		sel->r.height = format->height;
> 
> -		mutex_unlock(&imx290->lock);
>  		return 0;
>  	}
> 
> @@ -970,11 +943,13 @@ static int imx290_get_selection(struct v4l2_subdev
> *sd, static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
>  				  struct v4l2_subdev_state *sd_state)
>  {
> -	struct v4l2_subdev_format fmt = { 0 };
> -
> -	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : 
V4L2_SUBDEV_FORMAT_ACTIVE;
> -	fmt.format.width = 1920;
> -	fmt.format.height = 1080;
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_TRY,
> +		.format = {
> +			.width = 1920,
> +			.height = 1080,
> +		},
> +	};
> 
>  	imx290_set_fmt(subdev, sd_state, &fmt);
> 
> @@ -989,7 +964,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops =
> { .init_cfg = imx290_entity_init_cfg,
>  	.enum_mbus_code = imx290_enum_mbus_code,
>  	.enum_frame_size = imx290_enum_frame_size,
> -	.get_fmt = imx290_get_fmt,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = imx290_set_fmt,
>  	.get_selection = imx290_get_selection,
>  };
> @@ -1008,18 +983,8 @@ static int imx290_subdev_init(struct imx290 *imx290)
>  	struct i2c_client *client = to_i2c_client(imx290->dev);
>  	int ret;
> 
> -	/*
> -	 * Initialize the frame format. In particular, imx290->current_mode
> -	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() 
call
> -	 * below relies on these fields.
> -	 */
> -	imx290_entity_init_cfg(&imx290->sd, NULL);
> -
> -	ret = imx290_ctrl_init(imx290);
> -	if (ret < 0) {
> -		dev_err(imx290->dev, "Control initialization error %d\n", 
ret);
> -		return ret;
> -	}
> +	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> +	imx290->bpp = imx290_formats[0].bpp;
> 
>  	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
>  	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1031,15 +996,37 @@ static int imx290_subdev_init(struct imx290 *imx290)
>  	ret = media_entity_pads_init(&imx290->sd.entity, 1, &imx290->pad);
>  	if (ret < 0) {
>  		dev_err(imx290->dev, "Could not register media entity\n");
> -		v4l2_ctrl_handler_free(&imx290->ctrls);
>  		return ret;
>  	}
> 
> +	ret = imx290_ctrl_init(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Control initialization error %d\n", 
ret);
> +		goto err_media;
> +	}
> +
> +	imx290->sd.state_lock = imx290->ctrls.lock;
> +
> +	ret = v4l2_subdev_init_finalize(&imx290->sd);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "subdev initialization error %d\n", 
ret);
> +		goto err_ctrls;
> +	}
> +
> +	imx290_ctrl_update(imx290, imx290->current_mode);
> +
>  	return 0;
> +
> +err_ctrls:
> +	v4l2_ctrl_handler_free(&imx290->ctrls);
> +err_media:
> +	media_entity_cleanup(&imx290->sd.entity);
> +	return ret;
>  }
> 
>  static void imx290_subdev_cleanup(struct imx290 *imx290)
>  {
> +	v4l2_subdev_cleanup(&imx290->sd);
>  	media_entity_cleanup(&imx290->sd.entity);
>  	v4l2_ctrl_handler_free(&imx290->ctrls);
>  }
> @@ -1270,12 +1257,10 @@ static int imx290_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
> 
> -	mutex_init(&imx290->lock);
> -
>  	/* Initialize and register subdev. */
>  	ret = imx290_subdev_init(imx290);
>  	if (ret)
> -		goto err_mutex;
> +		return ret;
> 
>  	ret = v4l2_async_register_subdev(&imx290->sd);
>  	if (ret < 0) {
> @@ -1298,8 +1283,6 @@ static int imx290_probe(struct i2c_client *client)
> 
>  err_subdev:
>  	imx290_subdev_cleanup(imx290);
> -err_mutex:
> -	mutex_destroy(&imx290->lock);
> 
>  	return ret;
>  }
> @@ -1312,8 +1295,6 @@ static void imx290_remove(struct i2c_client *client)
>  	v4l2_async_unregister_subdev(sd);
>  	imx290_subdev_cleanup(imx290);
> 
> -	mutex_destroy(&imx290->lock);
> -
>  	pm_runtime_disable(imx290->dev);
>  	if (!pm_runtime_status_suspended(imx290->dev))
>  		imx290_power_off(imx290->dev);