On Fri, Jan 27, 2023 at 02:31:24AM +0200, Laurent Pinchart wrote:
> Use the V4L2 subdev active state API to store the active format and crop
> rectangle. This simplifies the driver not only by dropping the state
> stored in the rkisp1_csi structure, but also by replacing the ops_lock
> with the state lock.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Reviewed-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> ---
> .../platform/rockchip/rkisp1/rkisp1-common.h | 6 -
> .../platform/rockchip/rkisp1/rkisp1-csi.c | 107 ++++++------------
> 2 files changed, 33 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index d43ff1b549d9..dbd8725f2ec9 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -167,9 +167,6 @@ struct rkisp1_sensor_async {
> * @is_dphy_errctrl_disabled: if dphy errctrl is disabled (avoid endless interrupt)
> * @sd: v4l2_subdev variable
> * @pads: media pads
> - * @pad_cfg: configurations for the pads
> - * @sink_fmt: input format
> - * @lock: protects pad_cfg and sink_fmt
> * @source: source in-use, set when starting streaming
> */
> struct rkisp1_csi {
> @@ -178,9 +175,6 @@ struct rkisp1_csi {
> bool is_dphy_errctrl_disabled;
> struct v4l2_subdev sd;
> struct media_pad pads[RKISP1_CSI_PAD_NUM];
> - struct v4l2_subdev_pad_config pad_cfg[RKISP1_CSI_PAD_NUM];
> - const struct rkisp1_mbus_info *sink_fmt;
> - struct mutex lock;
> struct v4l2_subdev *source;
> };
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> index d7acc94e10f8..d8d50270f6db 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> @@ -30,23 +30,6 @@ static inline struct rkisp1_csi *to_rkisp1_csi(struct v4l2_subdev *sd)
> return container_of(sd, struct rkisp1_csi, sd);
> }
>
> -static struct v4l2_mbus_framefmt *
> -rkisp1_csi_get_pad_fmt(struct rkisp1_csi *csi,
> - struct v4l2_subdev_state *sd_state,
> - unsigned int pad, u32 which)
> -{
> - struct v4l2_subdev_state state = {
> - .pads = csi->pad_cfg
> - };
> -
> - lockdep_assert_held(&csi->lock);
> -
> - if (which == V4L2_SUBDEV_FORMAT_TRY)
> - return v4l2_subdev_get_try_format(&csi->sd, sd_state, pad);
> - else
> - return v4l2_subdev_get_try_format(&csi->sd, &state, pad);
> -}
> -
> int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
> struct rkisp1_sensor_async *s_asd,
> unsigned int source_pad)
> @@ -76,7 +59,8 @@ int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
> }
>
> static int rkisp1_csi_config(struct rkisp1_csi *csi,
> - const struct rkisp1_sensor_async *sensor)
> + const struct rkisp1_sensor_async *sensor,
> + const struct rkisp1_mbus_info *format)
> {
> struct rkisp1_device *rkisp1 = csi->rkisp1;
> unsigned int lanes = sensor->lanes;
> @@ -98,7 +82,7 @@ static int rkisp1_csi_config(struct rkisp1_csi *csi,
>
> /* Configure Data Type and Virtual Channel */
> rkisp1_write(rkisp1, RKISP1_CIF_MIPI_IMG_DATA_SEL,
> - RKISP1_CIF_MIPI_DATA_SEL_DT(csi->sink_fmt->mipi_dt) |
> + RKISP1_CIF_MIPI_DATA_SEL_DT(format->mipi_dt) |
> RKISP1_CIF_MIPI_DATA_SEL_VC(0));
>
> /* Clear MIPI interrupts */
> @@ -151,7 +135,8 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi)
> }
>
> static int rkisp1_csi_start(struct rkisp1_csi *csi,
> - const struct rkisp1_sensor_async *sensor)
> + const struct rkisp1_sensor_async *sensor,
> + const struct rkisp1_mbus_info *format)
> {
> struct rkisp1_device *rkisp1 = csi->rkisp1;
> union phy_configure_opts opts;
> @@ -159,7 +144,7 @@ static int rkisp1_csi_start(struct rkisp1_csi *csi,
> s64 pixel_clock;
> int ret;
>
> - ret = rkisp1_csi_config(csi, sensor);
> + ret = rkisp1_csi_config(csi, sensor, format);
> if (ret)
> return ret;
>
> @@ -169,7 +154,7 @@ static int rkisp1_csi_start(struct rkisp1_csi *csi,
> return -EINVAL;
> }
>
> - phy_mipi_dphy_get_default_config(pixel_clock, csi->sink_fmt->bus_width,
> + phy_mipi_dphy_get_default_config(pixel_clock, format->bus_width,
> sensor->lanes, cfg);
> phy_set_mode(csi->dphy, PHY_MODE_MIPI_DPHY);
> phy_configure(csi->dphy, &opts);
> @@ -248,7 +233,6 @@ static int rkisp1_csi_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> - struct rkisp1_csi *csi = to_rkisp1_csi(sd);
> unsigned int i;
> int pos = 0;
>
> @@ -258,15 +242,10 @@ static int rkisp1_csi_enum_mbus_code(struct v4l2_subdev *sd,
> if (code->index)
> return -EINVAL;
>
> - mutex_lock(&csi->lock);
> -
> - sink_fmt = rkisp1_csi_get_pad_fmt(csi, sd_state,
> - RKISP1_CSI_PAD_SINK,
> - code->which);
> + sink_fmt = v4l2_subdev_get_pad_format(sd, sd_state,
> + RKISP1_CSI_PAD_SINK);
> code->code = sink_fmt->code;
>
> - mutex_unlock(&csi->lock);
> -
> return 0;
> }
>
> @@ -296,9 +275,9 @@ static int rkisp1_csi_init_config(struct v4l2_subdev *sd,
> {
> struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
>
> - sink_fmt = v4l2_subdev_get_try_format(sd, sd_state,
> + sink_fmt = v4l2_subdev_get_pad_format(sd, sd_state,
> RKISP1_CSI_PAD_SINK);
> - src_fmt = v4l2_subdev_get_try_format(sd, sd_state,
> + src_fmt = v4l2_subdev_get_pad_format(sd, sd_state,
> RKISP1_CSI_PAD_SRC);
>
> sink_fmt->width = RKISP1_DEFAULT_WIDTH;
> @@ -311,36 +290,18 @@ static int rkisp1_csi_init_config(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static int rkisp1_csi_get_fmt(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> -{
> - struct rkisp1_csi *csi = to_rkisp1_csi(sd);
> -
> - mutex_lock(&csi->lock);
> - fmt->format = *rkisp1_csi_get_pad_fmt(csi, sd_state, fmt->pad,
> - fmt->which);
> - mutex_unlock(&csi->lock);
> -
> - return 0;
> -}
> -
> static int rkisp1_csi_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *fmt)
> {
> - struct rkisp1_csi *csi = to_rkisp1_csi(sd);
> const struct rkisp1_mbus_info *mbus_info;
> struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
>
> /* The format on the source pad always matches the sink pad. */
> if (fmt->pad == RKISP1_CSI_PAD_SRC)
> - return rkisp1_csi_get_fmt(sd, sd_state, fmt);
> + return v4l2_subdev_get_fmt(sd, sd_state, fmt);
>
> - mutex_lock(&csi->lock);
> -
> - sink_fmt = rkisp1_csi_get_pad_fmt(csi, sd_state, RKISP1_CSI_PAD_SINK,
> - fmt->which);
> + sink_fmt = v4l2_subdev_get_pad_format(sd, sd_state, RKISP1_CSI_PAD_SINK);
>
> sink_fmt->code = fmt->format.code;
>
> @@ -359,16 +320,10 @@ static int rkisp1_csi_set_fmt(struct v4l2_subdev *sd,
>
> fmt->format = *sink_fmt;
>
> - if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> - csi->sink_fmt = mbus_info;
> -
> /* Propagate the format to the source pad. */
> - src_fmt = rkisp1_csi_get_pad_fmt(csi, sd_state, RKISP1_CSI_PAD_SRC,
> - fmt->which);
> + src_fmt = v4l2_subdev_get_pad_format(sd, sd_state, RKISP1_CSI_PAD_SRC);
> *src_fmt = *sink_fmt;
>
> - mutex_unlock(&csi->lock);
> -
> return 0;
> }
>
> @@ -380,7 +335,10 @@ static int rkisp1_csi_s_stream(struct v4l2_subdev *sd, int enable)
> {
> struct rkisp1_csi *csi = to_rkisp1_csi(sd);
> struct rkisp1_device *rkisp1 = csi->rkisp1;
> + const struct v4l2_mbus_framefmt *sink_fmt;
> + const struct rkisp1_mbus_info *format;
> struct rkisp1_sensor_async *source_asd;
> + struct v4l2_subdev_state *sd_state;
> struct media_pad *source_pad;
> struct v4l2_subdev *source;
> int ret;
> @@ -410,9 +368,12 @@ static int rkisp1_csi_s_stream(struct v4l2_subdev *sd, int enable)
> if (source_asd->mbus_type != V4L2_MBUS_CSI2_DPHY)
> return -EINVAL;
>
> - mutex_lock(&csi->lock);
> - ret = rkisp1_csi_start(csi, source_asd);
> - mutex_unlock(&csi->lock);
> + sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> + sink_fmt = v4l2_subdev_get_pad_format(sd, sd_state, RKISP1_CSI_PAD_SINK);
> + format = rkisp1_mbus_info_get_by_code(sink_fmt->code);
> + v4l2_subdev_unlock_state(sd_state);
> +
> + ret = rkisp1_csi_start(csi, source_asd, format);
> if (ret)
> return ret;
>
> @@ -442,7 +403,7 @@ static const struct v4l2_subdev_video_ops rkisp1_csi_video_ops = {
> static const struct v4l2_subdev_pad_ops rkisp1_csi_pad_ops = {
> .enum_mbus_code = rkisp1_csi_enum_mbus_code,
> .init_cfg = rkisp1_csi_init_config,
> - .get_fmt = rkisp1_csi_get_fmt,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = rkisp1_csi_set_fmt,
> };
>
> @@ -454,13 +415,11 @@ static const struct v4l2_subdev_ops rkisp1_csi_ops = {
> int rkisp1_csi_register(struct rkisp1_device *rkisp1)
> {
> struct rkisp1_csi *csi = &rkisp1->csi;
> - struct v4l2_subdev_state state = {};
> struct media_pad *pads;
> struct v4l2_subdev *sd;
> int ret;
>
> csi->rkisp1 = rkisp1;
> - mutex_init(&csi->lock);
>
> sd = &csi->sd;
> v4l2_subdev_init(sd, &rkisp1_csi_ops);
> @@ -476,26 +435,26 @@ int rkisp1_csi_register(struct rkisp1_device *rkisp1)
> pads[RKISP1_CSI_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE |
> MEDIA_PAD_FL_MUST_CONNECT;
>
> - csi->sink_fmt = rkisp1_mbus_info_get_by_code(RKISP1_CSI_DEF_FMT);
> -
> ret = media_entity_pads_init(&sd->entity, RKISP1_CSI_PAD_NUM, pads);
> if (ret)
> - goto error;
> + goto err_entity_cleanup;
>
> - state.pads = csi->pad_cfg;
> - rkisp1_csi_init_config(sd, &state);
> + ret = v4l2_subdev_init_finalize(sd);
> + if (ret)
> + goto err_entity_cleanup;
>
> ret = v4l2_device_register_subdev(&csi->rkisp1->v4l2_dev, sd);
> if (ret) {
> dev_err(sd->dev, "Failed to register csi receiver subdev\n");
> - goto error;
> + goto err_subdev_cleanup;
> }
>
> return 0;
>
> -error:
> +err_subdev_cleanup:
> + v4l2_subdev_cleanup(sd);
> +err_entity_cleanup:
> media_entity_cleanup(&sd->entity);
> - mutex_destroy(&csi->lock);
> csi->rkisp1 = NULL;
> return ret;
> }
> @@ -508,8 +467,8 @@ void rkisp1_csi_unregister(struct rkisp1_device *rkisp1)
> return;
>
> v4l2_device_unregister_subdev(&csi->sd);
> + v4l2_subdev_cleanup(&csi->sd);
> media_entity_cleanup(&csi->sd.entity);
> - mutex_destroy(&csi->lock);
> }
>
> int rkisp1_csi_init(struct rkisp1_device *rkisp1)
> --
> Regards,
>
> Laurent Pinchart
>
