[PATCH 0/5] rcar-vin: Add support for digital input on Gen3

[PATCH 0/5] rcar-vin: Add support for digital input on Gen3

[Jacopo Mondi]

Hello,
   this series add support for 'digital' input to the Gen3 version of rcar-vin
driver.

'Digital' inputs (the terms comes from the existing Gen2 version of the driver)
describe parallel video input sources connected to a VIN instance. So far, the
Gen3-version of the driver (the media-controller compliant one) only supported
CSI-2 inputs.

This series extends the device tree parsing to accept a connection on port@0,
and parses the 'digital' subdevice, as implemented in patches [2/5] and [3/5].

The series has been tested on D3 Draak platform, which has an HDMI decoder
connected to the parallel input of VIN4. To have capture operations working
properly two additional patches have been added to the series.
[4/5] is a general fix which should imo be included regardless of this series.
[5/5] won't please Niklas, as it discards buffer overflow protection for
the digital capture operations. As explained in the commit message, I had to
fall back to use field toggling on VSYNC input to have images correctly
captured. A possible protection against buffer overflow may be enabling
interrupt for the FIFO overflow and stop capture at that point, but this have to
be discussed later.

A separate series for the VIN4 and HDMI input enabling on Draak board has been
sent to renesas-soc list.

The vin-tests repository patches to automate capture testing have been extended
to support D3 board and capture from HDMI output, and patches have been sent
to Niklas.

The series is based on the media-master tree, where VIN patches have been
recently merged.

Tested capturing HDMI input images on D3 and for backward compatibility on
Salvator-X M3-W too (seems like I didn't break anything there).

Patches for testing on D3 are available at:
git://jmondi.org/linux d3/media-master/driver
git://jmondi.org/linux d3/media-master/dts
git://jmondi.org/linux d3/media-master/test
git://jmondi.org/vin-tests d3

Patches to test on M3-W (based on latest renesas drivers, which includes an
older version of VIN series, but has CSI-2 driver) available at:
git://jmondi.org/linux d3/renesas-drivers/test

Thanks
    j

Jacopo Mondi (5):
  media: rcar-vin: Add support for R-Car R8A77995 SoC
  media: rcar-vin: Add digital input subdevice parsing
  media: rcar-vin: [un]bind and link digital subdevice
  media: rcar-vin: Do not use crop if not configured
  media: rcar-vin: Use FTEV for digital input

 drivers/media/platform/rcar-vin/rcar-core.c | 315 +++++++++++++++++++++++-----
 drivers/media/platform/rcar-vin/rcar-dma.c  |  33 ++-
 drivers/media/platform/rcar-vin/rcar-vin.h  |  13 ++
 3 files changed, 305 insertions(+), 56 deletions(-)

--
2.7.4

[PATCH 1/5] media: rcar-vin: Add support for R-Car R8A77995 SoC

[Jacopo Mondi]

Add R-Car R8A77995 SoC to the rcar-vin supported ones.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d3072e1..e547ef7 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -985,6 +985,10 @@ static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
 	{ /* Sentinel */ }
 };
 
+static const struct rvin_group_route _rcar_info_r8a77995_routes[] = {
+	{ /* Sentinel */ }
+};
+
 static const struct rvin_info rcar_info_r8a77970 = {
 	.model = RCAR_GEN3,
 	.use_mc = true,
@@ -993,6 +997,14 @@ static const struct rvin_info rcar_info_r8a77970 = {
 	.routes = _rcar_info_r8a77970_routes,
 };
 
+static const struct rvin_info rcar_info_r8a77995 = {
+	.model = RCAR_GEN3,
+	.use_mc = true,
+	.max_width = 4096,
+	.max_height = 4096,
+	.routes = _rcar_info_r8a77995_routes,
+};
+
 static const struct of_device_id rvin_of_id_table[] = {
 	{
 		.compatible = "renesas,vin-r8a7778",
@@ -1034,6 +1046,10 @@ static const struct of_device_id rvin_of_id_table[] = {
 		.compatible = "renesas,vin-r8a77970",
 		.data = &rcar_info_r8a77970,
 	},
+	{
+		.compatible = "renesas,vin-r8a77995",
+		.data = &rcar_info_r8a77995,
+	},
 	{ /* Sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, rvin_of_id_table);
-- 
2.7.4

[PATCH 2/5] media: rcar-vin: Add digital input subdevice parsing

[Jacopo Mondi]

Add support for digital input subdevices to Gen-3 rcar-vin.
The Gen-3, media-controller compliant, version has so far only accepted
CSI-2 input subdevices. Remove assumptions on the supported bus_type and
accepted number of subdevices, and allow digital input connections on port@0.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 166 +++++++++++++++++++++++-----
 drivers/media/platform/rcar-vin/rcar-vin.h  |  13 +++
 2 files changed, 153 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index e547ef7..105b6b6 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -697,29 +697,21 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
 	.complete = rvin_group_notify_complete,
 };
 
-static int rvin_mc_parse_of_endpoint(struct device *dev,
-				     struct v4l2_fwnode_endpoint *vep,
-				     struct v4l2_async_subdev *asd)
+static int rvin_mc_parse_of_csi2(struct rvin_dev *vin,
+				 struct v4l2_fwnode_endpoint *vep,
+				 struct v4l2_async_subdev *asd)
 {
-	struct rvin_dev *vin = dev_get_drvdata(dev);
-
-	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
+	if (vep->base.id >= RVIN_CSI_MAX)
 		return -EINVAL;
 
-	if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
-
-		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
-			to_of_node(asd->match.fwnode));
-		return -ENOTCONN;
-
-	}
-
 	if (vin->group->csi[vep->base.id].fwnode) {
 		vin_dbg(vin, "OF device %pOF already handled\n",
 			to_of_node(asd->match.fwnode));
 		return -ENOTCONN;
 	}
 
+	vin->mbus_cfg.type = V4L2_MBUS_CSI2;
+	vin->mbus_cfg.flags = 0;
 	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
 
 	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
@@ -728,9 +720,97 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
 	return 0;
 }
 
+static int rvin_mc_parse_of_digital(struct rvin_dev *vin,
+				    struct v4l2_fwnode_endpoint *vep,
+				    struct v4l2_async_subdev *asd)
+{
+	if (vep->base.id)
+		return -EINVAL;
+
+	vin->mbus_cfg.type = vep->bus_type;
+	if (vin->mbus_cfg.type == V4L2_MBUS_PARALLEL)
+		vin->mbus_cfg.flags = vep->bus.parallel.flags;
+	else
+		vin->mbus_cfg.flags = 0;
+
+	/*
+	 * 'v4l2_async_notifier_fwnode_parse_endpoint()' has reserved
+	 * enough memory for a whole 'rvin_grap_entity' structure, whose 'asd'
+	 * is the first member of. Safely cast it to the 'digital' field of
+	 * the selected vin instance.
+	 */
+	vin->digital = (struct rvin_graph_entity *)asd;
+
+	vin_dbg(vin, "Add OF device %pOF as VIN%u digital input\n",
+		to_of_node(asd->match.fwnode), vin->id);
+
+	return 0;
+}
+
+static int rvin_mc_parse_of_endpoint(struct device *dev,
+				     struct v4l2_fwnode_endpoint *vep,
+				     struct v4l2_async_subdev *asd)
+{
+	struct rvin_dev *group_vin = dev_get_drvdata(dev);
+	struct rvin_group *group = group_vin->group;
+	struct fwnode_handle *fw_vin;
+	struct fwnode_handle *fw_port;
+	struct rvin_dev *vin;
+	unsigned int i;
+	int ret;
+
+	if (!fwnode_device_is_available(asd->match.fwnode)) {
+		vin_dbg(group_vin, "OF device %pOF disabled, ignoring\n",
+			to_of_node(asd->match.fwnode));
+		return -ENOTCONN;
+	}
+
+	/*
+	 * Find out to which VIN instance this ep belongs to.
+	 *
+	 * While the list of async subdevices and the async notifier
+	 * belong to the whole group, the bus configuration properties
+	 * are instance specific; find the instance by matching its fwnode.
+	 */
+	fw_port = fwnode_get_parent(vep->base.local_fwnode);
+	fw_vin = fwnode_graph_get_port_parent(fw_port);
+	fwnode_handle_put(fw_port);
+	if (!fw_vin)
+		return -ENOENT;
+
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		if (!group->vin[i])
+			continue;
+
+		if (fw_vin == dev_fwnode(group->vin[i]->dev))
+			break;
+	}
+	fwnode_handle_put(fw_vin);
+
+	if (i == RCAR_VIN_NUM) {
+		vin_err(group_vin, "Unable to find VIN device for %pOF\n",
+			to_of_node(asd->match.fwnode));
+		return -ENOENT;
+	}
+	vin = group->vin[i];
+
+	switch (vep->base.port) {
+	case RVIN_PORT_DIGITAL:
+		ret = rvin_mc_parse_of_digital(vin, vep, asd);
+		break;
+	case RVIN_PORT_CSI2:
+	default:
+		ret = rvin_mc_parse_of_csi2(vin, vep, asd);
+		break;
+	}
+
+	return ret;
+}
+
 static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 {
 	unsigned int count = 0;
+	size_t asd_struct_size;
 	unsigned int i;
 	int ret;
 
@@ -753,25 +833,58 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 	}
 
 	/*
-	 * Have all VIN's look for subdevices. Some subdevices will overlap
-	 * but the parser function can handle it, so each subdevice will
-	 * only be registered once with the notifier.
+	 * Have all VIN's look for subdevices. Some CSI-2 subdevices will
+	 * overlap but the parser function can handle it, so each subdevice
+	 * will only be registered once with the notifier.
 	 */
 
 	vin->group->notifier = &vin->notifier;
 
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		struct fwnode_handle *fwdev;
+		struct fwnode_handle *fwep;
+		struct fwnode_endpoint ep;
+
 		if (!vin->group->vin[i])
 			continue;
 
+		/*
+		 * If a digital input is described in port@0 proceed to parse
+		 * it and register a single sub-device for this VIN instance.
+		 * If no digital input is available go inspect the CSI-2
+		 * connections described in port@1.
+		 */
+		fwdev = dev_fwnode(vin->group->vin[i]->dev);
+		fwep = fwnode_graph_get_next_endpoint(fwdev, NULL);
+		if (!fwep) {
+			ret = PTR_ERR(fwep);
+			goto error_unlock_return;
+		}
+
+		ret = fwnode_graph_parse_endpoint(fwep, &ep);
+		fwnode_handle_put(fwep);
+		if (ret)
+			goto error_unlock_return;
+
+		switch (ep.port) {
+		case RVIN_PORT_DIGITAL:
+			asd_struct_size = sizeof(struct rvin_graph_entity);
+			break;
+		case RVIN_PORT_CSI2:
+			asd_struct_size = sizeof(struct v4l2_async_subdev);
+			break;
+		default:
+			vin_err(vin, "port%u not allowed\n", ep.port);
+			ret = -EINVAL;
+			goto error_unlock_return;
+		}
+
 		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
 				vin->group->vin[i]->dev, vin->group->notifier,
-				sizeof(struct v4l2_async_subdev), 1,
+				asd_struct_size, ep.port,
 				rvin_mc_parse_of_endpoint);
-		if (ret) {
-			mutex_unlock(&vin->group->lock);
-			return ret;
-		}
+		if (ret)
+			goto error_unlock_return;
 	}
 
 	mutex_unlock(&vin->group->lock);
