| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <175760104354.935713.5466359754087343394@ping.linuxembedded.co.uk>
Date: Thu, 11 Sep 2025 15:30:43 +0100
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Jai Luthra <jai.luthra@...asonboard.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, Tommaso Merciai <tomm.merciai@...il.com>
Subject: Re: [PATCH v2 6/8] media: imx355: Use subdev active state
Quoting Jai Luthra (2025-09-11 09:14:22)
> Port the driver to use the subdev active state. This simplifies locking,
> and makes it easier to support different crop sizes for binned modes, by
> storing the crop rectangle inside the subdev state.
>
> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> ---
> drivers/media/i2c/imx335.c | 79 ++++++++++++----------------------------------
> 1 file changed, 21 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 7631f41e6f1e65695fb76a66d9ac5a3588c69658..9b9bbe6473d545bc5618c0fe8191df0b18037cd8 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -204,7 +204,6 @@ struct imx335_mode {
> * @vblank: Vertical blanking in lines
> * @lane_mode: Mode for number of connected data lanes
> * @cur_mode: Pointer to current selected sensor mode
> - * @mutex: Mutex for serializing sensor controls
> * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
> * @cur_mbus_code: Currently selected media bus format code
> */
> @@ -231,7 +230,6 @@ struct imx335 {
> u32 vblank;
> u32 lane_mode;
> const struct imx335_mode *cur_mode;
> - struct mutex mutex;
> unsigned long link_freq_bitmap;
> u32 cur_mbus_code;
> };
> @@ -766,36 +764,6 @@ static void imx335_fill_pad_format(struct imx335 *imx335,
> fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
> }
>
> -/**
> - * imx335_get_pad_format() - Get subdevice pad format
> - * @sd: pointer to imx335 V4L2 sub-device structure
> - * @sd_state: V4L2 sub-device configuration
> - * @fmt: V4L2 sub-device format need to be set
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int imx335_get_pad_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> -{
> - struct imx335 *imx335 = to_imx335(sd);
> -
> - mutex_lock(&imx335->mutex);
> -
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - struct v4l2_mbus_framefmt *framefmt;
> -
> - framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> - fmt->format = *framefmt;
> - } else {
> - imx335_fill_pad_format(imx335, imx335->cur_mode, fmt);
> - }
> -
> - mutex_unlock(&imx335->mutex);
> -
> - return 0;
> -}
> -
> /**
> * imx335_set_pad_format() - Set subdevice pad format
> * @sd: pointer to imx335 V4L2 sub-device structure
> @@ -809,12 +777,12 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *fmt)
> {
> struct imx335 *imx335 = to_imx335(sd);
> + struct v4l2_mbus_framefmt *format;
> const struct imx335_mode *mode;
> int i, ret = 0;
>
> - mutex_lock(&imx335->mutex);
> -
> mode = &supported_mode;
> +
> for (i = 0; i < ARRAY_SIZE(imx335_mbus_codes); i++) {
> if (imx335_mbus_codes[i] == fmt->format.code)
> imx335->cur_mbus_code = imx335_mbus_codes[i];
> @@ -822,19 +790,15 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
>
> imx335_fill_pad_format(imx335, mode, fmt);
>
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - struct v4l2_mbus_framefmt *framefmt;
> + format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> + *format = fmt->format;
>
> - framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> - *framefmt = fmt->format;
> - } else {
It's nice to drop that - and clean up the locking.
Reviewed-by: Kieran Bingham <kieran.bingham@...asonboard.com>
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> ret = imx335_update_controls(imx335, mode);
> if (!ret)
> imx335->cur_mode = mode;
> }
>
> - mutex_unlock(&imx335->mutex);
> -
> return ret;
> }
>
> @@ -854,12 +818,10 @@ static int imx335_init_state(struct v4l2_subdev *sd,
> fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> imx335_fill_pad_format(imx335, &supported_mode, &fmt);
>
> - mutex_lock(&imx335->mutex);
> __v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
> __fls(imx335->link_freq_bitmap),
> ~(imx335->link_freq_bitmap),
> __ffs(imx335->link_freq_bitmap));
> - mutex_unlock(&imx335->mutex);
>
> return imx335_set_pad_format(sd, sd_state, &fmt);
> }
> @@ -1001,16 +963,17 @@ static void imx335_stop_streaming(struct imx335 *imx335)
> static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
> {
> struct imx335 *imx335 = to_imx335(sd);
> + struct v4l2_subdev_state *state;
> int ret = 0;
>
> - mutex_lock(&imx335->mutex);
> + state = v4l2_subdev_lock_and_get_active_state(sd);
>
> if (enable)
> ret = imx335_start_streaming(imx335);
> else
> imx335_stop_streaming(imx335);
>
> - mutex_unlock(&imx335->mutex);
> + v4l2_subdev_unlock_state(state);
>
> return ret;
> }
> @@ -1138,7 +1101,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
> .enum_frame_size = imx335_enum_frame_size,
> .get_selection = imx335_get_selection,
> .set_selection = imx335_get_selection,
> - .get_fmt = imx335_get_pad_format,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = imx335_set_pad_format,
> };
>
> @@ -1233,9 +1196,6 @@ static int imx335_init_controls(struct imx335 *imx335)
> if (ret)
> return ret;
>
> - /* Serialize controls with sensor device */
> - ctrl_hdlr->lock = &imx335->mutex;
> -
> /* Initialize exposure and gain */
> lpfr = mode->vblank + mode->height;
> imx335->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> @@ -1355,12 +1315,10 @@ static int imx335_probe(struct i2c_client *client)
> return ret;
> }
>
> - mutex_init(&imx335->mutex);
> -
> ret = imx335_power_on(imx335->dev);
> if (ret) {
> dev_err(imx335->dev, "failed to power-on the sensor\n");
> - goto error_mutex_destroy;
> + return ret;
> }
>
> /* Check module identity */
> @@ -1393,11 +1351,18 @@ static int imx335_probe(struct i2c_client *client)
> goto error_handler_free;
> }
>
> + imx335->sd.state_lock = imx335->ctrl_handler.lock;
> + ret = v4l2_subdev_init_finalize(&imx335->sd);
> + if (ret < 0) {
> + dev_err(imx335->dev, "subdev init error\n");
> + goto error_media_entity;
> + }
> +
> ret = v4l2_async_register_subdev_sensor(&imx335->sd);
> if (ret < 0) {
> dev_err(imx335->dev,
> "failed to register async subdev: %d\n", ret);
> - goto error_media_entity;
> + goto error_subdev_cleanup;
> }
>
> pm_runtime_set_active(imx335->dev);
> @@ -1406,14 +1371,14 @@ static int imx335_probe(struct i2c_client *client)
>
> return 0;
>
> +error_subdev_cleanup:
> + v4l2_subdev_cleanup(&imx335->sd);
> error_media_entity:
> media_entity_cleanup(&imx335->sd.entity);
> error_handler_free:
> v4l2_ctrl_handler_free(imx335->sd.ctrl_handler);
> error_power_off:
> imx335_power_off(imx335->dev);
> -error_mutex_destroy:
> - mutex_destroy(&imx335->mutex);
>
> return ret;
> }
> @@ -1427,9 +1392,9 @@ static int imx335_probe(struct i2c_client *client)
> static void imx335_remove(struct i2c_client *client)
> {
> struct v4l2_subdev *sd = i2c_get_clientdata(client);
> - struct imx335 *imx335 = to_imx335(sd);
>
> v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> media_entity_cleanup(&sd->entity);
> v4l2_ctrl_handler_free(sd->ctrl_handler);
>
> @@ -1437,8 +1402,6 @@ static void imx335_remove(struct i2c_client *client)
> if (!pm_runtime_status_suspended(&client->dev))
> imx335_power_off(&client->dev);
> pm_runtime_set_suspended(&client->dev);
> -
> - mutex_destroy(&imx335->mutex);
> }
>
> static const struct dev_pm_ops imx335_pm_ops = {
>
> --
> 2.51.0
>
Powered by blists - more mailing lists