[1/4] media: i2c: st-vgxy61: Use sub-device active state
Commit Message
Use sub-device active state. Rely on control handler lock to serialize
access to the active state.
Signed-off-by: Julien Massot <julien.massot@collabora.com>
---
drivers/media/i2c/st-vgxy61.c | 109 ++++++++++++----------------------
1 file changed, 39 insertions(+), 70 deletions(-)
Comments
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@collabora.com>
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);
@@ -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);