@@ -785,16 +898,17 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 	}
 
 	return 0;
+
+error_unlock_return:
+	mutex_unlock(&vin->group->lock);
+
+	return ret;
 }
 
 static int rvin_mc_init(struct rvin_dev *vin)
 {
 	int ret;
 
-	/* All our sources are CSI-2 */
-	vin->mbus_cfg.type = V4L2_MBUS_CSI2;
-	vin->mbus_cfg.flags = 0;
-
 	vin->pad.flags = MEDIA_PAD_FL_SINK;
 	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
 	if (ret)
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index c2aef78..a63f3c6 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -52,6 +52,19 @@ enum rvin_csi_id {
 };
 
 /**
+ * enum rvin_port_id
+ *
+ * List the available VIN port functions.
+ *
+ * RVIN_PORT_DIGITAL	- Input port for digital video connection
+ * RVIN_PORT_CSI2	- Input port for CSI-2 video connection
+ */
+enum rvin_port_id {
+	RVIN_PORT_DIGITAL,
+	RVIN_PORT_CSI2
+};
+
+/**
  * STOPPED  - No operation in progress
  * RUNNING  - Operation in progress have buffers
  * STOPPING - Stopping operation
-- 
2.7.4

[PATCH 3/5] media: rcar-vin: [un]bind and link digital subdevice

[Jacopo Mondi]

Add support for binding and unbinding digital subdevices to rcar-vin.
On 'complete' also create direct links between the VIN instance and the
digital subdevice.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 133 +++++++++++++++++++++++-----
 1 file changed, 110 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 105b6b6..93c37b0 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -168,10 +168,37 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
 	}
 
 	/* Add the new link to the existing mask and check if it works. */
-	csi_id = rvin_group_entity_to_csi_id(group, link->source->entity);
 	channel = rvin_group_csi_pad_to_channel(link->source->index);
-	mask_new = mask & rvin_group_get_mask(vin, csi_id, channel);
+	csi_id = rvin_group_entity_to_csi_id(group, link->source->entity);
+	if (csi_id == -ENODEV) {
+		struct v4l2_subdev *sd;
+		unsigned int i;
+
+		/*
+		 * Make sure the source entity subdevice is registered as
+		 * a digital input of one of the enabled VINs if it is not
+		 * one of the CSI-2 subdevices.
+		 *
+		 * No hardware configuration required for digital inputs,
+		 * we can return here.
+		 */
+		sd = media_entity_to_v4l2_subdev(link->source->entity);
+
+		for (i = 0; i < RCAR_VIN_NUM; i++) {
+			if (group->vin[i] && group->vin[i]->digital &&
+			    group->vin[i]->digital->subdev == sd) {
+				ret = 0;
+				goto out;
+			}
+		}
 
+		vin_err(vin, "Subdevice %s not registered to any VIN\n",
+			link->source->entity->name);
+		ret = -ENODEV;
+		goto out;
+	}
+
+	mask_new = mask & rvin_group_get_mask(vin, csi_id, channel);
 	vin_dbg(vin, "Try link change mask: 0x%x new: 0x%x\n", mask, mask_new);
 
 	if (!mask_new) {
@@ -583,50 +610,70 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
 
 static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 {
-	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct rvin_dev *gvin = notifier_to_vin(notifier);
 	const struct rvin_group_route *route;
+	struct media_entity *source;
+	struct media_entity *sink;
 	unsigned int i;
 	int ret;
 
-	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
+	ret = v4l2_device_register_subdev_nodes(&gvin->v4l2_dev);
 	if (ret) {
-		vin_err(vin, "Failed to register subdev nodes\n");
+		vin_err(gvin, "Failed to register subdev nodes\n");
 		return ret;
 	}
 
-	/* Register all video nodes for the group. */
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
-		if (vin->group->vin[i]) {
-			ret = rvin_v4l2_register(vin->group->vin[i]);
-			if (ret)
-				return ret;
+		struct rvin_dev *ivin;
+
+		if (!gvin->group->vin[i])
+			continue;
+
+		/* Register all video nodes for the group. */
+		ivin = gvin->group->vin[i];
+		ret = rvin_v4l2_register(ivin);
+		if (ret)
+			return ret;
+
+		/* Link the digital input, if any. */
+		if (!ivin->digital || !ivin->digital->subdev)
+			continue;
+
+		source = &ivin->digital->subdev->entity;
+		sink = &ivin->vdev.entity;
+
+		ret = media_create_pad_link(source, ivin->digital->source_pad,
+					    sink, ivin->digital->sink_pad, 0);
+		if (ret) {
+			vin_err(gvin, "Error adding link from %s to %s\n",
+				source->name, sink->name);
+			return ret;
 		}
 	}
 
 	/* Create all media device links between VINs and CSI-2's. */
-	mutex_lock(&vin->group->lock);
-	for (route = vin->info->routes; route->mask; route++) {
+	mutex_lock(&gvin->group->lock);
+	for (route = gvin->info->routes; route->mask; route++) {
 		struct media_pad *source_pad, *sink_pad;
-		struct media_entity *source, *sink;
 		unsigned int source_idx;
 
 		/* Check that VIN is part of the group. */
-		if (!vin->group->vin[route->vin])
+		if (!gvin->group->vin[route->vin])
 			continue;
 
 		/* Check that VIN' master is part of the group. */
-		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
+		if (!gvin->group->vin[rvin_group_id_to_master(route->vin)])
 			continue;
 
 		/* Check that CSI-2 is part of the group. */
-		if (!vin->group->csi[route->csi].subdev)
+		if (!gvin->group->csi[route->csi].subdev)
 			continue;
 
-		source = &vin->group->csi[route->csi].subdev->entity;
+		source = &gvin->group->csi[route->csi].subdev->entity;
 		source_idx = rvin_group_csi_channel_to_pad(route->channel);
 		source_pad = &source->pads[source_idx];
 
-		sink = &vin->group->vin[route->vin]->vdev.entity;
+		sink = &gvin->group->vin[route->vin]->vdev.entity;
 		sink_pad = &sink->pads[0];
 
 		/* Skip if link already exists. */
@@ -635,12 +682,12 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 
 		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
 		if (ret) {
-			vin_err(vin, "Error adding link from %s to %s\n",
+			vin_err(gvin, "Error adding link from %s to %s\n",
 				source->name, sink->name);
 			break;
 		}
 	}
-	mutex_unlock(&vin->group->lock);
+	mutex_unlock(&gvin->group->lock);
 
 	return ret;
 }
@@ -650,6 +697,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
 				     struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct rvin_group *group = vin->group;
 	unsigned int i;
 
 	for (i = 0; i < RCAR_VIN_NUM; i++)
@@ -658,6 +706,23 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
 
 	mutex_lock(&vin->group->lock);
 
+	/* Check if this is a digital subdevice first, then try with CSI-2. */
+	for (i = 0; i < RCAR_VIN_NUM; i++)
+		if (group->vin[i] && group->vin[i]->digital &&
+		    group->vin[i]->digital->asd.match.fwnode ==
+		    asd->match.fwnode)
+			break;
+
+	if (i < RCAR_VIN_NUM) {
+		group->vin[i]->digital->subdev = NULL;
+		vin_dbg(vin, "Unbind digital subdevice %s from VIN %u\n",
+			subdev->name, i);
+
+		mutex_unlock(&vin->group->lock);
+
+		return;
+	}
+
 	for (i = 0; i < RVIN_CSI_MAX; i++) {
 		if (vin->group->csi[i].fwnode != asd->match.fwnode)
 			continue;
@@ -674,14 +739,36 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
 				   struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct rvin_group *group = vin->group;
 	unsigned int i;
 
-	mutex_lock(&vin->group->lock);
+	mutex_lock(&group->lock);
+
+	/* Check if this is a digital subdevice first, then try with CSI-2. */
+	for (i = 0; i < RCAR_VIN_NUM; i++)
+		if (group->vin[i] && group->vin[i]->digital &&
+		    group->vin[i]->digital->asd.match.fwnode ==
+		    asd->match.fwnode)
+			break;
+
+	if (i < RCAR_VIN_NUM) {
+		group->vin[i]->digital->subdev = subdev;
+		group->vin[i]->digital->sink_pad = RVIN_PORT_DIGITAL;
+		group->vin[i]->digital->source_pad = rvin_find_pad(subdev,
+							   MEDIA_PAD_FL_SOURCE);
+
+		vin_dbg(vin, "Bound digital subdevice %s to VIN %u\n",
+			subdev->name, i);
+
+		mutex_unlock(&vin->group->lock);
+
+		return 0;
+	}
 
 	for (i = 0; i < RVIN_CSI_MAX; i++) {
-		if (vin->group->csi[i].fwnode != asd->match.fwnode)
+		if (group->csi[i].fwnode != asd->match.fwnode)
 			continue;
-		vin->group->csi[i].subdev = subdev;
+		group->csi[i].subdev = subdev;
 		vin_dbg(vin, "Bound CSI-2 %s to slot %u\n", subdev->name, i);
 		break;
 	}
-- 
2.7.4

[PATCH 4/5] media: rcar-vin: Do not use crop if not configured

[Jacopo Mondi]

The crop_scale routine uses the crop rectangle memebers to configure the
VIN clipping rectangle. When crop is not configured all its fields are
0s, and setting the clipping rectangle vertical and horizontal extensions
to (0 - 1) causes the registers to be written with an incorrect
0xffffffff value.

Fix this by using the actual format width and height when no crop
rectangle has been programmed.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index b41ba9a..ea7a120 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -579,22 +579,25 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
 
 void rvin_crop_scale_comp(struct rvin_dev *vin)
 {
-	/* Set Start/End Pixel/Line Pre-Clip */
+	u32 width = vin->crop.width ? vin->crop.left + vin->crop.width :
+				      vin->format.width;
+	u32 height = vin->crop.height ? vin->crop.top + vin->crop.height :
+					vin->format.height;
+
+	/* Set Start/End Pixel/Line Pre-Clip if crop is configured. */
 	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
-	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
+	rvin_write(vin, width - 1, VNEPPRC_REG);
 
 	switch (vin->format.field) {
 	case V4L2_FIELD_INTERLACED:
 	case V4L2_FIELD_INTERLACED_TB:
 	case V4L2_FIELD_INTERLACED_BT:
 		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
-		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
-			   VNELPRC_REG);
+		rvin_write(vin, height / 2 - 1, VNELPRC_REG);
 		break;
 	default:
 		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
-		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
-			   VNELPRC_REG);
+		rvin_write(vin, height - 1, VNELPRC_REG);
 		break;
 	}
 
-- 
2.7.4

[PATCH 5/5] media: rcar-vin: Use FTEV for digital input

[Jacopo Mondi]

Since commit (015060cb "media: rcar-vin: enable field toggle after a set
number of lines for Gen3) the VIN generates an internal field signal
toggle after a fixed number of received lines, and uses the internal
field signal to drive capture operations. When capturing from digital
input, using FTEH driven field signal toggling messes up the received
image sizes. Fall back to use FTEV driven signal toggling when capturing
from digital input.

As explained in the comment, this disables buffer overflow protection
for digital input capture, for which the FOE overflow might be used in
future.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index ea7a120..8dc3455 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -685,11 +685,27 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	}
 
-	if (vin->info->model == RCAR_GEN3) {
+	if (vin->info->model == RCAR_GEN3 &&
+	    vin->mbus_cfg.type == V4L2_MBUS_CSI2) {
 		/* Enable HSYNC Field Toggle mode after height HSYNC inputs. */
 		lines = vin->format.height / (halfsize ? 2 : 1);
 		dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines);
 		vin_dbg(vin, "Field Toogle after %u lines\n", lines);
+	} else if (vin->info->model == RCAR_GEN3 &&
+		   vin->mbus_cfg.type == V4L2_MBUS_PARALLEL) {
+		/*
+		 * FIXME
+		 * Section 26.3.17 specifies that for digital input there's no
+		 * need to program FTEH or FTEV to generate internal
+		 * field toggle signal to driver capture. Although when
+		 * running on GEN3 with digital input no EFE interrupt is ever
+		 * generated, and we need to rely on FTEV driven field signal
+		 * toggling, as using FTEH as in the CSI-2 case, messes up
+		 * the output image size. This implies no protection
+		 * against buffer overflow is in place for Gen3 digital input
+		 * capture.
+		 */
+		dmr2 = VNDMR2_FTEV;
 	} else {
 		/* Enable VSYNC Field Toogle mode after one VSYNC input. */
 		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
-- 
2.7.4

Re: [PATCH 5/5] media: rcar-vin: Use FTEV for digital input

[Hans Verkuil]

On 05/11/18 11:59, Jacopo Mondi wrote:
> Since commit (015060cb "media: rcar-vin: enable field toggle after a set
> number of lines for Gen3) the VIN generates an internal field signal
> toggle after a fixed number of received lines, and uses the internal
> field signal to drive capture operations. When capturing from digital
> input, using FTEH driven field signal toggling messes up the received
> image sizes. Fall back to use FTEV driven signal toggling when capturing
> from digital input.
> 
> As explained in the comment, this disables buffer overflow protection
> for digital input capture, for which the FOE overflow might be used in
> future.

I don't know the details of the hardware, but this sounds dangerous.

You should know that with HDMI input it is perfectly possible that you get
more data than you should. I.e. instead of 1080 lines (assuming full HD)
you might get more lines. This happens if the vertical sync is missed due
to pin bounce when connecting a source.

Other reasons for this are flaky signals, bad clocks, etc.

It's rare, but it really happens.

A good DMA engine will refuse to write more than fits in the buffer.

If you disable that, then you will get overflows eventually.

The reality with HDMI input is that you should never assume clean valid
data. You do not control the source and it can send anything it likes.

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index ea7a120..8dc3455 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -685,11 +685,27 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	}
>  
> -	if (vin->info->model == RCAR_GEN3) {
> +	if (vin->info->model == RCAR_GEN3 &&
> +	    vin->mbus_cfg.type == V4L2_MBUS_CSI2) {
>  		/* Enable HSYNC Field Toggle mode after height HSYNC inputs. */
>  		lines = vin->format.height / (halfsize ? 2 : 1);
>  		dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines);
>  		vin_dbg(vin, "Field Toogle after %u lines\n", lines);

Typo: Toogle -> Toggle

> +	} else if (vin->info->model == RCAR_GEN3 &&
> +		   vin->mbus_cfg.type == V4L2_MBUS_PARALLEL) {
> +		/*
> +		 * FIXME
> +		 * Section 26.3.17 specifies that for digital input there's no
> +		 * need to program FTEH or FTEV to generate internal
> +		 * field toggle signal to driver capture. Although when
> +		 * running on GEN3 with digital input no EFE interrupt is ever
> +		 * generated, and we need to rely on FTEV driven field signal
> +		 * toggling, as using FTEH as in the CSI-2 case, messes up
> +		 * the output image size. This implies no protection
> +		 * against buffer overflow is in place for Gen3 digital input
> +		 * capture.
> +		 */
> +		dmr2 = VNDMR2_FTEV;
>  	} else {
>  		/* Enable VSYNC Field Toogle mode after one VSYNC input. */

Ditto. Search and replace?

>  		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
> 

Regards,

	Hans

Re: [PATCH 1/5] media: rcar-vin: Add support for R-Car R8A77995 SoC

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-11 11:59:37 +0200, Jacopo Mondi wrote:
> Add R-Car R8A77995 SoC to the rcar-vin supported ones.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I would move this to be the last patch in the series as a indication 
that capture on R8A77995 is now ready to be used. But for the change 
itself.

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d3072e1..e547ef7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -985,6 +985,10 @@ static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
>  	{ /* Sentinel */ }
>  };
>  
> +static const struct rvin_group_route _rcar_info_r8a77995_routes[] = {
> +	{ /* Sentinel */ }
> +};
> +
>  static const struct rvin_info rcar_info_r8a77970 = {
>  	.model = RCAR_GEN3,
>  	.use_mc = true,
> @@ -993,6 +997,14 @@ static const struct rvin_info rcar_info_r8a77970 = {
>  	.routes = _rcar_info_r8a77970_routes,
>  };
>  
> +static const struct rvin_info rcar_info_r8a77995 = {
> +	.model = RCAR_GEN3,
> +	.use_mc = true,
> +	.max_width = 4096,
> +	.max_height = 4096,
> +	.routes = _rcar_info_r8a77995_routes,
> +};
> +
>  static const struct of_device_id rvin_of_id_table[] = {
>  	{
>  		.compatible = "renesas,vin-r8a7778",
> @@ -1034,6 +1046,10 @@ static const struct of_device_id rvin_of_id_table[] = {
>  		.compatible = "renesas,vin-r8a77970",
>  		.data = &rcar_info_r8a77970,
>  	},
> +	{
> +		.compatible = "renesas,vin-r8a77995",
> +		.data = &rcar_info_r8a77995,
> +	},
>  	{ /* Sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, rvin_of_id_table);
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/5] media: rcar-vin: Add support for R-Car R8A77995 SoC

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-11 11:59:37 +0200, Jacopo Mondi wrote:
> Add R-Car R8A77995 SoC to the rcar-vin supported ones.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I would move this to be the last patch in the series as a indication 
that capture on R8A77995 is now ready to be used. But for the change 
itself.

Reviewed-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d3072e1..e547ef7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -985,6 +985,10 @@ static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
>  	{ /* Sentinel */ }
>  };
>  
> +static const struct rvin_group_route _rcar_info_r8a77995_routes[] = {
> +	{ /* Sentinel */ }
> +};
> +
>  static const struct rvin_info rcar_info_r8a77970 = {
>  	.model = RCAR_GEN3,
>  	.use_mc = true,
> @@ -993,6 +997,14 @@ static const struct rvin_info rcar_info_r8a77970 = {
>  	.routes = _rcar_info_r8a77970_routes,
>  };
>  
> +static const struct rvin_info rcar_info_r8a77995 = {
> +	.model = RCAR_GEN3,
> +	.use_mc = true,
> +	.max_width = 4096,
> +	.max_height = 4096,
> +	.routes = _rcar_info_r8a77995_routes,
> +};
> +
>  static const struct of_device_id rvin_of_id_table[] = {
>  	{
>  		.compatible = "renesas,vin-r8a7778",
> @@ -1034,6 +1046,10 @@ static const struct of_device_id rvin_of_id_table[] = {
>  		.compatible = "renesas,vin-r8a77970",
>  		.data = &rcar_info_r8a77970,
>  	},
> +	{
> +		.compatible = "renesas,vin-r8a77995",
> +		.data = &rcar_info_r8a77995,
> +	},
>  	{ /* Sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, rvin_of_id_table);
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 2/5] media: rcar-vin: Add digital input subdevice parsing

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work!

The comments here apply to both 2/5 and 3/5.

On 2018-05-11 11:59:38 +0200, Jacopo Mondi wrote:
> Add support for digital input subdevices to Gen-3 rcar-vin.
> The Gen-3, media-controller compliant, version has so far only accepted
> CSI-2 input subdevices. Remove assumptions on the supported bus_type and
> accepted number of subdevices, and allow digital input connections on port@0.

I feel this much more complex then it has to be. You defer parsing the 
DT graph until all VIN's are probed just like it's done for the port@1 
parsing. For the CSI-2 endpoints in port@1 this is needed as they are 
shared for all VIN's in the group. While for the parallel input this is 
local for each VIN and could be parsed at probe time for each individual 
VIN. As a bonus for doing that I think you could reuse the parallel 
parsing functions from the Gen2 code whit small additions.

Maybe something like this can be done:

- At each VIN probe() rework the block

    if (vin->info->use_mc)
	    ret = rvin_mc_init(vin);
    else
	    ret = rvin_digital_graph_init(vin);

  To:
    ret = rvin_digital_graph_init(vin);
    ...
    ret = rvin_mc_init(vin);
    ...

- Then in rvin_digital_graph_init() don't call 
  v4l2_async_notifier_register() if vin->info->use_mc is set.

  And instead as a last step in rvin_mc_init() call 
  v4l2_async_notifier_register() for each vin->notifier that contains a 
  subdevice.

- As a last step add a check for vin->info->use_mc in 
  rvin_digital_notify_complete() and handle the case for Gen2 behavior 
  (what it does now) and Gen3 behavior (adding the MC link).


I think my doing this you could greatly simplify the process of handling 
the parallel subdevice.

Please see bellow for one additional comment.

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 166 +++++++++++++++++++++++-----
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  13 +++
>  2 files changed, 153 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index e547ef7..105b6b6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -697,29 +697,21 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
>  	.complete = rvin_group_notify_complete,
>  };
>  
> -static int rvin_mc_parse_of_endpoint(struct device *dev,
> -				     struct v4l2_fwnode_endpoint *vep,
> -				     struct v4l2_async_subdev *asd)
> +static int rvin_mc_parse_of_csi2(struct rvin_dev *vin,
> +				 struct v4l2_fwnode_endpoint *vep,
> +				 struct v4l2_async_subdev *asd)
>  {
> -	struct rvin_dev *vin = dev_get_drvdata(dev);
> -
> -	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> +	if (vep->base.id >= RVIN_CSI_MAX)
>  		return -EINVAL;
>  
> -	if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> -
> -		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> -			to_of_node(asd->match.fwnode));
> -		return -ENOTCONN;
> -
> -	}
> -
>  	if (vin->group->csi[vep->base.id].fwnode) {
>  		vin_dbg(vin, "OF device %pOF already handled\n",
>  			to_of_node(asd->match.fwnode));
>  		return -ENOTCONN;
>  	}
>  
> +	vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> +	vin->mbus_cfg.flags = 0;
>  	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
>  
>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> @@ -728,9 +720,97 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  	return 0;
>  }
>  
> +static int rvin_mc_parse_of_digital(struct rvin_dev *vin,
> +				    struct v4l2_fwnode_endpoint *vep,
> +				    struct v4l2_async_subdev *asd)
> +{
> +	if (vep->base.id)
> +		return -EINVAL;
> +
> +	vin->mbus_cfg.type = vep->bus_type;
> +	if (vin->mbus_cfg.type == V4L2_MBUS_PARALLEL)
> +		vin->mbus_cfg.flags = vep->bus.parallel.flags;
> +	else
> +		vin->mbus_cfg.flags = 0;
> +
> +	/*
> +	 * 'v4l2_async_notifier_fwnode_parse_endpoint()' has reserved
> +	 * enough memory for a whole 'rvin_grap_entity' structure, whose 'asd'
> +	 * is the first member of. Safely cast it to the 'digital' field of
> +	 * the selected vin instance.
> +	 */
> +	vin->digital = (struct rvin_graph_entity *)asd;
> +
> +	vin_dbg(vin, "Add OF device %pOF as VIN%u digital input\n",
> +		to_of_node(asd->match.fwnode), vin->id);
> +
> +	return 0;
> +}
> +
> +static int rvin_mc_parse_of_endpoint(struct device *dev,
> +				     struct v4l2_fwnode_endpoint *vep,
> +				     struct v4l2_async_subdev *asd)
> +{
> +	struct rvin_dev *group_vin = dev_get_drvdata(dev);
> +	struct rvin_group *group = group_vin->group;
> +	struct fwnode_handle *fw_vin;
> +	struct fwnode_handle *fw_port;
> +	struct rvin_dev *vin;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!fwnode_device_is_available(asd->match.fwnode)) {
> +		vin_dbg(group_vin, "OF device %pOF disabled, ignoring\n",
> +			to_of_node(asd->match.fwnode));
> +		return -ENOTCONN;
> +	}
> +
> +	/*
> +	 * Find out to which VIN instance this ep belongs to.
> +	 *
> +	 * While the list of async subdevices and the async notifier
> +	 * belong to the whole group, the bus configuration properties
> +	 * are instance specific; find the instance by matching its fwnode.
> +	 */
> +	fw_port = fwnode_get_parent(vep->base.local_fwnode);
> +	fw_vin = fwnode_graph_get_port_parent(fw_port);
> +	fwnode_handle_put(fw_port);
> +	if (!fw_vin)
> +		return -ENOENT;
> +
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		if (!group->vin[i])
> +			continue;
> +
> +		if (fw_vin == dev_fwnode(group->vin[i]->dev))
> +			break;
> +	}
> +	fwnode_handle_put(fw_vin);
> +
> +	if (i == RCAR_VIN_NUM) {
> +		vin_err(group_vin, "Unable to find VIN device for %pOF\n",
> +			to_of_node(asd->match.fwnode));
> +		return -ENOENT;
> +	}
> +	vin = group->vin[i];
> +
> +	switch (vep->base.port) {
> +	case RVIN_PORT_DIGITAL:
> +		ret = rvin_mc_parse_of_digital(vin, vep, asd);
> +		break;
> +	case RVIN_PORT_CSI2:
> +	default:
> +		ret = rvin_mc_parse_of_csi2(vin, vep, asd);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  {
>  	unsigned int count = 0;
> +	size_t asd_struct_size;
>  	unsigned int i;
>  	int ret;
>  
> @@ -753,25 +833,58 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	}
>  
>  	/*
> -	 * Have all VIN's look for subdevices. Some subdevices will overlap
> -	 * but the parser function can handle it, so each subdevice will
> -	 * only be registered once with the notifier.
> +	 * Have all VIN's look for subdevices. Some CSI-2 subdevices will
> +	 * overlap but the parser function can handle it, so each subdevice
> +	 * will only be registered once with the notifier.
>  	 */
>  
>  	vin->group->notifier = &vin->notifier;
>  
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		struct fwnode_handle *fwdev;
> +		struct fwnode_handle *fwep;
> +		struct fwnode_endpoint ep;
> +
>  		if (!vin->group->vin[i])
>  			continue;
>  
> +		/*
> +		 * If a digital input is described in port@0 proceed to parse
> +		 * it and register a single sub-device for this VIN instance.
> +		 * If no digital input is available go inspect the CSI-2
> +		 * connections described in port@1.
> +		 */
> +		fwdev = dev_fwnode(vin->group->vin[i]->dev);
> +		fwep = fwnode_graph_get_next_endpoint(fwdev, NULL);
> +		if (!fwep) {
> +			ret = PTR_ERR(fwep);
> +			goto error_unlock_return;
> +		}
> +
> +		ret = fwnode_graph_parse_endpoint(fwep, &ep);
> +		fwnode_handle_put(fwep);
> +		if (ret)
> +			goto error_unlock_return;
> +
> +		switch (ep.port) {
> +		case RVIN_PORT_DIGITAL:
> +			asd_struct_size = sizeof(struct rvin_graph_entity);
> +			break;
> +		case RVIN_PORT_CSI2:
> +			asd_struct_size = sizeof(struct v4l2_async_subdev);
> +			break;
> +		default:
> +			vin_err(vin, "port%u not allowed\n", ep.port);
> +			ret = -EINVAL;
> +			goto error_unlock_return;
> +		}
> +
>  		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
>  				vin->group->vin[i]->dev, vin->group->notifier,
> -				sizeof(struct v4l2_async_subdev), 1,
> +				asd_struct_size, ep.port,
>  				rvin_mc_parse_of_endpoint);
> -		if (ret) {
> -			mutex_unlock(&vin->group->lock);
> -			return ret;
> -		}
> +		if (ret)
> +			goto error_unlock_return;
>  	}
>  
>  	mutex_unlock(&vin->group->lock);
> @@ -785,16 +898,17 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	}
>  
>  	return 0;
> +
> +error_unlock_return:
> +	mutex_unlock(&vin->group->lock);
> +
> +	return ret;
>  }
>  
>  static int rvin_mc_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
> -	/* All our sources are CSI-2 */
> -	vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> -	vin->mbus_cfg.flags = 0;
> -
>  	vin->pad.flags = MEDIA_PAD_FL_SINK;
>  	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
>  	if (ret)
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index c2aef78..a63f3c6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -52,6 +52,19 @@ enum rvin_csi_id {
>  };
>  
>  /**
> + * enum rvin_port_id
> + *
> + * List the available VIN port functions.
> + *
> + * RVIN_PORT_DIGITAL	- Input port for digital video connection
> + * RVIN_PORT_CSI2	- Input port for CSI-2 video connection
> + */
> +enum rvin_port_id {
> +	RVIN_PORT_DIGITAL,
> +	RVIN_PORT_CSI2
> +};

This enum is never used in the patch-set is it used later by a different 
patch-set or a left over from refactoring?

> +
> +/**
>   * STOPPED  - No operation in progress
>   * RUNNING  - Operation in progress have buffers
>   * STOPPING - Stopping operation
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 2/5] media: rcar-vin: Add digital input subdevice parsing

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work!

The comments here apply to both 2/5 and 3/5.

On 2018-05-11 11:59:38 +0200, Jacopo Mondi wrote:
> Add support for digital input subdevices to Gen-3 rcar-vin.
> The Gen-3, media-controller compliant, version has so far only accepted
> CSI-2 input subdevices. Remove assumptions on the supported bus_type and
> accepted number of subdevices, and allow digital input connections on port@0.

I feel this much more complex then it has to be. You defer parsing the 
DT graph until all VIN's are probed just like it's done for the port@1 
parsing. For the CSI-2 endpoints in port@1 this is needed as they are 
shared for all VIN's in the group. While for the parallel input this is 
local for each VIN and could be parsed at probe time for each individual 
VIN. As a bonus for doing that I think you could reuse the parallel 
parsing functions from the Gen2 code whit small additions.

Maybe something like this can be done:

- At each VIN probe() rework the block

    if (vin->info->use_mc)
	    ret = rvin_mc_init(vin);
    else
	    ret = rvin_digital_graph_init(vin);

  To:
    ret = rvin_digital_graph_init(vin);
    ...
    ret = rvin_mc_init(vin);
    ...

- Then in rvin_digital_graph_init() don't call 
  v4l2_async_notifier_register() if vin->info->use_mc is set.

  And instead as a last step in rvin_mc_init() call 
  v4l2_async_notifier_register() for each vin->notifier that contains a 
  subdevice.

- As a last step add a check for vin->info->use_mc in 
  rvin_digital_notify_complete() and handle the case for Gen2 behavior 
  (what it does now) and Gen3 behavior (adding the MC link).


I think my doing this you could greatly simplify the process of handling 
the parallel subdevice.

Please see bellow for one additional comment.

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 166 +++++++++++++++++++++++-----
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  13 +++
>  2 files changed, 153 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index e547ef7..105b6b6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -697,29 +697,21 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
>  	.complete = rvin_group_notify_complete,
>  };
>  
> -static int rvin_mc_parse_of_endpoint(struct device *dev,
> -				     struct v4l2_fwnode_endpoint *vep,
> -				     struct v4l2_async_subdev *asd)
> +static int rvin_mc_parse_of_csi2(struct rvin_dev *vin,
> +				 struct v4l2_fwnode_endpoint *vep,
> +				 struct v4l2_async_subdev *asd)
>  {
> -	struct rvin_dev *vin = dev_get_drvdata(dev);
> -
> -	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> +	if (vep->base.id >= RVIN_CSI_MAX)
>  		return -EINVAL;
>  
> -	if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> -
> -		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> -			to_of_node(asd->match.fwnode));
> -		return -ENOTCONN;
> -
> -	}
> -
>  	if (vin->group->csi[vep->base.id].fwnode) {
>  		vin_dbg(vin, "OF device %pOF already handled\n",
>  			to_of_node(asd->match.fwnode));
>  		return -ENOTCONN;
>  	}
>  
> +	vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> +	vin->mbus_cfg.flags = 0;
>  	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
>  
>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> @@ -728,9 +720,97 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  	return 0;
>  }
>  
> +static int rvin_mc_parse_of_digital(struct rvin_dev *vin,
> +				    struct v4l2_fwnode_endpoint *vep,
> +				    struct v4l2_async_subdev *asd)
> +{
> +	if (vep->base.id)
> +		return -EINVAL;
> +
> +	vin->mbus_cfg.type = vep->bus_type;
> +	if (vin->mbus_cfg.type == V4L2_MBUS_PARALLEL)
> +		vin->mbus_cfg.flags = vep->bus.parallel.flags;
> +	else
> +		vin->mbus_cfg.flags = 0;
> +
> +	/*
> +	 * 'v4l2_async_notifier_fwnode_parse_endpoint()' has reserved
> +	 * enough memory for a whole 'rvin_grap_entity' structure, whose 'asd'
> +	 * is the first member of. Safely cast it to the 'digital' field of
> +	 * the selected vin instance.
> +	 */
> +	vin->digital = (struct rvin_graph_entity *)asd;
> +
> +	vin_dbg(vin, "Add OF device %pOF as VIN%u digital input\n",
> +		to_of_node(asd->match.fwnode), vin->id);
> +
> +	return 0;
> +}
> +
> +static int rvin_mc_parse_of_endpoint(struct device *dev,
> +				     struct v4l2_fwnode_endpoint *vep,
> +				     struct v4l2_async_subdev *asd)
> +{
> +	struct rvin_dev *group_vin = dev_get_drvdata(dev);
> +	struct rvin_group *group = group_vin->group;
> +	struct fwnode_handle *fw_vin;
> +	struct fwnode_handle *fw_port;
> +	struct rvin_dev *vin;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!fwnode_device_is_available(asd->match.fwnode)) {
> +		vin_dbg(group_vin, "OF device %pOF disabled, ignoring\n",
> +			to_of_node(asd->match.fwnode));
> +		return -ENOTCONN;
> +	}
> +
> +	/*
> +	 * Find out to which VIN instance this ep belongs to.
> +	 *
> +	 * While the list of async subdevices and the async notifier
> +	 * belong to the whole group, the bus configuration properties
> +	 * are instance specific; find the instance by matching its fwnode.
> +	 */
> +	fw_port = fwnode_get_parent(vep->base.local_fwnode);
> +	fw_vin = fwnode_graph_get_port_parent(fw_port);
> +	fwnode_handle_put(fw_port);
> +	if (!fw_vin)
> +		return -ENOENT;
> +
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		if (!group->vin[i])
> +			continue;
> +
> +		if (fw_vin == dev_fwnode(group->vin[i]->dev))
> +			break;
> +	}
> +	fwnode_handle_put(fw_vin);
> +
> +	if (i == RCAR_VIN_NUM) {
> +		vin_err(group_vin, "Unable to find VIN device for %pOF\n",
> +			to_of_node(asd->match.fwnode));
> +		return -ENOENT;
> +	}
> +	vin = group->vin[i];
> +
> +	switch (vep->base.port) {
> +	case RVIN_PORT_DIGITAL:
> +		ret = rvin_mc_parse_of_digital(vin, vep, asd);
> +		break;
> +	case RVIN_PORT_CSI2:
> +	default:
> +		ret = rvin_mc_parse_of_csi2(vin, vep, asd);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  {
>  	unsigned int count = 0;
> +	size_t asd_struct_size;
>  	unsigned int i;
>  	int ret;
>  
> @@ -753,25 +833,58 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	}
>  
>  	/*
> -	 * Have all VIN's look for subdevices. Some subdevices will overlap
> -	 * but the parser function can handle it, so each subdevice will
> -	 * only be registered once with the notifier.
> +	 * Have all VIN's look for subdevices. Some CSI-2 subdevices will
> +	 * overlap but the parser function can handle it, so each subdevice
> +	 * will only be registered once with the notifier.
>  	 */
>  
>  	vin->group->notifier = &vin->notifier;
>  
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		struct fwnode_handle *fwdev;
> +		struct fwnode_handle *fwep;
> +		struct fwnode_endpoint ep;
> +
>  		if (!vin->group->vin[i])
>  			continue;
>  
> +		/*
> +		 * If a digital input is described in port@0 proceed to parse
> +		 * it and register a single sub-device for this VIN instance.
> +		 * If no digital input is available go inspect the CSI-2
> +		 * connections described in port@1.
> +		 */
> +		fwdev = dev_fwnode(vin->group->vin[i]->dev);
> +		fwep = fwnode_graph_get_next_endpoint(fwdev, NULL);
> +		if (!fwep) {
> +			ret = PTR_ERR(fwep);
> +			goto error_unlock_return;
> +		}
> +
> +		ret = fwnode_graph_parse_endpoint(fwep, &ep);
> +		fwnode_handle_put(fwep);
> +		if (ret)
> +			goto error_unlock_return;
> +
> +		switch (ep.port) {
> +		case RVIN_PORT_DIGITAL:
> +			asd_struct_size = sizeof(struct rvin_graph_entity);
> +			break;
> +		case RVIN_PORT_CSI2:
> +			asd_struct_size = sizeof(struct v4l2_async_subdev);
> +			break;
> +		default:
> +			vin_err(vin, "port%u not allowed\n", ep.port);
> +			ret = -EINVAL;
> +			goto error_unlock_return;
> +		}
> +
>  		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
>  				vin->group->vin[i]->dev, vin->group->notifier,
> -				sizeof(struct v4l2_async_subdev), 1,
> +				asd_struct_size, ep.port,
>  				rvin_mc_parse_of_endpoint);
> -		if (ret) {
> -			mutex_unlock(&vin->group->lock);
> -			return ret;
> -		}
> +		if (ret)
> +			goto error_unlock_return;
>  	}
>  
>  	mutex_unlock(&vin->group->lock);
> @@ -785,16 +898,17 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	}
>  
>  	return 0;
> +
> +error_unlock_return:
> +	mutex_unlock(&vin->group->lock);
> +
> +	return ret;
>  }
>  
>  static int rvin_mc_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
> -	/* All our sources are CSI-2 */
> -	vin->mbus_cfg.type = V4L2_MBUS_CSI2;
> -	vin->mbus_cfg.flags = 0;
> -
>  	vin->pad.flags = MEDIA_PAD_FL_SINK;
>  	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
>  	if (ret)
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index c2aef78..a63f3c6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -52,6 +52,19 @@ enum rvin_csi_id {
>  };
>  
>  /**
> + * enum rvin_port_id
> + *
> + * List the available VIN port functions.
> + *
> + * RVIN_PORT_DIGITAL	- Input port for digital video connection
> + * RVIN_PORT_CSI2	- Input port for CSI-2 video connection
> + */
> +enum rvin_port_id {
> +	RVIN_PORT_DIGITAL,
> +	RVIN_PORT_CSI2
> +};

