[PATCH 01/11] media: exynos4-is: Remove static driver data for S5PV210 FIMC variants

[PATCH 01/11] media: exynos4-is: Remove static driver data for S5PV210 FIMC variants

[Jonathan Bakker]

The S5PV210 platform only supports device tree based booting
where the FIMC variant data is parsed directly from
the device tree, hence the now unused static data can be removed.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 59 -------------------
 1 file changed, 59 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
index cde60fbb23a8..2258f3bfc929 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -1110,67 +1110,8 @@ static int fimc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-/* Image pixel limits, similar across several FIMC HW revisions. */
-static const struct fimc_pix_limit s5p_pix_limit[4] = {
-	[0] = {
-		.scaler_en_w	= 3264,
-		.scaler_dis_w	= 8192,
-		.out_rot_en_w	= 1920,
-		.out_rot_dis_w	= 4224,
-	},
-	[1] = {
-		.scaler_en_w	= 4224,
-		.scaler_dis_w	= 8192,
-		.out_rot_en_w	= 1920,
-		.out_rot_dis_w	= 4224,
-	},
-	[2] = {
-		.scaler_en_w	= 1920,
-		.scaler_dis_w	= 8192,
-		.out_rot_en_w	= 1280,
-		.out_rot_dis_w	= 1920,
-	},
-};
-
-static const struct fimc_variant fimc0_variant_s5pv210 = {
-	.has_inp_rot	 = 1,
-	.has_out_rot	 = 1,
-	.has_cam_if	 = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 8,
-	.min_vsize_align = 16,
-	.pix_limit	 = &s5p_pix_limit[1],
-};
-
-static const struct fimc_variant fimc1_variant_s5pv210 = {
-	.has_inp_rot	 = 1,
-	.has_out_rot	 = 1,
-	.has_cam_if	 = 1,
-	.has_mainscaler_ext = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 1,
-	.min_vsize_align = 1,
-	.pix_limit	 = &s5p_pix_limit[2],
-};
-
-static const struct fimc_variant fimc2_variant_s5pv210 = {
-	.has_cam_if	 = 1,
-	.min_inp_pixsize = 16,
-	.min_out_pixsize = 16,
-	.hor_offs_align	 = 8,
-	.min_vsize_align = 16,
-	.pix_limit	 = &s5p_pix_limit[2],
-};
-
 /* S5PV210, S5PC110 */
 static const struct fimc_drvdata fimc_drvdata_s5pv210 = {
-	.variant = {
-		[0] = &fimc0_variant_s5pv210,
-		[1] = &fimc1_variant_s5pv210,
-		[2] = &fimc2_variant_s5pv210,
-	},
 	.num_entities	= 3,
 	.lclk_frequency	= 166000000UL,
 	.out_buf_count	= 4,
-- 
2.20.1

[PATCH 02/11] media: exynos4-is: Request syscon only if ISP writeback is present

[Jonathan Bakker]

From: Tomasz Figa <tomasz.figa@gmail.com>

On FIMC variants which don't have writeback channel, there is no need to
access system registers. This patch makes the driver request sysreg
regmap conditionally depending on whether writeback is supported.

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
index 2258f3bfc929..08d1f39a914c 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -954,9 +954,11 @@ static int fimc_probe(struct platform_device *pdev)
 	spin_lock_init(&fimc->slock);
 	mutex_init(&fimc->lock);
 
-	fimc->sysreg = fimc_get_sysreg_regmap(dev->of_node);
-	if (IS_ERR(fimc->sysreg))
-		return PTR_ERR(fimc->sysreg);
+	if (fimc->variant->has_isp_wb) {
+		fimc->sysreg = fimc_get_sysreg_regmap(dev->of_node);
+		if (IS_ERR(fimc->sysreg))
+			return PTR_ERR(fimc->sysreg);
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	fimc->regs = devm_ioremap_resource(dev, res);
-- 
2.20.1

[PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS device present

[Jonathan Bakker]

Not all devices use the CSIS device, some may use the FIMC directly in
which case the CSIS device isn't registered.  This leads to a nullptr
exception when starting the stream as the CSIS device is always
referenced.  Instead, if getting the CSIS device fails, try getting the
FIMC directly to check if we are using the subdev API

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/media-dev.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 9aaf3b8060d5..5c32abc7251b 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -289,11 +289,26 @@ static int __fimc_pipeline_s_stream(struct exynos_media_pipeline *ep, bool on)
 		{ IDX_CSIS, IDX_FLITE, IDX_FIMC, IDX_SENSOR, IDX_IS_ISP },
 	};
 	struct fimc_pipeline *p = to_fimc_pipeline(ep);
-	struct fimc_md *fmd = entity_to_fimc_mdev(&p->subdevs[IDX_CSIS]->entity);
 	enum fimc_subdev_index sd_id;
 	int i, ret = 0;
 
 	if (p->subdevs[IDX_SENSOR] == NULL) {
+		struct fimc_md *fmd;
+		struct v4l2_subdev *sd = p->subdevs[IDX_CSIS];
+
+		if (!sd)
+			sd = p->subdevs[IDX_FIMC];
+
+		if (!sd) {
+			/*
+			 * If neither CSIS nor FIMC was set up,
+			 * it's impossible to have any sensors
+			 */
+			return -ENODEV;
+		}
+
+		fmd = entity_to_fimc_mdev(&sd->entity);
+
 		if (!fmd->user_subdev_api) {
 			/*
 			 * Sensor must be already discovered if we
-- 
2.20.1

[PATCH 04/11] media: exynos4-is: Correct missing entity function initialization

[Jonathan Bakker]

Commit bae4500399c4 ("[media] exynos4-is: Add missing entity function
initialization") tried to suppress the warnings such as

s5p-fimc-md camera: Entity type for entity FIMC.0 was not initialized!

However, this didn't work in all cases.  Correct this by calling the set
function earlier.

Fixes: bae4500399c4 ("exynos4-is: Add missing entity function initialization")
Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 705f182330ca..86c233e2f2c9 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -1799,7 +1799,6 @@ static int fimc_register_capture_device(struct fimc_dev *fimc,
 	vid_cap->wb_fmt.code = fmt->mbus_code;
 
 	vid_cap->vd_pad.flags = MEDIA_PAD_FL_SINK;
-	vfd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
 	ret = media_entity_pads_init(&vfd->entity, 1, &vid_cap->vd_pad);
 	if (ret)
 		goto err_free_ctx;
@@ -1898,6 +1897,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc)
 		return ret;
 
 	sd->entity.ops = &fimc_sd_media_ops;
+	sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
 	sd->internal_ops = &fimc_capture_sd_internal_ops;
 	v4l2_set_subdevdata(sd, fimc);
 	return 0;
-- 
2.20.1

[PATCH 05/11] media: exynos4-is: Improve support for sensors with multiple pads

[Jonathan Bakker]

Commit 1c9f5bd7cb8a ("[media] s5p-fimc: Add support for sensors with
multiple pads") caught the case where a sensor with multiple pads was
connected via CSIS, but missed the case where the sensor was directly
connected to the FIMC.

This still assumes that the last pad of a sensor is the source.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/media-dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 5c32abc7251b..b38445219c72 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -991,7 +991,8 @@ static int fimc_md_create_links(struct fimc_md *fmd)
 
 		case FIMC_BUS_TYPE_ITU_601...FIMC_BUS_TYPE_ITU_656:
 			source = &sensor->entity;
-			pad = 0;
+			/* Assume the last pad is the source */
+			pad = sensor->entity.num_pads - 1;
 			break;
 
 		default:
-- 
2.20.1

[PATCH 06/11] media: exynos4-is: Properly set JPEG options when not using CSIS

[Jonathan Bakker]

Commit ee7160e57c98 ("[media] s5p-fimc: Add support for JPEG capture")
added support for JPEG capture, but missed setting a register when the
CSIS device wasn't in use.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-reg.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/platform/exynos4-is/fimc-reg.c b/drivers/media/platform/exynos4-is/fimc-reg.c
index 5ce2bdebd424..269a98fca1e8 100644
--- a/drivers/media/platform/exynos4-is/fimc-reg.c
+++ b/drivers/media/platform/exynos4-is/fimc-reg.c
@@ -606,6 +606,11 @@ int fimc_hw_set_camera_source(struct fimc_dev *fimc,
 	switch (source->fimc_bus_type) {
 	case FIMC_BUS_TYPE_ITU_601:
 	case FIMC_BUS_TYPE_ITU_656:
+		if (fimc_fmt_is_user_defined(f->fmt->color)) {
+			cfg |= FIMC_REG_CISRCFMT_ITU601_8BIT;
+			break;
+		}
+
 		for (i = 0; i < ARRAY_SIZE(pix_desc); i++) {
 			if (vc->ci_fmt.code == pix_desc[i].pixelcode) {
 				cfg = pix_desc[i].cisrcfmt;
@@ -707,6 +712,8 @@ int fimc_hw_set_camera_type(struct fimc_dev *fimc,
 	case FIMC_BUS_TYPE_ITU_601...FIMC_BUS_TYPE_ITU_656:
 		if (source->mux_id == 0) /* ITU-A, ITU-B: 0, 1 */
 			cfg |= FIMC_REG_CIGCTRL_SELCAM_ITU_A;
+		if (vid_cap->ci_fmt.code == MEDIA_BUS_FMT_JPEG_1X8)
+			cfg |= FIMC_REG_CIGCTRL_CAM_JPEG;
 		break;
 	case FIMC_BUS_TYPE_LCD_WRITEBACK_A:
 		cfg |= FIMC_REG_CIGCTRL_CAMIF_SELWB;
-- 
2.20.1

[PATCH 07/11] media: exynos4-is: Add support for multiple sensors on one port

[Jonathan Bakker]

On some devices, there may be multiple camera sensors attached
to the same port.  Make sure we probe all of them, not just the
first one.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/media-dev.c | 32 ++++++++++++-------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index b38445219c72..a87ebd7913be 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -397,25 +397,28 @@ static void fimc_md_pipelines_free(struct fimc_md *fmd)
 /* Parse port node and register as a sub-device any sensor specified there. */
 static int fimc_md_parse_port_node(struct fimc_md *fmd,
 				   struct device_node *port,
-				   unsigned int index)
+				   unsigned int *index)
 {
-	struct fimc_source_info *pd = &fmd->sensor[index].pdata;
+	struct fimc_source_info *pd;
 	struct device_node *rem, *ep, *np;
-	struct v4l2_fwnode_endpoint endpoint = { .bus_type = 0 };
+	struct v4l2_fwnode_endpoint endpoint;
 	int ret;
 
-	/* Assume here a port node can have only one endpoint node. */
 	ep = of_get_next_child(port, NULL);
 	if (!ep)
 		return 0;
 
+parse_sensor:
+	pd = &fmd->sensor[*index].pdata;
+	endpoint.bus_type = 0;
+
 	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &endpoint);
 	if (ret) {
 		of_node_put(ep);
 		return ret;
 	}
 
-	if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS) {
+	if (WARN_ON(endpoint.base.port == 0) || *index >= FIMC_MAX_SENSORS) {
 		of_node_put(ep);
 		return -EINVAL;
 	}
@@ -462,16 +465,16 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 		pd->fimc_bus_type = pd->sensor_bus_type;
 	of_node_put(np);
 
-	if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) {
+	if (WARN_ON(*index >= ARRAY_SIZE(fmd->sensor))) {
 		of_node_put(rem);
 		return -EINVAL;
 	}
 
-	fmd->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-	fmd->sensor[index].asd.match.fwnode = of_fwnode_handle(rem);
+	fmd->sensor[*index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+	fmd->sensor[*index].asd.match.fwnode = of_fwnode_handle(rem);
 
 	ret = v4l2_async_notifier_add_subdev(&fmd->subdev_notifier,
-					     &fmd->sensor[index].asd);
+					     &fmd->sensor[*index].asd);
 	if (ret) {
 		of_node_put(rem);
 		return ret;
@@ -479,6 +482,13 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 
 	fmd->num_sensors++;
 
+	/* Check for additional sensors on same port */
+	ep = of_get_next_child(port, ep);
+	if (ep) {
+		(*index)++;
+		goto parse_sensor;
+	}
+
 	return 0;
 }
 
@@ -515,7 +525,7 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 		if (!port)
 			continue;
 
-		ret = fimc_md_parse_port_node(fmd, port, index);
+		ret = fimc_md_parse_port_node(fmd, port, &index);
 		of_node_put(port);
 		if (ret < 0) {
 			of_node_put(node);
@@ -530,7 +540,7 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 		goto rpm_put;
 
 	for_each_child_of_node(ports, node) {
-		ret = fimc_md_parse_port_node(fmd, node, index);
+		ret = fimc_md_parse_port_node(fmd, node, &index);
 		if (ret < 0) {
 			of_node_put(node);
 			goto cleanup;
-- 
2.20.1

[PATCH 08/11] media: exynos4-is: Remove inh_sensor_ctrls

[Jonathan Bakker]

This is a no-op as it is never set and is a remnant from non-DT days
that can be safely removed.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-capture.c | 13 +------------
 drivers/media/platform/exynos4-is/fimc-core.h    |  3 ---
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 86c233e2f2c9..95d4a667bffb 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -492,17 +492,6 @@ static int fimc_capture_open(struct file *file)
 
 		ret = fimc_pipeline_call(ve, open, &ve->vdev.entity, true);
 
-		if (ret == 0 && vc->user_subdev_api && vc->inh_sensor_ctrls) {
-			/*
-			 * Recreate controls of the the video node to drop
-			 * any controls inherited from the sensor subdev.
-			 */
-			fimc_ctrls_delete(vc->ctx);
-
-			ret = fimc_ctrls_create(vc->ctx);
-			if (ret == 0)
-				vc->inh_sensor_ctrls = false;
-		}
 		if (ret == 0)
 			ve->vdev.entity.use_count++;
 
@@ -1408,7 +1397,7 @@ static int fimc_link_setup(struct media_entity *entity,
 
 	vc->input = sd->grp_id;
 
-	if (vc->user_subdev_api || vc->inh_sensor_ctrls)
+	if (vc->user_subdev_api)
 		return 0;
 
 	/* Inherit V4L2 controls from the image sensor subdev. */
diff --git a/drivers/media/platform/exynos4-is/fimc-core.h b/drivers/media/platform/exynos4-is/fimc-core.h
index d130f664a60b..31f81bcb8483 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.h
+++ b/drivers/media/platform/exynos4-is/fimc-core.h
@@ -299,8 +299,6 @@ struct fimc_m2m_device {
  * @input_index: input (camera sensor) index
  * @input: capture input type, grp_id of the attached subdev
  * @user_subdev_api: true if subdevs are not configured by the host driver
- * @inh_sensor_ctrls: a flag indicating v4l2 controls are inherited from
- *		      an image sensor subdev
  */
 struct fimc_vid_cap {
 	struct fimc_ctx			*ctx;
@@ -322,7 +320,6 @@ struct fimc_vid_cap {
 	int				input_index;
 	u32				input;
 	bool				user_subdev_api;
-	bool				inh_sensor_ctrls;
 };
 
 /**
-- 
2.20.1

[PATCH 09/11] media: exynos4-is: Remove unused struct member input_index

[Jonathan Bakker]

This is no longer used since the conversion to DT

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-core.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.h b/drivers/media/platform/exynos4-is/fimc-core.h
index 31f81bcb8483..e4a56232907a 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.h
+++ b/drivers/media/platform/exynos4-is/fimc-core.h
@@ -296,7 +296,6 @@ struct fimc_m2m_device {
  * @buf_index: index for managing the output DMA buffers
  * @frame_count: the frame counter for statistics
  * @reqbufs_count: the number of buffers requested in REQBUFS ioctl
- * @input_index: input (camera sensor) index
  * @input: capture input type, grp_id of the attached subdev
  * @user_subdev_api: true if subdevs are not configured by the host driver
  */
@@ -317,7 +316,6 @@ struct fimc_vid_cap {
 	unsigned int			frame_count;
 	unsigned int			reqbufs_count;
 	bool				streaming;
-	int				input_index;
 	u32				input;
 	bool				user_subdev_api;
 };
-- 
2.20.1

[PATCH 10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop

[Jonathan Bakker]

media_pipeline_stop can be called from both release and streamoff,
so make sure they're both protected under the streaming flag and
not just one of them.

This probably became noticeable after commit 2a2599c66368 ("[media] media:
entity: Catch unbalanced media_pipeline_stop calls") was merged as it
added a WARN.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-capture.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 95d4a667bffb..d3ef1268da07 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -1232,8 +1232,11 @@ static int fimc_cap_streamoff(struct file *file, void *priv,
 	if (ret < 0)
 		return ret;
 
-	media_pipeline_stop(&vc->ve.vdev.entity);
-	vc->streaming = false;
+	if (vc->streaming) {
+		media_pipeline_stop(&vc->ve.vdev.entity);
+		vc->streaming = false;
+	}
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH 11/11] media: exynos4-is: Correct parallel port probing

[Jonathan Bakker]

According to the binding doc[1], port A should be reg = 0
and port B reg = 1.  Unfortunately, the driver was treating 0
as invalid and 1 as camera port A.  Match the binding doc and
make 0=A and 1=B.

[1] Documentation/devicetree/bindings/media/samsung-fimc.txt

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/media-dev.c | 18 +++++++++++++-----
 drivers/media/platform/exynos4-is/media-dev.h |  1 +
 include/media/drv-intf/exynos-fimc.h          |  2 +-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index a87ebd7913be..9c4fdf726b92 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -418,13 +418,21 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 		return ret;
 	}
 
-	if (WARN_ON(endpoint.base.port == 0) || *index >= FIMC_MAX_SENSORS) {
-		of_node_put(ep);
-		return -EINVAL;
+	if (fimc_input_is_parallel(endpoint.base.port)) {
+		if (WARN_ON(*index >= FIMC_MAX_PARALLEL)) {
+			of_node_put(ep);
+			return -EINVAL;
+		}
+		pd->mux_id = endpoint.base.port;
+	} else {
+		if (WARN_ON(endpoint.base.port == 0 ||
+				*index >= FIMC_MAX_SENSORS)) {
+			of_node_put(ep);
+			return -EINVAL;
+		}
+		pd->mux_id = (endpoint.base.port - 1) & 0x1;
 	}
 
-	pd->mux_id = (endpoint.base.port - 1) & 0x1;
-
 	rem = of_graph_get_remote_port_parent(ep);
 	of_node_put(ep);
 	if (rem == NULL) {
diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
index 4b8f9ac52ebc..7bd93fd11b33 100644
--- a/drivers/media/platform/exynos4-is/media-dev.h
+++ b/drivers/media/platform/exynos4-is/media-dev.h
@@ -29,6 +29,7 @@
 
 #define PINCTRL_STATE_IDLE	"idle"
 
+#define FIMC_MAX_PARALLEL	2
 #define FIMC_MAX_SENSORS	4
 #define FIMC_MAX_CAMCLKS	2
 #define DEFAULT_SENSOR_CLK_FREQ	24000000U
diff --git a/include/media/drv-intf/exynos-fimc.h b/include/media/drv-intf/exynos-fimc.h
index 6b9ef631d6bb..5a07576c378b 100644
--- a/include/media/drv-intf/exynos-fimc.h
+++ b/include/media/drv-intf/exynos-fimc.h
@@ -44,7 +44,7 @@ enum fimc_bus_type {
 	FIMC_BUS_TYPE_ISP_WRITEBACK = FIMC_BUS_TYPE_LCD_WRITEBACK_B,
 };
 
-#define fimc_input_is_parallel(x) ((x) == 1 || (x) == 2)
+#define fimc_input_is_parallel(x) ((x) == 0 || (x) == 1)
 #define fimc_input_is_mipi_csi(x) ((x) == 3 || (x) == 4)
 
 /*
-- 
2.20.1

Re: [PATCH 01/11] media: exynos4-is: Remove static driver data for S5PV210 FIMC variants

[Tomasz Figa]

On Sat, Apr 25, 2020 at 07:26:40PM -0700, Jonathan Bakker wrote:
> The S5PV210 platform only supports device tree based booting
> where the FIMC variant data is parsed directly from
> the device tree, hence the now unused static data can be removed.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/fimc-core.c | 59 -------------------
>  1 file changed, 59 deletions(-)
> 

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCH 02/11] media: exynos4-is: Request syscon only if ISP writeback is present

[Tomasz Figa]

On Sat, Apr 25, 2020 at 07:26:41PM -0700, Jonathan Bakker wrote:
> From: Tomasz Figa <tomasz.figa@gmail.com>
> 
> On FIMC variants which don't have writeback channel, there is no need to
> access system registers. This patch makes the driver request sysreg
> regmap conditionally depending on whether writeback is supported.
> 
> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/fimc-core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS device present

[Tomasz Figa]

Hi Jonathan,

On Sat, Apr 25, 2020 at 07:26:42PM -0700, Jonathan Bakker wrote:
> Not all devices use the CSIS device, some may use the FIMC directly in
> which case the CSIS device isn't registered.  This leads to a nullptr
> exception when starting the stream as the CSIS device is always
> referenced.  Instead, if getting the CSIS device fails, try getting the
> FIMC directly to check if we are using the subdev API
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/media-dev.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index 9aaf3b8060d5..5c32abc7251b 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -289,11 +289,26 @@ static int __fimc_pipeline_s_stream(struct exynos_media_pipeline *ep, bool on)
>  		{ IDX_CSIS, IDX_FLITE, IDX_FIMC, IDX_SENSOR, IDX_IS_ISP },
>  	};
>  	struct fimc_pipeline *p = to_fimc_pipeline(ep);
> -	struct fimc_md *fmd = entity_to_fimc_mdev(&p->subdevs[IDX_CSIS]->entity);
>  	enum fimc_subdev_index sd_id;
>  	int i, ret = 0;
>  
>  	if (p->subdevs[IDX_SENSOR] == NULL) {
> +		struct fimc_md *fmd;
> +		struct v4l2_subdev *sd = p->subdevs[IDX_CSIS];
> +
> +		if (!sd)
> +			sd = p->subdevs[IDX_FIMC];
> +
> +		if (!sd) {
> +			/*
> +			 * If neither CSIS nor FIMC was set up,
> +			 * it's impossible to have any sensors
> +			 */
> +			return -ENODEV;
> +		}
> +
> +		fmd = entity_to_fimc_mdev(&sd->entity);
> +

Are you sure this is the correct thing to do here? In general, the media
controller should be instantiated only if there are sensors in the system.

What do you mean by using "the FIMC directly"? Do you mean using it only as
an m2m image processor or with a sensor, but without the CSIS, which would
be the case for parallel I/F sensors?

Could you point me to the place where CSIS is always dereferenced? A quick
look through the code only revealed that everywhere it seems to be guarded
by a NULL check.

Another thought from looking at the implementation of
__fimc_pipeline_s_stream() is that it probably shouldn't call s_stream on
all the subdevices included in seq[], but only on those that are actually
included as a part of the pipeline. It would be quite a waste of power to
enable unnecessary hardware.

Best regards,
Tomasz

Re: [PATCH 04/11] media: exynos4-is: Correct missing entity function initialization

[Tomasz Figa]

Hi Jonathan,

On Sat, Apr 25, 2020 at 07:26:43PM -0700, Jonathan Bakker wrote:
> Commit bae4500399c4 ("[media] exynos4-is: Add missing entity function
> initialization") tried to suppress the warnings such as
> 
> s5p-fimc-md camera: Entity type for entity FIMC.0 was not initialized!
> 
> However, this didn't work in all cases.  Correct this by calling the set
> function earlier.
> 
> Fixes: bae4500399c4 ("exynos4-is: Add missing entity function initialization")
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
> index 705f182330ca..86c233e2f2c9 100644
> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
> @@ -1799,7 +1799,6 @@ static int fimc_register_capture_device(struct fimc_dev *fimc,
>  	vid_cap->wb_fmt.code = fmt->mbus_code;
>  
>  	vid_cap->vd_pad.flags = MEDIA_PAD_FL_SINK;
> -	vfd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;

Isn't vfd->entity above a different entity than sd->entity below? If so,
this line must stay.

>  	ret = media_entity_pads_init(&vfd->entity, 1, &vid_cap->vd_pad);
>  	if (ret)
>  		goto err_free_ctx;
> @@ -1898,6 +1897,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc)
>  		return ret;
>  
>  	sd->entity.ops = &fimc_sd_media_ops;
> +	sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;

My understanding is that this is the capture subdev and not the scaler.
Looking at the other drivers, MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER
could be the right function to use here.

Best regards,
Tomasz

Re: [PATCH 05/11] media: exynos4-is: Improve support for sensors with multiple pads

[Tomasz Figa]

Hi Jonathan,

On Sat, Apr 25, 2020 at 07:26:44PM -0700, Jonathan Bakker wrote:
> Commit 1c9f5bd7cb8a ("[media] s5p-fimc: Add support for sensors with
> multiple pads") caught the case where a sensor with multiple pads was
> connected via CSIS, but missed the case where the sensor was directly
> connected to the FIMC.
> 
> This still assumes that the last pad of a sensor is the source.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/media-dev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index 5c32abc7251b..b38445219c72 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -991,7 +991,8 @@ static int fimc_md_create_links(struct fimc_md *fmd)
>  
>  		case FIMC_BUS_TYPE_ITU_601...FIMC_BUS_TYPE_ITU_656:
>  			source = &sensor->entity;
> -			pad = 0;
> +			/* Assume the last pad is the source */
> +			pad = sensor->entity.num_pads - 1;

Is 0 really any worse than num_pads - 1? This sounds like quite an ugly
hack.

Could you iterate over the pads of the sensor entity and explicitly find
a source pad instead?

Best regards,
Tomasz

Re: [PATCH 06/11] media: exynos4-is: Properly set JPEG options when not using CSIS

[Tomasz Figa]

Hi Jonathan,

On Sat, Apr 25, 2020 at 07:26:45PM -0700, Jonathan Bakker wrote:
> Commit ee7160e57c98 ("[media] s5p-fimc: Add support for JPEG capture")
> added support for JPEG capture, but missed setting a register when the
> CSIS device wasn't in use.

nit: Since this isn't really about using the CSIS device or not, but
rather about the interface that the sensor is connected with, could we
instead say "when a parallel interface is used"?

> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/fimc-reg.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/platform/exynos4-is/fimc-reg.c b/drivers/media/platform/exynos4-is/fimc-reg.c
> index 5ce2bdebd424..269a98fca1e8 100644
> --- a/drivers/media/platform/exynos4-is/fimc-reg.c
> +++ b/drivers/media/platform/exynos4-is/fimc-reg.c
> @@ -606,6 +606,11 @@ int fimc_hw_set_camera_source(struct fimc_dev *fimc,
>  	switch (source->fimc_bus_type) {
>  	case FIMC_BUS_TYPE_ITU_601:
>  	case FIMC_BUS_TYPE_ITU_656:
> +		if (fimc_fmt_is_user_defined(f->fmt->color)) {
> +			cfg |= FIMC_REG_CISRCFMT_ITU601_8BIT;
> +			break;
> +		}
> +
>  		for (i = 0; i < ARRAY_SIZE(pix_desc); i++) {
>  			if (vc->ci_fmt.code == pix_desc[i].pixelcode) {
>  				cfg = pix_desc[i].cisrcfmt;
> @@ -707,6 +712,8 @@ int fimc_hw_set_camera_type(struct fimc_dev *fimc,
>  	case FIMC_BUS_TYPE_ITU_601...FIMC_BUS_TYPE_ITU_656:
>  		if (source->mux_id == 0) /* ITU-A, ITU-B: 0, 1 */
>  			cfg |= FIMC_REG_CIGCTRL_SELCAM_ITU_A;
> +		if (vid_cap->ci_fmt.code == MEDIA_BUS_FMT_JPEG_1X8)
> +			cfg |= FIMC_REG_CIGCTRL_CAM_JPEG;

Should we also handle MEDIA_BUS_FMT_S5C_UYVY_JPEG_1X8 as in the CSI
case? The S5C73M3 sensor supports the parallel interface as well.

Best regards,
Tomasz

Re: [PATCH 07/11] media: exynos4-is: Add support for multiple sensors on one port

[Tomasz Figa]

On Sat, Apr 25, 2020 at 07:26:46PM -0700, Jonathan Bakker wrote:
> On some devices, there may be multiple camera sensors attached
> to the same port.  Make sure we probe all of them, not just the
> first one.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/media-dev.c | 32 ++++++++++++-------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index b38445219c72..a87ebd7913be 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -397,25 +397,28 @@ static void fimc_md_pipelines_free(struct fimc_md *fmd)
>  /* Parse port node and register as a sub-device any sensor specified there. */
>  static int fimc_md_parse_port_node(struct fimc_md *fmd,
>  				   struct device_node *port,
> -				   unsigned int index)
> +				   unsigned int *index)
>  {
> -	struct fimc_source_info *pd = &fmd->sensor[index].pdata;
> +	struct fimc_source_info *pd;
>  	struct device_node *rem, *ep, *np;
> -	struct v4l2_fwnode_endpoint endpoint = { .bus_type = 0 };
> +	struct v4l2_fwnode_endpoint endpoint;
>  	int ret;
>  
> -	/* Assume here a port node can have only one endpoint node. */
>  	ep = of_get_next_child(port, NULL);
>  	if (!ep)
>  		return 0;
>  
> +parse_sensor:
> +	pd = &fmd->sensor[*index].pdata;
> +	endpoint.bus_type = 0;
> +
>  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &endpoint);
>  	if (ret) {
>  		of_node_put(ep);
>  		return ret;
>  	}
>  
> -	if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS) {
> +	if (WARN_ON(endpoint.base.port == 0) || *index >= FIMC_MAX_SENSORS) {
>  		of_node_put(ep);
>  		return -EINVAL;
>  	}
> @@ -462,16 +465,16 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>  		pd->fimc_bus_type = pd->sensor_bus_type;
>  	of_node_put(np);
>  
> -	if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) {
> +	if (WARN_ON(*index >= ARRAY_SIZE(fmd->sensor))) {
>  		of_node_put(rem);
>  		return -EINVAL;
>  	}
>  
> -	fmd->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -	fmd->sensor[index].asd.match.fwnode = of_fwnode_handle(rem);
> +	fmd->sensor[*index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	fmd->sensor[*index].asd.match.fwnode = of_fwnode_handle(rem);
>  
>  	ret = v4l2_async_notifier_add_subdev(&fmd->subdev_notifier,
> -					     &fmd->sensor[index].asd);
> +					     &fmd->sensor[*index].asd);
>  	if (ret) {
>  		of_node_put(rem);
>  		return ret;
> @@ -479,6 +482,13 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>  
>  	fmd->num_sensors++;
>  
> +	/* Check for additional sensors on same port */
> +	ep = of_get_next_child(port, ep);
> +	if (ep) {
> +		(*index)++;

Do we need this index argument at all? I can see that we already have
fmd->num_sensors and we increment it every time we discover a sensor.
Perhaps we could just use it instead?

> +		goto parse_sensor;

As we know, goto in principle isn't the best coding pattern. There is a
number of exceptions where it is welcome, e.g. error handling, but
reimplementing a loop using goto is not very nice.

Instead, could you separate the code that probes one sensor into
fimc_md_parse_one_endpoint() and in this one simply iterate over all child
nodes of the port using for_each_child_of_node()?

Best regards,
Tomasz

Re: [PATCH 08/11] media: exynos4-is: Remove inh_sensor_ctrls

[Tomasz Figa]

On Sat, Apr 25, 2020 at 07:26:47PM -0700, Jonathan Bakker wrote:
> This is a no-op as it is never set and is a remnant from non-DT days
> that can be safely removed.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/fimc-capture.c | 13 +------------
>  drivers/media/platform/exynos4-is/fimc-core.h    |  3 ---
>  2 files changed, 1 insertion(+), 15 deletions(-)
> 

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCH 09/11] media: exynos4-is: Remove unused struct member input_index

[Tomasz Figa]

On Sat, Apr 25, 2020 at 07:26:48PM -0700, Jonathan Bakker wrote:
> This is no longer used since the conversion to DT
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/fimc-core.h | 2 --
>  1 file changed, 2 deletions(-)
> 

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCH 10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop

[Tomasz Figa]

Hi Jonathan,

On Sat, Apr 25, 2020 at 07:26:49PM -0700, Jonathan Bakker wrote:
> media_pipeline_stop can be called from both release and streamoff,
> so make sure they're both protected under the streaming flag and
> not just one of them.

First of all, thanks for the patch.

Shouldn't it be that release calls streamoff, so that only streamoff
is supposed to have the call to media_pipeline_stop()?

Best regards,
Tomasz

Re: [PATCH 11/11] media: exynos4-is: Correct parallel port probing

[Tomasz Figa]

Hi Jonathan,

On Sat, Apr 25, 2020 at 07:26:50PM -0700, Jonathan Bakker wrote:
> According to the binding doc[1], port A should be reg = 0
> and port B reg = 1.  Unfortunately, the driver was treating 0
> as invalid and 1 as camera port A.  Match the binding doc and
> make 0=A and 1=B.
> 
> [1] Documentation/devicetree/bindings/media/samsung-fimc.txt
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/media-dev.c | 18 +++++++++++++-----
>  drivers/media/platform/exynos4-is/media-dev.h |  1 +
>  include/media/drv-intf/exynos-fimc.h          |  2 +-
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index a87ebd7913be..9c4fdf726b92 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -418,13 +418,21 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>  		return ret;
>  	}
>  
> -	if (WARN_ON(endpoint.base.port == 0) || *index >= FIMC_MAX_SENSORS) {
> -		of_node_put(ep);
> -		return -EINVAL;
> +	if (fimc_input_is_parallel(endpoint.base.port)) {
> +		if (WARN_ON(*index >= FIMC_MAX_PARALLEL)) {
> +			of_node_put(ep);
> +			return -EINVAL;
> +		}

This check seems to be generic, so could we just move it above the
if/else block?

> +		pd->mux_id = endpoint.base.port;
> +	} else {
> +		if (WARN_ON(endpoint.base.port == 0 ||

Isn't this impossible, since if port == 0, the 'then' branch would've been
taken instead?

Best regards,
Tomasz

Re: [PATCH 01/11] media: exynos4-is: Remove static driver data for S5PV210 FIMC variants

[Sylwester Nawrocki]

On 26.04.2020 04:26, Jonathan Bakker wrote:
> The S5PV210 platform only supports device tree based booting
> where the FIMC variant data is parsed directly from
> the device tree, hence the now unused static data can be removed.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH 02/11] media: exynos4-is: Request syscon only if ISP writeback is present

[Sylwester Nawrocki]

On 26.04.2020 04:26, Jonathan Bakker wrote:
> From: Tomasz Figa <tomasz.figa@gmail.com>
> 
> On FIMC variants which don't have writeback channel, there is no need to
> access system registers. This patch makes the driver request sysreg
> regmap conditionally depending on whether writeback is supported.
> 
> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS device present

[Sylwester Nawrocki]

On 26.04.2020 04:26, Jonathan Bakker wrote:
> Not all devices use the CSIS device, some may use the FIMC directly in
> which case the CSIS device isn't registered.  This leads to a nullptr
> exception when starting the stream as the CSIS device is always
> referenced.  Instead, if getting the CSIS device fails, try getting the
> FIMC directly to check if we are using the subdev API
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>

Thanks, the change looks good to me.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS device present

[Sylwester Nawrocki]

Hi,

On 07.07.2020 19:55, Tomasz Figa wrote:
> On Sat, Apr 25, 2020 at 07:26:42PM -0700, Jonathan Bakker wrote:
>> Not all devices use the CSIS device, some may use the FIMC directly in
>> which case the CSIS device isn't registered.  This leads to a nullptr
>> exception when starting the stream as the CSIS device is always
>> referenced.  Instead, if getting the CSIS device fails, try getting the
>> FIMC directly to check if we are using the subdev API
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> ---
>>  drivers/media/platform/exynos4-is/media-dev.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)

> Thank you for the patch. Please see my comments inline.

>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>> index 9aaf3b8060d5..5c32abc7251b 100644
>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>> @@ -289,11 +289,26 @@ static int __fimc_pipeline_s_stream(struct exynos_media_pipeline *ep, bool on)
>>  		{ IDX_CSIS, IDX_FLITE, IDX_FIMC, IDX_SENSOR, IDX_IS_ISP },
>>  	};
>>  	struct fimc_pipeline *p = to_fimc_pipeline(ep);
>> -	struct fimc_md *fmd = entity_to_fimc_mdev(&p->subdevs[IDX_CSIS]->entity);
>>  	enum fimc_subdev_index sd_id;
>>  	int i, ret = 0;
>>  
>>  	if (p->subdevs[IDX_SENSOR] == NULL) {
>> +		struct fimc_md *fmd;
>> +		struct v4l2_subdev *sd = p->subdevs[IDX_CSIS];
>> +
>> +		if (!sd)
>> +			sd = p->subdevs[IDX_FIMC];
>> +
>> +		if (!sd) {
>> +			/*
>> +			 * If neither CSIS nor FIMC was set up,
>> +			 * it's impossible to have any sensors
>> +			 */
>> +			return -ENODEV;
>> +		}
>> +
>> +		fmd = entity_to_fimc_mdev(&sd->entity);
>> +
> 
> Are you sure this is the correct thing to do here? In general, the media
> controller should be instantiated only if there are sensors in the system.

The code being changed is only about getting a pointer to the driver private 
data structure 'struct fimc_md', for that we need to get hold of a media 
entity that represents a subdev that is actually available in the pipeline.
In original code it is overlooked that there might camera pipelines without
the CSIS subdev. 

> What do you mean by using "the FIMC directly"? Do you mean using it only as
> an m2m image processor or with a sensor, but without the CSIS, which would
> be the case for parallel I/F sensors?

I think it is about a use case where the sensor is connected directly to the 
FIMC capture interface (parallel), without the MIPI-CSI2 receiverin between.

> Could you point me to the place where CSIS is always dereferenced? A quick
> look through the code only revealed that everywhere it seems to be guarded
> by a NULL check.

It's in this patch, above, the very first line that the patch removes.

> Another thought from looking at the implementation of
> __fimc_pipeline_s_stream() is that it probably shouldn't call s_stream on
> all the subdevices included in seq[], but only on those that are actually
> included as a part of the pipeline. It would be quite a waste of power to
> enable unnecessary hardware.

As we talked on IRC, only subdev in current active media pipeline will be
powered on/off. The p->subdevs[] array is sparsely populated and there is 
a test for NULL in __subdev_set_power(). Perhaps the test should be moved
to the caller instead (fimc_pipeline_s_power()), so we don't ignore any
ENXIO errors from the s_power v4l2_subdev_call. But is a material for 
a separate patch.

-- 
Regards,
Sylwester

Re: [PATCH 04/11] media: exynos4-is: Correct missing entity function initialization

[Sylwester Nawrocki]

Hi,

On 07.07.2020 20:09, Tomasz Figa wrote:
> On Sat, Apr 25, 2020 at 07:26:43PM -0700, Jonathan Bakker wrote:
>> Commit bae4500399c4 ("[media] exynos4-is: Add missing entity function
>> initialization") tried to suppress the warnings such as
>>
>> s5p-fimc-md camera: Entity type for entity FIMC.0 was not initialized!
>>
>> However, this didn't work in all cases.  Correct this by calling the set
>> function earlier.
>>
>> Fixes: bae4500399c4 ("exynos4-is: Add missing entity function initialization")
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> ---
>>  drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)

> Thank you for the patch. Please see my comments inline.

>> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
>> index 705f182330ca..86c233e2f2c9 100644
>> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
>> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
>> @@ -1799,7 +1799,6 @@ static int fimc_register_capture_device(struct fimc_dev *fimc,
>>  	vid_cap->wb_fmt.code = fmt->mbus_code;
>>  
>>  	vid_cap->vd_pad.flags = MEDIA_PAD_FL_SINK;
>> -	vfd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;

I think we should leave above line as is, or perhaps change the function
to MEDIA_ENT_F_PROC_VIDEO_COMPOSER and...

> Isn't vfd->entity above a different entity than sd->entity below? If so,
> this line must stay.
> 
>>  	ret = media_entity_pads_init(&vfd->entity, 1, &vid_cap->vd_pad);
>>  	if (ret)
>>  		goto err_free_ctx;
>> @@ -1898,6 +1897,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc)
>>  		return ret;
>>  
>>  	sd->entity.ops = &fimc_sd_media_ops;
>> +	sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;

...also add an assignment like this.

vfd->entity and sd->entity are different entities, vfd->entity corresponds to
the capture video node and sd->entity is the capture subdevice media entity. 

> My understanding is that this is the capture subdev and not the scaler.
> Looking at the other drivers, MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER
> could be the right function to use here.

Scaling can also be configured on that subdev, actually both functions would
be valid.

-- 
Regards,
Sylwester

Re: [PATCH 06/11] media: exynos4-is: Properly set JPEG options when not using CSIS

[Sylwester Nawrocki]

Hi,

On 07.07.2020 20:23, Tomasz Figa wrote:
> On Sat, Apr 25, 2020 at 07:26:45PM -0700, Jonathan Bakker wrote:
>> Commit ee7160e57c98 ("[media] s5p-fimc: Add support for JPEG capture")
>> added support for JPEG capture, but missed setting a register when the
>> CSIS device wasn't in use.

> nit: Since this isn't really about using the CSIS device or not, but
> rather about the interface that the sensor is connected with, could we
> instead say "when a parallel interface is used"?

>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> ---
>>  drivers/media/platform/exynos4-is/fimc-reg.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/media/platform/exynos4-is/fimc-reg.c b/drivers/media/platform/exynos4-is/fimc-reg.c
>> index 5ce2bdebd424..269a98fca1e8 100644
>> --- a/drivers/media/platform/exynos4-is/fimc-reg.c
>> +++ b/drivers/media/platform/exynos4-is/fimc-reg.c
>> @@ -606,6 +606,11 @@ int fimc_hw_set_camera_source(struct fimc_dev *fimc,
>>  	switch (source->fimc_bus_type) {
>>  	case FIMC_BUS_TYPE_ITU_601:
>>  	case FIMC_BUS_TYPE_ITU_656:
>> +		if (fimc_fmt_is_user_defined(f->fmt->color)) {
>> +			cfg |= FIMC_REG_CISRCFMT_ITU601_8BIT;
>> +			break;
>> +		}
>> +
>>  		for (i = 0; i < ARRAY_SIZE(pix_desc); i++) {
>>  			if (vc->ci_fmt.code == pix_desc[i].pixelcode) {
>>  				cfg = pix_desc[i].cisrcfmt;
>> @@ -707,6 +712,8 @@ int fimc_hw_set_camera_type(struct fimc_dev *fimc,
>>  	case FIMC_BUS_TYPE_ITU_601...FIMC_BUS_TYPE_ITU_656:
>>  		if (source->mux_id == 0) /* ITU-A, ITU-B: 0, 1 */
>>  			cfg |= FIMC_REG_CIGCTRL_SELCAM_ITU_A;
>> +		if (vid_cap->ci_fmt.code == MEDIA_BUS_FMT_JPEG_1X8)
>> +			cfg |= FIMC_REG_CIGCTRL_CAM_JPEG;
> 
> Should we also handle MEDIA_BUS_FMT_S5C_UYVY_JPEG_1X8 as in the CSI
> case? The S5C73M3 sensor supports the parallel interface as well.

The parallel interface has too low bandwidth to transfer image data
with the maximum supported resolution and frame rate, I doubt anyone would
ever use S5C73MC in such a configuration.

-- 
Regards,
Sylwester

Re: [PATCH 06/11] media: exynos4-is: Properly set JPEG options when not using CSIS

[Sylwester Nawrocki]

On 26.04.2020 04:26, Jonathan Bakker wrote:
> Commit ee7160e57c98 ("[media] s5p-fimc: Add support for JPEG capture")
> added support for JPEG capture, but missed setting a register when the
> CSIS device wasn't in use.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH 08/11] media: exynos4-is: Remove inh_sensor_ctrls

[Sylwester Nawrocki]

On 26.04.2020 04:26, Jonathan Bakker wrote:
> This is a no-op as it is never set and is a remnant from non-DT days
> that can be safely removed.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH 09/11] media: exynos4-is: Remove unused struct member input_index

[Sylwester Nawrocki]

On 26.04.2020 04:26, Jonathan Bakker wrote:
> This is no longer used since the conversion to DT
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCH 11/11] media: exynos4-is: Correct parallel port probing

[Sylwester Nawrocki]

Hi,

On 26.04.2020 04:26, Jonathan Bakker wrote:
> According to the binding doc[1], port A should be reg = 0
> and port B reg = 1.  Unfortunately, the driver was treating 0
> as invalid and 1 as camera port A.  Match the binding doc and
> make 0=A and 1=B.
> 
> [1] Documentation/devicetree/bindings/media/samsung-fimc.txt

Thank you for correcting this. I would prefer to correct the binding
documentation instead, so it says reg=1 for A and reg=2 for B. 
Then it would also match what we have in dts for Goni and 
enum fimc_input in include/media/drv-intf/exynos-fimc.h.


-- 
Regards,
Sylwester

Re: [PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS device present

[Jonathan Bakker]

Hi Sylwester and Tomasz,

On 2020-07-08 8:11 a.m., Sylwester Nawrocki wrote:
> Hi,
> 
> On 07.07.2020 19:55, Tomasz Figa wrote:
>> On Sat, Apr 25, 2020 at 07:26:42PM -0700, Jonathan Bakker wrote:
>>> Not all devices use the CSIS device, some may use the FIMC directly in
>>> which case the CSIS device isn't registered.  This leads to a nullptr
>>> exception when starting the stream as the CSIS device is always
>>> referenced.  Instead, if getting the CSIS device fails, try getting the
>>> FIMC directly to check if we are using the subdev API
>>>
>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>> ---
>>>  drivers/media/platform/exynos4-is/media-dev.c | 17 ++++++++++++++++-
>>>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
>> Thank you for the patch. Please see my comments inline.
> 

Thank you both for taking the time to review the patches.

>>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>>> index 9aaf3b8060d5..5c32abc7251b 100644
>>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>>> @@ -289,11 +289,26 @@ static int __fimc_pipeline_s_stream(struct exynos_media_pipeline *ep, bool on)
>>>  		{ IDX_CSIS, IDX_FLITE, IDX_FIMC, IDX_SENSOR, IDX_IS_ISP },
>>>  	};
>>>  	struct fimc_pipeline *p = to_fimc_pipeline(ep);
>>> -	struct fimc_md *fmd = entity_to_fimc_mdev(&p->subdevs[IDX_CSIS]->entity);
>>>  	enum fimc_subdev_index sd_id;
>>>  	int i, ret = 0;
>>>  
>>>  	if (p->subdevs[IDX_SENSOR] == NULL) {
>>> +		struct fimc_md *fmd;
>>> +		struct v4l2_subdev *sd = p->subdevs[IDX_CSIS];
>>> +
>>> +		if (!sd)
>>> +			sd = p->subdevs[IDX_FIMC];
>>> +
>>> +		if (!sd) {
>>> +			/*
>>> +			 * If neither CSIS nor FIMC was set up,
>>> +			 * it's impossible to have any sensors
>>> +			 */
>>> +			return -ENODEV;
>>> +		}
>>> +
>>> +		fmd = entity_to_fimc_mdev(&sd->entity);
>>> +
>>
>> Are you sure this is the correct thing to do here? In general, the media
>> controller should be instantiated only if there are sensors in the system.
> 
> The code being changed is only about getting a pointer to the driver private 
> data structure 'struct fimc_md', for that we need to get hold of a media 
> entity that represents a subdev that is actually available in the pipeline.
> In original code it is overlooked that there might camera pipelines without
> the CSIS subdev. 
> 
>> What do you mean by using "the FIMC directly"? Do you mean using it only as
>> an m2m image processor or with a sensor, but without the CSIS, which would
>> be the case for parallel I/F sensors?
> 
> I think it is about a use case where the sensor is connected directly to the 
> FIMC capture interface (parallel), without the MIPI-CSI2 receiverin between.
> 

Yep, that's exactly what I mean.  The device that I'm working with (first gen
Galaxy S) has two devices (S5KA3DFX and CE147) connected via parallel input
to Camera Port A (yes, both of them to the same port).

Is it more correct to say "using the parallel port instead of the CSIS device"?
I'm happy to reword the commit message if that is the case.  

>> Could you point me to the place where CSIS is always dereferenced? A quick
>> look through the code only revealed that everywhere it seems to be guarded
>> by a NULL check.
> 
> It's in this patch, above, the very first line that the patch removes.
> 

Exactly :)

>> Another thought from looking at the implementation of
>> __fimc_pipeline_s_stream() is that it probably shouldn't call s_stream on
>> all the subdevices included in seq[], but only on those that are actually
>> included as a part of the pipeline. It would be quite a waste of power to
>> enable unnecessary hardware.
> 
> As we talked on IRC, only subdev in current active media pipeline will be
> powered on/off. The p->subdevs[] array is sparsely populated and there is 
> a test for NULL in __subdev_set_power(). Perhaps the test should be moved
> to the caller instead (fimc_pipeline_s_power()), so we don't ignore any
> ENXIO errors from the s_power v4l2_subdev_call. But is a material for 
> a separate patch.
> 

Ok, I'll leave it be for now.

Thanks,
Jonathan

Re: [PATCH 04/11] media: exynos4-is: Correct missing entity function initialization

[Jonathan Bakker]

Hi Sylwester and Tomasz,

On 2020-07-08 8:34 a.m., Sylwester Nawrocki wrote:
> Hi,
> 
> On 07.07.2020 20:09, Tomasz Figa wrote:
>> On Sat, Apr 25, 2020 at 07:26:43PM -0700, Jonathan Bakker wrote:
>>> Commit bae4500399c4 ("[media] exynos4-is: Add missing entity function
>>> initialization") tried to suppress the warnings such as
>>>
>>> s5p-fimc-md camera: Entity type for entity FIMC.0 was not initialized!
>>>
>>> However, this didn't work in all cases.  Correct this by calling the set
>>> function earlier.
>>>
>>> Fixes: bae4500399c4 ("exynos4-is: Add missing entity function initialization")
>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>> ---
>>>  drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>> Thank you for the patch. Please see my comments inline.
> 
>>> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
>>> index 705f182330ca..86c233e2f2c9 100644
>>> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
>>> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
>>> @@ -1799,7 +1799,6 @@ static int fimc_register_capture_device(struct fimc_dev *fimc,
>>>  	vid_cap->wb_fmt.code = fmt->mbus_code;
>>>  
>>>  	vid_cap->vd_pad.flags = MEDIA_PAD_FL_SINK;
>>> -	vfd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
> 
> I think we should leave above line as is, or perhaps change the function
> to MEDIA_ENT_F_PROC_VIDEO_COMPOSER and...
> 
>> Isn't vfd->entity above a different entity than sd->entity below? If so,
>> this line must stay.
>>
>>>  	ret = media_entity_pads_init(&vfd->entity, 1, &vid_cap->vd_pad);
>>>  	if (ret)
>>>  		goto err_free_ctx;
>>> @@ -1898,6 +1897,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc)
>>>  		return ret;
>>>  
>>>  	sd->entity.ops = &fimc_sd_media_ops;
>>> +	sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
> 
> ...also add an assignment like this.
> 
> vfd->entity and sd->entity are different entities, vfd->entity corresponds to
> the capture video node and sd->entity is the capture subdevice media entity. 
> 

Whoops, I totally misinterpreted the fact that they are  different entities.  For v2
I'll remove the removal from vfd and just add MEDIA_ENT_F_PROC_VIDEO_SCALER to the sd
entity.

>> My understanding is that this is the capture subdev and not the scaler.
>> Looking at the other drivers, MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER
>> could be the right function to use here.
> 
> Scaling can also be configured on that subdev, actually both functions would
> be valid.
> 

Thanks,
Jonathan

Re: [PATCH 06/11] media: exynos4-is: Properly set JPEG options when not using CSIS

[Jonathan Bakker]

Hi Sylwester and Tomasz,

On 2020-07-08 8:45 a.m., Sylwester Nawrocki wrote:
> Hi,
> 
> On 07.07.2020 20:23, Tomasz Figa wrote:
>> On Sat, Apr 25, 2020 at 07:26:45PM -0700, Jonathan Bakker wrote:
>>> Commit ee7160e57c98 ("[media] s5p-fimc: Add support for JPEG capture")
>>> added support for JPEG capture, but missed setting a register when the
>>> CSIS device wasn't in use.
> 
>> nit: Since this isn't really about using the CSIS device or not, but
>> rather about the interface that the sensor is connected with, could we
>> instead say "when a parallel interface is used"?

Yes, that's a better way of stating it.  I'll reword the commit message.

> 
>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>>> ---
>>>  drivers/media/platform/exynos4-is/fimc-reg.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/exynos4-is/fimc-reg.c b/drivers/media/platform/exynos4-is/fimc-reg.c
>>> index 5ce2bdebd424..269a98fca1e8 100644
>>> --- a/drivers/media/platform/exynos4-is/fimc-reg.c
>>> +++ b/drivers/media/platform/exynos4-is/fimc-reg.c
>>> @@ -606,6 +606,11 @@ int fimc_hw_set_camera_source(struct fimc_dev *fimc,
>>>  	switch (source->fimc_bus_type) {
>>>  	case FIMC_BUS_TYPE_ITU_601:
>>>  	case FIMC_BUS_TYPE_ITU_656:
>>> +		if (fimc_fmt_is_user_defined(f->fmt->color)) {
>>> +			cfg |= FIMC_REG_CISRCFMT_ITU601_8BIT;
>>> +			break;
>>> +		}
>>> +
>>>  		for (i = 0; i < ARRAY_SIZE(pix_desc); i++) {
>>>  			if (vc->ci_fmt.code == pix_desc[i].pixelcode) {
>>>  				cfg = pix_desc[i].cisrcfmt;
>>> @@ -707,6 +712,8 @@ int fimc_hw_set_camera_type(struct fimc_dev *fimc,
>>>  	case FIMC_BUS_TYPE_ITU_601...FIMC_BUS_TYPE_ITU_656:
>>>  		if (source->mux_id == 0) /* ITU-A, ITU-B: 0, 1 */
>>>  			cfg |= FIMC_REG_CIGCTRL_SELCAM_ITU_A;
>>> +		if (vid_cap->ci_fmt.code == MEDIA_BUS_FMT_JPEG_1X8)
>>> +			cfg |= FIMC_REG_CIGCTRL_CAM_JPEG;
>>
>> Should we also handle MEDIA_BUS_FMT_S5C_UYVY_JPEG_1X8 as in the CSI
>> case? The S5C73M3 sensor supports the parallel interface as well.
> 
> The parallel interface has too low bandwidth to transfer image data
> with the maximum supported resolution and frame rate, I doubt anyone would
> ever use S5C73MC in such a configuration.
> 

Ok, good to know.  I'll leave it as-is then.

Thanks,
Jonathan

Re: [PATCH 07/11] media: exynos4-is: Add support for multiple sensors on one port

[Jonathan Bakker]

Hi Tomasz,

On 2020-07-07 11:36 a.m., Tomasz Figa wrote:
> On Sat, Apr 25, 2020 at 07:26:46PM -0700, Jonathan Bakker wrote:
>> On some devices, there may be multiple camera sensors attached
>> to the same port.  Make sure we probe all of them, not just the
>> first one.
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> ---
>>  drivers/media/platform/exynos4-is/media-dev.c | 32 ++++++++++++-------
>>  1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>> index b38445219c72..a87ebd7913be 100644
>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>> @@ -397,25 +397,28 @@ static void fimc_md_pipelines_free(struct fimc_md *fmd)
>>  /* Parse port node and register as a sub-device any sensor specified there. */
>>  static int fimc_md_parse_port_node(struct fimc_md *fmd,
>>  				   struct device_node *port,
>> -				   unsigned int index)
>> +				   unsigned int *index)
>>  {
>> -	struct fimc_source_info *pd = &fmd->sensor[index].pdata;
>> +	struct fimc_source_info *pd;
>>  	struct device_node *rem, *ep, *np;
>> -	struct v4l2_fwnode_endpoint endpoint = { .bus_type = 0 };
>> +	struct v4l2_fwnode_endpoint endpoint;
>>  	int ret;
>>  
>> -	/* Assume here a port node can have only one endpoint node. */
>>  	ep = of_get_next_child(port, NULL);
>>  	if (!ep)
>>  		return 0;
>>  
>> +parse_sensor:
>> +	pd = &fmd->sensor[*index].pdata;
>> +	endpoint.bus_type = 0;
>> +
>>  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &endpoint);
>>  	if (ret) {
>>  		of_node_put(ep);
>>  		return ret;
>>  	}
>>  
>> -	if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS) {
>> +	if (WARN_ON(endpoint.base.port == 0) || *index >= FIMC_MAX_SENSORS) {
>>  		of_node_put(ep);
>>  		return -EINVAL;
>>  	}
>> @@ -462,16 +465,16 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>>  		pd->fimc_bus_type = pd->sensor_bus_type;
>>  	of_node_put(np);
>>  
>> -	if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor))) {
>> +	if (WARN_ON(*index >= ARRAY_SIZE(fmd->sensor))) {
>>  		of_node_put(rem);
>>  		return -EINVAL;
>>  	}
>>  
>> -	fmd->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>> -	fmd->sensor[index].asd.match.fwnode = of_fwnode_handle(rem);
>> +	fmd->sensor[*index].asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>> +	fmd->sensor[*index].asd.match.fwnode = of_fwnode_handle(rem);
>>  
>>  	ret = v4l2_async_notifier_add_subdev(&fmd->subdev_notifier,
>> -					     &fmd->sensor[index].asd);
>> +					     &fmd->sensor[*index].asd);
>>  	if (ret) {
>>  		of_node_put(rem);
>>  		return ret;
>> @@ -479,6 +482,13 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>>  
>>  	fmd->num_sensors++;
>>  
>> +	/* Check for additional sensors on same port */
>> +	ep = of_get_next_child(port, ep);
>> +	if (ep) {
>> +		(*index)++;
> 
> Do we need this index argument at all? I can see that we already have
> fmd->num_sensors and we increment it every time we discover a sensor.
> Perhaps we could just use it instead?
> 
>> +		goto parse_sensor;
> 
> As we know, goto in principle isn't the best coding pattern. There is a
> number of exceptions where it is welcome, e.g. error handling, but
> reimplementing a loop using goto is not very nice.
> 
> Instead, could you separate the code that probes one sensor into
> fimc_md_parse_one_endpoint() and in this one simply iterate over all child
> nodes of the port using for_each_child_of_node()?
> 

That definitely looks doable, thanks for the suggestion.  I'll work on implementing
and testing this.  It should then also be possible to remove the index hack as well.

> Best regards,
> Tomasz
> 

Thanks,
Jonathan

Re: [PATCH 10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop

[Jonathan Bakker]

Hi Tomasz,

On 2020-07-07 11:44 a.m., Tomasz Figa wrote:
> Hi Jonathan,
> 
> On Sat, Apr 25, 2020 at 07:26:49PM -0700, Jonathan Bakker wrote:
>> media_pipeline_stop can be called from both release and streamoff,
>> so make sure they're both protected under the streaming flag and
>> not just one of them.
> 
> First of all, thanks for the patch.
> 
> Shouldn't it be that release calls streamoff, so that only streamoff
> is supposed to have the call to media_pipeline_stop()?
> 

I can't say that I understand the whole media subsystem enough to know :)
Since media_pipeline_start is called in streamon, it makes sense that streamoff
should have the media_pipeline_stop call.  However, even after removing the call
in fimc_capture_release I'm still getting a backtrace such as

[   73.843117] ------------[ cut here ]------------
[   73.843251] WARNING: CPU: 0 PID: 1575 at drivers/media/mc/mc-entity.c:554 media_pipeline_stop+0x20/0x2c [mc]
[   73.843265] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig pvrsrvkm_s5pv210_sgx540_120 videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 videobuf2_common hci_uart sha256_generic libsha256 btbcm bluetooth cfg80211 brcmutil ecdh_generic ecc ce147 libaes s5ka3dfx videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
[   73.843471] CPU: 0 PID: 1575 Comm: v4l2-ctl Not tainted 5.7.0-14534-g2b33418b254e-dirty #669
[   73.843487] Hardware name: Samsung S5PC110/S5PV210-based board
[   73.843562] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
[   73.843613] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
[   73.843661] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
[   73.843734] [<c01170b0>] (warn_slowpath_fmt) from [<bf00c20c>] (media_pipeline_stop+0x20/0x2c [mc])
[   73.843867] [<bf00c20c>] (media_pipeline_stop [mc]) from [<bf145c48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
[   73.844109] [<bf145c48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf03cbf4>] (__video_do_ioctl+0x220/0x448 [videodev])
[   73.844308] [<bf03cbf4>] (__video_do_ioctl [videodev]) from [<bf03d600>] (video_usercopy+0x114/0x498 [videodev])
[   73.844438] [<bf03d600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
[   73.844484] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
[   73.844505] Exception stack(0xe5083fa8 to 0xe5083ff0)
[   73.844546] 3fa0:                   0049908d bef8f8c0 00000003 40045613 bef8d5ac 004c1d16
[   73.844590] 3fc0: 0049908d bef8f8c0 bef8f8c0 00000036 bef8d5ac 00000000 b6d6b320 bef8faf8
[   73.844620] 3fe0: 004e3ed4 bef8c718 004990bb b6f00d0a
[   73.844642] ---[ end trace e6a4a8b2f20addd4 ]---

The command I'm using for testing is

v4l2-ctl --verbose -d 1 --stream-mmap=3 --stream-skip=2 --stream-to=./test.yuv --stream-count=1

Since I noticed that the streaming flag was being checked fimc_capture_release
but not in fimc_cap_streamoff, I assumed that it was simply a missed check.  Comparing
with other drivers, they seem to call media_pipeline_stop in their vb2_ops stop_streaming
callback.

I'm willing to test various options

> Best regards,
> Tomasz
> 

Thanks,
Jonathan

Re: [PATCH 11/11] media: exynos4-is: Correct parallel port probing

[Jonathan Bakker]

Hi Sylwester,

On 2020-07-08 9:15 a.m., Sylwester Nawrocki wrote:
> Hi,
> 
> On 26.04.2020 04:26, Jonathan Bakker wrote:
>> According to the binding doc[1], port A should be reg = 0
>> and port B reg = 1.  Unfortunately, the driver was treating 0
>> as invalid and 1 as camera port A.  Match the binding doc and
>> make 0=A and 1=B.
>>
>> [1] Documentation/devicetree/bindings/media/samsung-fimc.txt
> 
> Thank you for correcting this. I would prefer to correct the binding
> documentation instead, so it says reg=1 for A and reg=2 for B. 
> Then it would also match what we have in dts for Goni and 
> enum fimc_input in include/media/drv-intf/exynos-fimc.h

No problem, that works for me.  I'll drop this patch and replace it with one
changing the documentation.

> 
> 

Thanks,
Jonathan

Re: [PATCH 05/11] media: exynos4-is: Improve support for sensors with multiple pads

[Jonathan Bakker]

Hi Tomasz,

On 2020-07-07 11:13 a.m., Tomasz Figa wrote:
> Hi Jonathan,
> 
> On Sat, Apr 25, 2020 at 07:26:44PM -0700, Jonathan Bakker wrote:
>> Commit 1c9f5bd7cb8a ("[media] s5p-fimc: Add support for sensors with
>> multiple pads") caught the case where a sensor with multiple pads was
>> connected via CSIS, but missed the case where the sensor was directly
>> connected to the FIMC.
>>
>> This still assumes that the last pad of a sensor is the source.
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> ---
>>  drivers/media/platform/exynos4-is/media-dev.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
> 
> Thank you for the patch. Please see my comments inline.
> 
>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>> index 5c32abc7251b..b38445219c72 100644
>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>> @@ -991,7 +991,8 @@ static int fimc_md_create_links(struct fimc_md *fmd)
>>  
>>  		case FIMC_BUS_TYPE_ITU_601...FIMC_BUS_TYPE_ITU_656:
>>  			source = &sensor->entity;
>> -			pad = 0;
>> +			/* Assume the last pad is the source */
>> +			pad = sensor->entity.num_pads - 1;
> 
> Is 0 really any worse than num_pads - 1? This sounds like quite an ugly
> hack.
> 
> Could you iterate over the pads of the sensor entity and explicitly find
> a source pad instead?

Yes, iterating would work better.  This comes from when I was trying to integrate
video-mux, before I realized I could connect multiple sensors.  I will drop this patch
from v2 as it's not necessary.

> 
> Best regards,
> Tomasz
> 

Thanks,
Jonathan

Re: [PATCH 10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop

[Tomasz Figa]

On Sat, Jul 11, 2020 at 8:17 PM Jonathan Bakker <xc-racer2@live.ca> wrote:
>
> Hi Tomasz,
>
> On 2020-07-07 11:44 a.m., Tomasz Figa wrote:
> > Hi Jonathan,
> >
> > On Sat, Apr 25, 2020 at 07:26:49PM -0700, Jonathan Bakker wrote:
> >> media_pipeline_stop can be called from both release and streamoff,
> >> so make sure they're both protected under the streaming flag and
> >> not just one of them.
> >
> > First of all, thanks for the patch.
> >
> > Shouldn't it be that release calls streamoff, so that only streamoff
> > is supposed to have the call to media_pipeline_stop()?
> >
>
> I can't say that I understand the whole media subsystem enough to know :)
> Since media_pipeline_start is called in streamon, it makes sense that streamoff
> should have the media_pipeline_stop call.  However, even after removing the call
> in fimc_capture_release I'm still getting a backtrace such as
>
> [   73.843117] ------------[ cut here ]------------
> [   73.843251] WARNING: CPU: 0 PID: 1575 at drivers/media/mc/mc-entity.c:554 media_pipeline_stop+0x20/0x2c [mc]
> [   73.843265] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig pvrsrvkm_s5pv210_sgx540_120 videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 videobuf2_common hci_uart sha256_generic libsha256 btbcm bluetooth cfg80211 brcmutil ecdh_generic ecc ce147 libaes s5ka3dfx videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
> [   73.843471] CPU: 0 PID: 1575 Comm: v4l2-ctl Not tainted 5.7.0-14534-g2b33418b254e-dirty #669
> [   73.843487] Hardware name: Samsung S5PC110/S5PV210-based board
> [   73.843562] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
> [   73.843613] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
> [   73.843661] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
> [   73.843734] [<c01170b0>] (warn_slowpath_fmt) from [<bf00c20c>] (media_pipeline_stop+0x20/0x2c [mc])
> [   73.843867] [<bf00c20c>] (media_pipeline_stop [mc]) from [<bf145c48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
> [   73.844109] [<bf145c48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf03cbf4>] (__video_do_ioctl+0x220/0x448 [videodev])
> [   73.844308] [<bf03cbf4>] (__video_do_ioctl [videodev]) from [<bf03d600>] (video_usercopy+0x114/0x498 [videodev])
> [   73.844438] [<bf03d600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
> [   73.844484] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
> [   73.844505] Exception stack(0xe5083fa8 to 0xe5083ff0)
> [   73.844546] 3fa0:                   0049908d bef8f8c0 00000003 40045613 bef8d5ac 004c1d16
> [   73.844590] 3fc0: 0049908d bef8f8c0 bef8f8c0 00000036 bef8d5ac 00000000 b6d6b320 bef8faf8
> [   73.844620] 3fe0: 004e3ed4 bef8c718 004990bb b6f00d0a
> [   73.844642] ---[ end trace e6a4a8b2f20addd4 ]---
>
> The command I'm using for testing is
>
> v4l2-ctl --verbose -d 1 --stream-mmap=3 --stream-skip=2 --stream-to=./test.yuv --stream-count=1
>
> Since I noticed that the streaming flag was being checked fimc_capture_release
> but not in fimc_cap_streamoff, I assumed that it was simply a missed check.  Comparing
> with other drivers, they seem to call media_pipeline_stop in their vb2_ops stop_streaming
> callback.

vb2 does a lot of state handling internally and makes sure that driver
ops are not called when unnecessary, preventing double calls for
example. I suppose it could be a better place to stop the pipeline
indeed. However, ...

>
> I'm willing to test various options
>

I think it could make sense to add something like WARN_ON(1) inside
media_pipeline_stop() and then check where the first call came from.

Best regards,
Tomasz

Re: [PATCH 10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop

[Jonathan Bakker]

Hi Tomasz,

On 2020-07-20 6:10 a.m., Tomasz Figa wrote:
> On Sat, Jul 11, 2020 at 8:17 PM Jonathan Bakker <xc-racer2@live.ca> wrote:
>>
>> Hi Tomasz,
>>
>> On 2020-07-07 11:44 a.m., Tomasz Figa wrote:
>>> Hi Jonathan,
>>>
>>> On Sat, Apr 25, 2020 at 07:26:49PM -0700, Jonathan Bakker wrote:
>>>> media_pipeline_stop can be called from both release and streamoff,
>>>> so make sure they're both protected under the streaming flag and
>>>> not just one of them.
>>>
>>> First of all, thanks for the patch.
>>>
>>> Shouldn't it be that release calls streamoff, so that only streamoff
>>> is supposed to have the call to media_pipeline_stop()?
>>>
>>
>> I can't say that I understand the whole media subsystem enough to know :)
>> Since media_pipeline_start is called in streamon, it makes sense that streamoff
>> should have the media_pipeline_stop call.  However, even after removing the call
>> in fimc_capture_release I'm still getting a backtrace such as
>>
>> [   73.843117] ------------[ cut here ]------------
>> [   73.843251] WARNING: CPU: 0 PID: 1575 at drivers/media/mc/mc-entity.c:554 media_pipeline_stop+0x20/0x2c [mc]
>> [   73.843265] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig pvrsrvkm_s5pv210_sgx540_120 videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 videobuf2_common hci_uart sha256_generic libsha256 btbcm bluetooth cfg80211 brcmutil ecdh_generic ecc ce147 libaes s5ka3dfx videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
>> [   73.843471] CPU: 0 PID: 1575 Comm: v4l2-ctl Not tainted 5.7.0-14534-g2b33418b254e-dirty #669
>> [   73.843487] Hardware name: Samsung S5PC110/S5PV210-based board
>> [   73.843562] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
>> [   73.843613] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
>> [   73.843661] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
>> [   73.843734] [<c01170b0>] (warn_slowpath_fmt) from [<bf00c20c>] (media_pipeline_stop+0x20/0x2c [mc])
>> [   73.843867] [<bf00c20c>] (media_pipeline_stop [mc]) from [<bf145c48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
>> [   73.844109] [<bf145c48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf03cbf4>] (__video_do_ioctl+0x220/0x448 [videodev])
>> [   73.844308] [<bf03cbf4>] (__video_do_ioctl [videodev]) from [<bf03d600>] (video_usercopy+0x114/0x498 [videodev])
>> [   73.844438] [<bf03d600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
>> [   73.844484] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
>> [   73.844505] Exception stack(0xe5083fa8 to 0xe5083ff0)
>> [   73.844546] 3fa0:                   0049908d bef8f8c0 00000003 40045613 bef8d5ac 004c1d16
>> [   73.844590] 3fc0: 0049908d bef8f8c0 bef8f8c0 00000036 bef8d5ac 00000000 b6d6b320 bef8faf8
>> [   73.844620] 3fe0: 004e3ed4 bef8c718 004990bb b6f00d0a
>> [   73.844642] ---[ end trace e6a4a8b2f20addd4 ]---
>>
>> The command I'm using for testing is
>>
>> v4l2-ctl --verbose -d 1 --stream-mmap=3 --stream-skip=2 --stream-to=./test.yuv --stream-count=1
>>
>> Since I noticed that the streaming flag was being checked fimc_capture_release
>> but not in fimc_cap_streamoff, I assumed that it was simply a missed check.  Comparing
>> with other drivers, they seem to call media_pipeline_stop in their vb2_ops stop_streaming
>> callback.
> 
> vb2 does a lot of state handling internally and makes sure that driver
> ops are not called when unnecessary, preventing double calls for
> example. I suppose it could be a better place to stop the pipeline
> indeed. However, ...
> 
>>
>> I'm willing to test various options
>>
> 
> I think it could make sense to add something like WARN_ON(1) inside
> media_pipeline_stop() and then check where the first call came from.

Here's the results of that:

[   69.876823] ------------[ cut here ]------------
[   69.876962] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:550 __media_pipeline_stop+0x24/0xfc [mc]
[   69.876976] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
[   69.877182] CPU: 0 PID: 1566 Comm: v4l2-ctl Not tainted 5.7.0-14540-gb1220848c797-dirty #681
[   69.877198] Hardware name: Samsung S5PC110/S5PV210-based board
[   69.877274] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
[   69.877326] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
[   69.877375] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
[   69.877448] [<c01170b0>] (warn_slowpath_fmt) from [<bf010130>] (__media_pipeline_stop+0x24/0xfc [mc])
[   69.877540] [<bf010130>] (__media_pipeline_stop [mc]) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
[   69.877663] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
[   69.877904] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
[   69.878105] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
[   69.878234] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
[   69.878281] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
[   69.878301] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
[   69.878342] 1fa0:                   004ef08d 00539d0c 00000003 40045613 bec1578c 00517d16
[   69.878386] 1fc0: 004ef08d 00539d0c bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
[   69.878415] 1fe0: 00539ed4 bec15730 004ef0bb b6f84d0a
[   69.878436] ---[ end trace d004ab573a72c329 ]---
[   69.879704] ------------[ cut here ]------------
[   69.879794] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:550 __media_pipeline_stop+0x24/0xfc [mc]
[   69.879806] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
[   69.880002] CPU: 0 PID: 1566 Comm: v4l2-ctl Tainted: G        W         5.7.0-14540-gb1220848c797-dirty #681
[   69.880016] Hardware name: Samsung S5PC110/S5PV210-based board
[   69.880071] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
[   69.880115] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
[   69.880161] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
[   69.880231] [<c01170b0>] (warn_slowpath_fmt) from [<bf010130>] (__media_pipeline_stop+0x24/0xfc [mc])
[   69.880318] [<bf010130>] (__media_pipeline_stop [mc]) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
[   69.880419] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
[   69.880582] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
[   69.880776] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
[   69.880895] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
[   69.880939] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
[   69.880958] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
[   69.880997] 1fa0:                   004ef08d bec188c0 00000003 40045613 bec165ac 00517d16
[   69.881040] 1fc0: 004ef08d bec188c0 bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
[   69.881070] 1fe0: 00539ed4 bec15718 004ef0bb b6f84d0a
[   69.881089] ---[ end trace d004ab573a72c32a ]---
[   69.881102] ------------[ cut here ]------------
[   69.881163] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:556 media_pipeline_stop+0x20/0x2c [mc]
[   69.881174] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
[   69.881367] CPU: 0 PID: 1566 Comm: v4l2-ctl Tainted: G        W         5.7.0-14540-gb1220848c797-dirty #681
[   69.881381] Hardware name: Samsung S5PC110/S5PV210-based board
[   69.881424] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
[   69.881465] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
[   69.881511] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
[   69.881580] [<c01170b0>] (warn_slowpath_fmt) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
[   69.881683] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
[   69.881834] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
[   69.882025] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
[   69.882246] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
[   69.882291] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
[   69.882309] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
[   69.882348] 1fa0:                   004ef08d bec188c0 00000003 40045613 bec165ac 00517d16
[   69.882391] 1fc0: 004ef08d bec188c0 bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
[   69.882420] 1fe0: 00539ed4 bec15718 004ef0bb b6f84d0a
[   69.882439] ---[ end trace d004ab573a72c32b ]---

With the final trace being the original one that I was having.

So it looks to me as if streamoff is being called twice.  Is this a possibility for all drivers
or is there a different bug that I should be trying to track down?  In any event, my patch does
prevent the warning (although my reasoning was wrong as I thought it was being stopped on the
call to release).

Thanks,
Jonathan

> 
> Best regards,
> Tomasz
> 

Re: [PATCH 10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop

[Tomasz Figa]

On Sat, Jul 25, 2020 at 1:46 AM Jonathan Bakker <xc-racer2@live.ca> wrote:
>
> Hi Tomasz,
>
> On 2020-07-20 6:10 a.m., Tomasz Figa wrote:
> > On Sat, Jul 11, 2020 at 8:17 PM Jonathan Bakker <xc-racer2@live.ca> wrote:
> >>
> >> Hi Tomasz,
> >>
> >> On 2020-07-07 11:44 a.m., Tomasz Figa wrote:
> >>> Hi Jonathan,
> >>>
> >>> On Sat, Apr 25, 2020 at 07:26:49PM -0700, Jonathan Bakker wrote:
> >>>> media_pipeline_stop can be called from both release and streamoff,
> >>>> so make sure they're both protected under the streaming flag and
> >>>> not just one of them.
> >>>
> >>> First of all, thanks for the patch.
> >>>
> >>> Shouldn't it be that release calls streamoff, so that only streamoff
> >>> is supposed to have the call to media_pipeline_stop()?
> >>>
> >>
> >> I can't say that I understand the whole media subsystem enough to know :)
> >> Since media_pipeline_start is called in streamon, it makes sense that streamoff
> >> should have the media_pipeline_stop call.  However, even after removing the call
> >> in fimc_capture_release I'm still getting a backtrace such as
> >>
> >> [   73.843117] ------------[ cut here ]------------
> >> [   73.843251] WARNING: CPU: 0 PID: 1575 at drivers/media/mc/mc-entity.c:554 media_pipeline_stop+0x20/0x2c [mc]
> >> [   73.843265] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig pvrsrvkm_s5pv210_sgx540_120 videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 videobuf2_common hci_uart sha256_generic libsha256 btbcm bluetooth cfg80211 brcmutil ecdh_generic ecc ce147 libaes s5ka3dfx videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
> >> [   73.843471] CPU: 0 PID: 1575 Comm: v4l2-ctl Not tainted 5.7.0-14534-g2b33418b254e-dirty #669
> >> [   73.843487] Hardware name: Samsung S5PC110/S5PV210-based board
> >> [   73.843562] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
> >> [   73.843613] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
> >> [   73.843661] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
> >> [   73.843734] [<c01170b0>] (warn_slowpath_fmt) from [<bf00c20c>] (media_pipeline_stop+0x20/0x2c [mc])
> >> [   73.843867] [<bf00c20c>] (media_pipeline_stop [mc]) from [<bf145c48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
> >> [   73.844109] [<bf145c48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf03cbf4>] (__video_do_ioctl+0x220/0x448 [videodev])
> >> [   73.844308] [<bf03cbf4>] (__video_do_ioctl [videodev]) from [<bf03d600>] (video_usercopy+0x114/0x498 [videodev])
> >> [   73.844438] [<bf03d600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
> >> [   73.844484] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
> >> [   73.844505] Exception stack(0xe5083fa8 to 0xe5083ff0)
> >> [   73.844546] 3fa0:                   0049908d bef8f8c0 00000003 40045613 bef8d5ac 004c1d16
> >> [   73.844590] 3fc0: 0049908d bef8f8c0 bef8f8c0 00000036 bef8d5ac 00000000 b6d6b320 bef8faf8
> >> [   73.844620] 3fe0: 004e3ed4 bef8c718 004990bb b6f00d0a
> >> [   73.844642] ---[ end trace e6a4a8b2f20addd4 ]---
> >>
> >> The command I'm using for testing is
> >>
> >> v4l2-ctl --verbose -d 1 --stream-mmap=3 --stream-skip=2 --stream-to=./test.yuv --stream-count=1
> >>
> >> Since I noticed that the streaming flag was being checked fimc_capture_release
> >> but not in fimc_cap_streamoff, I assumed that it was simply a missed check.  Comparing
> >> with other drivers, they seem to call media_pipeline_stop in their vb2_ops stop_streaming
> >> callback.
> >
> > vb2 does a lot of state handling internally and makes sure that driver
> > ops are not called when unnecessary, preventing double calls for
> > example. I suppose it could be a better place to stop the pipeline
> > indeed. However, ...
> >
> >>
> >> I'm willing to test various options
> >>
> >
> > I think it could make sense to add something like WARN_ON(1) inside
> > media_pipeline_stop() and then check where the first call came from.
>
> Here's the results of that:
>
> [   69.876823] ------------[ cut here ]------------
> [   69.876962] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:550 __media_pipeline_stop+0x24/0xfc [mc]
> [   69.876976] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
> [   69.877182] CPU: 0 PID: 1566 Comm: v4l2-ctl Not tainted 5.7.0-14540-gb1220848c797-dirty #681
> [   69.877198] Hardware name: Samsung S5PC110/S5PV210-based board
> [   69.877274] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
> [   69.877326] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
> [   69.877375] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
> [   69.877448] [<c01170b0>] (warn_slowpath_fmt) from [<bf010130>] (__media_pipeline_stop+0x24/0xfc [mc])
> [   69.877540] [<bf010130>] (__media_pipeline_stop [mc]) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
> [   69.877663] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
> [   69.877904] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
> [   69.878105] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
> [   69.878234] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
> [   69.878281] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
> [   69.878301] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
> [   69.878342] 1fa0:                   004ef08d 00539d0c 00000003 40045613 bec1578c 00517d16
> [   69.878386] 1fc0: 004ef08d 00539d0c bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
> [   69.878415] 1fe0: 00539ed4 bec15730 004ef0bb b6f84d0a
> [   69.878436] ---[ end trace d004ab573a72c329 ]---
> [   69.879704] ------------[ cut here ]------------
> [   69.879794] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:550 __media_pipeline_stop+0x24/0xfc [mc]
> [   69.879806] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
> [   69.880002] CPU: 0 PID: 1566 Comm: v4l2-ctl Tainted: G        W         5.7.0-14540-gb1220848c797-dirty #681
> [   69.880016] Hardware name: Samsung S5PC110/S5PV210-based board
> [   69.880071] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
> [   69.880115] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
> [   69.880161] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
> [   69.880231] [<c01170b0>] (warn_slowpath_fmt) from [<bf010130>] (__media_pipeline_stop+0x24/0xfc [mc])
> [   69.880318] [<bf010130>] (__media_pipeline_stop [mc]) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
> [   69.880419] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
> [   69.880582] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
> [   69.880776] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
> [   69.880895] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
> [   69.880939] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
> [   69.880958] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
> [   69.880997] 1fa0:                   004ef08d bec188c0 00000003 40045613 bec165ac 00517d16
> [   69.881040] 1fc0: 004ef08d bec188c0 bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
> [   69.881070] 1fe0: 00539ed4 bec15718 004ef0bb b6f84d0a
> [   69.881089] ---[ end trace d004ab573a72c32a ]---
> [   69.881102] ------------[ cut here ]------------
> [   69.881163] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:556 media_pipeline_stop+0x20/0x2c [mc]
> [   69.881174] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
> [   69.881367] CPU: 0 PID: 1566 Comm: v4l2-ctl Tainted: G        W         5.7.0-14540-gb1220848c797-dirty #681
> [   69.881381] Hardware name: Samsung S5PC110/S5PV210-based board
> [   69.881424] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
> [   69.881465] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
> [   69.881511] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
> [   69.881580] [<c01170b0>] (warn_slowpath_fmt) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
> [   69.881683] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
> [   69.881834] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
> [   69.882025] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
> [   69.882246] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
> [   69.882291] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
> [   69.882309] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
> [   69.882348] 1fa0:                   004ef08d bec188c0 00000003 40045613 bec165ac 00517d16
> [   69.882391] 1fc0: 004ef08d bec188c0 bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
> [   69.882420] 1fe0: 00539ed4 bec15718 004ef0bb b6f84d0a
> [   69.882439] ---[ end trace d004ab573a72c32b ]---
>
> With the final trace being the original one that I was having.
>
> So it looks to me as if streamoff is being called twice.  Is this a possibility for all drivers
> or is there a different bug that I should be trying to track down?  In any event, my patch does
> prevent the warning (although my reasoning was wrong as I thought it was being stopped on the
> call to release).

It certainly is possible for the userspace call STREAMOFF twice and
the driver needs to ensure that the second call is essentially a
no-op, as per [1]. FWIW, if the driver defers the stream start/stop
operation entirely to the vb2 .start/stop_streaming callback, then vb2
would handle this automatically.

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html#description

Best regards,
Tomasz