[PATCH 0/4] media: staging: rkisp1: add serialization to the isp and resizer ops

[PATCH 0/4] media: staging: rkisp1: add serialization to the isp and resizer ops

[Dafna Hirschfeld]

Subdevices' ops callbacks can be called either through uAPI with
ioctl or through kAPI with the v4l2_subdev_call macro. Therefore
the lock of the subdevice node is not set and instead the driver
itself is responsible for serializing the ops.
This patchset adds serialization to the isp and resizer subdevices.
A mutex field 'ops_lock' is added to the inner struct of each of them.

The ops_lock is then used in the following operations:

set_fmt, get_fmt, set_selection, get_selection, s_stream

Serialization for enum_mbus_code is not needed since this operation
is independent of the specific configuration.

Patches summary:

patch 1 Changes two functions that return int to return void
        and removes a redundant check of error since the functions don't fail.

patch 2 moves the check that the bus type is DPHY before initializing registers
        in the s_stream callback of the isp.

patch 3 adds serialization for the isp subdevice.

patch 4 adds serialization for the resizer subdevice.


Dafna Hirschfeld (4):
  media: staging: rkisp1: change function to return void instead of int
  media: staging: rkisp1: isp: check for dphy bus before initializations
    in s_stream
  media: staging: rkisp1: add serialization to the isp subdev ops
  media: staging: rkisp1: add serialization to the resizer subdev ops

 drivers/staging/media/rkisp1/rkisp1-common.h  |  3 ++
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 51 ++++++++++---------
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 16 +++++-
 3 files changed, 45 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH 1/4] media: staging: rkisp1: change function to return void instead of int

[Dafna Hirschfeld]

There are functions that return int but actually return always 0.
Change them to return void and then there is no need to check
for error return value.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-isp.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 2b0513e826fe..56bd95d01f65 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -504,7 +504,7 @@ static int rkisp1_config_cif(struct rkisp1_device *rkisp1)
 	return 0;
 }
 
-static int rkisp1_isp_stop(struct rkisp1_device *rkisp1)
+static void rkisp1_isp_stop(struct rkisp1_device *rkisp1)
 {
 	u32 val;
 
@@ -540,8 +540,6 @@ static int rkisp1_isp_stop(struct rkisp1_device *rkisp1)
 		     RKISP1_CIF_IRCL_MIPI_SW_RST | RKISP1_CIF_IRCL_ISP_SW_RST,
 		     RKISP1_CIF_IRCL);
 	rkisp1_write(rkisp1, 0x0, RKISP1_CIF_IRCL);
-
-	return 0;
 }
 
 static void rkisp1_config_clk(struct rkisp1_device *rkisp1)
@@ -555,7 +553,7 @@ static void rkisp1_config_clk(struct rkisp1_device *rkisp1)
 	rkisp1_write(rkisp1, val, RKISP1_CIF_ICCL);
 }
 
