Hi Julien,
Thank you for your patch.
First, sorry for the delay. I have a lot of stuff ongoing. I'll be more
available now.
Second, I don't currently have a setup that can handle the multi stream
serie. Therefore I'm not able to test your serie. Following your
patchset acquiring such a platform went up in my todo list.
On 3/15/24 09:51, Julien Massot wrote:
> Use sub-device active state. Rely on control handler lock to serialize
> access to the active state.
>
> Signed-off-by: Julien Massot <julien.massot@xxxxxxxxxxxxx>
I have yet to dive deep into active states.
I find curious that the 'current_mode' field in vgxy61_dev is still
here. From my understanding it should be replaced by
'v4l2_subdev_state_get_format', and all the 'current_mode->crop'
replaced by 'v4l2_subdev_state_get_crop'.
Someone tell me if this is incorrect.
> ---
> drivers/media/i2c/st-vgxy61.c | 109 ++++++++++++----------------------
> 1 file changed, 39 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/media/i2c/st-vgxy61.c b/drivers/media/i2c/st-vgxy61.c
> index b9e7c57027b1..733713f909cf 100644
> --- a/drivers/media/i2c/st-vgxy61.c
> +++ b/drivers/media/i2c/st-vgxy61.c
> @@ -397,8 +397,6 @@ struct vgxy61_dev {
> u16 line_length;
> u16 rot_term;
> bool gpios_polarity;
> - /* Lock to protect all members below */
> - struct mutex lock;
> struct v4l2_ctrl_handler ctrl_handler;
> struct v4l2_ctrl *pixel_rate_ctrl;
> struct v4l2_ctrl *expo_ctrl;
> @@ -686,27 +684,6 @@ static int vgxy61_enum_mbus_code(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static int vgxy61_get_fmt(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *format)
> -{
> - struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
> - struct v4l2_mbus_framefmt *fmt;
> -
> - mutex_lock(&sensor->lock);
> -
> - if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> - fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
> - else
> - fmt = &sensor->fmt;
> -
> - format->format = *fmt;
> -
> - mutex_unlock(&sensor->lock);
> -
> - return 0;
> -}
> -
> static u16 vgxy61_get_vblank_min(struct vgxy61_dev *sensor,
> enum vgxy61_hdr_mode hdr)
> {
> @@ -1167,16 +1144,17 @@ static int vgxy61_stream_disable(struct vgxy61_dev *sensor)
> static int vgxy61_s_stream(struct v4l2_subdev *sd, int enable)
> {
> struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
> + struct v4l2_subdev_state *sd_state;
> int ret = 0;
>
> - mutex_lock(&sensor->lock);
> + sd_state = v4l2_subdev_lock_and_get_active_state(&sensor->sd);
>
> ret = enable ? vgxy61_stream_enable(sensor) :
> vgxy61_stream_disable(sensor);
> if (!ret)
> sensor->streaming = enable;
>
> - mutex_unlock(&sensor->lock);
> + v4l2_subdev_unlock_state(sd_state);
>
> return ret;
> }
> @@ -1187,51 +1165,39 @@ static int vgxy61_set_fmt(struct v4l2_subdev *sd,
> {
> struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
> const struct vgxy61_mode_info *new_mode;
> - struct v4l2_mbus_framefmt *fmt;
> int ret;
>
> - mutex_lock(&sensor->lock);
> -
> - if (sensor->streaming) {
> - ret = -EBUSY;
> - goto out;
> - }
> + if (sensor->streaming)
> + return -EBUSY;
>
> ret = vgxy61_try_fmt_internal(sd, &format->format, &new_mode);
> if (ret)
> - goto out;
> -
> - if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> - fmt = v4l2_subdev_state_get_format(sd_state, 0);
> - *fmt = format->format;
> - } else if (sensor->current_mode != new_mode ||
> - sensor->fmt.code != format->format.code) {
> - fmt = &sensor->fmt;
> - *fmt = format->format;
> -
> - sensor->current_mode = new_mode;
> -
> - /* Reset vblank and framelength to default */
> - ret = vgxy61_update_vblank(sensor,
> - VGXY61_FRAME_LENGTH_DEF -
> - new_mode->crop.height,
> - sensor->hdr);
> -
> - /* Update controls to reflect new mode */
> - __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_ctrl,
> - get_pixel_rate(sensor));
> - __v4l2_ctrl_modify_range(sensor->vblank_ctrl,
> - sensor->vblank_min,
> - 0xffff - new_mode->crop.height,
> - 1, sensor->vblank);
> - __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl, sensor->vblank);
> - __v4l2_ctrl_modify_range(sensor->expo_ctrl, sensor->expo_min,
> - sensor->expo_max, 1,
> - sensor->expo_long);
> - }
> + return ret;
> +
> + *v4l2_subdev_state_get_format(sd_state, format->pad) = format->format;
>
> -out:
> - mutex_unlock(&sensor->lock);
> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> + return 0;
> +
> + sensor->current_mode = new_mode;
> +
> + /* Reset vblank and framelength to default */
> + ret = vgxy61_update_vblank(sensor,
> + VGXY61_FRAME_LENGTH_DEF -
> + new_mode->crop.height,
> + sensor->hdr);
> +
> + /* Update controls to reflect new mode */
> + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_ctrl,
> + get_pixel_rate(sensor));
> + __v4l2_ctrl_modify_range(sensor->vblank_ctrl,
> + sensor->vblank_min,
> + 0xffff - new_mode->crop.height,
> + 1, sensor->vblank);
> + __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl, sensor->vblank);
> + __v4l2_ctrl_modify_range(sensor->expo_ctrl, sensor->expo_min,
> + sensor->expo_max, 1,
> + sensor->expo_long);
>
> return ret;
> }
> @@ -1321,8 +1287,6 @@ static int vgxy61_init_controls(struct vgxy61_dev *sensor)
> int ret;
>
> v4l2_ctrl_handler_init(hdl, 16);
> - /* We can use our own mutex for the ctrl lock */
> - hdl->lock = &sensor->lock;
> v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1,
> sensor->analog_gain);
> v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1,
> @@ -1398,7 +1362,7 @@ static const struct v4l2_subdev_video_ops vgxy61_video_ops = {
>
> static const struct v4l2_subdev_pad_ops vgxy61_pad_ops = {
> .enum_mbus_code = vgxy61_enum_mbus_code,
> - .get_fmt = vgxy61_get_fmt,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = vgxy61_set_fmt,
> .get_selection = vgxy61_get_selection,
> .enum_frame_size = vgxy61_enum_frame_size,
> @@ -1801,7 +1765,7 @@ static int vgxy61_probe(struct i2c_client *client)
> vgxy61_fill_framefmt(sensor, sensor->current_mode, &sensor->fmt,
> VGXY61_MEDIA_BUS_FMT_DEF);
>
> - mutex_init(&sensor->lock);
> + sensor->sd.state_lock = sensor->ctrl_handler.lock;
>
> ret = vgxy61_update_hdr(sensor, sensor->hdr);
> if (ret)
> @@ -1820,6 +1784,10 @@ static int vgxy61_probe(struct i2c_client *client)
> goto error_handler_free;
> }
>
> + ret = v4l2_subdev_init_finalize(&sensor->sd);
> + if (ret)
> + goto error_media_entity_cleanup;
> +
> /* Enable runtime PM and turn off the device */
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> @@ -1841,11 +1809,12 @@ static int vgxy61_probe(struct i2c_client *client)
> error_pm_runtime:
> pm_runtime_disable(&client->dev);
> pm_runtime_set_suspended(&client->dev);
> + v4l2_subdev_cleanup(&sensor->sd);
> +error_media_entity_cleanup:
> media_entity_cleanup(&sensor->sd.entity);
> error_handler_free:
> v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
> error_power_off:
> - mutex_destroy(&sensor->lock);
> vgxy61_power_off(dev);
>
> return ret;
> @@ -1857,7 +1826,7 @@ static void vgxy61_remove(struct i2c_client *client)
> struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>
> v4l2_async_unregister_subdev(&sensor->sd);
> - mutex_destroy(&sensor->lock);
> + v4l2_subdev_cleanup(&sensor->sd);
> media_entity_cleanup(&sensor->sd.entity);
>
> pm_runtime_disable(&client->dev);
--
Regards,
Benjamin