This enum is never used in the patch-set is it used later by a different 
patch-set or a left over from refactoring?

> +
> +/**
>   * STOPPED  - No operation in progress
>   * RUNNING  - Operation in progress have buffers
>   * STOPPING - Stopping operation
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 4/5] media: rcar-vin: Do not use crop if not configured

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-11 11:59:40 +0200, Jacopo Mondi wrote:
> The crop_scale routine uses the crop rectangle memebers to configure the
> VIN clipping rectangle. When crop is not configured all its fields are
> 0s, and setting the clipping rectangle vertical and horizontal extensions
> to (0 - 1) causes the registers to be written with an incorrect
> 0xffffffff value.

This is an interesting find and a clear bug in my code. But I can't see 
how crop ever could be 0. When s_fmt is called it always resets the crop 
and compose width's to the requested format size.

I'm curious how you found this bug, I tried to reproduce it but could 
not. This is indeed something we should fix! But I think the proper fix 
is not allowing crop to be 0 and not treating the symptom in 
rvin_crop_scale_comp().

> 
> Fix this by using the actual format width and height when no crop
> rectangle has been programmed.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index b41ba9a..ea7a120 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -579,22 +579,25 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
>  
>  void rvin_crop_scale_comp(struct rvin_dev *vin)
>  {
> -	/* Set Start/End Pixel/Line Pre-Clip */
> +	u32 width = vin->crop.width ? vin->crop.left + vin->crop.width :
> +				      vin->format.width;
> +	u32 height = vin->crop.height ? vin->crop.top + vin->crop.height :
> +					vin->format.height;
> +
> +	/* Set Start/End Pixel/Line Pre-Clip if crop is configured. */
>  	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> -	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> +	rvin_write(vin, width - 1, VNEPPRC_REG);
>  
>  	switch (vin->format.field) {
>  	case V4L2_FIELD_INTERLACED:
>  	case V4L2_FIELD_INTERLACED_TB:
>  	case V4L2_FIELD_INTERLACED_BT:
>  		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> -		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> -			   VNELPRC_REG);
> +		rvin_write(vin, height / 2 - 1, VNELPRC_REG);
>  		break;
>  	default:
>  		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> -		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> -			   VNELPRC_REG);
> +		rvin_write(vin, height - 1, VNELPRC_REG);
>  		break;
>  	}
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 4/5] media: rcar-vin: Do not use crop if not configured

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-11 11:59:40 +0200, Jacopo Mondi wrote:
> The crop_scale routine uses the crop rectangle memebers to configure the
> VIN clipping rectangle. When crop is not configured all its fields are
> 0s, and setting the clipping rectangle vertical and horizontal extensions
> to (0 - 1) causes the registers to be written with an incorrect
> 0xffffffff value.

This is an interesting find and a clear bug in my code. But I can't see 
how crop ever could be 0. When s_fmt is called it always resets the crop 
and compose width's to the requested format size.

I'm curious how you found this bug, I tried to reproduce it but could 
not. This is indeed something we should fix! But I think the proper fix 
is not allowing crop to be 0 and not treating the symptom in 
rvin_crop_scale_comp().

> 
> Fix this by using the actual format width and height when no crop
> rectangle has been programmed.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index b41ba9a..ea7a120 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -579,22 +579,25 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
>  
>  void rvin_crop_scale_comp(struct rvin_dev *vin)
>  {
> -	/* Set Start/End Pixel/Line Pre-Clip */
> +	u32 width = vin->crop.width ? vin->crop.left + vin->crop.width :
> +				      vin->format.width;
> +	u32 height = vin->crop.height ? vin->crop.top + vin->crop.height :
> +					vin->format.height;
> +
> +	/* Set Start/End Pixel/Line Pre-Clip if crop is configured. */
>  	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> -	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> +	rvin_write(vin, width - 1, VNEPPRC_REG);
>  
>  	switch (vin->format.field) {
>  	case V4L2_FIELD_INTERLACED:
>  	case V4L2_FIELD_INTERLACED_TB:
>  	case V4L2_FIELD_INTERLACED_BT:
>  		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> -		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> -			   VNELPRC_REG);
> +		rvin_write(vin, height / 2 - 1, VNELPRC_REG);
>  		break;
>  	default:
>  		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> -		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> -			   VNELPRC_REG);
> +		rvin_write(vin, height - 1, VNELPRC_REG);
>  		break;
>  	}
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 4/5] media: rcar-vin: Do not use crop if not configured

[Niklas Söderlund]

Hi again,

On 2018-05-11 13:10:37 +0200, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your work.
> 
> On 2018-05-11 11:59:40 +0200, Jacopo Mondi wrote:
> > The crop_scale routine uses the crop rectangle memebers to configure the
> > VIN clipping rectangle. When crop is not configured all its fields are
> > 0s, and setting the clipping rectangle vertical and horizontal extensions
> > to (0 - 1) causes the registers to be written with an incorrect
> > 0xffffffff value.
> 
> This is an interesting find and a clear bug in my code. But I can't see 
> how crop ever could be 0. When s_fmt is called it always resets the crop 
> and compose width's to the requested format size.
> 
> I'm curious how you found this bug, I tried to reproduce it but could 
> not. 

My bad I was looking at the wrong thing, yes I can reproduce this on 
CSI-2 capture as well. Really nice find!

> This is indeed something we should fix! But I think the proper fix is 
>not allowing crop to be 0 and not treating the symptom in 
>rvin_crop_scale_comp().
> 
> > 
> > Fix this by using the actual format width and height when no crop
> > rectangle has been programmed.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index b41ba9a..ea7a120 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -579,22 +579,25 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
> >  
> >  void rvin_crop_scale_comp(struct rvin_dev *vin)
> >  {
> > -	/* Set Start/End Pixel/Line Pre-Clip */
> > +	u32 width = vin->crop.width ? vin->crop.left + vin->crop.width :
> > +				      vin->format.width;
> > +	u32 height = vin->crop.height ? vin->crop.top + vin->crop.height :
> > +					vin->format.height;
> > +
> > +	/* Set Start/End Pixel/Line Pre-Clip if crop is configured. */
> >  	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> > -	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> > +	rvin_write(vin, width - 1, VNEPPRC_REG);
> >  
> >  	switch (vin->format.field) {
> >  	case V4L2_FIELD_INTERLACED:
> >  	case V4L2_FIELD_INTERLACED_TB:
> >  	case V4L2_FIELD_INTERLACED_BT:
> >  		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> > -		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> > -			   VNELPRC_REG);
> > +		rvin_write(vin, height / 2 - 1, VNELPRC_REG);
> >  		break;
> >  	default:
> >  		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > -		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> > -			   VNELPRC_REG);
> > +		rvin_write(vin, height - 1, VNELPRC_REG);
> >  		break;
> >  	}
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Regards,
Niklas Söderlund

Re: [PATCH 4/5] media: rcar-vin: Do not use crop if not configured

[Niklas Söderlund]

Hi again,

On 2018-05-11 13:10:37 +0200, Niklas S�derlund wrote:
> Hi Jacopo,
> 
> Thanks for your work.
> 
> On 2018-05-11 11:59:40 +0200, Jacopo Mondi wrote:
> > The crop_scale routine uses the crop rectangle memebers to configure the
> > VIN clipping rectangle. When crop is not configured all its fields are
> > 0s, and setting the clipping rectangle vertical and horizontal extensions
> > to (0 - 1) causes the registers to be written with an incorrect
> > 0xffffffff value.
> 
> This is an interesting find and a clear bug in my code. But I can't see 
> how crop ever could be 0. When s_fmt is called it always resets the crop 
> and compose width's to the requested format size.
> 
> I'm curious how you found this bug, I tried to reproduce it but could 
> not. 

My bad I was looking at the wrong thing, yes I can reproduce this on 
CSI-2 capture as well. Really nice find!

> This is indeed something we should fix! But I think the proper fix is 
>not allowing crop to be 0 and not treating the symptom in 
>rvin_crop_scale_comp().
> 
> > 
> > Fix this by using the actual format width and height when no crop
> > rectangle has been programmed.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index b41ba9a..ea7a120 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -579,22 +579,25 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
> >  
> >  void rvin_crop_scale_comp(struct rvin_dev *vin)
> >  {
> > -	/* Set Start/End Pixel/Line Pre-Clip */
> > +	u32 width = vin->crop.width ? vin->crop.left + vin->crop.width :
> > +				      vin->format.width;
> > +	u32 height = vin->crop.height ? vin->crop.top + vin->crop.height :
> > +					vin->format.height;
> > +
> > +	/* Set Start/End Pixel/Line Pre-Clip if crop is configured. */
> >  	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> > -	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> > +	rvin_write(vin, width - 1, VNEPPRC_REG);
> >  
> >  	switch (vin->format.field) {
> >  	case V4L2_FIELD_INTERLACED:
> >  	case V4L2_FIELD_INTERLACED_TB:
> >  	case V4L2_FIELD_INTERLACED_BT:
> >  		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> > -		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> > -			   VNELPRC_REG);
> > +		rvin_write(vin, height / 2 - 1, VNELPRC_REG);
> >  		break;
> >  	default:
> >  		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > -		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> > -			   VNELPRC_REG);
> > +		rvin_write(vin, height - 1, VNELPRC_REG);
> >  		break;
> >  	}
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Regards,
> Niklas S�derlund

-- 
Regards,
Niklas S�derlund

Re: [PATCH 5/5] media: rcar-vin: Use FTEV for digital input

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3698 bytes --]

Hi Hans,

On Fri, May 11, 2018 at 12:28:55PM +0200, Hans Verkuil wrote:
> On 05/11/18 11:59, Jacopo Mondi wrote:
> > Since commit (015060cb "media: rcar-vin: enable field toggle after a set
> > number of lines for Gen3) the VIN generates an internal field signal
> > toggle after a fixed number of received lines, and uses the internal
> > field signal to drive capture operations. When capturing from digital
> > input, using FTEH driven field signal toggling messes up the received
> > image sizes. Fall back to use FTEV driven signal toggling when capturing
> > from digital input.
> >
> > As explained in the comment, this disables buffer overflow protection
> > for digital input capture, for which the FOE overflow might be used in
> > future.
>
> I don't know the details of the hardware, but this sounds dangerous.
>
> You should know that with HDMI input it is perfectly possible that you get
> more data than you should. I.e. instead of 1080 lines (assuming full HD)
> you might get more lines. This happens if the vertical sync is missed due
> to pin bounce when connecting a source.
>
> Other reasons for this are flaky signals, bad clocks, etc.
>
> It's rare, but it really happens.
>
> A good DMA engine will refuse to write more than fits in the buffer.
>
> If you disable that, then you will get overflows eventually.
>
> The reality with HDMI input is that you should never assume clean valid
> data. You do not control the source and it can send anything it likes.

Thanks for the informations. I agree HDMI input is lot of fun (-.-)
and we've seen weird things happening too.

With the patch Niklas has just sent that fixes the crop/compose
rectangle, also the previously in-place protection agains overflow has
been reverted, so this patch is not required anymore.

I re-send re-basing this on top of Niklas' latest fix.

Thanks
   j

>
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index ea7a120..8dc3455 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -685,11 +685,27 @@ static int rvin_setup(struct rvin_dev *vin)
> >  		break;
> >  	}
> >
> > -	if (vin->info->model == RCAR_GEN3) {
> > +	if (vin->info->model == RCAR_GEN3 &&
> > +	    vin->mbus_cfg.type == V4L2_MBUS_CSI2) {
> >  		/* Enable HSYNC Field Toggle mode after height HSYNC inputs. */
> >  		lines = vin->format.height / (halfsize ? 2 : 1);
> >  		dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines);
> >  		vin_dbg(vin, "Field Toogle after %u lines\n", lines);
>
> Typo: Toogle -> Toggle
>
> > +	} else if (vin->info->model == RCAR_GEN3 &&
> > +		   vin->mbus_cfg.type == V4L2_MBUS_PARALLEL) {
> > +		/*
> > +		 * FIXME
> > +		 * Section 26.3.17 specifies that for digital input there's no
> > +		 * need to program FTEH or FTEV to generate internal
> > +		 * field toggle signal to driver capture. Although when
> > +		 * running on GEN3 with digital input no EFE interrupt is ever
> > +		 * generated, and we need to rely on FTEV driven field signal
> > +		 * toggling, as using FTEH as in the CSI-2 case, messes up
> > +		 * the output image size. This implies no protection
> > +		 * against buffer overflow is in place for Gen3 digital input
> > +		 * capture.
> > +		 */
> > +		dmr2 = VNDMR2_FTEV;
> >  	} else {
> >  		/* Enable VSYNC Field Toogle mode after one VSYNC input. */
>
> Ditto. Search and replace?
>
> >  		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
> >
>
> Regards,
>
> 	Hans

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/5] media: rcar-vin: Use FTEV for digital input

[Sergei Shtylyov]

Hello!

On 5/11/2018 12:59 PM, Jacopo Mondi wrote:

> Since commit (015060cb

    Need 12 digits here, and SHA1 ID should be cited outside the parens.

> "media: rcar-vin: enable field toggle after a set
> number of lines for Gen3)

    The commit summary must be enclosed in ("").
    And I think Niklas has posted the patches reverting that commit and fixing 
the driver properly.

> the VIN generates an internal field signal
> toggle after a fixed number of received lines, and uses the internal
> field signal to drive capture operations. When capturing from digital
> input, using FTEH driven field signal toggling messes up the received
> image sizes. Fall back to use FTEV driven signal toggling when capturing
> from digital input.
> 
> As explained in the comment, this disables buffer overflow protection
> for digital input capture, for which the FOE overflow might be used in
> future.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
[...]

MBR, Sergei

Re: [PATCH 1/5] media: rcar-vin: Add support for R-Car R8A77995 SoC

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 11 May 2018 12:59:37 EEST Jacopo Mondi wrote:
> Add R-Car R8A77995 SoC to the rcar-vin supported ones.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index d3072e1..e547ef7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -985,6 +985,10 @@ static const struct rvin_group_route
> _rcar_info_r8a77970_routes[] = { { /* Sentinel */ }
>  };
> 
> +static const struct rvin_group_route _rcar_info_r8a77995_routes[] = {
> +	{ /* Sentinel */ }
> +};
> +
>  static const struct rvin_info rcar_info_r8a77970 = {
>  	.model = RCAR_GEN3,
>  	.use_mc = true,
> @@ -993,6 +997,14 @@ static const struct rvin_info rcar_info_r8a77970 = {
>  	.routes = _rcar_info_r8a77970_routes,
>  };
> 
> +static const struct rvin_info rcar_info_r8a77995 = {
> +	.model = RCAR_GEN3,
> +	.use_mc = true,
> +	.max_width = 4096,
> +	.max_height = 4096,
> +	.routes = _rcar_info_r8a77995_routes,
> +};
> +
>  static const struct of_device_id rvin_of_id_table[] = {
>  	{
>  		.compatible = "renesas,vin-r8a7778",
> @@ -1034,6 +1046,10 @@ static const struct of_device_id rvin_of_id_table[] =
> { .compatible = "renesas,vin-r8a77970",
>  		.data = &rcar_info_r8a77970,
>  	},
> +	{
> +		.compatible = "renesas,vin-r8a77995",
> +		.data = &rcar_info_r8a77995,
> +	},
>  	{ /* Sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, rvin_of_id_table);


-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] media: rcar-vin: Add digital input subdevice parsing

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4198 bytes --]

Hi Niklas,

On Fri, May 11, 2018 at 01:01:24PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work!
>
> The comments here apply to both 2/5 and 3/5.
>
> On 2018-05-11 11:59:38 +0200, Jacopo Mondi wrote:
> > Add support for digital input subdevices to Gen-3 rcar-vin.
> > The Gen-3, media-controller compliant, version has so far only accepted
> > CSI-2 input subdevices. Remove assumptions on the supported bus_type and
> > accepted number of subdevices, and allow digital input connections on port@0.
>
> I feel this much more complex then it has to be. You defer parsing the
> DT graph until all VIN's are probed just like it's done for the port@1
> parsing. For the CSI-2 endpoints in port@1 this is needed as they are
> shared for all VIN's in the group. While for the parallel input this is
> local for each VIN and could be parsed at probe time for each individual
> VIN. As a bonus for doing that I think you could reuse the parallel
> parsing functions from the Gen2 code whit small additions.
>
> Maybe something like this can be done:
>
> - At each VIN probe() rework the block
>
>     if (vin->info->use_mc)
> 	    ret = rvin_mc_init(vin);
>     else
> 	    ret = rvin_digital_graph_init(vin);
>
>   To:
>     ret = rvin_digital_graph_init(vin);
>     ...
>     ret = rvin_mc_init(vin);
>     ...
>
> - Then in rvin_digital_graph_init() don't call
>   v4l2_async_notifier_register() if vin->info->use_mc is set.
>
>   And instead as a last step in rvin_mc_init() call
>   v4l2_async_notifier_register() for each vin->notifier that contains a
>   subdevice.
>
> - As a last step add a check for vin->info->use_mc in
>   rvin_digital_notify_complete() and handle the case for Gen2 behavior
>   (what it does now) and Gen3 behavior (adding the MC link).
>
>
> I think my doing this you could greatly simplify the process of handling
> the parallel subdevice.
>
> Please see bellow for one additional comment.

Thanks for the suggestion.
My understanding was that the 'Gen2' was planned for removal in the
long term, and we should have paved the road for a
media-controller-only version of the driver. But I admit I didn't even
try to re-use that code part, I'll give your suggestions a spin, if
this won't prevent future removal of the non-mc part of the driver.
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 166 +++++++++++++++++++++++-----
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  13 +++
> >  2 files changed, 153 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index e547ef7..105b6b6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -697,29 +697,21 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
> >  	.complete = rvin_group_notify_complete,

[snip]

> > +
> > +	switch (vep->base.port) {
> > +	case RVIN_PORT_DIGITAL:
> > +		ret = rvin_mc_parse_of_digital(vin, vep, asd);
> > +		break;
> > +	case RVIN_PORT_CSI2:
> > +	default:
> > +		ret = rvin_mc_parse_of_csi2(vin, vep, asd);
> > +		break;
> > +	}
> > +		/*

[snip]

> > +
> > +		switch (ep.port) {
> > +		case RVIN_PORT_DIGITAL:
> > +			asd_struct_size = sizeof(struct rvin_graph_entity);
> > +			break;
> > +		case RVIN_PORT_CSI2:
> > +			asd_struct_size = sizeof(struct v4l2_async_subdev);
> > +			break;
> > +		default:
> >  };

[snip]

> >
> >  /**
> > + * enum rvin_port_id
> > + *
> > + * List the available VIN port functions.
> > + *
> > + * RVIN_PORT_DIGITAL	- Input port for digital video connection
> > + * RVIN_PORT_CSI2	- Input port for CSI-2 video connection
> > + */
> > +enum rvin_port_id {
> > +	RVIN_PORT_DIGITAL,
> > +	RVIN_PORT_CSI2
> > +};
>
> This enum is never used in the patch-set is it used later by a different
> patch-set or a left over from refactoring?

I see two occurencies each of this enumeration members in this patch.
I left them un-cut above.

Thanks
   j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]