-static int rkisp1_isp_start(struct rkisp1_device *rkisp1)
+static void rkisp1_isp_start(struct rkisp1_device *rkisp1)
 {
 	struct rkisp1_sensor_async *sensor = rkisp1->active_sensor;
 	u32 val;
@@ -580,8 +578,6 @@ static int rkisp1_isp_start(struct rkisp1_device *rkisp1)
 	 * the MIPI interface and before starting the sensor output.
 	 */
 	usleep_range(1000, 1200);
-
-	return 0;
 }
 
 /* ----------------------------------------------------------------------------
@@ -956,9 +952,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret = 0;
 
 	if (!enable) {
-		ret = rkisp1_isp_stop(rkisp1);
-		if (ret)
-			return ret;
+		rkisp1_isp_stop(rkisp1);
 		rkisp1_mipi_csi2_stop(rkisp1->active_sensor);
 		return 0;
 	}
@@ -981,9 +975,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 	if (ret)
 		return ret;
 
-	ret = rkisp1_isp_start(rkisp1);
-	if (ret)
-		rkisp1_mipi_csi2_stop(rkisp1->active_sensor);
+	rkisp1_isp_start(rkisp1);
 
 	return ret;
 }
-- 
2.17.1

[PATCH 2/4] media: staging: rkisp1: isp: check for dphy bus before initializations in s_stream

[Dafna Hirschfeld]

In rkisp1_isp_s_stream it is better to return error in case the
bus type is not dphy before initializing the registers.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-isp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 56bd95d01f65..c98e3c16f520 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -963,14 +963,14 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 	rkisp1->active_sensor = container_of(sensor_sd->asd,
 					     struct rkisp1_sensor_async, asd);
 
+	if (rkisp1->active_sensor->mbus.type != V4L2_MBUS_CSI2_DPHY)
+		return -EINVAL;
+
 	atomic_set(&rkisp1->isp.frame_sequence, -1);
 	ret = rkisp1_config_cif(rkisp1);
 	if (ret)
 		return ret;
 
-	if (rkisp1->active_sensor->mbus.type != V4L2_MBUS_CSI2_DPHY)
-		return -EINVAL;
-
 	ret = rkisp1_mipi_csi2_start(&rkisp1->isp, rkisp1->active_sensor);
 	if (ret)
 		return ret;
-- 
2.17.1

[PATCH 3/4] media: staging: rkisp1: add serialization to the isp subdev ops

[Dafna Hirschfeld]

For subdevices drivers, the drivers themself are responsible
for serializing their operations.
This patch adds serialization to the isp subdevice.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h |  2 ++
 drivers/staging/media/rkisp1/rkisp1-isp.c    | 29 ++++++++++++++------
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 4e773d611d1b..7c668ac4bdd5 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -96,6 +96,7 @@ struct rkisp1_sensor_async {
  * @sink_crop: crop for sink pad
  * @src_fmt: output format
  * @src_crop: output size
+ * @ops_lock: ops serialization
  *
  * @is_dphy_errctrl_disabled : if dphy errctrl is disabled (avoid endless interrupt)
  * @frame_sequence: used to synchronize frame_id between video devices.
@@ -107,6 +108,7 @@ struct rkisp1_isp {
 	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
 	const struct rkisp1_isp_mbus_info *sink_fmt;
 	const struct rkisp1_isp_mbus_info *src_fmt;
+	struct mutex ops_lock;
 	bool is_dphy_errctrl_disabled;
 	atomic_t frame_sequence;
 };
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index c98e3c16f520..aa7a842f97f8 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -791,7 +791,9 @@ static int rkisp1_isp_get_fmt(struct v4l2_subdev *sd,
 {
 	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
 
+	mutex_lock(&isp->ops_lock);
 	fmt->format = *rkisp1_isp_get_pad_fmt(isp, cfg, fmt->pad, fmt->which);
+	mutex_unlock(&isp->ops_lock);
 	return 0;
 }
 
@@ -801,6 +803,7 @@ static int rkisp1_isp_set_fmt(struct v4l2_subdev *sd,
 {
 	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
 
+	mutex_lock(&isp->ops_lock);
 	if (fmt->pad == RKISP1_ISP_PAD_SINK_VIDEO)
 		rkisp1_isp_set_sink_fmt(isp, cfg, &fmt->format, fmt->which);
 	else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_VIDEO)
@@ -809,6 +812,7 @@ static int rkisp1_isp_set_fmt(struct v4l2_subdev *sd,
 		fmt->format = *rkisp1_isp_get_pad_fmt(isp, cfg, fmt->pad,
 						      fmt->which);
 
+	mutex_unlock(&isp->ops_lock);
 	return 0;
 }
 
@@ -817,11 +821,13 @@ static int rkisp1_isp_get_selection(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_selection *sel)
 {
 	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
+	int ret = 0;
 
 	if (sel->pad != RKISP1_ISP_PAD_SOURCE_VIDEO &&
 	    sel->pad != RKISP1_ISP_PAD_SINK_VIDEO)
 		return -EINVAL;
 
+	mutex_lock(&isp->ops_lock);
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 		if (sel->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
@@ -844,10 +850,10 @@ static int rkisp1_isp_get_selection(struct v4l2_subdev *sd,
 						  sel->which);
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
-
-	return 0;
+	mutex_unlock(&isp->ops_lock);
+	return ret;
 }
 
 static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
@@ -857,21 +863,23 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
 	struct rkisp1_device *rkisp1 =
 		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
 	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
+	int ret = 0;
 
 	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
 	dev_dbg(rkisp1->dev, "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__,
 		sel->pad, sel->r.left, sel->r.top, sel->r.width, sel->r.height);
-
+	mutex_lock(&isp->ops_lock);
 	if (sel->pad == RKISP1_ISP_PAD_SINK_VIDEO)
 		rkisp1_isp_set_sink_crop(isp, cfg, &sel->r, sel->which);
 	else if (sel->pad == RKISP1_ISP_PAD_SOURCE_VIDEO)
 		rkisp1_isp_set_src_crop(isp, cfg, &sel->r, sel->which);
 	else
-		return -EINVAL;
+		ret = -EINVAL;
 
-	return 0;
+	mutex_unlock(&isp->ops_lock);
+	return ret;
 }
 
 static int rkisp1_subdev_link_validate(struct media_link *link)
@@ -948,6 +956,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct rkisp1_device *rkisp1 =
 		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
+	struct rkisp1_isp *isp = &rkisp1->isp;
 	struct v4l2_subdev *sensor_sd;
 	int ret = 0;
 
@@ -967,16 +976,19 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 		return -EINVAL;
 
 	atomic_set(&rkisp1->isp.frame_sequence, -1);
+	mutex_lock(&isp->ops_lock);
 	ret = rkisp1_config_cif(rkisp1);
 	if (ret)
-		return ret;
+		goto mutex_unlock;
 
 	ret = rkisp1_mipi_csi2_start(&rkisp1->isp, rkisp1->active_sensor);
 	if (ret)
-		return ret;
+		goto mutex_unlock;
 
 	rkisp1_isp_start(rkisp1);
 
+mutex_unlock:
+	mutex_unlock(&isp->ops_lock);
 	return ret;
 }
 
@@ -1036,6 +1048,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1,
 	isp->sink_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SINK_PAD_FMT);
 	isp->src_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SRC_PAD_FMT);
 
+	mutex_init(&isp->ops_lock);
 	ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX, pads);
 	if (ret)
 		return ret;
-- 
2.17.1

[PATCH 4/4] media: staging: rkisp1: add serialization to the resizer subdev ops

[Dafna Hirschfeld]

For subdevices drivers, the drivers themself are responsible
for serializing their operations.
This patch adds serialization to the resizer subdevice.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h  |  1 +
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 7c668ac4bdd5..18507f5b6f3c 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -226,6 +226,7 @@ struct rkisp1_resizer {
 	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
 	const struct rkisp1_rsz_config *config;
 	enum rkisp1_fmt_pix_type fmt_type;
+	struct mutex ops_lock;
 };
 
 struct rkisp1_debug {
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 9de6744f0900..87799fbf0363 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -562,7 +562,9 @@ static int rkisp1_rsz_get_fmt(struct v4l2_subdev *sd,
 	struct rkisp1_resizer *rsz =
 		container_of(sd, struct rkisp1_resizer, sd);
 
+	mutex_lock(&rsz->ops_lock);
 	fmt->format = *rkisp1_rsz_get_pad_fmt(rsz, cfg, fmt->pad, fmt->which);
+	mutex_unlock(&rsz->ops_lock);
 	return 0;
 }
 
@@ -573,11 +575,13 @@ static int rkisp1_rsz_set_fmt(struct v4l2_subdev *sd,
 	struct rkisp1_resizer *rsz =
 		container_of(sd, struct rkisp1_resizer, sd);
 
+	mutex_lock(&rsz->ops_lock);
 	if (fmt->pad == RKISP1_RSZ_PAD_SINK)
 		rkisp1_rsz_set_sink_fmt(rsz, cfg, &fmt->format, fmt->which);
 	else
 		rkisp1_rsz_set_src_fmt(rsz, cfg, &fmt->format, fmt->which);
 
+	mutex_unlock(&rsz->ops_lock);
 	return 0;
 }
 
@@ -588,10 +592,12 @@ static int rkisp1_rsz_get_selection(struct v4l2_subdev *sd,
 	struct rkisp1_resizer *rsz =
 		container_of(sd, struct rkisp1_resizer, sd);
 	struct v4l2_mbus_framefmt *mf_sink;
+	int ret = 0;
 
 	if (sel->pad == RKISP1_RSZ_PAD_SRC)
 		return -EINVAL;
 
+	mutex_lock(&rsz->ops_lock);
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 		mf_sink = rkisp1_rsz_get_pad_fmt(rsz, cfg, RKISP1_RSZ_PAD_SINK,
@@ -606,10 +612,11 @@ static int rkisp1_rsz_get_selection(struct v4l2_subdev *sd,
 						  sel->which);
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
 
-	return 0;
+	mutex_unlock(&rsz->ops_lock);
+	return ret;
 }
 
 static int rkisp1_rsz_set_selection(struct v4l2_subdev *sd,
@@ -625,7 +632,9 @@ static int rkisp1_rsz_set_selection(struct v4l2_subdev *sd,
 	dev_dbg(sd->dev, "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__,
 		sel->pad, sel->r.left, sel->r.top, sel->r.width, sel->r.height);
 
+	mutex_lock(&rsz->ops_lock);
 	rkisp1_rsz_set_sink_crop(rsz, cfg, &sel->r, sel->which);
+	mutex_unlock(&rsz->ops_lock);
 
 	return 0;
 }
@@ -665,9 +674,11 @@ static int rkisp1_rsz_s_stream(struct v4l2_subdev *sd, int enable)
 	if (other->is_streaming)
 		when = RKISP1_SHADOW_REGS_ASYNC;
 
+	mutex_lock(&rsz->ops_lock);
 	rkisp1_rsz_config(rsz, when);
 	rkisp1_dcrop_config(rsz);
 
+	mutex_unlock(&rsz->ops_lock);
 	return 0;
 }
 
@@ -713,6 +724,7 @@ static int rkisp1_rsz_register(struct rkisp1_resizer *rsz)
 
 	rsz->fmt_type = RKISP1_DEF_FMT_TYPE;
 
+	mutex_init(&rsz->ops_lock);
 	ret = media_entity_pads_init(&sd->entity, 2, pads);
 	if (ret)
 		return ret;
-- 
2.17.1

Re: [PATCH 1/4] media: staging: rkisp1: change function to return void instead of int

[Helen Koike]

On 2/7/20 6:59 AM, Dafna Hirschfeld wrote:
> There are functions that return int but actually return always 0.
> Change them to return void and then there is no need to check
> for error return value.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-isp.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 2b0513e826fe..56bd95d01f65 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -504,7 +504,7 @@ static int rkisp1_config_cif(struct rkisp1_device *rkisp1)
>  	return 0;
>  }
>  
> -static int rkisp1_isp_stop(struct rkisp1_device *rkisp1)
> +static void rkisp1_isp_stop(struct rkisp1_device *rkisp1)
>  {
>  	u32 val;
>  
> @@ -540,8 +540,6 @@ static int rkisp1_isp_stop(struct rkisp1_device *rkisp1)
>  		     RKISP1_CIF_IRCL_MIPI_SW_RST | RKISP1_CIF_IRCL_ISP_SW_RST,
>  		     RKISP1_CIF_IRCL);
>  	rkisp1_write(rkisp1, 0x0, RKISP1_CIF_IRCL);
> -
> -	return 0;
>  }
>  
>  static void rkisp1_config_clk(struct rkisp1_device *rkisp1)
> @@ -555,7 +553,7 @@ static void rkisp1_config_clk(struct rkisp1_device *rkisp1)
>  	rkisp1_write(rkisp1, val, RKISP1_CIF_ICCL);
>  }
>  
> -static int rkisp1_isp_start(struct rkisp1_device *rkisp1)
> +static void rkisp1_isp_start(struct rkisp1_device *rkisp1)
>  {
>  	struct rkisp1_sensor_async *sensor = rkisp1->active_sensor;
>  	u32 val;
> @@ -580,8 +578,6 @@ static int rkisp1_isp_start(struct rkisp1_device *rkisp1)
>  	 * the MIPI interface and before starting the sensor output.
>  	 */
>  	usleep_range(1000, 1200);
> -
> -	return 0;
>  }
>  
>  /* ----------------------------------------------------------------------------
> @@ -956,9 +952,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret = 0;
>  
>  	if (!enable) {
> -		ret = rkisp1_isp_stop(rkisp1);
> -		if (ret)
> -			return ret;
> +		rkisp1_isp_stop(rkisp1);
>  		rkisp1_mipi_csi2_stop(rkisp1->active_sensor);
>  		return 0;
>  	}
> @@ -981,9 +975,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (ret)
>  		return ret;
>  
> -	ret = rkisp1_isp_start(rkisp1);
> -	if (ret)
> -		rkisp1_mipi_csi2_stop(rkisp1->active_sensor);
> +	rkisp1_isp_start(rkisp1);
>  
>  	return ret;
>  }
> 

Re: [PATCH 2/4] media: staging: rkisp1: isp: check for dphy bus before initializations in s_stream

[Helen Koike]

On 2/7/20 6:59 AM, Dafna Hirschfeld wrote:
> In rkisp1_isp_s_stream it is better to return error in case the
> bus type is not dphy before initializing the registers.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>


Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-isp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 56bd95d01f65..c98e3c16f520 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -963,14 +963,14 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  	rkisp1->active_sensor = container_of(sensor_sd->asd,
>  					     struct rkisp1_sensor_async, asd);
>  
> +	if (rkisp1->active_sensor->mbus.type != V4L2_MBUS_CSI2_DPHY)
> +		return -EINVAL;
> +
>  	atomic_set(&rkisp1->isp.frame_sequence, -1);
>  	ret = rkisp1_config_cif(rkisp1);
>  	if (ret)
>  		return ret;
>  
> -	if (rkisp1->active_sensor->mbus.type != V4L2_MBUS_CSI2_DPHY)
> -		return -EINVAL;
> -
>  	ret = rkisp1_mipi_csi2_start(&rkisp1->isp, rkisp1->active_sensor);
>  	if (ret)
>  		return ret;
> 

Re: [PATCH 3/4] media: staging: rkisp1: add serialization to the isp subdev ops

[Helen Koike]

On 2/7/20 6:59 AM, Dafna Hirschfeld wrote:
> For subdevices drivers, the drivers themself are responsible
> for serializing their operations.
> This patch adds serialization to the isp subdevice.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

Sakari, if you could take a look at this too it would be great.

Thanks,
Helen


> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  2 ++
>  drivers/staging/media/rkisp1/rkisp1-isp.c    | 29 ++++++++++++++------
>  2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 4e773d611d1b..7c668ac4bdd5 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -96,6 +96,7 @@ struct rkisp1_sensor_async {
>   * @sink_crop: crop for sink pad
>   * @src_fmt: output format
>   * @src_crop: output size
> + * @ops_lock: ops serialization
>   *
>   * @is_dphy_errctrl_disabled : if dphy errctrl is disabled (avoid endless interrupt)
>   * @frame_sequence: used to synchronize frame_id between video devices.
> @@ -107,6 +108,7 @@ struct rkisp1_isp {
>  	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
>  	const struct rkisp1_isp_mbus_info *sink_fmt;
>  	const struct rkisp1_isp_mbus_info *src_fmt;
> +	struct mutex ops_lock;
>  	bool is_dphy_errctrl_disabled;
>  	atomic_t frame_sequence;
>  };
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index c98e3c16f520..aa7a842f97f8 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -791,7 +791,9 @@ static int rkisp1_isp_get_fmt(struct v4l2_subdev *sd,
>  {
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
>  
> +	mutex_lock(&isp->ops_lock);
>  	fmt->format = *rkisp1_isp_get_pad_fmt(isp, cfg, fmt->pad, fmt->which);
> +	mutex_unlock(&isp->ops_lock);
>  	return 0;
>  }
>  
> @@ -801,6 +803,7 @@ static int rkisp1_isp_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
>  
> +	mutex_lock(&isp->ops_lock);
>  	if (fmt->pad == RKISP1_ISP_PAD_SINK_VIDEO)
>  		rkisp1_isp_set_sink_fmt(isp, cfg, &fmt->format, fmt->which);
>  	else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_VIDEO)
> @@ -809,6 +812,7 @@ static int rkisp1_isp_set_fmt(struct v4l2_subdev *sd,
>  		fmt->format = *rkisp1_isp_get_pad_fmt(isp, cfg, fmt->pad,
>  						      fmt->which);
>  
> +	mutex_unlock(&isp->ops_lock);
>  	return 0;
>  }
>  
> @@ -817,11 +821,13 @@ static int rkisp1_isp_get_selection(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_selection *sel)
>  {
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
> +	int ret = 0;
>  
>  	if (sel->pad != RKISP1_ISP_PAD_SOURCE_VIDEO &&
>  	    sel->pad != RKISP1_ISP_PAD_SINK_VIDEO)
>  		return -EINVAL;
>  
> +	mutex_lock(&isp->ops_lock);
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP_BOUNDS:
>  		if (sel->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
> @@ -844,10 +850,10 @@ static int rkisp1_isp_get_selection(struct v4l2_subdev *sd,
>  						  sel->which);
>  		break;
>  	default:
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
> -
> -	return 0;
> +	mutex_unlock(&isp->ops_lock);
> +	return ret;
>  }
>  
>  static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
> @@ -857,21 +863,23 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
>  	struct rkisp1_device *rkisp1 =
>  		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
> +	int ret = 0;
>  
>  	if (sel->target != V4L2_SEL_TGT_CROP)
>  		return -EINVAL;
>  
>  	dev_dbg(rkisp1->dev, "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__,
>  		sel->pad, sel->r.left, sel->r.top, sel->r.width, sel->r.height);
> -
> +	mutex_lock(&isp->ops_lock);
>  	if (sel->pad == RKISP1_ISP_PAD_SINK_VIDEO)
>  		rkisp1_isp_set_sink_crop(isp, cfg, &sel->r, sel->which);
>  	else if (sel->pad == RKISP1_ISP_PAD_SOURCE_VIDEO)
>  		rkisp1_isp_set_src_crop(isp, cfg, &sel->r, sel->which);
>  	else
> -		return -EINVAL;
> +		ret = -EINVAL;
>  
> -	return 0;
> +	mutex_unlock(&isp->ops_lock);
> +	return ret;
>  }
>  
>  static int rkisp1_subdev_link_validate(struct media_link *link)
> @@ -948,6 +956,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct rkisp1_device *rkisp1 =
>  		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
> +	struct rkisp1_isp *isp = &rkisp1->isp;
>  	struct v4l2_subdev *sensor_sd;
>  	int ret = 0;
>  
> @@ -967,16 +976,19 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  		return -EINVAL;
>  
>  	atomic_set(&rkisp1->isp.frame_sequence, -1);
> +	mutex_lock(&isp->ops_lock);
>  	ret = rkisp1_config_cif(rkisp1);
>  	if (ret)
> -		return ret;
> +		goto mutex_unlock;
>  
>  	ret = rkisp1_mipi_csi2_start(&rkisp1->isp, rkisp1->active_sensor);
>  	if (ret)
> -		return ret;
> +		goto mutex_unlock;
>  
>  	rkisp1_isp_start(rkisp1);
>  
> +mutex_unlock:
> +	mutex_unlock(&isp->ops_lock);
>  	return ret;
>  }
>  
> @@ -1036,6 +1048,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1,
>  	isp->sink_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SINK_PAD_FMT);
>  	isp->src_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SRC_PAD_FMT);
>  
> +	mutex_init(&isp->ops_lock);
>  	ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX, pads);
>  	if (ret)
>  		return ret;
> 

Re: [PATCH 4/4] media: staging: rkisp1: add serialization to the resizer subdev ops

[Helen Koike]

On 2/7/20 6:59 AM, Dafna Hirschfeld wrote:
> For subdevices drivers, the drivers themself are responsible
> for serializing their operations.
> This patch adds serialization to the resizer subdevice.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>


Acked-by: Helen Koike <helen.koike@collabora.com>

Sakari, if you could take a look at this too it would be great.

Dafna, could you also send a patch removing the item in the TODO list
regarding ioctls serialization?

Thanks,
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  1 +
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 16 ++++++++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 7c668ac4bdd5..18507f5b6f3c 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -226,6 +226,7 @@ struct rkisp1_resizer {
>  	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
>  	const struct rkisp1_rsz_config *config;
>  	enum rkisp1_fmt_pix_type fmt_type;
> +	struct mutex ops_lock;
>  };
>  
>  struct rkisp1_debug {
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index 9de6744f0900..87799fbf0363 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -562,7 +562,9 @@ static int rkisp1_rsz_get_fmt(struct v4l2_subdev *sd,
>  	struct rkisp1_resizer *rsz =
>  		container_of(sd, struct rkisp1_resizer, sd);
>  
> +	mutex_lock(&rsz->ops_lock);
>  	fmt->format = *rkisp1_rsz_get_pad_fmt(rsz, cfg, fmt->pad, fmt->which);
> +	mutex_unlock(&rsz->ops_lock);
>  	return 0;
>  }
>  
> @@ -573,11 +575,13 @@ static int rkisp1_rsz_set_fmt(struct v4l2_subdev *sd,
>  	struct rkisp1_resizer *rsz =
>  		container_of(sd, struct rkisp1_resizer, sd);
>  
> +	mutex_lock(&rsz->ops_lock);
>  	if (fmt->pad == RKISP1_RSZ_PAD_SINK)
>  		rkisp1_rsz_set_sink_fmt(rsz, cfg, &fmt->format, fmt->which);
>  	else
>  		rkisp1_rsz_set_src_fmt(rsz, cfg, &fmt->format, fmt->which);
>  
> +	mutex_unlock(&rsz->ops_lock);
>  	return 0;
>  }
>  
> @@ -588,10 +592,12 @@ static int rkisp1_rsz_get_selection(struct v4l2_subdev *sd,
>  	struct rkisp1_resizer *rsz =
>  		container_of(sd, struct rkisp1_resizer, sd);
>  	struct v4l2_mbus_framefmt *mf_sink;
> +	int ret = 0;
>  
>  	if (sel->pad == RKISP1_RSZ_PAD_SRC)
>  		return -EINVAL;
>  
> +	mutex_lock(&rsz->ops_lock);
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP_BOUNDS:
>  		mf_sink = rkisp1_rsz_get_pad_fmt(rsz, cfg, RKISP1_RSZ_PAD_SINK,
> @@ -606,10 +612,11 @@ static int rkisp1_rsz_get_selection(struct v4l2_subdev *sd,
>  						  sel->which);
>  		break;
>  	default:
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
>  
> -	return 0;
> +	mutex_unlock(&rsz->ops_lock);
> +	return ret;
>  }
>  
>  static int rkisp1_rsz_set_selection(struct v4l2_subdev *sd,
> @@ -625,7 +632,9 @@ static int rkisp1_rsz_set_selection(struct v4l2_subdev *sd,
>  	dev_dbg(sd->dev, "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__,
>  		sel->pad, sel->r.left, sel->r.top, sel->r.width, sel->r.height);
>  
> +	mutex_lock(&rsz->ops_lock);
>  	rkisp1_rsz_set_sink_crop(rsz, cfg, &sel->r, sel->which);
> +	mutex_unlock(&rsz->ops_lock);
>  
>  	return 0;
>  }
> @@ -665,9 +674,11 @@ static int rkisp1_rsz_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (other->is_streaming)
>  		when = RKISP1_SHADOW_REGS_ASYNC;
>  
> +	mutex_lock(&rsz->ops_lock);
>  	rkisp1_rsz_config(rsz, when);
>  	rkisp1_dcrop_config(rsz);
>  
> +	mutex_unlock(&rsz->ops_lock);
>  	return 0;
>  }
>  
> @@ -713,6 +724,7 @@ static int rkisp1_rsz_register(struct rkisp1_resizer *rsz)
>  
>  	rsz->fmt_type = RKISP1_DEF_FMT_TYPE;
>  
> +	mutex_init(&rsz->ops_lock);
>  	ret = media_entity_pads_init(&sd->entity, 2, pads);
>  	if (ret)
>  		return ret;
> 

Re: [PATCH 3/4] media: staging: rkisp1: add serialization to the isp subdev ops

[Sakari Ailus]

Hi Dafna,

Thanks for the patchset.

On Fri, Feb 07, 2020 at 09:59:50AM +0100, Dafna Hirschfeld wrote:
> For subdevices drivers, the drivers themself are responsible
> for serializing their operations.
> This patch adds serialization to the isp subdevice.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  2 ++
>  drivers/staging/media/rkisp1/rkisp1-isp.c    | 29 ++++++++++++++------
>  2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 4e773d611d1b..7c668ac4bdd5 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -96,6 +96,7 @@ struct rkisp1_sensor_async {
>   * @sink_crop: crop for sink pad
>   * @src_fmt: output format
>   * @src_crop: output size
> + * @ops_lock: ops serialization
>   *
>   * @is_dphy_errctrl_disabled : if dphy errctrl is disabled (avoid endless interrupt)
>   * @frame_sequence: used to synchronize frame_id between video devices.
> @@ -107,6 +108,7 @@ struct rkisp1_isp {
>  	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
>  	const struct rkisp1_isp_mbus_info *sink_fmt;
>  	const struct rkisp1_isp_mbus_info *src_fmt;
> +	struct mutex ops_lock;
>  	bool is_dphy_errctrl_disabled;
>  	atomic_t frame_sequence;
>  };
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index c98e3c16f520..aa7a842f97f8 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -791,7 +791,9 @@ static int rkisp1_isp_get_fmt(struct v4l2_subdev *sd,
>  {
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
>  
> +	mutex_lock(&isp->ops_lock);
>  	fmt->format = *rkisp1_isp_get_pad_fmt(isp, cfg, fmt->pad, fmt->which);
> +	mutex_unlock(&isp->ops_lock);
>  	return 0;
>  }
>  
> @@ -801,6 +803,7 @@ static int rkisp1_isp_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
>  
> +	mutex_lock(&isp->ops_lock);
>  	if (fmt->pad == RKISP1_ISP_PAD_SINK_VIDEO)
>  		rkisp1_isp_set_sink_fmt(isp, cfg, &fmt->format, fmt->which);
>  	else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_VIDEO)
> @@ -809,6 +812,7 @@ static int rkisp1_isp_set_fmt(struct v4l2_subdev *sd,
>  		fmt->format = *rkisp1_isp_get_pad_fmt(isp, cfg, fmt->pad,
>  						      fmt->which);
>  
> +	mutex_unlock(&isp->ops_lock);
>  	return 0;
>  }
>  
> @@ -817,11 +821,13 @@ static int rkisp1_isp_get_selection(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_selection *sel)
>  {
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
> +	int ret = 0;
>  
>  	if (sel->pad != RKISP1_ISP_PAD_SOURCE_VIDEO &&
>  	    sel->pad != RKISP1_ISP_PAD_SINK_VIDEO)
>  		return -EINVAL;
>  
> +	mutex_lock(&isp->ops_lock);
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP_BOUNDS:
>  		if (sel->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
> @@ -844,10 +850,10 @@ static int rkisp1_isp_get_selection(struct v4l2_subdev *sd,
>  						  sel->which);
>  		break;
>  	default:
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
> -
> -	return 0;
> +	mutex_unlock(&isp->ops_lock);
> +	return ret;
>  }
>  
>  static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
> @@ -857,21 +863,23 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
>  	struct rkisp1_device *rkisp1 =
>  		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
> +	int ret = 0;
>  
>  	if (sel->target != V4L2_SEL_TGT_CROP)
>  		return -EINVAL;
>  
>  	dev_dbg(rkisp1->dev, "%s: pad: %d sel(%d,%d)/%dx%d\n", __func__,
>  		sel->pad, sel->r.left, sel->r.top, sel->r.width, sel->r.height);
> -
> +	mutex_lock(&isp->ops_lock);
>  	if (sel->pad == RKISP1_ISP_PAD_SINK_VIDEO)
>  		rkisp1_isp_set_sink_crop(isp, cfg, &sel->r, sel->which);
>  	else if (sel->pad == RKISP1_ISP_PAD_SOURCE_VIDEO)
>  		rkisp1_isp_set_src_crop(isp, cfg, &sel->r, sel->which);
>  	else
> -		return -EINVAL;
> +		ret = -EINVAL;
>  
> -	return 0;
> +	mutex_unlock(&isp->ops_lock);
> +	return ret;
>  }
>  
>  static int rkisp1_subdev_link_validate(struct media_link *link)
> @@ -948,6 +956,7 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct rkisp1_device *rkisp1 =
>  		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
> +	struct rkisp1_isp *isp = &rkisp1->isp;
>  	struct v4l2_subdev *sensor_sd;
>  	int ret = 0;
>  
> @@ -967,16 +976,19 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  		return -EINVAL;
>  
>  	atomic_set(&rkisp1->isp.frame_sequence, -1);
> +	mutex_lock(&isp->ops_lock);
>  	ret = rkisp1_config_cif(rkisp1);
>  	if (ret)
> -		return ret;
> +		goto mutex_unlock;
>  
>  	ret = rkisp1_mipi_csi2_start(&rkisp1->isp, rkisp1->active_sensor);
>  	if (ret)
> -		return ret;
> +		goto mutex_unlock;
>  
>  	rkisp1_isp_start(rkisp1);
>  
> +mutex_unlock:
> +	mutex_unlock(&isp->ops_lock);
>  	return ret;
>  }
>  
> @@ -1036,6 +1048,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1,
>  	isp->sink_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SINK_PAD_FMT);
>  	isp->src_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SRC_PAD_FMT);
>  
> +	mutex_init(&isp->ops_lock);
>  	ret = media_entity_pads_init(&sd->entity, RKISP1_ISP_PAD_MAX, pads);
>  	if (ret)
>  		return ret;

Could you add mutex_destroy() on the mutex in rkisp1_isp_unregister(),
please? Similar comment on the 4th patch. This could be a follow-up patch,
too.

For the set,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus