[PATCH v3 0/9] rcar-vin: Add support for parallel input on Gen3

[PATCH v3 0/9] rcar-vin: Add support for parallel input on Gen3

[Jacopo Mondi]

Hello,
   this series adds support for parallel video input to the Gen3 version of
rcar-vin driver.

Thanks to Niklas' review of v2, this new version allows the same VIN instance
to have both a parallel and a CSI-2 subdevice connected, and to switch
between them at runtime through the modification of links between the media
controller entities in the media-controller graph.

The series starts with a rename s/digital/parallel, as the use of the term
digital was not appropriate and Niklas agreed on the change.

Then a group-wise notifier is created. As detailed in the commit message, to
avoid registering the same notifier twice, for parallel and CSI-2 subdevices,
a shared one is created and used to collect all CSI-2 subdevs for the group.

As we can now switch between CSI-2 and digital subdevs, the media bus
configuration properties have been removed from the VIN device, and cached
in a dedicated structure. They will be taken into account only when a parallel
subdevice is linked to the VIN instance, as suggested by Niklas.

The parallel input parsing has been moved to probe function, and the involved
functions modified to take the media-controller case into account. Again, this
was initially suggested by Niklas, but I failed to do this in v2.

Once the parallel subdevice is collected and bound, at 'complete' time a link
in the media controller has to be created if we're running on an mc-compliant
configuration. This makes the parallel complete function a bit more verbose.

Last step is to properly handle the link state change notification function,
switching between CSI-2 and parallel inputs. This patch is almost the
same as the v2 one, handling of the 'is_csi' flag apart.

At the end of the series, after a small cleanup requested by Niklas, support
for the R8A77995 SoC is added to the driver.

Testing:
As there are no boards I'm aware of with both CSI-2 and digital subdevs
connected to the same VIN instance, I have faked that on a Salvator-x M3-W
board, registering an adv7612 chip connected to VIN5 instance, where also
a CSI20 and CSI40 are routed to. For the interested, the testing branch is
available at:
git://jmondi.org/linux d3/media-master/salvator-x-dts_csi2/digital_input

Image capture from HDMI input works as expected, as well as link switching.
Of course capturing from adv7612 is not tested, as the chip is not actually
there.

On D3, where a single parallel input provided by an adv7612 is connected to
VIN4, image capture from the HDMI port has been tested using a modified version
of vin-tests available at:
git://jmondi.org/vin-tests d3

and an additional patch to Draak device tree to enable HDMI capture, which is
by default disabled in favor of CVBS one:
git://jmondi.org/linux d3/media-master/salvator-x-dts_csi2/d3-hdmi

Image capture from HDMI works as expected with a 640x480 image source.

No changelog as the series is changed quite significantly from v2 and v1 and
is fully described in this cover letter.

Thanks Niklas for the prompt review of v1 and v2, I hope this one is closer
to what is expected to properly support parallel capture on Gen3 boards.

(On a side note, am I wrong or with this series we may easily move all Gen2
devices to run with media-controller without any major modifications?)

Thanks
   j

Jacopo Mondi (9):
  media: rcar-vin: Rename 'digital' to 'parallel'
  media: rcar-vin: Remove two empty lines
  media: rcar-vin: Create a group notifier
  media: rcar-vin: Cache the mbus configuration flags
  media: rcar-vin: Parse parallel input on Gen3
  media: rcar-vin: Link parallel input media entities
  media: rcar-vin: Handle parallel subdev in link_notify
  media: rcar-vin: Rename _rcar_info to rcar_info
  media: rcar-vin: Add support for R-Car R8A77995 SoC

 drivers/media/platform/rcar-vin/rcar-core.c | 318 +++++++++++++++++++---------
 drivers/media/platform/rcar-vin/rcar-dma.c  |  28 ++-
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  12 +-
 drivers/media/platform/rcar-vin/rcar-vin.h  |  31 ++-
 4 files changed, 260 insertions(+), 129 deletions(-)

--
2.7.4

[PATCH v3 1/9] media: rcar-vin: Rename 'digital' to 'parallel'

[Jacopo Mondi]

As the term 'digital' is used all over the rcar-vin code in place of
'parallel', rename all the occurrencies.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 72 ++++++++++++++---------------
 drivers/media/platform/rcar-vin/rcar-dma.c  |  4 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++---
 drivers/media/platform/rcar-vin/rcar-vin.h  |  6 +--
 4 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d3072e1..6b80f98 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -376,12 +376,12 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
 }
 
 /* -----------------------------------------------------------------------------
- * Digital async notifier
+ * Parallel async notifier
  */
 
 /* The vin lock should be held when calling the subdevice attach and detach */
-static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
-					 struct v4l2_subdev *subdev)
+static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
+					  struct v4l2_subdev *subdev)
 {
 	struct v4l2_subdev_mbus_code_enum code = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
@@ -392,15 +392,15 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
 	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
 	if (ret < 0)
 		return ret;
-	vin->digital->source_pad = ret;
+	vin->parallel->source_pad = ret;
 
 	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
-	vin->digital->sink_pad = ret < 0 ? 0 : ret;
+	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
 
 	/* Find compatible subdevices mbus format */
 	vin->mbus_code = 0;
 	code.index = 0;
-	code.pad = vin->digital->source_pad;
+	code.pad = vin->parallel->source_pad;
 	while (!vin->mbus_code &&
 	       !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
 		code.index++;
@@ -450,21 +450,21 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
 
 	vin->vdev.ctrl_handler = &vin->ctrl_handler;
 
-	vin->digital->subdev = subdev;
+	vin->parallel->subdev = subdev;
 
 	return 0;
 }
 
-static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
+static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 {
 	rvin_v4l2_unregister(vin);
 	v4l2_ctrl_handler_free(&vin->ctrl_handler);
 
 	vin->vdev.ctrl_handler = NULL;
-	vin->digital->subdev = NULL;
+	vin->parallel->subdev = NULL;
 }
 
-static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
+static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 	int ret;
@@ -478,28 +478,28 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 	return rvin_v4l2_register(vin);
 }
 
-static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
-				       struct v4l2_subdev *subdev,
-				       struct v4l2_async_subdev *asd)
+static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
+				        struct v4l2_subdev *subdev,
+				        struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 
-	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
+	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
 
 	mutex_lock(&vin->lock);
-	rvin_digital_subdevice_detach(vin);
+	rvin_parallel_subdevice_detach(vin);
 	mutex_unlock(&vin->lock);
 }
 
-static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
-				     struct v4l2_subdev *subdev,
-				     struct v4l2_async_subdev *asd)
+static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
+				      struct v4l2_subdev *subdev,
+				      struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 	int ret;
 
 	mutex_lock(&vin->lock);
-	ret = rvin_digital_subdevice_attach(vin, subdev);
+	ret = rvin_parallel_subdevice_attach(vin, subdev);
 	mutex_unlock(&vin->lock);
 	if (ret)
 		return ret;
@@ -507,21 +507,21 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
 	v4l2_set_subdev_hostdata(subdev, vin);
 
 	vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
-		subdev->name, vin->digital->source_pad,
-		vin->digital->sink_pad);
+		subdev->name, vin->parallel->source_pad,
+		vin->parallel->sink_pad);
 
 	return 0;
 }
 
-static const struct v4l2_async_notifier_operations rvin_digital_notify_ops = {
-	.bound = rvin_digital_notify_bound,
-	.unbind = rvin_digital_notify_unbind,
-	.complete = rvin_digital_notify_complete,
+static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
+	.bound = rvin_parallel_notify_bound,
+	.unbind = rvin_parallel_notify_unbind,
+	.complete = rvin_parallel_notify_complete,
 };
 
-static int rvin_digital_parse_v4l2(struct device *dev,
-				   struct v4l2_fwnode_endpoint *vep,
-				   struct v4l2_async_subdev *asd)
+static int rvin_parallel_parse_v4l2(struct device *dev,
+				    struct v4l2_fwnode_endpoint *vep,
+				    struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = dev_get_drvdata(dev);
 	struct rvin_graph_entity *rvge =
@@ -546,28 +546,28 @@ static int rvin_digital_parse_v4l2(struct device *dev,
 		return -EINVAL;
 	}
 
-	vin->digital = rvge;
+	vin->parallel = rvge;
 
 	return 0;
 }
 
-static int rvin_digital_graph_init(struct rvin_dev *vin)
+static int rvin_parallel_graph_init(struct rvin_dev *vin)
 {
 	int ret;
 
 	ret = v4l2_async_notifier_parse_fwnode_endpoints(
 		vin->dev, &vin->notifier,
-		sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
+		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
 	if (ret)
 		return ret;
 
-	if (!vin->digital)
+	if (!vin->parallel)
 		return -ENODEV;
 
-	vin_dbg(vin, "Found digital subdevice %pOF\n",
-		to_of_node(vin->digital->asd.match.fwnode));
+	vin_dbg(vin, "Found parallel subdevice %pOF\n",
+		to_of_node(vin->parallel->asd.match.fwnode));
 
-	vin->notifier.ops = &rvin_digital_notify_ops;
+	vin->notifier.ops = &rvin_parallel_notify_ops;
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
 	if (ret < 0) {
 		vin_err(vin, "Notifier registration failed\n");
@@ -1088,7 +1088,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	if (vin->info->use_mc)
 		ret = rvin_mc_init(vin);
 	else
-		ret = rvin_digital_graph_init(vin);
+		ret = rvin_parallel_graph_init(vin);
 	if (ret < 0)
 		goto error;
 
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index ac07f99..f1c3585 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -733,7 +733,7 @@ static int rvin_setup(struct rvin_dev *vin)
 		vnmc |= VNMC_BPS;
 
 	if (vin->info->model == RCAR_GEN3) {
-		/* Select between CSI-2 and Digital input */
+		/* Select between CSI-2 and parallel input */
 		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
 			vnmc &= ~VNMC_DPINE;
 		else
@@ -1074,7 +1074,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
 
 	/* No media controller used, simply pass operation to subdevice. */
 	if (!vin->info->use_mc) {
-		ret = v4l2_subdev_call(vin->digital->subdev, video, s_stream,
+		ret = v4l2_subdev_call(vin->parallel->subdev, video, s_stream,
 				       on);
 
 		return ret == -ENOIOCTLCMD ? 0 : ret;
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index e78fba8..87a718b 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -144,7 +144,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
 {
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-		.pad = vin->digital->source_pad,
+		.pad = vin->parallel->source_pad,
 	};
 	int ret;
 
@@ -175,7 +175,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 	struct v4l2_subdev_pad_config *pad_cfg;
 	struct v4l2_subdev_format format = {
 		.which = which,
-		.pad = vin->digital->source_pad,
+		.pad = vin->parallel->source_pad,
 	};
 	enum v4l2_field field;
 	u32 width, height;
@@ -517,7 +517,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
 	if (timings->pad)
 		return -EINVAL;
 
-	timings->pad = vin->digital->sink_pad;
+	timings->pad = vin->parallel->sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
 
@@ -569,7 +569,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
 	if (cap->pad)
 		return -EINVAL;
 
-	cap->pad = vin->digital->sink_pad;
+	cap->pad = vin->parallel->sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
 
@@ -587,7 +587,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->digital->sink_pad;
+	edid->pad = vin->parallel->sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
 
@@ -605,7 +605,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->digital->sink_pad;
+	edid->pad = vin->parallel->sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
 
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index c2aef78..755ac3c 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -146,7 +146,7 @@ struct rvin_info {
  * @v4l2_dev:		V4L2 device
  * @ctrl_handler:	V4L2 control handler
  * @notifier:		V4L2 asynchronous subdevs notifier
- * @digital:		entity in the DT for local digital subdevice
+ * @parallel:		entity in the DT for local parallel subdevice
  *
  * @group:		Gen3 CSI group
  * @id:			Gen3 group id for this VIN
@@ -182,7 +182,7 @@ struct rvin_dev {
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_async_notifier notifier;
-	struct rvin_graph_entity *digital;
+	struct rvin_graph_entity *parallel;
 
 	struct rvin_group *group;
 	unsigned int id;
@@ -209,7 +209,7 @@ struct rvin_dev {
 	v4l2_std_id std;
 };
 
-#define vin_to_source(vin)		((vin)->digital->subdev)
+#define vin_to_source(vin)		((vin)->parallel->subdev)
 
 /* Debug */
 #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
-- 
2.7.4

[PATCH v3 2/9] media: rcar-vin: Remove two empty lines

[Jacopo Mondi]

Remove un-necessary empty lines.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 6b80f98..1aadd90 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -707,11 +707,9 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
 		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) {
-- 
2.7.4

[PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Jacopo Mondi]

As CSI-2 subdevices are shared between several VIN instances, a shared
notifier to collect the CSI-2 async subdevices is required. So far, the
rcar-vin driver used the notifier of the last VIN instance to probe but
with the forth-coming introduction of parallel input subdevices support
in mc-compliant code path, each VIN may register its own notifier if any
parallel subdevice is connected there.

To avoid registering a notifier twice (once for parallel subdev and one
for the CSI-2 subdevs) create a group notifier, shared by all the VIN
instances.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 1aadd90..c6e603f 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -583,7 +583,7 @@ static int rvin_parallel_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 *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	const struct rvin_group_route *route;
 	unsigned int i;
 	int ret;
@@ -649,7 +649,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
 				     struct v4l2_subdev *subdev,
 				     struct v4l2_async_subdev *asd)
 {
-	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	unsigned int i;
 
 	for (i = 0; i < RCAR_VIN_NUM; i++)
@@ -673,7 +673,7 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
 				   struct v4l2_subdev *subdev,
 				   struct v4l2_async_subdev *asd)
 {
-	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	unsigned int i;
 
 	mutex_lock(&vin->group->lock);
@@ -734,12 +734,6 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 
 	mutex_lock(&vin->group->lock);
 
-	/* If there already is a notifier something has gone wrong, bail out. */
-	if (WARN_ON(vin->group->notifier)) {
-		mutex_unlock(&vin->group->lock);
-		return -EINVAL;
-	}
-
 	/* If not all VIN's are registered don't register the notifier. */
 	for (i = 0; i < RCAR_VIN_NUM; i++)
 		if (vin->group->vin[i])
@@ -751,19 +745,16 @@ 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 CSI-2 subdevices. Some subdevices will
+	 * overlap but the parser function can handle it, so each subdevice
+	 * will only be registered once with the group notifier.
 	 */
-
-	vin->group->notifier = &vin->notifier;
-
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
 		if (!vin->group->vin[i])
 			continue;
 
 		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
-				vin->group->vin[i]->dev, vin->group->notifier,
+				vin->group->vin[i]->dev, &vin->group->notifier,
 				sizeof(struct v4l2_async_subdev), 1,
 				rvin_mc_parse_of_endpoint);
 		if (ret) {
@@ -774,9 +765,12 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 
 	mutex_unlock(&vin->group->lock);
 
-	vin->group->notifier->ops = &rvin_group_notify_ops;
+	if (!vin->group->notifier.num_subdevs)
+		return 0;
 
-	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
+	vin->group->notifier.ops = &rvin_group_notify_ops;
+	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
+					   &vin->group->notifier);
 	if (ret < 0) {
 		vin_err(vin, "Notifier registration failed\n");
 		return ret;
@@ -1112,15 +1106,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
 	v4l2_async_notifier_unregister(&vin->notifier);
 	v4l2_async_notifier_cleanup(&vin->notifier);
 
-	if (vin->info->use_mc) {
-		mutex_lock(&vin->group->lock);
-		if (vin->group->notifier == &vin->notifier)
-			vin->group->notifier = NULL;
-		mutex_unlock(&vin->group->lock);
+	if (vin->info->use_mc)
 		rvin_group_put(vin);
-	} else {
+	else
 		v4l2_ctrl_handler_free(&vin->ctrl_handler);
-	}
 
 	rvin_dma_unregister(vin);
 
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 755ac3c..7d0ffe08 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -209,6 +209,8 @@ struct rvin_dev {
 	v4l2_std_id std;
 };
 
+#define v4l2_dev_to_vin(d)	container_of(d, struct rvin_dev, v4l2_dev)
+
 #define vin_to_source(vin)		((vin)->parallel->subdev)
 
 /* Debug */
@@ -225,8 +227,7 @@ struct rvin_dev {
  *
  * @lock:		protects the count, notifier, vin and csi members
  * @count:		number of enabled VIN instances found in DT
- * @notifier:		pointer to the notifier of a VIN which handles the
- *			groups async sub-devices.
+ * @notifier:		group notifier for CSI-2 async subdevices
  * @vin:		VIN instances which are part of the group
  * @csi:		array of pairs of fwnode and subdev pointers
  *			to all CSI-2 subdevices.
@@ -238,7 +239,7 @@ struct rvin_group {
 
 	struct mutex lock;
 	unsigned int count;
-	struct v4l2_async_notifier *notifier;
+	struct v4l2_async_notifier notifier;
 	struct rvin_dev *vin[RCAR_VIN_NUM];
 
 	struct {
-- 
2.7.4

[PATCH v3 4/9] media: rcar-vin: Cache the mbus configuration flags

[Jacopo Mondi]

Media bus configuration flags and media bus type were so far a property
of each VIN instance, as the subdevice they were connected to was
immutable during the whole system life time.

With the forth-coming introduction of parallel input devices support,
a VIN instance can have the subdevice it is connected to switched at
runtime, from a CSI-2 subdevice to a parallel one and viceversa, through
the modification of links between media entities in the media controller
graph. To avoid discarding the per-subdevice configuration flags retrieved by
v4l2_fwnode parsing facilities, cache them in the 'rvin_graph_entity'
member of each VIN instance, opportunely renamed to 'rvin_parallel_entity'.

Also modify the register configuration function to take mbus flags into
account when running on a bus type that supports them.

The media bus type currently in use will be updated in a follow-up patch
to the link state change notification function.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 23 ++++++++++-------------
 drivers/media/platform/rcar-vin/rcar-dma.c  | 24 +++++++++++++++++-------
 drivers/media/platform/rcar-vin/rcar-vin.h  | 22 ++++++++++++++++------
 3 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index c6e603f..0a35a98 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -524,30 +524,30 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
 				    struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = dev_get_drvdata(dev);
-	struct rvin_graph_entity *rvge =
-		container_of(asd, struct rvin_graph_entity, asd);
+	struct rvin_parallel_entity *rvpe =
+		container_of(asd, struct rvin_parallel_entity, asd);
 
 	if (vep->base.port || vep->base.id)
 		return -ENOTCONN;
 
-	vin->mbus_cfg.type = vep->bus_type;
+	vin->is_csi = false;
+	vin->parallel = rvpe;
+	vin->parallel->mbus_type = vep->bus_type;
 
-	switch (vin->mbus_cfg.type) {
+	switch (vin->parallel->mbus_type) {
 	case V4L2_MBUS_PARALLEL:
 		vin_dbg(vin, "Found PARALLEL media bus\n");
-		vin->mbus_cfg.flags = vep->bus.parallel.flags;
+		vin->parallel->mbus_flags = vep->bus.parallel.flags;
 		break;
 	case V4L2_MBUS_BT656:
 		vin_dbg(vin, "Found BT656 media bus\n");
-		vin->mbus_cfg.flags = 0;
+		vin->parallel->mbus_flags = 0;
 		break;
 	default:
 		vin_err(vin, "Unknown media bus type\n");
 		return -EINVAL;
 	}
 
-	vin->parallel = rvge;
-
 	return 0;
 }
 
@@ -557,7 +557,7 @@ static int rvin_parallel_graph_init(struct rvin_dev *vin)
 
 	ret = v4l2_async_notifier_parse_fwnode_endpoints(
 		vin->dev, &vin->notifier,
-		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
+		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
 	if (ret)
 		return ret;
 
@@ -718,6 +718,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
 		return -ENOTCONN;
 	}
 
+	vin->is_csi = true;
 	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
 
 	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
@@ -783,10 +784,6 @@ 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-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index f1c3585..17f291f 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -659,8 +659,12 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
-		vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
-			VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
+		if (!vin->is_csi &&
+		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
+			vnmc |= VNMC_INF_YUV8_BT656;
+		else
+			vnmc |= VNMC_INF_YUV8_BT601;
+
 		input_is_yuv = true;
 		break;
 	case MEDIA_BUS_FMT_RGB888_1X24:
@@ -668,8 +672,12 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	case MEDIA_BUS_FMT_UYVY10_2X10:
 		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
-		vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
-			VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
+		if (!vin->is_csi &&
+		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
+			vnmc |= VNMC_INF_YUV10_BT656;
+		else
+			vnmc |= VNMC_INF_YUV10_BT601;
+
 		input_is_yuv = true;
 		break;
 	default:
@@ -683,11 +691,13 @@ static int rvin_setup(struct rvin_dev *vin)
 		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
 
 	/* Hsync Signal Polarity Select */
-	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
+	if (!vin->is_csi &&
+	    !(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
 		dmr2 |= VNDMR2_HPS;
 
 	/* Vsync Signal Polarity Select */
-	if (!(vin->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
+	if (!vin->is_csi &&
+	    !(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
 		dmr2 |= VNDMR2_VPS;
 
 	/*
@@ -734,7 +744,7 @@ static int rvin_setup(struct rvin_dev *vin)
 
 	if (vin->info->model == RCAR_GEN3) {
 		/* Select between CSI-2 and parallel input */
-		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
+		if (vin->is_csi)
 			vnmc &= ~VNMC_DPINE;
 		else
 			vnmc |= VNMC_DPINE;
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 7d0ffe08..c5f7fd1 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -73,16 +73,22 @@ struct rvin_video_format {
 };
 
 /**
- * struct rvin_graph_entity - Video endpoint from async framework
+ * struct rvin_parallel_entity - Parallel video input endpoint descriptor
  * @asd:	sub-device descriptor for async framework
  * @subdev:	subdevice matched using async framework
+ * @mbus_type:	media bus type
+ * @mbus_flags:	media bus configuration flags
  * @source_pad:	source pad of remote subdevice
  * @sink_pad:	sink pad of remote subdevice
+ *
  */
-struct rvin_graph_entity {
+struct rvin_parallel_entity {
 	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *subdev;
 
+	enum v4l2_mbus_type mbus_type;
+	unsigned int mbus_flags;
+
 	unsigned int source_pad;
 	unsigned int sink_pad;
 };
@@ -146,7 +152,8 @@ struct rvin_info {
  * @v4l2_dev:		V4L2 device
  * @ctrl_handler:	V4L2 control handler
  * @notifier:		V4L2 asynchronous subdevs notifier
- * @parallel:		entity in the DT for local parallel subdevice
+ *
+ * @parallel:		parallel input subdevice descriptor
  *
  * @group:		Gen3 CSI group
  * @id:			Gen3 group id for this VIN
@@ -164,7 +171,8 @@ struct rvin_info {
  * @sequence:		V4L2 buffers sequence number
  * @state:		keeps track of operation state
  *
- * @mbus_cfg:		media bus configuration from DT
+ * @is_csi:		flag to mark the VIN as using a CSI-2 subdevice
+ *
  * @mbus_code:		media bus format code
  * @format:		active V4L2 pixel format
  *
@@ -182,7 +190,8 @@ struct rvin_dev {
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_async_notifier notifier;
-	struct rvin_graph_entity *parallel;
+
+	struct rvin_parallel_entity *parallel;
 
 	struct rvin_group *group;
 	unsigned int id;
@@ -199,7 +208,8 @@ struct rvin_dev {
 	unsigned int sequence;
 	enum rvin_dma_state state;
 
-	struct v4l2_mbus_config mbus_cfg;
+	bool is_csi;
+
 	u32 mbus_code;
 	struct v4l2_pix_format format;
 
-- 
2.7.4

[PATCH v3 5/9] media: rcar-vin: Parse parallel input on Gen3

[Jacopo Mondi]

The rcar-vin driver so far had a mutually exclusive code path for
handling parallel and CSI-2 video input subdevices, with only the CSI-2
use case supporting media-controller. As we add support for parallel
inputs to Gen3 media-controller compliant code path now parse both port@0
and port@1, handling the media-controller use case in the parallel
bound/unbind notifier operations and delay notifier registration to the
last probing VIN instance in case we have links to setup between media
entities.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 0a35a98..745e8ee 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -397,6 +397,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
 	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
 
+	vin->parallel->subdev = subdev;
+
+	if (vin->info->use_mc)
+		return 0;
+
 	/* Find compatible subdevices mbus format */
 	vin->mbus_code = 0;
 	code.index = 0;
@@ -422,46 +427,52 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 	if (!vin->mbus_code) {
 		vin_err(vin, "Unsupported media bus format for %s\n",
 			subdev->name);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto error_reset_subdev;
 	}
 
 	/* Read tvnorms */
 	ret = v4l2_subdev_call(subdev, video, g_tvnorms, &vin->vdev.tvnorms);
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
+		goto error_reset_subdev;
 
 	/* Read standard */
 	vin->std = V4L2_STD_UNKNOWN;
 	ret = v4l2_subdev_call(subdev, video, g_std, &vin->std);
 	if (ret < 0 && ret != -ENOIOCTLCMD)
-		return ret;
+		goto error_reset_subdev;
 
 	/* Add the controls */
 	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
 	if (ret < 0)
-		return ret;
+		goto error_reset_subdev;
 
 	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
 				    NULL);
 	if (ret < 0) {
 		v4l2_ctrl_handler_free(&vin->ctrl_handler);
-		return ret;
+		goto error_reset_subdev;
 	}
 
 	vin->vdev.ctrl_handler = &vin->ctrl_handler;
 
-	vin->parallel->subdev = subdev;
-
 	return 0;
+
+error_reset_subdev:
+	vin->parallel->subdev = NULL;
+
+	return ret;
 }
 
 static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 {
 	rvin_v4l2_unregister(vin);
-	v4l2_ctrl_handler_free(&vin->ctrl_handler);
-
-	vin->vdev.ctrl_handler = NULL;
 	vin->parallel->subdev = NULL;
+
+	if (!vin->info->use_mc) {
+		v4l2_ctrl_handler_free(&vin->ctrl_handler);
+		vin->vdev.ctrl_handler = NULL;
+	}
 }
 
 static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
@@ -551,22 +562,26 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
 	return 0;
 }
 
-static int rvin_parallel_graph_init(struct rvin_dev *vin)
+static int rvin_parallel_init(struct rvin_dev *vin)
 {
 	int ret;
 
-	ret = v4l2_async_notifier_parse_fwnode_endpoints(
-		vin->dev, &vin->notifier,
-		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
+	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
+		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
+		0, rvin_parallel_parse_v4l2);
 	if (ret)
 		return ret;
 
 	if (!vin->parallel)
-		return -ENODEV;
+		return -ENOTCONN;
 
 	vin_dbg(vin, "Found parallel subdevice %pOF\n",
 		to_of_node(vin->parallel->asd.match.fwnode));
 
+	/* If we use media-controller, notifier registration is post-poned. */
+	if (vin->info->use_mc)
+		return 0;
+
 	vin->notifier.ops = &rvin_parallel_notify_ops;
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
 	if (ret < 0) {
@@ -766,6 +781,30 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 
 	mutex_unlock(&vin->group->lock);
 
+	/*
+	 * Go and register all notifiers for parallel subdevs, and
+	 * the group notifier for CSI-2 subdevs, if any.
+	 */
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		struct rvin_dev *ivin = vin->group->vin[i];
+		struct v4l2_async_notifier *inotifier;
+
+		if (!ivin)
+			continue;
+
+		inotifier = &ivin->notifier;
+		if (!inotifier->num_subdevs)
+			continue;
+
+		inotifier->ops = &rvin_parallel_notify_ops;
+		ret = v4l2_async_notifier_register(&ivin->v4l2_dev, inotifier);
+		if (ret < 0) {
+			vin_err(ivin,
+				"Notifier registration failed: %d\n", ret);
+			goto error_unregister_notifiers;
+		}
+	}
+
 	if (!vin->group->notifier.num_subdevs)
 		return 0;
 
@@ -773,25 +812,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
 					   &vin->group->notifier);
 	if (ret < 0) {
-		vin_err(vin, "Notifier registration failed\n");
+		vin_err(vin, "Notifier registration failed: %d\n", ret);
 		return ret;
 	}
 
 	return 0;
+
+error_unregister_notifiers:
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		if (!vin->group->vin[i])
+			continue;
+
+		v4l2_async_notifier_unregister(&vin->group->vin[i]->notifier);
+	}
+
+	return ret;
 }
 
 static int rvin_mc_init(struct rvin_dev *vin)
 {
 	int ret;
 
-	vin->pad.flags = MEDIA_PAD_FL_SINK;
-	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
-	if (ret)
-		return ret;
-
-	ret = rvin_group_get(vin);
-	if (ret)
-		return ret;
+	if (!vin->info->use_mc)
+		return 0;
 
 	ret = rvin_mc_parse_of_graph(vin);
 	if (ret)
@@ -1074,13 +1117,26 @@ static int rcar_vin_probe(struct platform_device *pdev)
 		return ret;
 
 	platform_set_drvdata(pdev, vin);
-	if (vin->info->use_mc)
-		ret = rvin_mc_init(vin);
-	else
-		ret = rvin_parallel_graph_init(vin);
-	if (ret < 0)
+
+	if (vin->info->use_mc) {
+		vin->pad.flags = MEDIA_PAD_FL_SINK;
+		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
+		if (ret)
+			return ret;
+
+		ret = rvin_group_get(vin);
+		if (ret)
+			return ret;
+	}
+
+	ret = rvin_parallel_init(vin);
+	if (ret < 0 && ret != -ENOTCONN)
 		goto error;
 
+	ret = rvin_mc_init(vin);
+	if (ret)
+		return ret;
+
 	pm_suspend_ignore_children(&pdev->dev, true);
 	pm_runtime_enable(&pdev->dev);
 
-- 
2.7.4

[PATCH v3 6/9] media: rcar-vin: Link parallel input media entities

[Jacopo Mondi]

When running with media-controller link the parallel input
media entities with the VIN entities at 'complete' callback time.

To create media links the v4l2_device should be registered first.
Check if the device is already registered, to avoid double registrations.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 745e8ee..d13bbcf 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -478,6 +478,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct media_entity *source;
+	struct media_entity *sink;
 	int ret;
 
 	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
@@ -486,7 +488,26 @@ static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
 		return ret;
 	}
 
-	return rvin_v4l2_register(vin);
+	if (!video_is_registered(&vin->vdev)) {
+		ret = rvin_v4l2_register(vin);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (!vin->info->use_mc)
+		return 0;
+
+	/* If we're running with media-controller, link the subdevs. */
+	source = &vin->parallel->subdev->entity;
+	sink = &vin->vdev.entity;
+
+	ret = media_create_pad_link(source, vin->parallel->source_pad,
+				    sink, vin->parallel->sink_pad, 0);
+	if (ret)
+		vin_err(vin, "Error adding link from %s to %s: %d\n",
+			source->name, sink->name, ret);
+
+	return ret;
 }
 
 static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
@@ -611,7 +632,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 
 	/* Register all video nodes for the group. */
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
-		if (vin->group->vin[i]) {
+		if (vin->group->vin[i] &&
+		    !video_is_registered(&vin->group->vin[i]->vdev)) {
 			ret = rvin_v4l2_register(vin->group->vin[i]);
 			if (ret)
 				return ret;
-- 
2.7.4

[PATCH v3 7/9] media: rcar-vin: Handle parallel subdev in link_notify

[Jacopo Mondi]

Handle parallel subdevices in link_notify callback. If the notified link
involves a parallel subdevice, do not change routing of the VIN-CSI-2
devices and mark the VIN instance as using a parallel input. If the
CSI-2 link setup succeeds instead, mark the VIN instance as using CSI-2.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d13bbcf..dcebb42 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 parallel input of one of the enabled VINs if it is not
+		 * one of the CSI-2 subdevices.
+		 *
+		 * No hardware configuration required for parallel 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]->parallel &&
+			    group->vin[i]->parallel->subdev == sd) {
+				group->vin[i]->is_csi = false;
+				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) {
@@ -181,6 +208,11 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
 
 	/* New valid CHSEL found, set the new value. */
 	ret = rvin_set_channel_routing(group->vin[master_id], __ffs(mask_new));
+	if (ret)
+		goto out;
+
+	vin->is_csi = true;
+
 out:
 	mutex_unlock(&group->lock);
 
-- 
2.7.4

[PATCH v3 8/9] media: rcar-vin: Rename _rcar_info to rcar_info

[Jacopo Mondi]

Remove trailing underscore to align all rcar_group_route structure
declarations.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index dcebb42..b740f41 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1060,7 +1060,7 @@ static const struct rvin_info rcar_info_r8a7796 = {
 	.routes = rcar_info_r8a7796_routes,
 };
 
-static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
+static const struct rvin_group_route rcar_info_r8a77970_routes[] = {
 	{ .csi = RVIN_CSI40, .channel = 0, .vin = 0, .mask = BIT(0) | BIT(3) },
 	{ .csi = RVIN_CSI40, .channel = 0, .vin = 1, .mask = BIT(2) },
 	{ .csi = RVIN_CSI40, .channel = 1, .vin = 1, .mask = BIT(3) },
@@ -1076,7 +1076,7 @@ static const struct rvin_info rcar_info_r8a77970 = {
 	.use_mc = true,
 	.max_width = 4096,
 	.max_height = 4096,
-	.routes = _rcar_info_r8a77970_routes,
+	.routes = rcar_info_r8a77970_routes,
 };
 
 static const struct of_device_id rvin_of_id_table[] = {
-- 
2.7.4

[PATCH v3 9/9] 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>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
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 b740f41..0e088d9 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1079,6 +1079,18 @@ static const struct rvin_info rcar_info_r8a77970 = {
 	.routes = rcar_info_r8a77970_routes,
 };
 
+static const struct rvin_group_route rcar_info_r8a77995_routes[] = {
+	{ /* Sentinel */ }
+};
+
+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",
@@ -1120,6 +1132,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

Re: [PATCH v3 8/9] media: rcar-vin: Rename _rcar_info to rcar_info

[Sergei Shtylyov]

Hello!

On 5/18/2018 5:40 PM, Jacopo Mondi wrote:

> Remove trailing underscore to align all rcar_group_route structure

    Leading, not trailing (judging on the patch).

> declarations.
>  Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>   drivers/media/platform/rcar-vin/rcar-core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index dcebb42..b740f41 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -1060,7 +1060,7 @@ static const struct rvin_info rcar_info_r8a7796 = {
>   	.routes = rcar_info_r8a7796_routes,
>   };
>   
> -static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
> +static const struct rvin_group_route rcar_info_r8a77970_routes[] = {
>   	{ .csi = RVIN_CSI40, .channel = 0, .vin = 0, .mask = BIT(0) | BIT(3) },
>   	{ .csi = RVIN_CSI40, .channel = 0, .vin = 1, .mask = BIT(2) },
>   	{ .csi = RVIN_CSI40, .channel = 1, .vin = 1, .mask = BIT(3) },
> @@ -1076,7 +1076,7 @@ static const struct rvin_info rcar_info_r8a77970 = {
>   	.use_mc = true,
>   	.max_width = 4096,
>   	.max_height = 4096,
> -	.routes = _rcar_info_r8a77970_routes,
> +	.routes = rcar_info_r8a77970_routes,
>   };
>   
>   static const struct of_device_id rvin_of_id_table[] = {

MBR, Sergei

Re: [PATCH v3 1/9] media: rcar-vin: Rename 'digital' to 'parallel'

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:37 +0200, Jacopo Mondi wrote:
> As the term 'digital' is used all over the rcar-vin code in place of
> 'parallel', rename all the occurrencies.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 72 ++++++++++++++---------------
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  4 +-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++---
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  6 +--
>  4 files changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d3072e1..6b80f98 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -376,12 +376,12 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
>  }
>  
>  /* -----------------------------------------------------------------------------
> - * Digital async notifier
> + * Parallel async notifier
>   */
>  
>  /* The vin lock should be held when calling the subdevice attach and detach */
> -static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> -					 struct v4l2_subdev *subdev)
> +static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> +					  struct v4l2_subdev *subdev)
>  {
>  	struct v4l2_subdev_mbus_code_enum code = {
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> @@ -392,15 +392,15 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
>  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
>  	if (ret < 0)
>  		return ret;
> -	vin->digital->source_pad = ret;
> +	vin->parallel->source_pad = ret;
>  
>  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> -	vin->digital->sink_pad = ret < 0 ? 0 : ret;
> +	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
>  
>  	/* Find compatible subdevices mbus format */
>  	vin->mbus_code = 0;
>  	code.index = 0;
> -	code.pad = vin->digital->source_pad;
> +	code.pad = vin->parallel->source_pad;
>  	while (!vin->mbus_code &&
>  	       !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
>  		code.index++;
> @@ -450,21 +450,21 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
>  
>  	vin->vdev.ctrl_handler = &vin->ctrl_handler;
>  
> -	vin->digital->subdev = subdev;
> +	vin->parallel->subdev = subdev;
>  
>  	return 0;
>  }
>  
> -static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
> +static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  {
>  	rvin_v4l2_unregister(vin);
>  	v4l2_ctrl_handler_free(&vin->ctrl_handler);
>  
>  	vin->vdev.ctrl_handler = NULL;
> -	vin->digital->subdev = NULL;
> +	vin->parallel->subdev = NULL;
>  }
>  
> -static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> +static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
>  {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
>  	int ret;
> @@ -478,28 +478,28 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
>  	return rvin_v4l2_register(vin);
>  }
>  
> -static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> -				       struct v4l2_subdev *subdev,
> -				       struct v4l2_async_subdev *asd)
> +static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
> +				        struct v4l2_subdev *subdev,
> +				        struct v4l2_async_subdev *asd)

When I run my indentation script this indentation changes from spaces to 
all tabs. If possible I would like to keep that as I usually run it on 
these files before submitting any patches, but it's not a big deal.

Whit this fixed, thanks for clearing this up!

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

>  {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
>  
> -	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> +	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
>  
>  	mutex_lock(&vin->lock);
> -	rvin_digital_subdevice_detach(vin);
> +	rvin_parallel_subdevice_detach(vin);
>  	mutex_unlock(&vin->lock);
>  }
>  
> -static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> -				     struct v4l2_subdev *subdev,
> -				     struct v4l2_async_subdev *asd)
> +static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> +				      struct v4l2_subdev *subdev,
> +				      struct v4l2_async_subdev *asd)
>  {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
>  	int ret;
>  
>  	mutex_lock(&vin->lock);
> -	ret = rvin_digital_subdevice_attach(vin, subdev);
> +	ret = rvin_parallel_subdevice_attach(vin, subdev);
>  	mutex_unlock(&vin->lock);
>  	if (ret)
>  		return ret;
> @@ -507,21 +507,21 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
>  	v4l2_set_subdev_hostdata(subdev, vin);
>  
>  	vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
> -		subdev->name, vin->digital->source_pad,
> -		vin->digital->sink_pad);
> +		subdev->name, vin->parallel->source_pad,
> +		vin->parallel->sink_pad);
>  
>  	return 0;
>  }
>  
> -static const struct v4l2_async_notifier_operations rvin_digital_notify_ops = {
> -	.bound = rvin_digital_notify_bound,
> -	.unbind = rvin_digital_notify_unbind,
> -	.complete = rvin_digital_notify_complete,
> +static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> +	.bound = rvin_parallel_notify_bound,
> +	.unbind = rvin_parallel_notify_unbind,
> +	.complete = rvin_parallel_notify_complete,
>  };
>  
> -static int rvin_digital_parse_v4l2(struct device *dev,
> -				   struct v4l2_fwnode_endpoint *vep,
> -				   struct v4l2_async_subdev *asd)
> +static int rvin_parallel_parse_v4l2(struct device *dev,
> +				    struct v4l2_fwnode_endpoint *vep,
> +				    struct v4l2_async_subdev *asd)
>  {
>  	struct rvin_dev *vin = dev_get_drvdata(dev);
>  	struct rvin_graph_entity *rvge =
> @@ -546,28 +546,28 @@ static int rvin_digital_parse_v4l2(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	vin->digital = rvge;
> +	vin->parallel = rvge;
>  
>  	return 0;
>  }
>  
> -static int rvin_digital_graph_init(struct rvin_dev *vin)
> +static int rvin_parallel_graph_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
>  	ret = v4l2_async_notifier_parse_fwnode_endpoints(
>  		vin->dev, &vin->notifier,
> -		sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
> +		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
>  	if (ret)
>  		return ret;
>  
> -	if (!vin->digital)
> +	if (!vin->parallel)
>  		return -ENODEV;
>  
> -	vin_dbg(vin, "Found digital subdevice %pOF\n",
> -		to_of_node(vin->digital->asd.match.fwnode));
> +	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> +		to_of_node(vin->parallel->asd.match.fwnode));
>  
> -	vin->notifier.ops = &rvin_digital_notify_ops;
> +	vin->notifier.ops = &rvin_parallel_notify_ops;
>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
>  	if (ret < 0) {
>  		vin_err(vin, "Notifier registration failed\n");
> @@ -1088,7 +1088,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  	if (vin->info->use_mc)
>  		ret = rvin_mc_init(vin);
>  	else
> -		ret = rvin_digital_graph_init(vin);
> +		ret = rvin_parallel_graph_init(vin);
>  	if (ret < 0)
>  		goto error;
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index ac07f99..f1c3585 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -733,7 +733,7 @@ static int rvin_setup(struct rvin_dev *vin)
>  		vnmc |= VNMC_BPS;
>  
>  	if (vin->info->model == RCAR_GEN3) {
> -		/* Select between CSI-2 and Digital input */
> +		/* Select between CSI-2 and parallel input */
>  		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
>  			vnmc &= ~VNMC_DPINE;
>  		else
> @@ -1074,7 +1074,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
>  
>  	/* No media controller used, simply pass operation to subdevice. */
>  	if (!vin->info->use_mc) {
> -		ret = v4l2_subdev_call(vin->digital->subdev, video, s_stream,
> +		ret = v4l2_subdev_call(vin->parallel->subdev, video, s_stream,
>  				       on);
>  
>  		return ret == -ENOIOCTLCMD ? 0 : ret;
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index e78fba8..87a718b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -144,7 +144,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
>  {
>  	struct v4l2_subdev_format fmt = {
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> -		.pad = vin->digital->source_pad,
> +		.pad = vin->parallel->source_pad,
>  	};
>  	int ret;
>  
> @@ -175,7 +175,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>  	struct v4l2_subdev_pad_config *pad_cfg;
>  	struct v4l2_subdev_format format = {
>  		.which = which,
> -		.pad = vin->digital->source_pad,
> +		.pad = vin->parallel->source_pad,
>  	};
>  	enum v4l2_field field;
>  	u32 width, height;
> @@ -517,7 +517,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
>  	if (timings->pad)
>  		return -EINVAL;
>  
> -	timings->pad = vin->digital->sink_pad;
> +	timings->pad = vin->parallel->sink_pad;
>  
>  	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
>  
> @@ -569,7 +569,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
>  	if (cap->pad)
>  		return -EINVAL;
>  
> -	cap->pad = vin->digital->sink_pad;
> +	cap->pad = vin->parallel->sink_pad;
>  
>  	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
>  
> @@ -587,7 +587,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>  	if (edid->pad)
>  		return -EINVAL;
>  
> -	edid->pad = vin->digital->sink_pad;
> +	edid->pad = vin->parallel->sink_pad;
>  
>  	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
>  
> @@ -605,7 +605,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>  	if (edid->pad)
>  		return -EINVAL;
>  
> -	edid->pad = vin->digital->sink_pad;
> +	edid->pad = vin->parallel->sink_pad;
>  
>  	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index c2aef78..755ac3c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -146,7 +146,7 @@ struct rvin_info {
>   * @v4l2_dev:		V4L2 device
>   * @ctrl_handler:	V4L2 control handler
>   * @notifier:		V4L2 asynchronous subdevs notifier
> - * @digital:		entity in the DT for local digital subdevice
> + * @parallel:		entity in the DT for local parallel subdevice
>   *
>   * @group:		Gen3 CSI group
>   * @id:			Gen3 group id for this VIN
> @@ -182,7 +182,7 @@ struct rvin_dev {
>  	struct v4l2_device v4l2_dev;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_async_notifier notifier;
> -	struct rvin_graph_entity *digital;
> +	struct rvin_graph_entity *parallel;
>  
>  	struct rvin_group *group;
>  	unsigned int id;
> @@ -209,7 +209,7 @@ struct rvin_dev {
>  	v4l2_std_id std;
>  };
>  
> -#define vin_to_source(vin)		((vin)->digital->subdev)
> +#define vin_to_source(vin)		((vin)->parallel->subdev)
>  
>  /* Debug */
>  #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 1/9] media: rcar-vin: Rename 'digital' to 'parallel'

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:37 +0200, Jacopo Mondi wrote:
> As the term 'digital' is used all over the rcar-vin code in place of
> 'parallel', rename all the occurrencies.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 72 ++++++++++++++---------------
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  4 +-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++---
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  6 +--
>  4 files changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d3072e1..6b80f98 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -376,12 +376,12 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
>  }
>  
>  /* -----------------------------------------------------------------------------
> - * Digital async notifier
> + * Parallel async notifier
>   */
>  
>  /* The vin lock should be held when calling the subdevice attach and detach */
> -static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> -					 struct v4l2_subdev *subdev)
> +static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> +					  struct v4l2_subdev *subdev)
>  {
>  	struct v4l2_subdev_mbus_code_enum code = {
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> @@ -392,15 +392,15 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
>  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
>  	if (ret < 0)
>  		return ret;
> -	vin->digital->source_pad = ret;
> +	vin->parallel->source_pad = ret;
>  
>  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> -	vin->digital->sink_pad = ret < 0 ? 0 : ret;
> +	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
>  
>  	/* Find compatible subdevices mbus format */
>  	vin->mbus_code = 0;
>  	code.index = 0;
> -	code.pad = vin->digital->source_pad;
> +	code.pad = vin->parallel->source_pad;
>  	while (!vin->mbus_code &&
>  	       !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
>  		code.index++;
> @@ -450,21 +450,21 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
>  
>  	vin->vdev.ctrl_handler = &vin->ctrl_handler;
>  
> -	vin->digital->subdev = subdev;
> +	vin->parallel->subdev = subdev;
>  
>  	return 0;
>  }
>  
> -static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
> +static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  {
>  	rvin_v4l2_unregister(vin);
>  	v4l2_ctrl_handler_free(&vin->ctrl_handler);
>  
>  	vin->vdev.ctrl_handler = NULL;
> -	vin->digital->subdev = NULL;
> +	vin->parallel->subdev = NULL;
>  }
>  
> -static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> +static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
>  {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
>  	int ret;
> @@ -478,28 +478,28 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
>  	return rvin_v4l2_register(vin);
>  }
>  
> -static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> -				       struct v4l2_subdev *subdev,
> -				       struct v4l2_async_subdev *asd)
> +static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
> +				        struct v4l2_subdev *subdev,
> +				        struct v4l2_async_subdev *asd)

When I run my indentation script this indentation changes from spaces to 
all tabs. If possible I would like to keep that as I usually run it on 
these files before submitting any patches, but it's not a big deal.

Whit this fixed, thanks for clearing this up!

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

>  {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
>  
> -	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> +	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
>  
>  	mutex_lock(&vin->lock);
> -	rvin_digital_subdevice_detach(vin);
> +	rvin_parallel_subdevice_detach(vin);
>  	mutex_unlock(&vin->lock);
>  }
>  
> -static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> -				     struct v4l2_subdev *subdev,
> -				     struct v4l2_async_subdev *asd)
> +static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> +				      struct v4l2_subdev *subdev,
> +				      struct v4l2_async_subdev *asd)
>  {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
>  	int ret;
>  
>  	mutex_lock(&vin->lock);
> -	ret = rvin_digital_subdevice_attach(vin, subdev);
> +	ret = rvin_parallel_subdevice_attach(vin, subdev);
>  	mutex_unlock(&vin->lock);
>  	if (ret)
>  		return ret;
> @@ -507,21 +507,21 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
>  	v4l2_set_subdev_hostdata(subdev, vin);
>  
>  	vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
> -		subdev->name, vin->digital->source_pad,
> -		vin->digital->sink_pad);
> +		subdev->name, vin->parallel->source_pad,
> +		vin->parallel->sink_pad);
>  
>  	return 0;
>  }
>  
> -static const struct v4l2_async_notifier_operations rvin_digital_notify_ops = {
> -	.bound = rvin_digital_notify_bound,
> -	.unbind = rvin_digital_notify_unbind,
> -	.complete = rvin_digital_notify_complete,
> +static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> +	.bound = rvin_parallel_notify_bound,
> +	.unbind = rvin_parallel_notify_unbind,
> +	.complete = rvin_parallel_notify_complete,
>  };
>  
> -static int rvin_digital_parse_v4l2(struct device *dev,
> -				   struct v4l2_fwnode_endpoint *vep,
> -				   struct v4l2_async_subdev *asd)
> +static int rvin_parallel_parse_v4l2(struct device *dev,
> +				    struct v4l2_fwnode_endpoint *vep,
> +				    struct v4l2_async_subdev *asd)
>  {
>  	struct rvin_dev *vin = dev_get_drvdata(dev);
>  	struct rvin_graph_entity *rvge =
> @@ -546,28 +546,28 @@ static int rvin_digital_parse_v4l2(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	vin->digital = rvge;
> +	vin->parallel = rvge;
>  
>  	return 0;
>  }
>  
> -static int rvin_digital_graph_init(struct rvin_dev *vin)
> +static int rvin_parallel_graph_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
>  	ret = v4l2_async_notifier_parse_fwnode_endpoints(
>  		vin->dev, &vin->notifier,
> -		sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
> +		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
>  	if (ret)
>  		return ret;
>  
> -	if (!vin->digital)
> +	if (!vin->parallel)
>  		return -ENODEV;
>  
> -	vin_dbg(vin, "Found digital subdevice %pOF\n",
> -		to_of_node(vin->digital->asd.match.fwnode));
> +	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> +		to_of_node(vin->parallel->asd.match.fwnode));
>  
> -	vin->notifier.ops = &rvin_digital_notify_ops;
> +	vin->notifier.ops = &rvin_parallel_notify_ops;
>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
>  	if (ret < 0) {
>  		vin_err(vin, "Notifier registration failed\n");
> @@ -1088,7 +1088,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  	if (vin->info->use_mc)
>  		ret = rvin_mc_init(vin);
>  	else
> -		ret = rvin_digital_graph_init(vin);
> +		ret = rvin_parallel_graph_init(vin);
>  	if (ret < 0)
>  		goto error;
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index ac07f99..f1c3585 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -733,7 +733,7 @@ static int rvin_setup(struct rvin_dev *vin)
>  		vnmc |= VNMC_BPS;
>  
>  	if (vin->info->model == RCAR_GEN3) {
> -		/* Select between CSI-2 and Digital input */
> +		/* Select between CSI-2 and parallel input */
>  		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
>  			vnmc &= ~VNMC_DPINE;
>  		else
> @@ -1074,7 +1074,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
>  
>  	/* No media controller used, simply pass operation to subdevice. */
>  	if (!vin->info->use_mc) {
> -		ret = v4l2_subdev_call(vin->digital->subdev, video, s_stream,
> +		ret = v4l2_subdev_call(vin->parallel->subdev, video, s_stream,
>  				       on);
>  
>  		return ret == -ENOIOCTLCMD ? 0 : ret;
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index e78fba8..87a718b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -144,7 +144,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
>  {
>  	struct v4l2_subdev_format fmt = {
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> -		.pad = vin->digital->source_pad,
> +		.pad = vin->parallel->source_pad,
>  	};
>  	int ret;
>  
> @@ -175,7 +175,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
>  	struct v4l2_subdev_pad_config *pad_cfg;
>  	struct v4l2_subdev_format format = {
>  		.which = which,
> -		.pad = vin->digital->source_pad,
> +		.pad = vin->parallel->source_pad,
>  	};
>  	enum v4l2_field field;
>  	u32 width, height;
> @@ -517,7 +517,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
>  	if (timings->pad)
>  		return -EINVAL;
>  
> -	timings->pad = vin->digital->sink_pad;
> +	timings->pad = vin->parallel->sink_pad;
>  
>  	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
>  
> @@ -569,7 +569,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
>  	if (cap->pad)
>  		return -EINVAL;
>  
> -	cap->pad = vin->digital->sink_pad;
> +	cap->pad = vin->parallel->sink_pad;
>  
>  	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
>  
> @@ -587,7 +587,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>  	if (edid->pad)
>  		return -EINVAL;
>  
> -	edid->pad = vin->digital->sink_pad;
> +	edid->pad = vin->parallel->sink_pad;
>  
>  	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
>  
> @@ -605,7 +605,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>  	if (edid->pad)
>  		return -EINVAL;
>  
> -	edid->pad = vin->digital->sink_pad;
> +	edid->pad = vin->parallel->sink_pad;
>  
>  	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index c2aef78..755ac3c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -146,7 +146,7 @@ struct rvin_info {
>   * @v4l2_dev:		V4L2 device
>   * @ctrl_handler:	V4L2 control handler
>   * @notifier:		V4L2 asynchronous subdevs notifier
> - * @digital:		entity in the DT for local digital subdevice
> + * @parallel:		entity in the DT for local parallel subdevice
>   *
>   * @group:		Gen3 CSI group
>   * @id:			Gen3 group id for this VIN
> @@ -182,7 +182,7 @@ struct rvin_dev {
>  	struct v4l2_device v4l2_dev;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_async_notifier notifier;
> -	struct rvin_graph_entity *digital;
> +	struct rvin_graph_entity *parallel;
>  
>  	struct rvin_group *group;
>  	unsigned int id;
> @@ -209,7 +209,7 @@ struct rvin_dev {
>  	v4l2_std_id std;
>  };
>  
> -#define vin_to_source(vin)		((vin)->digital->subdev)
> +#define vin_to_source(vin)		((vin)->parallel->subdev)
>  
>  /* Debug */
>  #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v3 2/9] media: rcar-vin: Remove two empty lines

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-18 16:40:38 +0200, Jacopo Mondi wrote:
> Remove un-necessary empty lines.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 6b80f98..1aadd90 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -707,11 +707,9 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  		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) {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 2/9] media: rcar-vin: Remove two empty lines

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-18 16:40:38 +0200, Jacopo Mondi wrote:
> Remove un-necessary empty lines.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 6b80f98..1aadd90 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -707,11 +707,9 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  		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) {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:39 +0200, Jacopo Mondi wrote:
> As CSI-2 subdevices are shared between several VIN instances, a shared
> notifier to collect the CSI-2 async subdevices is required. So far, the
> rcar-vin driver used the notifier of the last VIN instance to probe but
> with the forth-coming introduction of parallel input subdevices support
> in mc-compliant code path, each VIN may register its own notifier if any
> parallel subdevice is connected there.
> 
> To avoid registering a notifier twice (once for parallel subdev and one
> for the CSI-2 subdevs) create a group notifier, shared by all the VIN
> instances.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 39 +++++++++++------------------
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  7 +++---
>  2 files changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 1aadd90..c6e603f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -583,7 +583,7 @@ static int rvin_parallel_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 *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>  	const struct rvin_group_route *route;
>  	unsigned int i;
>  	int ret;
> @@ -649,7 +649,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>  				     struct v4l2_subdev *subdev,
>  				     struct v4l2_async_subdev *asd)
>  {
> -	struct rvin_dev *vin = notifier_to_vin(notifier);
> +	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>  	unsigned int i;
>  
>  	for (i = 0; i < RCAR_VIN_NUM; i++)
> @@ -673,7 +673,7 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_subdev *subdev,
>  				   struct v4l2_async_subdev *asd)
>  {
> -	struct rvin_dev *vin = notifier_to_vin(notifier);
> +	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>  	unsigned int i;
>  
>  	mutex_lock(&vin->group->lock);
> @@ -734,12 +734,6 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  
>  	mutex_lock(&vin->group->lock);
>  
> -	/* If there already is a notifier something has gone wrong, bail out. */
> -	if (WARN_ON(vin->group->notifier)) {
> -		mutex_unlock(&vin->group->lock);
> -		return -EINVAL;
> -	}
> -
>  	/* If not all VIN's are registered don't register the notifier. */
>  	for (i = 0; i < RCAR_VIN_NUM; i++)
>  		if (vin->group->vin[i])
> @@ -751,19 +745,16 @@ 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 CSI-2 subdevices. Some subdevices will
> +	 * overlap but the parser function can handle it, so each subdevice
> +	 * will only be registered once with the group notifier.
>  	 */
> -
> -	vin->group->notifier = &vin->notifier;
> -
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
>  		if (!vin->group->vin[i])
>  			continue;
>  
>  		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> -				vin->group->vin[i]->dev, vin->group->notifier,
> +				vin->group->vin[i]->dev, &vin->group->notifier,
>  				sizeof(struct v4l2_async_subdev), 1,
>  				rvin_mc_parse_of_endpoint);
>  		if (ret) {
> @@ -774,9 +765,12 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  
>  	mutex_unlock(&vin->group->lock);
>  
> -	vin->group->notifier->ops = &rvin_group_notify_ops;
> +	if (!vin->group->notifier.num_subdevs)
> +		return 0;
>  
> -	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
> +	vin->group->notifier.ops = &rvin_group_notify_ops;
> +	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
> +					   &vin->group->notifier);
>  	if (ret < 0) {
>  		vin_err(vin, "Notifier registration failed\n");
>  		return ret;
> @@ -1112,15 +1106,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
>  	v4l2_async_notifier_unregister(&vin->notifier);
>  	v4l2_async_notifier_cleanup(&vin->notifier);

If the vin being removed is the one which v4l2_dev is used to register 
the group you should unregister the group notifier here. You got this 
for 'free' with the above code before this change so it's easy to miss 
:-)

>  
> -	if (vin->info->use_mc) {
> -		mutex_lock(&vin->group->lock);
> -		if (vin->group->notifier == &vin->notifier)
> -			vin->group->notifier = NULL;
> -		mutex_unlock(&vin->group->lock);
> +	if (vin->info->use_mc)
>  		rvin_group_put(vin);
> -	} else {
> +	else
>  		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -	}
>  
>  	rvin_dma_unregister(vin);
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 755ac3c..7d0ffe08 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -209,6 +209,8 @@ struct rvin_dev {
>  	v4l2_std_id std;
>  };
>  
> +#define v4l2_dev_to_vin(d)	container_of(d, struct rvin_dev, v4l2_dev)

I think you should either define this in rcar-core.c (see 
notifier_to_vin()) or use the container_of() directly to limit it's 
scope.

Over all I'm not happy about my initial usage these defines in the 
driver, I was young and dumb :-) I won't object if you wish to keep it 
but then please move it to the .c file.

> +
>  #define vin_to_source(vin)		((vin)->parallel->subdev)

This in particular I hate and at some point I hope to remove it or move 
it to rcar-v4l2.c. :-) But that is a task for later and not related to 
your patch-set.

>  
>  /* Debug */
> @@ -225,8 +227,7 @@ struct rvin_dev {
>   *
>   * @lock:		protects the count, notifier, vin and csi members
>   * @count:		number of enabled VIN instances found in DT
> - * @notifier:		pointer to the notifier of a VIN which handles the
> - *			groups async sub-devices.
> + * @notifier:		group notifier for CSI-2 async subdevices
>   * @vin:		VIN instances which are part of the group
>   * @csi:		array of pairs of fwnode and subdev pointers
>   *			to all CSI-2 subdevices.
> @@ -238,7 +239,7 @@ struct rvin_group {
>  
>  	struct mutex lock;
>  	unsigned int count;
> -	struct v4l2_async_notifier *notifier;
> +	struct v4l2_async_notifier notifier;
>  	struct rvin_dev *vin[RCAR_VIN_NUM];
>  
>  	struct {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:39 +0200, Jacopo Mondi wrote:
> As CSI-2 subdevices are shared between several VIN instances, a shared
> notifier to collect the CSI-2 async subdevices is required. So far, the
> rcar-vin driver used the notifier of the last VIN instance to probe but
> with the forth-coming introduction of parallel input subdevices support
> in mc-compliant code path, each VIN may register its own notifier if any
> parallel subdevice is connected there.
> 
> To avoid registering a notifier twice (once for parallel subdev and one
> for the CSI-2 subdevs) create a group notifier, shared by all the VIN
> instances.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 39 +++++++++++------------------
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  7 +++---
>  2 files changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 1aadd90..c6e603f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -583,7 +583,7 @@ static int rvin_parallel_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 *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>  	const struct rvin_group_route *route;
>  	unsigned int i;
>  	int ret;
> @@ -649,7 +649,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>  				     struct v4l2_subdev *subdev,
>  				     struct v4l2_async_subdev *asd)
>  {
> -	struct rvin_dev *vin = notifier_to_vin(notifier);
> +	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>  	unsigned int i;
>  
>  	for (i = 0; i < RCAR_VIN_NUM; i++)
> @@ -673,7 +673,7 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_subdev *subdev,
>  				   struct v4l2_async_subdev *asd)
>  {
> -	struct rvin_dev *vin = notifier_to_vin(notifier);
> +	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>  	unsigned int i;
>  
>  	mutex_lock(&vin->group->lock);
> @@ -734,12 +734,6 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  
>  	mutex_lock(&vin->group->lock);
>  
> -	/* If there already is a notifier something has gone wrong, bail out. */
> -	if (WARN_ON(vin->group->notifier)) {
> -		mutex_unlock(&vin->group->lock);
> -		return -EINVAL;
> -	}
> -
>  	/* If not all VIN's are registered don't register the notifier. */
>  	for (i = 0; i < RCAR_VIN_NUM; i++)
>  		if (vin->group->vin[i])
> @@ -751,19 +745,16 @@ 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 CSI-2 subdevices. Some subdevices will
> +	 * overlap but the parser function can handle it, so each subdevice
> +	 * will only be registered once with the group notifier.
>  	 */
> -
> -	vin->group->notifier = &vin->notifier;
> -
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
>  		if (!vin->group->vin[i])
>  			continue;
>  
>  		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> -				vin->group->vin[i]->dev, vin->group->notifier,
> +				vin->group->vin[i]->dev, &vin->group->notifier,
>  				sizeof(struct v4l2_async_subdev), 1,
>  				rvin_mc_parse_of_endpoint);
>  		if (ret) {
> @@ -774,9 +765,12 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  
>  	mutex_unlock(&vin->group->lock);
>  
> -	vin->group->notifier->ops = &rvin_group_notify_ops;
> +	if (!vin->group->notifier.num_subdevs)
> +		return 0;
>  
> -	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
> +	vin->group->notifier.ops = &rvin_group_notify_ops;
> +	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
> +					   &vin->group->notifier);
>  	if (ret < 0) {
>  		vin_err(vin, "Notifier registration failed\n");
>  		return ret;
> @@ -1112,15 +1106,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
>  	v4l2_async_notifier_unregister(&vin->notifier);
>  	v4l2_async_notifier_cleanup(&vin->notifier);

If the vin being removed is the one which v4l2_dev is used to register 
the group you should unregister the group notifier here. You got this 
for 'free' with the above code before this change so it's easy to miss 
:-)

>  
> -	if (vin->info->use_mc) {
> -		mutex_lock(&vin->group->lock);
> -		if (vin->group->notifier == &vin->notifier)
> -			vin->group->notifier = NULL;
> -		mutex_unlock(&vin->group->lock);
> +	if (vin->info->use_mc)
>  		rvin_group_put(vin);
> -	} else {
> +	else
>  		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -	}
>  
>  	rvin_dma_unregister(vin);
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 755ac3c..7d0ffe08 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -209,6 +209,8 @@ struct rvin_dev {
>  	v4l2_std_id std;
>  };
>  
> +#define v4l2_dev_to_vin(d)	container_of(d, struct rvin_dev, v4l2_dev)

I think you should either define this in rcar-core.c (see 
notifier_to_vin()) or use the container_of() directly to limit it's 
scope.

Over all I'm not happy about my initial usage these defines in the 
driver, I was young and dumb :-) I won't object if you wish to keep it 
but then please move it to the .c file.

> +
>  #define vin_to_source(vin)		((vin)->parallel->subdev)

This in particular I hate and at some point I hope to remove it or move 
it to rcar-v4l2.c. :-) But that is a task for later and not related to 
your patch-set.

>  
>  /* Debug */
> @@ -225,8 +227,7 @@ struct rvin_dev {
>   *
>   * @lock:		protects the count, notifier, vin and csi members
>   * @count:		number of enabled VIN instances found in DT
> - * @notifier:		pointer to the notifier of a VIN which handles the
> - *			groups async sub-devices.
> + * @notifier:		group notifier for CSI-2 async subdevices
>   * @vin:		VIN instances which are part of the group
>   * @csi:		array of pairs of fwnode and subdev pointers
>   *			to all CSI-2 subdevices.
> @@ -238,7 +239,7 @@ struct rvin_group {
>  
>  	struct mutex lock;
>  	unsigned int count;
> -	struct v4l2_async_notifier *notifier;
> +	struct v4l2_async_notifier notifier;
>  	struct rvin_dev *vin[RCAR_VIN_NUM];
>  
>  	struct {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v3 4/9] media: rcar-vin: Cache the mbus configuration flags

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:40 +0200, Jacopo Mondi wrote:
> Media bus configuration flags and media bus type were so far a property
> of each VIN instance, as the subdevice they were connected to was
> immutable during the whole system life time.
> 
> With the forth-coming introduction of parallel input devices support,
> a VIN instance can have the subdevice it is connected to switched at
> runtime, from a CSI-2 subdevice to a parallel one and viceversa, through
> the modification of links between media entities in the media controller
> graph. To avoid discarding the per-subdevice configuration flags retrieved by
> v4l2_fwnode parsing facilities, cache them in the 'rvin_graph_entity'
> member of each VIN instance, opportunely renamed to 'rvin_parallel_entity'.
> 
> Also modify the register configuration function to take mbus flags into
> account when running on a bus type that supports them.
> 
> The media bus type currently in use will be updated in a follow-up patch
> to the link state change notification function.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 23 ++++++++++-------------
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 24 +++++++++++++++++-------
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 22 ++++++++++++++++------
>  3 files changed, 43 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index c6e603f..0a35a98 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -524,30 +524,30 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
>  				    struct v4l2_async_subdev *asd)
>  {
>  	struct rvin_dev *vin = dev_get_drvdata(dev);
> -	struct rvin_graph_entity *rvge =
> -		container_of(asd, struct rvin_graph_entity, asd);
> +	struct rvin_parallel_entity *rvpe =
> +		container_of(asd, struct rvin_parallel_entity, asd);
>  
>  	if (vep->base.port || vep->base.id)
>  		return -ENOTCONN;
>  
> -	vin->mbus_cfg.type = vep->bus_type;
> +	vin->is_csi = false;

No need to initialize is_csi here, the link_notify() callback needs to 
be called before any streaming can happen and that will with your later 
patches in this series take care of setting up this flag.

> +	vin->parallel = rvpe;
> +	vin->parallel->mbus_type = vep->bus_type;
>  
> -	switch (vin->mbus_cfg.type) {
> +	switch (vin->parallel->mbus_type) {
>  	case V4L2_MBUS_PARALLEL:
>  		vin_dbg(vin, "Found PARALLEL media bus\n");
> -		vin->mbus_cfg.flags = vep->bus.parallel.flags;
> +		vin->parallel->mbus_flags = vep->bus.parallel.flags;
>  		break;
>  	case V4L2_MBUS_BT656:
>  		vin_dbg(vin, "Found BT656 media bus\n");
> -		vin->mbus_cfg.flags = 0;
> +		vin->parallel->mbus_flags = 0;
>  		break;
>  	default:
>  		vin_err(vin, "Unknown media bus type\n");
>  		return -EINVAL;
>  	}
>  
> -	vin->parallel = rvge;
> -
>  	return 0;
>  }
>  
> @@ -557,7 +557,7 @@ static int rvin_parallel_graph_init(struct rvin_dev *vin)
>  
>  	ret = v4l2_async_notifier_parse_fwnode_endpoints(
>  		vin->dev, &vin->notifier,
> -		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
> +		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
>  	if (ret)
>  		return ret;
>  
> @@ -718,6 +718,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  		return -ENOTCONN;
>  	}
>  
> +	vin->is_csi = true;

Same here, no need to set is_csi.

>  	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
>  
>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> @@ -783,10 +784,6 @@ 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-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index f1c3585..17f291f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -659,8 +659,12 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>  		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
> -		vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
> -			VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
> +		if (!vin->is_csi &&
> +		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
> +			vnmc |= VNMC_INF_YUV8_BT656;
> +		else
> +			vnmc |= VNMC_INF_YUV8_BT601;
> +
>  		input_is_yuv = true;
>  		break;
>  	case MEDIA_BUS_FMT_RGB888_1X24:
> @@ -668,8 +672,12 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	case MEDIA_BUS_FMT_UYVY10_2X10:
>  		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
> -		vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
> -			VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
> +		if (!vin->is_csi &&
> +		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
> +			vnmc |= VNMC_INF_YUV10_BT656;
> +		else
> +			vnmc |= VNMC_INF_YUV10_BT601;
> +
>  		input_is_yuv = true;
>  		break;
>  	default:
> @@ -683,11 +691,13 @@ static int rvin_setup(struct rvin_dev *vin)
>  		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
>  
>  	/* Hsync Signal Polarity Select */
> -	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> +	if (!vin->is_csi &&
> +	    !(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
>  		dmr2 |= VNDMR2_HPS;
>  
>  	/* Vsync Signal Polarity Select */
> -	if (!(vin->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> +	if (!vin->is_csi &&
> +	    !(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
>  		dmr2 |= VNDMR2_VPS;

Would it not make sens to have nested if-statements here?

    if (!vin->is_csi) {
        if (!(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
            ...

        ...
    }

>  
>  	/*
> @@ -734,7 +744,7 @@ static int rvin_setup(struct rvin_dev *vin)
>  
>  	if (vin->info->model == RCAR_GEN3) {
>  		/* Select between CSI-2 and parallel input */
> -		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
> +		if (vin->is_csi)
>  			vnmc &= ~VNMC_DPINE;
>  		else
>  			vnmc |= VNMC_DPINE;
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 7d0ffe08..c5f7fd1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -73,16 +73,22 @@ struct rvin_video_format {
>  };
>  
>  /**
> - * struct rvin_graph_entity - Video endpoint from async framework
> + * struct rvin_parallel_entity - Parallel video input endpoint descriptor
>   * @asd:	sub-device descriptor for async framework
>   * @subdev:	subdevice matched using async framework
> + * @mbus_type:	media bus type
> + * @mbus_flags:	media bus configuration flags
>   * @source_pad:	source pad of remote subdevice
>   * @sink_pad:	sink pad of remote subdevice
> + *
>   */
> -struct rvin_graph_entity {
> +struct rvin_parallel_entity {
>  	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *subdev;
>  
> +	enum v4l2_mbus_type mbus_type;
> +	unsigned int mbus_flags;
> +
>  	unsigned int source_pad;
>  	unsigned int sink_pad;
>  };
> @@ -146,7 +152,8 @@ struct rvin_info {
>   * @v4l2_dev:		V4L2 device
>   * @ctrl_handler:	V4L2 control handler
>   * @notifier:		V4L2 asynchronous subdevs notifier
> - * @parallel:		entity in the DT for local parallel subdevice
> + *
> + * @parallel:		parallel input subdevice descriptor
>   *
>   * @group:		Gen3 CSI group
>   * @id:			Gen3 group id for this VIN
> @@ -164,7 +171,8 @@ struct rvin_info {
>   * @sequence:		V4L2 buffers sequence number
>   * @state:		keeps track of operation state
>   *
> - * @mbus_cfg:		media bus configuration from DT
> + * @is_csi:		flag to mark the VIN as using a CSI-2 subdevice
> + *
>   * @mbus_code:		media bus format code
>   * @format:		active V4L2 pixel format
>   *
> @@ -182,7 +190,8 @@ struct rvin_dev {
>  	struct v4l2_device v4l2_dev;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_async_notifier notifier;
> -	struct rvin_graph_entity *parallel;
> +
> +	struct rvin_parallel_entity *parallel;
>  
>  	struct rvin_group *group;
>  	unsigned int id;
> @@ -199,7 +208,8 @@ struct rvin_dev {
>  	unsigned int sequence;
>  	enum rvin_dma_state state;
>  
> -	struct v4l2_mbus_config mbus_cfg;
> +	bool is_csi;
> +
>  	u32 mbus_code;
>  	struct v4l2_pix_format format;
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 4/9] media: rcar-vin: Cache the mbus configuration flags

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:40 +0200, Jacopo Mondi wrote:
> Media bus configuration flags and media bus type were so far a property
> of each VIN instance, as the subdevice they were connected to was
> immutable during the whole system life time.
> 
> With the forth-coming introduction of parallel input devices support,
> a VIN instance can have the subdevice it is connected to switched at
> runtime, from a CSI-2 subdevice to a parallel one and viceversa, through
> the modification of links between media entities in the media controller
> graph. To avoid discarding the per-subdevice configuration flags retrieved by
> v4l2_fwnode parsing facilities, cache them in the 'rvin_graph_entity'
> member of each VIN instance, opportunely renamed to 'rvin_parallel_entity'.
> 
> Also modify the register configuration function to take mbus flags into
> account when running on a bus type that supports them.
> 
> The media bus type currently in use will be updated in a follow-up patch
> to the link state change notification function.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 23 ++++++++++-------------
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 24 +++++++++++++++++-------
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 22 ++++++++++++++++------
>  3 files changed, 43 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index c6e603f..0a35a98 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -524,30 +524,30 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
>  				    struct v4l2_async_subdev *asd)
>  {
>  	struct rvin_dev *vin = dev_get_drvdata(dev);
> -	struct rvin_graph_entity *rvge =
> -		container_of(asd, struct rvin_graph_entity, asd);
> +	struct rvin_parallel_entity *rvpe =
> +		container_of(asd, struct rvin_parallel_entity, asd);
>  
>  	if (vep->base.port || vep->base.id)
>  		return -ENOTCONN;
>  
> -	vin->mbus_cfg.type = vep->bus_type;
> +	vin->is_csi = false;

No need to initialize is_csi here, the link_notify() callback needs to 
be called before any streaming can happen and that will with your later 
patches in this series take care of setting up this flag.

> +	vin->parallel = rvpe;
> +	vin->parallel->mbus_type = vep->bus_type;
>  
> -	switch (vin->mbus_cfg.type) {
> +	switch (vin->parallel->mbus_type) {
>  	case V4L2_MBUS_PARALLEL:
>  		vin_dbg(vin, "Found PARALLEL media bus\n");
> -		vin->mbus_cfg.flags = vep->bus.parallel.flags;
> +		vin->parallel->mbus_flags = vep->bus.parallel.flags;
>  		break;
>  	case V4L2_MBUS_BT656:
>  		vin_dbg(vin, "Found BT656 media bus\n");
> -		vin->mbus_cfg.flags = 0;
> +		vin->parallel->mbus_flags = 0;
>  		break;
>  	default:
>  		vin_err(vin, "Unknown media bus type\n");
>  		return -EINVAL;
>  	}
>  
> -	vin->parallel = rvge;
> -
>  	return 0;
>  }
>  
> @@ -557,7 +557,7 @@ static int rvin_parallel_graph_init(struct rvin_dev *vin)
>  
>  	ret = v4l2_async_notifier_parse_fwnode_endpoints(
>  		vin->dev, &vin->notifier,
> -		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
> +		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
>  	if (ret)
>  		return ret;
>  
> @@ -718,6 +718,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  		return -ENOTCONN;
>  	}
>  
> +	vin->is_csi = true;

Same here, no need to set is_csi.

>  	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
>  
>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> @@ -783,10 +784,6 @@ 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-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index f1c3585..17f291f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -659,8 +659,12 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>  		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
> -		vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
> -			VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
> +		if (!vin->is_csi &&
> +		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
> +			vnmc |= VNMC_INF_YUV8_BT656;
> +		else
> +			vnmc |= VNMC_INF_YUV8_BT601;
> +
>  		input_is_yuv = true;
>  		break;
>  	case MEDIA_BUS_FMT_RGB888_1X24:
> @@ -668,8 +672,12 @@ static int rvin_setup(struct rvin_dev *vin)
>  		break;
>  	case MEDIA_BUS_FMT_UYVY10_2X10:
>  		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
> -		vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
> -			VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
> +		if (!vin->is_csi &&
> +		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
> +			vnmc |= VNMC_INF_YUV10_BT656;
> +		else
> +			vnmc |= VNMC_INF_YUV10_BT601;
> +
>  		input_is_yuv = true;
>  		break;
>  	default:
> @@ -683,11 +691,13 @@ static int rvin_setup(struct rvin_dev *vin)
>  		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
>  
>  	/* Hsync Signal Polarity Select */
> -	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> +	if (!vin->is_csi &&
> +	    !(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
>  		dmr2 |= VNDMR2_HPS;
>  
>  	/* Vsync Signal Polarity Select */
> -	if (!(vin->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> +	if (!vin->is_csi &&
> +	    !(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
>  		dmr2 |= VNDMR2_VPS;

Would it not make sens to have nested if-statements here?

    if (!vin->is_csi) {
        if (!(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
            ...

        ...
    }

>  
>  	/*
> @@ -734,7 +744,7 @@ static int rvin_setup(struct rvin_dev *vin)
>  
>  	if (vin->info->model == RCAR_GEN3) {
>  		/* Select between CSI-2 and parallel input */
> -		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
> +		if (vin->is_csi)
>  			vnmc &= ~VNMC_DPINE;
>  		else
>  			vnmc |= VNMC_DPINE;
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 7d0ffe08..c5f7fd1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -73,16 +73,22 @@ struct rvin_video_format {
>  };
>  
>  /**
> - * struct rvin_graph_entity - Video endpoint from async framework
> + * struct rvin_parallel_entity - Parallel video input endpoint descriptor
>   * @asd:	sub-device descriptor for async framework
>   * @subdev:	subdevice matched using async framework
> + * @mbus_type:	media bus type
> + * @mbus_flags:	media bus configuration flags
>   * @source_pad:	source pad of remote subdevice
>   * @sink_pad:	sink pad of remote subdevice
> + *
>   */
> -struct rvin_graph_entity {
> +struct rvin_parallel_entity {
>  	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *subdev;
>  
> +	enum v4l2_mbus_type mbus_type;
> +	unsigned int mbus_flags;
> +
>  	unsigned int source_pad;
>  	unsigned int sink_pad;
>  };
> @@ -146,7 +152,8 @@ struct rvin_info {
>   * @v4l2_dev:		V4L2 device
>   * @ctrl_handler:	V4L2 control handler
>   * @notifier:		V4L2 asynchronous subdevs notifier
> - * @parallel:		entity in the DT for local parallel subdevice
> + *
> + * @parallel:		parallel input subdevice descriptor
>   *
>   * @group:		Gen3 CSI group
>   * @id:			Gen3 group id for this VIN
> @@ -164,7 +171,8 @@ struct rvin_info {
>   * @sequence:		V4L2 buffers sequence number
>   * @state:		keeps track of operation state
>   *
> - * @mbus_cfg:		media bus configuration from DT
> + * @is_csi:		flag to mark the VIN as using a CSI-2 subdevice
> + *
>   * @mbus_code:		media bus format code
>   * @format:		active V4L2 pixel format
>   *
> @@ -182,7 +190,8 @@ struct rvin_dev {
>  	struct v4l2_device v4l2_dev;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_async_notifier notifier;
> -	struct rvin_graph_entity *parallel;
> +
> +	struct rvin_parallel_entity *parallel;
>  
>  	struct rvin_group *group;
>  	unsigned int id;
> @@ -199,7 +208,8 @@ struct rvin_dev {
>  	unsigned int sequence;
>  	enum rvin_dma_state state;
>  
> -	struct v4l2_mbus_config mbus_cfg;
> +	bool is_csi;
> +
>  	u32 mbus_code;
>  	struct v4l2_pix_format format;
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v3 5/9] media: rcar-vin: Parse parallel input on Gen3

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-18 16:40:41 +0200, Jacopo Mondi wrote:
> The rcar-vin driver so far had a mutually exclusive code path for
> handling parallel and CSI-2 video input subdevices, with only the CSI-2
> use case supporting media-controller. As we add support for parallel
> inputs to Gen3 media-controller compliant code path now parse both port@0
> and port@1, handling the media-controller use case in the parallel
> bound/unbind notifier operations and delay notifier registration to the
> last probing VIN instance in case we have links to setup between media
> entities.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 114 +++++++++++++++++++++-------
>  1 file changed, 85 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 0a35a98..745e8ee 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -397,6 +397,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
>  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
>  
> +	vin->parallel->subdev = subdev;
> +
> +	if (vin->info->use_mc)
> +		return 0;
> +

I think it would be cleaner if you wrote this as

        if (vin->info->use_mc) {
            vin->parallel->subdev = subdev;
            return 0;
        }

And then not use the goto bellow.


>  	/* Find compatible subdevices mbus format */
>  	vin->mbus_code = 0;
>  	code.index = 0;
> @@ -422,46 +427,52 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  	if (!vin->mbus_code) {
>  		vin_err(vin, "Unsupported media bus format for %s\n",
>  			subdev->name);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto error_reset_subdev;
>  	}
>  
>  	/* Read tvnorms */
>  	ret = v4l2_subdev_call(subdev, video, g_tvnorms, &vin->vdev.tvnorms);
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> +		goto error_reset_subdev;
>  
>  	/* Read standard */
>  	vin->std = V4L2_STD_UNKNOWN;
>  	ret = v4l2_subdev_call(subdev, video, g_std, &vin->std);
>  	if (ret < 0 && ret != -ENOIOCTLCMD)
> -		return ret;
> +		goto error_reset_subdev;
>  
>  	/* Add the controls */
>  	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
>  	if (ret < 0)
> -		return ret;
> +		goto error_reset_subdev;
>  
>  	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
>  				    NULL);
>  	if (ret < 0) {
>  		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		return ret;
> +		goto error_reset_subdev;
>  	}
>  
>  	vin->vdev.ctrl_handler = &vin->ctrl_handler;
>  
> -	vin->parallel->subdev = subdev;
> -
>  	return 0;
> +
> +error_reset_subdev:
> +	vin->parallel->subdev = NULL;
> +
> +	return ret;
>  }
>  
>  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  {
>  	rvin_v4l2_unregister(vin);
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -
> -	vin->vdev.ctrl_handler = NULL;
>  	vin->parallel->subdev = NULL;
> +
> +	if (!vin->info->use_mc) {
> +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +		vin->vdev.ctrl_handler = NULL;
> +	}
>  }
>  
>  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> @@ -551,22 +562,26 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
>  	return 0;
>  }
>  
> -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> +static int rvin_parallel_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
> -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> -		vin->dev, &vin->notifier,
> -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> +		0, rvin_parallel_parse_v4l2);
>  	if (ret)
>  		return ret;
>  
>  	if (!vin->parallel)
> -		return -ENODEV;
> +		return -ENOTCONN;
>  
>  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
>  		to_of_node(vin->parallel->asd.match.fwnode));
>  
> +	/* If we use media-controller, notifier registration is post-poned. */
> +	if (vin->info->use_mc)
> +		return 0;

Is this really needed? Se comment about CSI-2 and parallel ordering in 
rcar_vin_probe() bellow.

> +
>  	vin->notifier.ops = &rvin_parallel_notify_ops;
>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
>  	if (ret < 0) {
> @@ -766,6 +781,30 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  
>  	mutex_unlock(&vin->group->lock);
>  
> +	/*
> +	 * Go and register all notifiers for parallel subdevs, and
> +	 * the group notifier for CSI-2 subdevs, if any.
> +	 */
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		struct rvin_dev *ivin = vin->group->vin[i];
> +		struct v4l2_async_notifier *inotifier;
> +
> +		if (!ivin)
> +			continue;
> +
> +		inotifier = &ivin->notifier;
> +		if (!inotifier->num_subdevs)
> +			continue;
> +
> +		inotifier->ops = &rvin_parallel_notify_ops;
> +		ret = v4l2_async_notifier_register(&ivin->v4l2_dev, inotifier);
> +		if (ret < 0) {
> +			vin_err(ivin,
> +				"Notifier registration failed: %d\n", ret);
> +			goto error_unregister_notifiers;
> +		}
> +	}
> +
>  	if (!vin->group->notifier.num_subdevs)
>  		return 0;
>  
> @@ -773,25 +812,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
>  					   &vin->group->notifier);
>  	if (ret < 0) {
> -		vin_err(vin, "Notifier registration failed\n");
> +		vin_err(vin, "Notifier registration failed: %d\n", ret);

I won't object to this, if you think the return value is useful for 
user-space please keep it. But break it out to it's own patch :-)

>  		return ret;
>  	}
>  
>  	return 0;
> +
> +error_unregister_notifiers:
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		if (!vin->group->vin[i])
> +			continue;
> +
> +		v4l2_async_notifier_unregister(&vin->group->vin[i]->notifier);
> +	}
> +
> +	return ret;
>  }
>  
>  static int rvin_mc_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
> -	vin->pad.flags = MEDIA_PAD_FL_SINK;
> -	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> -	if (ret)
> -		return ret;
> -
> -	ret = rvin_group_get(vin);
> -	if (ret)
> -		return ret;
> +	if (!vin->info->use_mc)
> +		return 0;
>  
>  	ret = rvin_mc_parse_of_graph(vin);
>  	if (ret)
> @@ -1074,13 +1117,26 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	platform_set_drvdata(pdev, vin);
> -	if (vin->info->use_mc)
> -		ret = rvin_mc_init(vin);
> -	else
> -		ret = rvin_parallel_graph_init(vin);
> -	if (ret < 0)
> +
> +	if (vin->info->use_mc) {
> +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> +		if (ret)
> +			return ret;
> +
> +		ret = rvin_group_get(vin);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = rvin_parallel_init(vin);
> +	if (ret < 0 && ret != -ENOTCONN)
>  		goto error;
>  
> +	ret = rvin_mc_init(vin);
> +	if (ret)
> +		return ret;
> +

I think that if you first call rvin_mc_init() and then 
rvin_parallel_init() you can remove the delay of registering the VIN 
local notifier as the media device will have then been created and there 
is no reason to delay the notifier registration. And in the process this 
patch becomes much simpler (I think).

>  	pm_suspend_ignore_children(&pdev->dev, true);
>  	pm_runtime_enable(&pdev->dev);
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 5/9] media: rcar-vin: Parse parallel input on Gen3

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-18 16:40:41 +0200, Jacopo Mondi wrote:
> The rcar-vin driver so far had a mutually exclusive code path for
> handling parallel and CSI-2 video input subdevices, with only the CSI-2
> use case supporting media-controller. As we add support for parallel
> inputs to Gen3 media-controller compliant code path now parse both port@0
> and port@1, handling the media-controller use case in the parallel
> bound/unbind notifier operations and delay notifier registration to the
> last probing VIN instance in case we have links to setup between media
> entities.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 114 +++++++++++++++++++++-------
>  1 file changed, 85 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 0a35a98..745e8ee 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -397,6 +397,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
>  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
>  
> +	vin->parallel->subdev = subdev;
> +
> +	if (vin->info->use_mc)
> +		return 0;
> +

I think it would be cleaner if you wrote this as

        if (vin->info->use_mc) {
            vin->parallel->subdev = subdev;
            return 0;
        }

And then not use the goto bellow.


>  	/* Find compatible subdevices mbus format */
>  	vin->mbus_code = 0;
>  	code.index = 0;
> @@ -422,46 +427,52 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  	if (!vin->mbus_code) {
>  		vin_err(vin, "Unsupported media bus format for %s\n",
>  			subdev->name);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto error_reset_subdev;
>  	}
>  
>  	/* Read tvnorms */
>  	ret = v4l2_subdev_call(subdev, video, g_tvnorms, &vin->vdev.tvnorms);
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> +		goto error_reset_subdev;
>  
>  	/* Read standard */
>  	vin->std = V4L2_STD_UNKNOWN;
>  	ret = v4l2_subdev_call(subdev, video, g_std, &vin->std);
>  	if (ret < 0 && ret != -ENOIOCTLCMD)
> -		return ret;
> +		goto error_reset_subdev;
>  
>  	/* Add the controls */
>  	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
>  	if (ret < 0)
> -		return ret;
> +		goto error_reset_subdev;
>  
>  	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
>  				    NULL);
>  	if (ret < 0) {
>  		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		return ret;
> +		goto error_reset_subdev;
>  	}
>  
>  	vin->vdev.ctrl_handler = &vin->ctrl_handler;
>  
> -	vin->parallel->subdev = subdev;
> -
>  	return 0;
> +
> +error_reset_subdev:
> +	vin->parallel->subdev = NULL;
> +
> +	return ret;
>  }
>  
>  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  {
>  	rvin_v4l2_unregister(vin);
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -
> -	vin->vdev.ctrl_handler = NULL;
>  	vin->parallel->subdev = NULL;
> +
> +	if (!vin->info->use_mc) {
> +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +		vin->vdev.ctrl_handler = NULL;
> +	}
>  }
>  
>  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> @@ -551,22 +562,26 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
>  	return 0;
>  }
>  
> -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> +static int rvin_parallel_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
> -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> -		vin->dev, &vin->notifier,
> -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> +		0, rvin_parallel_parse_v4l2);
>  	if (ret)
>  		return ret;
>  
>  	if (!vin->parallel)
> -		return -ENODEV;
> +		return -ENOTCONN;
>  
>  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
>  		to_of_node(vin->parallel->asd.match.fwnode));
>  
> +	/* If we use media-controller, notifier registration is post-poned. */
> +	if (vin->info->use_mc)
> +		return 0;

Is this really needed? Se comment about CSI-2 and parallel ordering in 
rcar_vin_probe() bellow.

> +
>  	vin->notifier.ops = &rvin_parallel_notify_ops;
>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
>  	if (ret < 0) {
> @@ -766,6 +781,30 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  
>  	mutex_unlock(&vin->group->lock);
>  
> +	/*
> +	 * Go and register all notifiers for parallel subdevs, and
> +	 * the group notifier for CSI-2 subdevs, if any.
> +	 */
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		struct rvin_dev *ivin = vin->group->vin[i];
> +		struct v4l2_async_notifier *inotifier;
> +
> +		if (!ivin)
> +			continue;
> +
> +		inotifier = &ivin->notifier;
> +		if (!inotifier->num_subdevs)
> +			continue;
> +
> +		inotifier->ops = &rvin_parallel_notify_ops;
> +		ret = v4l2_async_notifier_register(&ivin->v4l2_dev, inotifier);
> +		if (ret < 0) {
> +			vin_err(ivin,
> +				"Notifier registration failed: %d\n", ret);
> +			goto error_unregister_notifiers;
> +		}
> +	}
> +
>  	if (!vin->group->notifier.num_subdevs)
>  		return 0;
>  
> @@ -773,25 +812,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
>  					   &vin->group->notifier);
>  	if (ret < 0) {
> -		vin_err(vin, "Notifier registration failed\n");
> +		vin_err(vin, "Notifier registration failed: %d\n", ret);

I won't object to this, if you think the return value is useful for 
user-space please keep it. But break it out to it's own patch :-)

>  		return ret;
>  	}
>  
>  	return 0;
> +
> +error_unregister_notifiers:
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		if (!vin->group->vin[i])
> +			continue;
> +
> +		v4l2_async_notifier_unregister(&vin->group->vin[i]->notifier);
> +	}
> +
> +	return ret;
>  }
>  
>  static int rvin_mc_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
> -	vin->pad.flags = MEDIA_PAD_FL_SINK;
> -	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> -	if (ret)
> -		return ret;
> -
> -	ret = rvin_group_get(vin);
> -	if (ret)
> -		return ret;
> +	if (!vin->info->use_mc)
> +		return 0;
>  
>  	ret = rvin_mc_parse_of_graph(vin);
>  	if (ret)
> @@ -1074,13 +1117,26 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	platform_set_drvdata(pdev, vin);
> -	if (vin->info->use_mc)
> -		ret = rvin_mc_init(vin);
> -	else
> -		ret = rvin_parallel_graph_init(vin);
> -	if (ret < 0)
> +
> +	if (vin->info->use_mc) {
> +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> +		if (ret)
> +			return ret;
> +
> +		ret = rvin_group_get(vin);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = rvin_parallel_init(vin);
> +	if (ret < 0 && ret != -ENOTCONN)
>  		goto error;
>  
> +	ret = rvin_mc_init(vin);
> +	if (ret)
> +		return ret;
> +

I think that if you first call rvin_mc_init() and then 
rvin_parallel_init() you can remove the delay of registering the VIN 
local notifier as the media device will have then been created and there 
is no reason to delay the notifier registration. And in the process this 
patch becomes much simpler (I think).

>  	pm_suspend_ignore_children(&pdev->dev, true);
>  	pm_runtime_enable(&pdev->dev);
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v3 6/9] media: rcar-vin: Link parallel input media entities

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:42 +0200, Jacopo Mondi wrote:
> When running with media-controller link the parallel input
> media entities with the VIN entities at 'complete' callback time.
> 
> To create media links the v4l2_device should be registered first.
> Check if the device is already registered, to avoid double registrations.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 745e8ee..d13bbcf 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -478,6 +478,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
>  {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
> +	struct media_entity *source;
> +	struct media_entity *sink;
>  	int ret;
>  
>  	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> @@ -486,7 +488,26 @@ static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
>  		return ret;
>  	}
>  
> -	return rvin_v4l2_register(vin);
> +	if (!video_is_registered(&vin->vdev)) {
> +		ret = rvin_v4l2_register(vin);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (!vin->info->use_mc)
> +		return 0;
> +
> +	/* If we're running with media-controller, link the subdevs. */
> +	source = &vin->parallel->subdev->entity;
> +	sink = &vin->vdev.entity;
> +
> +	ret = media_create_pad_link(source, vin->parallel->source_pad,
> +				    sink, vin->parallel->sink_pad, 0);
> +	if (ret)
> +		vin_err(vin, "Error adding link from %s to %s: %d\n",
> +			source->name, sink->name, ret);
> +
> +	return ret;
>  }
>  
>  static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -611,7 +632,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  
>  	/* Register all video nodes for the group. */
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> -		if (vin->group->vin[i]) {
> +		if (vin->group->vin[i] &&
> +		    !video_is_registered(&vin->group->vin[i]->vdev)) {
>  			ret = rvin_v4l2_register(vin->group->vin[i]);
>  			if (ret)
>  				return ret;
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 6/9] media: rcar-vin: Link parallel input media entities

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:42 +0200, Jacopo Mondi wrote:
> When running with media-controller link the parallel input
> media entities with the VIN entities at 'complete' callback time.
> 
> To create media links the v4l2_device should be registered first.
> Check if the device is already registered, to avoid double registrations.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 745e8ee..d13bbcf 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -478,6 +478,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
>  {
>  	struct rvin_dev *vin = notifier_to_vin(notifier);
> +	struct media_entity *source;
> +	struct media_entity *sink;
>  	int ret;
>  
>  	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> @@ -486,7 +488,26 @@ static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
>  		return ret;
>  	}
>  
> -	return rvin_v4l2_register(vin);
> +	if (!video_is_registered(&vin->vdev)) {
> +		ret = rvin_v4l2_register(vin);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (!vin->info->use_mc)
> +		return 0;
> +
> +	/* If we're running with media-controller, link the subdevs. */
> +	source = &vin->parallel->subdev->entity;
> +	sink = &vin->vdev.entity;
> +
> +	ret = media_create_pad_link(source, vin->parallel->source_pad,
> +				    sink, vin->parallel->sink_pad, 0);
> +	if (ret)
> +		vin_err(vin, "Error adding link from %s to %s: %d\n",
> +			source->name, sink->name, ret);
> +
> +	return ret;
>  }
>  
>  static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -611,7 +632,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  
>  	/* Register all video nodes for the group. */
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> -		if (vin->group->vin[i]) {
> +		if (vin->group->vin[i] &&
> +		    !video_is_registered(&vin->group->vin[i]->vdev)) {
>  			ret = rvin_v4l2_register(vin->group->vin[i]);
>  			if (ret)
>  				return ret;
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v3 7/9] media: rcar-vin: Handle parallel subdev in link_notify

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:43 +0200, Jacopo Mondi wrote:
> Handle parallel subdevices in link_notify callback. If the notified link
> involves a parallel subdevice, do not change routing of the VIN-CSI-2
> devices and mark the VIN instance as using a parallel input. If the
> CSI-2 link setup succeeds instead, mark the VIN instance as using CSI-2.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 36 +++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d13bbcf..dcebb42 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);

I think you should move the channel = ...; bellow the new if statement 
the same way you do with mask_new.

> -	mask_new = mask & rvin_group_get_mask(vin, csi_id, channel);
> +	csi_id = rvin_group_entity_to_csi_id(group, link->source->entity);

Missing new line.

With these two nits fixed:

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

> +	if (csi_id == -ENODEV) {
> +		struct v4l2_subdev *sd;
> +		unsigned int i;
> +
> +		/*
> +		 * Make sure the source entity subdevice is registered as
> +		 * a parallel input of one of the enabled VINs if it is not
> +		 * one of the CSI-2 subdevices.
> +		 *
> +		 * No hardware configuration required for parallel 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]->parallel &&
> +			    group->vin[i]->parallel->subdev == sd) {
> +				group->vin[i]->is_csi = false;
> +				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) {
> @@ -181,6 +208,11 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
>  
>  	/* New valid CHSEL found, set the new value. */
>  	ret = rvin_set_channel_routing(group->vin[master_id], __ffs(mask_new));
> +	if (ret)
> +		goto out;
> +
> +	vin->is_csi = true;
> +
>  out:
>  	mutex_unlock(&group->lock);
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 7/9] media: rcar-vin: Handle parallel subdev in link_notify

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2018-05-18 16:40:43 +0200, Jacopo Mondi wrote:
> Handle parallel subdevices in link_notify callback. If the notified link
> involves a parallel subdevice, do not change routing of the VIN-CSI-2
> devices and mark the VIN instance as using a parallel input. If the
> CSI-2 link setup succeeds instead, mark the VIN instance as using CSI-2.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 36 +++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d13bbcf..dcebb42 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);

I think you should move the channel = ...; bellow the new if statement 
the same way you do with mask_new.

> -	mask_new = mask & rvin_group_get_mask(vin, csi_id, channel);
> +	csi_id = rvin_group_entity_to_csi_id(group, link->source->entity);

Missing new line.

With these two nits fixed:

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

> +	if (csi_id == -ENODEV) {
> +		struct v4l2_subdev *sd;
> +		unsigned int i;
> +
> +		/*
> +		 * Make sure the source entity subdevice is registered as
> +		 * a parallel input of one of the enabled VINs if it is not
> +		 * one of the CSI-2 subdevices.
> +		 *
> +		 * No hardware configuration required for parallel 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]->parallel &&
> +			    group->vin[i]->parallel->subdev == sd) {
> +				group->vin[i]->is_csi = false;
> +				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) {
> @@ -181,6 +208,11 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
>  
>  	/* New valid CHSEL found, set the new value. */
>  	ret = rvin_set_channel_routing(group->vin[master_id], __ffs(mask_new));
> +	if (ret)
> +		goto out;
> +
> +	vin->is_csi = true;
> +
>  out:
>  	mutex_unlock(&group->lock);
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v3 8/9] media: rcar-vin: Rename _rcar_info to rcar_info

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-18 16:40:44 +0200, Jacopo Mondi wrote:
> Remove trailing underscore to align all rcar_group_route structure
> declarations.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

With Sergei's comment addressed:

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

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index dcebb42..b740f41 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -1060,7 +1060,7 @@ static const struct rvin_info rcar_info_r8a7796 = {
>  	.routes = rcar_info_r8a7796_routes,
>  };
>  
> -static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
> +static const struct rvin_group_route rcar_info_r8a77970_routes[] = {
>  	{ .csi = RVIN_CSI40, .channel = 0, .vin = 0, .mask = BIT(0) | BIT(3) },
>  	{ .csi = RVIN_CSI40, .channel = 0, .vin = 1, .mask = BIT(2) },
>  	{ .csi = RVIN_CSI40, .channel = 1, .vin = 1, .mask = BIT(3) },
> @@ -1076,7 +1076,7 @@ static const struct rvin_info rcar_info_r8a77970 = {
>  	.use_mc = true,
>  	.max_width = 4096,
>  	.max_height = 4096,
> -	.routes = _rcar_info_r8a77970_routes,
> +	.routes = rcar_info_r8a77970_routes,
>  };
>  
>  static const struct of_device_id rvin_of_id_table[] = {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 8/9] media: rcar-vin: Rename _rcar_info to rcar_info

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2018-05-18 16:40:44 +0200, Jacopo Mondi wrote:
> Remove trailing underscore to align all rcar_group_route structure
> declarations.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

With Sergei's comment addressed:

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

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index dcebb42..b740f41 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -1060,7 +1060,7 @@ static const struct rvin_info rcar_info_r8a7796 = {
>  	.routes = rcar_info_r8a7796_routes,
>  };
>  
> -static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
> +static const struct rvin_group_route rcar_info_r8a77970_routes[] = {
>  	{ .csi = RVIN_CSI40, .channel = 0, .vin = 0, .mask = BIT(0) | BIT(3) },
>  	{ .csi = RVIN_CSI40, .channel = 0, .vin = 1, .mask = BIT(2) },
>  	{ .csi = RVIN_CSI40, .channel = 1, .vin = 1, .mask = BIT(3) },
> @@ -1076,7 +1076,7 @@ static const struct rvin_info rcar_info_r8a77970 = {
>  	.use_mc = true,
>  	.max_width = 4096,
>  	.max_height = 4096,
> -	.routes = _rcar_info_r8a77970_routes,
> +	.routes = rcar_info_r8a77970_routes,
>  };
>  
>  static const struct of_device_id rvin_of_id_table[] = {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v3 1/9] media: rcar-vin: Rename 'digital' to 'parallel'

[jacopo mondi]

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

Hi Niklas,

On Thu, May 24, 2018 at 12:42:25AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2018-05-18 16:40:37 +0200, Jacopo Mondi wrote:
> > As the term 'digital' is used all over the rcar-vin code in place of
> > 'parallel', rename all the occurrencies.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 72 ++++++++++++++---------------
> >  drivers/media/platform/rcar-vin/rcar-dma.c  |  4 +-
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++---
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  6 +--
> >  4 files changed, 47 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index d3072e1..6b80f98 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -376,12 +376,12 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
> >  }
> >
> >  /* -----------------------------------------------------------------------------
> > - * Digital async notifier
> > + * Parallel async notifier
> >   */
> >
> >  /* The vin lock should be held when calling the subdevice attach and detach */
> > -static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> > -					 struct v4l2_subdev *subdev)
> > +static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > +					  struct v4l2_subdev *subdev)
> >  {
> >  	struct v4l2_subdev_mbus_code_enum code = {
> >  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > @@ -392,15 +392,15 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> >  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> >  	if (ret < 0)
> >  		return ret;
> > -	vin->digital->source_pad = ret;
> > +	vin->parallel->source_pad = ret;
> >
> >  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> > -	vin->digital->sink_pad = ret < 0 ? 0 : ret;
> > +	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
> >
> >  	/* Find compatible subdevices mbus format */
> >  	vin->mbus_code = 0;
> >  	code.index = 0;
> > -	code.pad = vin->digital->source_pad;
> > +	code.pad = vin->parallel->source_pad;
> >  	while (!vin->mbus_code &&
> >  	       !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> >  		code.index++;
> > @@ -450,21 +450,21 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> >
> >  	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> >
> > -	vin->digital->subdev = subdev;
> > +	vin->parallel->subdev = subdev;
> >
> >  	return 0;
> >  }
> >
> > -static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
> > +static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> >  {
> >  	rvin_v4l2_unregister(vin);
> >  	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> >
> >  	vin->vdev.ctrl_handler = NULL;
> > -	vin->digital->subdev = NULL;
> > +	vin->parallel->subdev = NULL;
> >  }
> >
> > -static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> > +static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> >  {
> >  	struct rvin_dev *vin = notifier_to_vin(notifier);
> >  	int ret;
> > @@ -478,28 +478,28 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> >  	return rvin_v4l2_register(vin);
> >  }
> >
> > -static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> > -				       struct v4l2_subdev *subdev,
> > -				       struct v4l2_async_subdev *asd)
> > +static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
> > +				        struct v4l2_subdev *subdev,
> > +				        struct v4l2_async_subdev *asd)
>
> When I run my indentation script this indentation changes from spaces to
> all tabs. If possible I would like to keep that as I usually run it on
> these files before submitting any patches, but it's not a big deal.

Oh I didn't notice that we now have exactly 8 spaces there.
I'll change this for sure!

Thanks
   j

>
> Whit this fixed, thanks for clearing this up!
>
> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> >  {
> >  	struct rvin_dev *vin = notifier_to_vin(notifier);
> >
> > -	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> > +	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
> >
> >  	mutex_lock(&vin->lock);
> > -	rvin_digital_subdevice_detach(vin);
> > +	rvin_parallel_subdevice_detach(vin);
> >  	mutex_unlock(&vin->lock);
> >  }
> >
> > -static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> > -				     struct v4l2_subdev *subdev,
> > -				     struct v4l2_async_subdev *asd)
> > +static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> > +				      struct v4l2_subdev *subdev,
> > +				      struct v4l2_async_subdev *asd)
> >  {
> >  	struct rvin_dev *vin = notifier_to_vin(notifier);
> >  	int ret;
> >
> >  	mutex_lock(&vin->lock);
> > -	ret = rvin_digital_subdevice_attach(vin, subdev);
> > +	ret = rvin_parallel_subdevice_attach(vin, subdev);
> >  	mutex_unlock(&vin->lock);
> >  	if (ret)
> >  		return ret;
> > @@ -507,21 +507,21 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> >  	v4l2_set_subdev_hostdata(subdev, vin);
> >
> >  	vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
> > -		subdev->name, vin->digital->source_pad,
> > -		vin->digital->sink_pad);
> > +		subdev->name, vin->parallel->source_pad,
> > +		vin->parallel->sink_pad);
> >
> >  	return 0;
> >  }
> >
> > -static const struct v4l2_async_notifier_operations rvin_digital_notify_ops = {
> > -	.bound = rvin_digital_notify_bound,
> > -	.unbind = rvin_digital_notify_unbind,
> > -	.complete = rvin_digital_notify_complete,
> > +static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> > +	.bound = rvin_parallel_notify_bound,
> > +	.unbind = rvin_parallel_notify_unbind,
> > +	.complete = rvin_parallel_notify_complete,
> >  };
> >
> > -static int rvin_digital_parse_v4l2(struct device *dev,
> > -				   struct v4l2_fwnode_endpoint *vep,
> > -				   struct v4l2_async_subdev *asd)
> > +static int rvin_parallel_parse_v4l2(struct device *dev,
> > +				    struct v4l2_fwnode_endpoint *vep,
> > +				    struct v4l2_async_subdev *asd)
> >  {
> >  	struct rvin_dev *vin = dev_get_drvdata(dev);
> >  	struct rvin_graph_entity *rvge =
> > @@ -546,28 +546,28 @@ static int rvin_digital_parse_v4l2(struct device *dev,
> >  		return -EINVAL;
> >  	}
> >
> > -	vin->digital = rvge;
> > +	vin->parallel = rvge;
> >
> >  	return 0;
> >  }
> >
> > -static int rvin_digital_graph_init(struct rvin_dev *vin)
> > +static int rvin_parallel_graph_init(struct rvin_dev *vin)
> >  {
> >  	int ret;
> >
> >  	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> >  		vin->dev, &vin->notifier,
> > -		sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
> > +		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
> >  	if (ret)
> >  		return ret;
> >
> > -	if (!vin->digital)
> > +	if (!vin->parallel)
> >  		return -ENODEV;
> >
> > -	vin_dbg(vin, "Found digital subdevice %pOF\n",
> > -		to_of_node(vin->digital->asd.match.fwnode));
> > +	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > +		to_of_node(vin->parallel->asd.match.fwnode));
> >
> > -	vin->notifier.ops = &rvin_digital_notify_ops;
> > +	vin->notifier.ops = &rvin_parallel_notify_ops;
> >  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
> >  	if (ret < 0) {
> >  		vin_err(vin, "Notifier registration failed\n");
> > @@ -1088,7 +1088,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  	if (vin->info->use_mc)
> >  		ret = rvin_mc_init(vin);
> >  	else
> > -		ret = rvin_digital_graph_init(vin);
> > +		ret = rvin_parallel_graph_init(vin);
> >  	if (ret < 0)
> >  		goto error;
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index ac07f99..f1c3585 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -733,7 +733,7 @@ static int rvin_setup(struct rvin_dev *vin)
> >  		vnmc |= VNMC_BPS;
> >
> >  	if (vin->info->model == RCAR_GEN3) {
> > -		/* Select between CSI-2 and Digital input */
> > +		/* Select between CSI-2 and parallel input */
> >  		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
> >  			vnmc &= ~VNMC_DPINE;
> >  		else
> > @@ -1074,7 +1074,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
> >
> >  	/* No media controller used, simply pass operation to subdevice. */
> >  	if (!vin->info->use_mc) {
> > -		ret = v4l2_subdev_call(vin->digital->subdev, video, s_stream,
> > +		ret = v4l2_subdev_call(vin->parallel->subdev, video, s_stream,
> >  				       on);
> >
> >  		return ret == -ENOIOCTLCMD ? 0 : ret;
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index e78fba8..87a718b 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -144,7 +144,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >  {
> >  	struct v4l2_subdev_format fmt = {
> >  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > -		.pad = vin->digital->source_pad,
> > +		.pad = vin->parallel->source_pad,
> >  	};
> >  	int ret;
> >
> > @@ -175,7 +175,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
> >  	struct v4l2_subdev_pad_config *pad_cfg;
> >  	struct v4l2_subdev_format format = {
> >  		.which = which,
> > -		.pad = vin->digital->source_pad,
> > +		.pad = vin->parallel->source_pad,
> >  	};
> >  	enum v4l2_field field;
> >  	u32 width, height;
> > @@ -517,7 +517,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
> >  	if (timings->pad)
> >  		return -EINVAL;
> >
> > -	timings->pad = vin->digital->sink_pad;
> > +	timings->pad = vin->parallel->sink_pad;
> >
> >  	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
> >
> > @@ -569,7 +569,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
> >  	if (cap->pad)
> >  		return -EINVAL;
> >
> > -	cap->pad = vin->digital->sink_pad;
> > +	cap->pad = vin->parallel->sink_pad;
> >
> >  	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
> >
> > @@ -587,7 +587,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> >  	if (edid->pad)
> >  		return -EINVAL;
> >
> > -	edid->pad = vin->digital->sink_pad;
> > +	edid->pad = vin->parallel->sink_pad;
> >
> >  	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
> >
> > @@ -605,7 +605,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> >  	if (edid->pad)
> >  		return -EINVAL;
> >
> > -	edid->pad = vin->digital->sink_pad;
> > +	edid->pad = vin->parallel->sink_pad;
> >
> >  	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index c2aef78..755ac3c 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -146,7 +146,7 @@ struct rvin_info {
> >   * @v4l2_dev:		V4L2 device
> >   * @ctrl_handler:	V4L2 control handler
> >   * @notifier:		V4L2 asynchronous subdevs notifier
> > - * @digital:		entity in the DT for local digital subdevice
> > + * @parallel:		entity in the DT for local parallel subdevice
> >   *
> >   * @group:		Gen3 CSI group
> >   * @id:			Gen3 group id for this VIN
> > @@ -182,7 +182,7 @@ struct rvin_dev {
> >  	struct v4l2_device v4l2_dev;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  	struct v4l2_async_notifier notifier;
> > -	struct rvin_graph_entity *digital;
> > +	struct rvin_graph_entity *parallel;
> >
> >  	struct rvin_group *group;
> >  	unsigned int id;
> > @@ -209,7 +209,7 @@ struct rvin_dev {
> >  	v4l2_std_id std;
> >  };
> >
> > -#define vin_to_source(vin)		((vin)->digital->subdev)
> > +#define vin_to_source(vin)		((vin)->parallel->subdev)
> >
> >  /* Debug */
> >  #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

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

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Eugeniu Rosca]

Hi Niklas, Jacopo cc: Steve

Apologize for reviving this old thread. Just one question below.

On 2018-05-24 10:14:52, Niklas Söderlund wrote:

> Hi Jacopo,

> Thanks for your patch.
>
> On 2018-05-18 16:40:39 +0200, Jacopo Mondi wrote:
>> As CSI-2 subdevices are shared between several VIN instances, a shared
>> notifier to collect the CSI-2 async subdevices is required. So far, the
>> rcar-vin driver used the notifier of the last VIN instance to probe but
>> with the forth-coming introduction of parallel input subdevices support
>> in mc-compliant code path, each VIN may register its own notifier if any
>> parallel subdevice is connected there.
>> 
>> To avoid registering a notifier twice (once for parallel subdev and one
>> for the CSI-2 subdevs) create a group notifier, shared by all the VIN
>> instances.
>> 
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>  drivers/media/platform/rcar-vin/rcar-core.c | 39 +++++++++++------------------
>>  drivers/media/platform/rcar-vin/rcar-vin.h  |  7 +++---
>>  2 files changed, 18 insertions(+), 28 deletions(-)

[..]

>> +
>>  #define vin_to_source(vin)		((vin)->parallel->subdev)

> This in particular I hate and at some point I hope to remove it or
> move  it to rcar-v4l2.c. :-) But that is a task for later and not
> related to  your patch-set.

What about below patch excerpt (courtesy of Steve) which is currently
under review in our tree? If we are on the same page here, we would
happily contribute a patch to you based on below.

Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()

Change the vin_to_source() macro to an inline function that will
retrieve the source subdevice for both media-control and non
media-control mode.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
[..]

diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 0b13b34d03e3..29d8c4a80c35 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -217,7 +217,21 @@ struct rvin_dev {
 	v4l2_std_id std;
 };
 
-#define vin_to_source(vin)		((vin)->parallel->subdev)
+static inline struct v4l2_subdev *
+vin_to_source(struct rvin_dev *vin)
+{
+	if (vin->info->use_mc) {
+		struct media_pad *pad;
+
+		pad = media_entity_remote_pad(&vin->pad);
+		if (!pad)
+			return NULL;
+
+		return media_entity_to_v4l2_subdev(pad->entity);
+	}
+
+	return vin->parallel->subdev;
+}
 
 /* Debug */
 #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)

Best regards,
Eugeniu.

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Steve Longerbeam]

Hi Niklas, all,


On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
> Hi Niklas, Jacopo cc: Steve
>
> Apologize for reviving this old thread. Just one question below.
>
> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
>
> [..]
>
>>> +
>>>   #define vin_to_source(vin)		((vin)->parallel->subdev)
>> This in particular I hate and at some point I hope to remove it or
>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
>> related to  your patch-set.
> What about below patch excerpt (courtesy of Steve) which is currently
> under review in our tree? If we are on the same page here, we would
> happily contribute a patch to you based on below.

I can submit this patch to media-tree ML for formal review.

But there are also other patches to rcar-vin applied to the tree 
mentioned by Eugeniu, which can broadly be described as:

1. If the rcar-csi2 sub-device's source pad format is changed via 
media-ctl, the connected VIN's crop bounds (vin->source rectangle) will 
be stale, since vin->source must always be equal to the source pad 
rectangle. So we have a patch that will catch the asynchronous 
V4L2_EVENT_SOURCE_CHANGE event sent from the rcar-csi2 sub-device when 
its source pad format is changed. In response, the VIN connected to that 
rcar-csi2 sub-device will reset the crop bounds to the new source pad 
format rectangle. In order to make this work however...

2. Sub-device events need to be forwarded to the VIN's that have enabled 
media links to the reporting sub-device. Currently, sub-device events 
are only forwarded to VIN7, because the notifier is registered only from 
VIN7, and so the sub-devices are registered only to the v4l2_dev owned 
by VIN7. So we have a set of patches that fix this: sub-device events 
get forwarded to the VIN's that have connected media paths to that 
sub-device. Besides allowing to reset the VIN crop bounds rectangle 
asynchronously, this also seems to be logically the correct behavior. It 
also makes the user interface a little more intuitive: userland knows 
which VIN it is capturing on, and that is the same VIN that will be 
receiving sub-device events in the active pipeline.

3. We have some patches under review that allow alternate -> none field 
mode. That is, source sub-device is sending alternate fields (top, 
bottom, top, ...), and userland is configured to receive those fields 
unmodified by setting field type to none. rcar-dma then detects and 
reports the field type of the captured field (top or bottom) in (struct 
v4l2_buffer)->field. Doing this allows for de-interlacing the captured 
fields using the FDP1 mem2mem device.

There are some other miscellaneous patches that I can submit, but the 
above describes the bulk of the changes.

Before I start on porting these patches to the media-tree, do the above 
changes make general sense, or are there fundamental problems with those 
ideas?

TIA,
Steve


>
> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
>
> Change the vin_to_source() macro to an inline function that will
> retrieve the source subdevice for both media-control and non
> media-control mode.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> [..]
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 0b13b34d03e3..29d8c4a80c35 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -217,7 +217,21 @@ struct rvin_dev {
>   	v4l2_std_id std;
>   };
>   
> -#define vin_to_source(vin)		((vin)->parallel->subdev)
> +static inline struct v4l2_subdev *
> +vin_to_source(struct rvin_dev *vin)
> +{
> +	if (vin->info->use_mc) {
> +		struct media_pad *pad;
> +
> +		pad = media_entity_remote_pad(&vin->pad);
> +		if (!pad)
> +			return NULL;
> +
> +		return media_entity_to_v4l2_subdev(pad->entity);
> +	}
> +
> +	return vin->parallel->subdev;
> +}
>   
>   /* Debug */
>   #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
>
> Best regards,
> Eugeniu.

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Niklas Söderlund]

Hi Steve, Eugeniu,

On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
> Hi Niklas, all,
> 
> 
> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
> > Hi Niklas, Jacopo cc: Steve
> > 
> > Apologize for reviving this old thread. Just one question below.
> > 
> > On 2018-05-24 10:14:52, Niklas Söderlund wrote:
> > 
> > [..]
> > 
> > > > +
> > > >   #define vin_to_source(vin)		((vin)->parallel->subdev)
> > > This in particular I hate and at some point I hope to remove it or
> > > move  it to rcar-v4l2.c. :-) But that is a task for later and not
> > > related to  your patch-set.
> > What about below patch excerpt (courtesy of Steve) which is currently
> > under review in our tree? If we are on the same page here, we would
> > happily contribute a patch to you based on below.
> 
> I can submit this patch to media-tree ML for formal review.

I see no problem with the patch itself but as you point out bellow there 
are other patches which makes of the change I assume? The change to 
vin_to_source() on it's own would not add much value as vin_to_source() 
is currently only used in the none-mc parts of the driver where the 
change bellow would never be used.

My dream would be to one day drop the none-mc mode of this driver, or 
split it out to a rcar-vin-legacy module or something ;-)

> 
> But there are also other patches to rcar-vin applied to the tree mentioned
> by Eugeniu, which can broadly be described as:
> 
> 1. If the rcar-csi2 sub-device's source pad format is changed via media-ctl,
> the connected VIN's crop bounds (vin->source rectangle) will be stale, since
> vin->source must always be equal to the source pad rectangle. So we have a
> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event sent
> from the rcar-csi2 sub-device when its source pad format is changed. In
> response, the VIN connected to that rcar-csi2 sub-device will reset the crop
> bounds to the new source pad format rectangle. In order to make this work
> however...

I'm no expert on when the crop/selection rectangles shall be reset and 
have wrestled with this in the past for this driver. I have partial 
rework of how formats especially crop/selection works planed. My goal of 
that is to try and make it simpler and more straight forward taking 
ideas from the vivid driver. There are some limitations in my 
implementation of this in the current driver which prevents me from 
adding sequential formats and enable the UDS scaler.

> 
> 2. Sub-device events need to be forwarded to the VIN's that have enabled
> media links to the reporting sub-device. Currently, sub-device events are
> only forwarded to VIN7, because the notifier is registered only from VIN7,
> and so the sub-devices are registered only to the v4l2_dev owned by VIN7. So
> we have a set of patches that fix this: sub-device events get forwarded to
> the VIN's that have connected media paths to that sub-device. Besides
> allowing to reset the VIN crop bounds rectangle asynchronously, this also
> seems to be logically the correct behavior. It also makes the user interface
> a little more intuitive: userland knows which VIN it is capturing on, and
> that is the same VIN that will be receiving sub-device events in the active
> pipeline.

This is a problem I ponder from time to time, happy to hear I'm not the 
only one :-) My view is that events are somewhat not functional in the 
media controller world and should be fixed at the framework level, maybe 
by moving them from the vdev to the mdev :-)

I think it's great that you worked on the problem but I'm a bit 
concerned with addressing this on a driver basis. Maybe this is 
something to bring up at the next v4l2 summit? I might be wrong here and 
there already is a consensus to address this as you describe in each
driver. Do you have any insights on the topic?

> 
> 3. We have some patches under review that allow alternate -> none field
> mode. That is, source sub-device is sending alternate fields (top, bottom,
> top, ...), and userland is configured to receive those fields unmodified by
> setting field type to none. rcar-dma then detects and reports the field type
> of the captured field (top or bottom) in (struct v4l2_buffer)->field. Doing
> this allows for de-interlacing the captured fields using the FDP1 mem2mem
> device.

Interesting, maybe I'm missing something but would it not be a better 
solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of 
abusing V4L2_FIELD_NONE?

> 
> There are some other miscellaneous patches that I can submit, but the above
> describes the bulk of the changes.

I'm happy to see work being done on the driver and would of course like 
to help you get all changes upstream so that all your use-cases would 
work out-of-the-box.

> 
> Before I start on porting these patches to the media-tree, do the above
> changes make general sense, or are there fundamental problems with those
> ideas?
> 
> TIA,
> Steve
> 
> 
> > 
> > Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
> > 
> > Change the vin_to_source() macro to an inline function that will
> > retrieve the source subdevice for both media-control and non
> > media-control mode.
> > 
> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > ---
> > [..]
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index 0b13b34d03e3..29d8c4a80c35 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -217,7 +217,21 @@ struct rvin_dev {
> >   	v4l2_std_id std;
> >   };
> > -#define vin_to_source(vin)		((vin)->parallel->subdev)
> > +static inline struct v4l2_subdev *
> > +vin_to_source(struct rvin_dev *vin)
> > +{
> > +	if (vin->info->use_mc) {
> > +		struct media_pad *pad;
> > +
> > +		pad = media_entity_remote_pad(&vin->pad);
> > +		if (!pad)
> > +			return NULL;
> > +
> > +		return media_entity_to_v4l2_subdev(pad->entity);
> > +	}
> > +
> > +	return vin->parallel->subdev;
> > +}
> >   /* Debug */
> >   #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
> > 
> > Best regards,
> > Eugeniu.
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Steve Longerbeam]

On 4/8/19 4:12 AM, Niklas Söderlund wrote:
> Hi Steve, Eugeniu,
>
> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
>> Hi Niklas, all,
>>
>>
>> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
>>> Hi Niklas, Jacopo cc: Steve
>>>
>>> Apologize for reviving this old thread. Just one question below.
>>>
>>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
>>>
>>> [..]
>>>
>>>>> +
>>>>>    #define vin_to_source(vin)		((vin)->parallel->subdev)
>>>> This in particular I hate and at some point I hope to remove it or
>>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
>>>> related to  your patch-set.
>>> What about below patch excerpt (courtesy of Steve) which is currently
>>> under review in our tree? If we are on the same page here, we would
>>> happily contribute a patch to you based on below.
>> I can submit this patch to media-tree ML for formal review.
> I see no problem with the patch itself but as you point out bellow there
> are other patches which makes of the change I assume? The change to
> vin_to_source() on it's own would not add much value as vin_to_source()
> is currently only used in the none-mc parts of the driver where the
> change bellow would never be used.
>
> My dream would be to one day drop the none-mc mode of this driver, or
> split it out to a rcar-vin-legacy module or something ;-)

Yes, that's something that has been confusing me. Why does there need to 
be a distinction between a media control mode non-mc mode in the 
rcar-vin driver? I understand the "digital"/"parallel" mode is to handle 
sensors with a parallel interface (as opposed to MIPI CSI-2), but why 
not make the parallel sensors also require media-control 
(enabling/disable media links, etc.)?


>
>> But there are also other patches to rcar-vin applied to the tree mentioned
>> by Eugeniu, which can broadly be described as:
>>
>> 1. If the rcar-csi2 sub-device's source pad format is changed via media-ctl,
>> the connected VIN's crop bounds (vin->source rectangle) will be stale, since
>> vin->source must always be equal to the source pad rectangle. So we have a
>> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event sent
>> from the rcar-csi2 sub-device when its source pad format is changed. In
>> response, the VIN connected to that rcar-csi2 sub-device will reset the crop
>> bounds to the new source pad format rectangle. In order to make this work
>> however...
> I'm no expert on when the crop/selection rectangles shall be reset and
> have wrestled with this in the past for this driver. I have partial
> rework of how formats especially crop/selection works planed. My goal of
> that is to try and make it simpler and more straight forward taking
> ideas from the vivid driver. There are some limitations in my
> implementation of this in the current driver which prevents me from
> adding sequential formats and enable the UDS scaler.

Ok. What I did for imx-media driver is to export a function from the 
video capture interface that allows the source sub-device connected to 
the capture interface to call it when the source pad format changes, 
which gives the capture interface the chance to adjust compose window 
(unlike rcar-vin, imx capture interface doesn't crop or scale, the 
connected source subdev does that, but it does pad the compose window so 
it needs to know the original compose window, sorry hope I haven't lost 
you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is 
maybe a cleaner way to update the crop bounds in rcar-vin than using an 
exported function.



>
>> 2. Sub-device events need to be forwarded to the VIN's that have enabled
>> media links to the reporting sub-device. Currently, sub-device events are
>> only forwarded to VIN7, because the notifier is registered only from VIN7,
>> and so the sub-devices are registered only to the v4l2_dev owned by VIN7. So
>> we have a set of patches that fix this: sub-device events get forwarded to
>> the VIN's that have connected media paths to that sub-device. Besides
>> allowing to reset the VIN crop bounds rectangle asynchronously, this also
>> seems to be logically the correct behavior. It also makes the user interface
>> a little more intuitive: userland knows which VIN it is capturing on, and
>> that is the same VIN that will be receiving sub-device events in the active
>> pipeline.
> This is a problem I ponder from time to time, happy to hear I'm not the
> only one :-) My view is that events are somewhat not functional in the
> media controller world and should be fixed at the framework level, maybe
> by moving them from the vdev to the mdev :-)

How about embedding the 'struct v4l2_device' into 'struct rvin_group' 
instead of 'struct rvin_dev'? That probably makes more sense, the 
v4l2_dev keeps track of media-device-wide info such as the list of 
sub-devices in the graph, so it seems more appropriate to create only 
one instance of v4l2_dev. That will force rcar-vin to lookup the correct 
VINs to forward the event to, since v4l2_dev is no longer attached to 
the VINs.

But moving events from v4l2 to media (e.g. media entities send events to 
the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev) 
is also an interesting idea.

> I think it's great that you worked on the problem but I'm a bit
> concerned with addressing this on a driver basis. Maybe this is
> something to bring up at the next v4l2 summit? I might be wrong here and
> there already is a consensus to address this as you describe in each
> driver. Do you have any insights on the topic?

I haven't been tuned into that topic, but yes I agree that events need a 
better framework.


>
>> 3. We have some patches under review that allow alternate -> none field
>> mode. That is, source sub-device is sending alternate fields (top, bottom,
>> top, ...), and userland is configured to receive those fields unmodified by
>> setting field type to none. rcar-dma then detects and reports the field type
>> of the captured field (top or bottom) in (struct v4l2_buffer)->field. Doing
>> this allows for de-interlacing the captured fields using the FDP1 mem2mem
>> device.
> Interesting, maybe I'm missing something but would it not be a better
> solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
> abusing V4L2_FIELD_NONE?

Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field 
frames (top, bottom and alternate fields)"),

"There was never proper support in the VIN driver to deliver ALTERNATING 
field format to user-space, remove this field option. The problem is 
that ALTERNATING field order requires the sequence numbers of buffers 
returned to userspace to reflect if fields were dropped or not, 
something which is not possible with the VIN drivers capture logic."

Is that still a concern, or can alternate mode be brought back but with 
possibly the above limitation?

Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it 
sounds like using this field type in this way may not be too much abuse :)

"Images are in progressive format, not interlaced. The driver may also 
indicate this order when it cannot distinguish 
between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."

[1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html


Steve

>
>> There are some other miscellaneous patches that I can submit, but the above
>> describes the bulk of the changes.
> I'm happy to see work being done on the driver and would of course like
> to help you get all changes upstream so that all your use-cases would
> work out-of-the-box.
>
>> Before I start on porting these patches to the media-tree, do the above
>> changes make general sense, or are there fundamental problems with those
>> ideas?
>>
>> TIA,
>> Steve
>>
>>
>>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
>>>
>>> Change the vin_to_source() macro to an inline function that will
>>> retrieve the source subdevice for both media-control and non
>>> media-control mode.
>>>
>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>> ---
>>> [..]
>>>
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
>>> index 0b13b34d03e3..29d8c4a80c35 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>>> @@ -217,7 +217,21 @@ struct rvin_dev {
>>>    	v4l2_std_id std;
>>>    };
>>> -#define vin_to_source(vin)		((vin)->parallel->subdev)
>>> +static inline struct v4l2_subdev *
>>> +vin_to_source(struct rvin_dev *vin)
>>> +{
>>> +	if (vin->info->use_mc) {
>>> +		struct media_pad *pad;
>>> +
>>> +		pad = media_entity_remote_pad(&vin->pad);
>>> +		if (!pad)
>>> +			return NULL;
>>> +
>>> +		return media_entity_to_v4l2_subdev(pad->entity);
>>> +	}
>>> +
>>> +	return vin->parallel->subdev;
>>> +}
>>>    /* Debug */
>>>    #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
>>>
>>> Best regards,
>>> Eugeniu.

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Kieran Bingham]

Hi Steve, Niklas,

Just a small 2-cents below:

On 09/04/2019 04:58, Steve Longerbeam wrote:
> 
> 
> On 4/8/19 4:12 AM, Niklas Söderlund wrote:
>> Hi Steve, Eugeniu,
>>
>> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
>>> Hi Niklas, all,
>>>
>>>
>>> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
>>>> Hi Niklas, Jacopo cc: Steve
>>>>
>>>> Apologize for reviving this old thread. Just one question below.
>>>>
>>>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
>>>>
>>>> [..]
>>>>
>>>>>> +
>>>>>>    #define vin_to_source(vin)        ((vin)->parallel->subdev)
>>>>> This in particular I hate and at some point I hope to remove it or
>>>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
>>>>> related to  your patch-set.
>>>> What about below patch excerpt (courtesy of Steve) which is currently
>>>> under review in our tree? If we are on the same page here, we would
>>>> happily contribute a patch to you based on below.
>>> I can submit this patch to media-tree ML for formal review.
>> I see no problem with the patch itself but as you point out bellow there
>> are other patches which makes of the change I assume? The change to
>> vin_to_source() on it's own would not add much value as vin_to_source()
>> is currently only used in the none-mc parts of the driver where the
>> change bellow would never be used.
>>
>> My dream would be to one day drop the none-mc mode of this driver, or
>> split it out to a rcar-vin-legacy module or something ;-)
> 
> Yes, that's something that has been confusing me. Why does there need to
> be a distinction between a media control mode non-mc mode in the
> rcar-vin driver? I understand the "digital"/"parallel" mode is to handle
> sensors with a parallel interface (as opposed to MIPI CSI-2), but why
> not make the parallel sensors also require media-control
> (enabling/disable media links, etc.)?
> 
> 
>>
>>> But there are also other patches to rcar-vin applied to the tree
>>> mentioned
>>> by Eugeniu, which can broadly be described as:
>>>
>>> 1. If the rcar-csi2 sub-device's source pad format is changed via
>>> media-ctl,
>>> the connected VIN's crop bounds (vin->source rectangle) will be
>>> stale, since
>>> vin->source must always be equal to the source pad rectangle. So we
>>> have a
>>> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event
>>> sent
>>> from the rcar-csi2 sub-device when its source pad format is changed. In
>>> response, the VIN connected to that rcar-csi2 sub-device will reset
>>> the crop
>>> bounds to the new source pad format rectangle. In order to make this
>>> work
>>> however...
>> I'm no expert on when the crop/selection rectangles shall be reset and
>> have wrestled with this in the past for this driver. I have partial
>> rework of how formats especially crop/selection works planed. My goal of
>> that is to try and make it simpler and more straight forward taking
>> ideas from the vivid driver. There are some limitations in my
>> implementation of this in the current driver which prevents me from
>> adding sequential formats and enable the UDS scaler.
> 
> Ok. What I did for imx-media driver is to export a function from the
> video capture interface that allows the source sub-device connected to
> the capture interface to call it when the source pad format changes,
> which gives the capture interface the chance to adjust compose window
> (unlike rcar-vin, imx capture interface doesn't crop or scale, the
> connected source subdev does that, but it does pad the compose window so
> it needs to know the original compose window, sorry hope I haven't lost
> you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is
> maybe a cleaner way to update the crop bounds in rcar-vin than using an
> exported function.
> 
> 
> 
>>
>>> 2. Sub-device events need to be forwarded to the VIN's that have enabled
>>> media links to the reporting sub-device. Currently, sub-device events
>>> are
>>> only forwarded to VIN7, because the notifier is registered only from
>>> VIN7,
>>> and so the sub-devices are registered only to the v4l2_dev owned by
>>> VIN7. So
>>> we have a set of patches that fix this: sub-device events get
>>> forwarded to
>>> the VIN's that have connected media paths to that sub-device. Besides
>>> allowing to reset the VIN crop bounds rectangle asynchronously, this
>>> also
>>> seems to be logically the correct behavior. It also makes the user
>>> interface
>>> a little more intuitive: userland knows which VIN it is capturing on,
>>> and
>>> that is the same VIN that will be receiving sub-device events in the
>>> active
>>> pipeline.
>> This is a problem I ponder from time to time, happy to hear I'm not the
>> only one :-) My view is that events are somewhat not functional in the
>> media controller world and should be fixed at the framework level, maybe
>> by moving them from the vdev to the mdev :-)
> 
> How about embedding the 'struct v4l2_device' into 'struct rvin_group'
> instead of 'struct rvin_dev'? That probably makes more sense, the
> v4l2_dev keeps track of media-device-wide info such as the list of
> sub-devices in the graph, so it seems more appropriate to create only
> one instance of v4l2_dev. That will force rcar-vin to lookup the correct
> VINs to forward the event to, since v4l2_dev is no longer attached to
> the VINs.
> 
> But moving events from v4l2 to media (e.g. media entities send events to
> the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev)
> is also an interesting idea.
> 
>> I think it's great that you worked on the problem but I'm a bit
>> concerned with addressing this on a driver basis. Maybe this is
>> something to bring up at the next v4l2 summit? I might be wrong here and
>> there already is a consensus to address this as you describe in each
>> driver. Do you have any insights on the topic?
> 
> I haven't been tuned into that topic, but yes I agree that events need a
> better framework.
> 
> 
>>
>>> 3. We have some patches under review that allow alternate -> none field
>>> mode. That is, source sub-device is sending alternate fields (top,
>>> bottom,
>>> top, ...), and userland is configured to receive those fields
>>> unmodified by
>>> setting field type to none. rcar-dma then detects and reports the
>>> field type
>>> of the captured field (top or bottom) in (struct v4l2_buffer)->field.
>>> Doing
>>> this allows for de-interlacing the captured fields using the FDP1
>>> mem2mem
>>> device.
>> Interesting, maybe I'm missing something but would it not be a better
>> solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
>> abusing V4L2_FIELD_NONE?
> 
> Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field
> frames (top, bottom and alternate fields)"),
> 
> "There was never proper support in the VIN driver to deliver ALTERNATING
> field format to user-space, remove this field option. The problem is
> that ALTERNATING field order requires the sequence numbers of buffers
> returned to userspace to reflect if fields were dropped or not,
> something which is not possible with the VIN drivers capture logic."
> 
> Is that still a concern, or can alternate mode be brought back but with
> possibly the above limitation?

I think that might have been before we added the scratch buffer to
support continuous capture?

Now that we have that - I think we should be able to correctly track
dropped frames right?

--
Kieran


> 
> Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it
> sounds like using this field type in this way may not be too much abuse :)
> 
> "Images are in progressive format, not interlaced. The driver may also
> indicate this order when it cannot distinguish
> between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."
> 
> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html
> 
> 
> Steve
> 
>>
>>> There are some other miscellaneous patches that I can submit, but the
>>> above
>>> describes the bulk of the changes.
>> I'm happy to see work being done on the driver and would of course like
>> to help you get all changes upstream so that all your use-cases would
>> work out-of-the-box.
>>
>>> Before I start on porting these patches to the media-tree, do the above
>>> changes make general sense, or are there fundamental problems with those
>>> ideas?
>>>
>>> TIA,
>>> Steve
>>>
>>>
>>>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
>>>>
>>>> Change the vin_to_source() macro to an inline function that will
>>>> retrieve the source subdevice for both media-control and non
>>>> media-control mode.
>>>>
>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>> ---
>>>> [..]
>>>>
>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
>>>> b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>> index 0b13b34d03e3..29d8c4a80c35 100644
>>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>> @@ -217,7 +217,21 @@ struct rvin_dev {
>>>>        v4l2_std_id std;
>>>>    };
>>>> -#define vin_to_source(vin)        ((vin)->parallel->subdev)
>>>> +static inline struct v4l2_subdev *
>>>> +vin_to_source(struct rvin_dev *vin)
>>>> +{
>>>> +    if (vin->info->use_mc) {
>>>> +        struct media_pad *pad;
>>>> +
>>>> +        pad = media_entity_remote_pad(&vin->pad);
>>>> +        if (!pad)
>>>> +            return NULL;
>>>> +
>>>> +        return media_entity_to_v4l2_subdev(pad->entity);
>>>> +    }
>>>> +
>>>> +    return vin->parallel->subdev;
>>>> +}
>>>>    /* Debug */
>>>>    #define vin_dbg(d, fmt, arg...)        dev_dbg(d->dev, fmt, ##arg)
>>>>
>>>> Best regards,
>>>> Eugeniu.
> 

-- 
Regards
--
Kieran

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Niklas Söderlund]

Hi Steve, Kieran,

On 2019-04-09 10:10:45 +0100, Kieran Bingham wrote:
> Hi Steve, Niklas,
> 
> Just a small 2-cents below:
> 
> On 09/04/2019 04:58, Steve Longerbeam wrote:
> > 
> > 
> > On 4/8/19 4:12 AM, Niklas Söderlund wrote:
> >> Hi Steve, Eugeniu,
> >>
> >> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
> >>> Hi Niklas, all,
> >>>
> >>>
> >>> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
> >>>> Hi Niklas, Jacopo cc: Steve
> >>>>
> >>>> Apologize for reviving this old thread. Just one question below.
> >>>>
> >>>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
> >>>>
> >>>> [..]
> >>>>
> >>>>>> +
> >>>>>>    #define vin_to_source(vin)        ((vin)->parallel->subdev)
> >>>>> This in particular I hate and at some point I hope to remove it or
> >>>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
> >>>>> related to  your patch-set.
> >>>> What about below patch excerpt (courtesy of Steve) which is currently
> >>>> under review in our tree? If we are on the same page here, we would
> >>>> happily contribute a patch to you based on below.
> >>> I can submit this patch to media-tree ML for formal review.
> >> I see no problem with the patch itself but as you point out bellow there
> >> are other patches which makes of the change I assume? The change to
> >> vin_to_source() on it's own would not add much value as vin_to_source()
> >> is currently only used in the none-mc parts of the driver where the
> >> change bellow would never be used.
> >>
> >> My dream would be to one day drop the none-mc mode of this driver, or
> >> split it out to a rcar-vin-legacy module or something ;-)
> > 
> > Yes, that's something that has been confusing me. Why does there need to
> > be a distinction between a media control mode non-mc mode in the
> > rcar-vin driver? I understand the "digital"/"parallel" mode is to handle
> > sensors with a parallel interface (as opposed to MIPI CSI-2), but why
> > not make the parallel sensors also require media-control
> > (enabling/disable media links, etc.)?
> > 
> > 

The issue is backward compatibility. The original driver targeted 
Renesas Gen2 board which was relatively simple and might even pre-date 
the media controller api. When I extended the diver to support Gen3 
boards which are more complex I had to add media controller support to 
the driver. There is a huge overlap (rcar-dma.c) between Gen2 and Gen3 
and it seemed silly to have two drivers.

As a side note Jacopo added support for the parallel node in the media 
controller for Gen3 boards which have the same pipeline as Gen2. So 
everything needed to remove the none MC code paths are in the driver but 
I'm afraid that would upset current users on Gen2 ;-)

> >>
> >>> But there are also other patches to rcar-vin applied to the tree
> >>> mentioned
> >>> by Eugeniu, which can broadly be described as:
> >>>
> >>> 1. If the rcar-csi2 sub-device's source pad format is changed via
> >>> media-ctl,
> >>> the connected VIN's crop bounds (vin->source rectangle) will be
> >>> stale, since
> >>> vin->source must always be equal to the source pad rectangle. So we
> >>> have a
> >>> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event
> >>> sent
> >>> from the rcar-csi2 sub-device when its source pad format is changed. In
> >>> response, the VIN connected to that rcar-csi2 sub-device will reset
> >>> the crop
> >>> bounds to the new source pad format rectangle. In order to make this
> >>> work
> >>> however...
> >> I'm no expert on when the crop/selection rectangles shall be reset and
> >> have wrestled with this in the past for this driver. I have partial
> >> rework of how formats especially crop/selection works planed. My goal of
> >> that is to try and make it simpler and more straight forward taking
> >> ideas from the vivid driver. There are some limitations in my
> >> implementation of this in the current driver which prevents me from
> >> adding sequential formats and enable the UDS scaler.
> > 
> > Ok. What I did for imx-media driver is to export a function from the
> > video capture interface that allows the source sub-device connected to
> > the capture interface to call it when the source pad format changes,
> > which gives the capture interface the chance to adjust compose window
> > (unlike rcar-vin, imx capture interface doesn't crop or scale, the
> > connected source subdev does that, but it does pad the compose window so
> > it needs to know the original compose window, sorry hope I haven't lost
> > you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is
> > maybe a cleaner way to update the crop bounds in rcar-vin than using an
> > exported function.
> > 

I agree V4L2_EVENT_SOURCE_CHANGE could be a nice way of doing this if 
the event routing is sorted out.

> > 
> > 
> >>
> >>> 2. Sub-device events need to be forwarded to the VIN's that have enabled
> >>> media links to the reporting sub-device. Currently, sub-device events
> >>> are
> >>> only forwarded to VIN7, because the notifier is registered only from
> >>> VIN7,
> >>> and so the sub-devices are registered only to the v4l2_dev owned by
> >>> VIN7. So
> >>> we have a set of patches that fix this: sub-device events get
> >>> forwarded to
> >>> the VIN's that have connected media paths to that sub-device. Besides
> >>> allowing to reset the VIN crop bounds rectangle asynchronously, this
> >>> also
> >>> seems to be logically the correct behavior. It also makes the user
> >>> interface
> >>> a little more intuitive: userland knows which VIN it is capturing on,
> >>> and
> >>> that is the same VIN that will be receiving sub-device events in the
> >>> active
> >>> pipeline.
> >> This is a problem I ponder from time to time, happy to hear I'm not the
> >> only one :-) My view is that events are somewhat not functional in the
> >> media controller world and should be fixed at the framework level, maybe
> >> by moving them from the vdev to the mdev :-)
> > 
> > How about embedding the 'struct v4l2_device' into 'struct rvin_group'
> > instead of 'struct rvin_dev'? That probably makes more sense, the
> > v4l2_dev keeps track of media-device-wide info such as the list of
> > sub-devices in the graph, so it seems more appropriate to create only
> > one instance of v4l2_dev. That will force rcar-vin to lookup the correct
> > VINs to forward the event to, since v4l2_dev is no longer attached to
> > the VINs.

It might solve the event routing problem but will create others. For 
example each VIN instance can have a private notifier if it has a 
parallel subdevice and be part of VIN group for the ones coming from 
CSI-2.

> > 
> > But moving events from v4l2 to media (e.g. media entities send events to
> > the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev)
> > is also an interesting idea.

I think this is the right way to go.

> > 
> >> I think it's great that you worked on the problem but I'm a bit
> >> concerned with addressing this on a driver basis. Maybe this is
> >> something to bring up at the next v4l2 summit? I might be wrong here and
> >> there already is a consensus to address this as you describe in each
> >> driver. Do you have any insights on the topic?
> > 
> > I haven't been tuned into that topic, but yes I agree that events need a
> > better framework.
> > 
> > 
> >>
> >>> 3. We have some patches under review that allow alternate -> none field
> >>> mode. That is, source sub-device is sending alternate fields (top,
> >>> bottom,
> >>> top, ...), and userland is configured to receive those fields
> >>> unmodified by
> >>> setting field type to none. rcar-dma then detects and reports the
> >>> field type
> >>> of the captured field (top or bottom) in (struct v4l2_buffer)->field.
> >>> Doing
> >>> this allows for de-interlacing the captured fields using the FDP1
> >>> mem2mem
> >>> device.
> >> Interesting, maybe I'm missing something but would it not be a better
> >> solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
> >> abusing V4L2_FIELD_NONE?
> > 
> > Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field
> > frames (top, bottom and alternate fields)"),
> > 
> > "There was never proper support in the VIN driver to deliver ALTERNATING
> > field format to user-space, remove this field option. The problem is
> > that ALTERNATING field order requires the sequence numbers of buffers
> > returned to userspace to reflect if fields were dropped or not,
> > something which is not possible with the VIN drivers capture logic."
> > 
> > Is that still a concern, or can alternate mode be brought back but with
> > possibly the above limitation?
> 
> I think that might have been before we added the scratch buffer to
> support continuous capture?
> 
> Now that we have that - I think we should be able to correctly track
> dropped frames right?

I think you are correct Kieran, we now have the scratch buffer so it 
should be possible to add proper support for delivering ALTERNATING to 
userspace. I'm sure we find new issues on the way tho ;-)

> 
> 
> > 
> > Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it
> > sounds like using this field type in this way may not be too much abuse :)

My bad I should have said using instead of abusing, sorry about that :-) 
I did not know that this was a valid use-case for FIELD_NONE so yes I 
think it could be used if we can't solve the requirements for adding 
ALTERNATING.

> > 
> > "Images are in progressive format, not interlaced. The driver may also
> > indicate this order when it cannot distinguish
> > between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."
> > 
> > [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html
> > 
> > 
> > Steve
> > 
> >>
> >>> There are some other miscellaneous patches that I can submit, but the
> >>> above
> >>> describes the bulk of the changes.
> >> I'm happy to see work being done on the driver and would of course like
> >> to help you get all changes upstream so that all your use-cases would
> >> work out-of-the-box.
> >>
> >>> Before I start on porting these patches to the media-tree, do the above
> >>> changes make general sense, or are there fundamental problems with those
> >>> ideas?
> >>>
> >>> TIA,
> >>> Steve
> >>>
> >>>
> >>>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
> >>>>
> >>>> Change the vin_to_source() macro to an inline function that will
> >>>> retrieve the source subdevice for both media-control and non
> >>>> media-control mode.
> >>>>
> >>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>> ---
> >>>> [..]
> >>>>
> >>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> >>>> b/drivers/media/platform/rcar-vin/rcar-vin.h
> >>>> index 0b13b34d03e3..29d8c4a80c35 100644
> >>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> >>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> >>>> @@ -217,7 +217,21 @@ struct rvin_dev {
> >>>>        v4l2_std_id std;
> >>>>    };
> >>>> -#define vin_to_source(vin)        ((vin)->parallel->subdev)
> >>>> +static inline struct v4l2_subdev *
> >>>> +vin_to_source(struct rvin_dev *vin)
> >>>> +{
> >>>> +    if (vin->info->use_mc) {
> >>>> +        struct media_pad *pad;
> >>>> +
> >>>> +        pad = media_entity_remote_pad(&vin->pad);
> >>>> +        if (!pad)
> >>>> +            return NULL;
> >>>> +
> >>>> +        return media_entity_to_v4l2_subdev(pad->entity);
> >>>> +    }
> >>>> +
> >>>> +    return vin->parallel->subdev;
> >>>> +}
> >>>>    /* Debug */
> >>>>    #define vin_dbg(d, fmt, arg...)        dev_dbg(d->dev, fmt, ##arg)
> >>>>
> >>>> Best regards,
> >>>> Eugeniu.
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Laurent Pinchart]

Hi Steve,

On Mon, Apr 08, 2019 at 08:58:25PM -0700, Steve Longerbeam wrote:
> On 4/8/19 4:12 AM, Niklas Söderlund wrote:
> > On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
> >> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
> >>> Hi Niklas, Jacopo cc: Steve
> >>>
> >>> Apologize for reviving this old thread. Just one question below.
> >>>
> >>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
> >>>
> >>> [..]
> >>>
> >>>>> +
> >>>>>    #define vin_to_source(vin)		((vin)->parallel->subdev)
> >>>> 
> >>>> This in particular I hate and at some point I hope to remove it or
> >>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
> >>>> related to  your patch-set.
> >>> 
> >>> What about below patch excerpt (courtesy of Steve) which is currently
> >>> under review in our tree? If we are on the same page here, we would
> >>> happily contribute a patch to you based on below.
> >> 
> >> I can submit this patch to media-tree ML for formal review.
> > 
> > I see no problem with the patch itself but as you point out bellow there
> > are other patches which makes of the change I assume? The change to
> > vin_to_source() on it's own would not add much value as vin_to_source()
> > is currently only used in the none-mc parts of the driver where the
> > change bellow would never be used.
> >
> > My dream would be to one day drop the none-mc mode of this driver, or
> > split it out to a rcar-vin-legacy module or something ;-)
> 
> Yes, that's something that has been confusing me. Why does there need to 
> be a distinction between a media control mode non-mc mode in the 
> rcar-vin driver? I understand the "digital"/"parallel" mode is to handle 
> sensors with a parallel interface (as opposed to MIPI CSI-2), but why 
> not make the parallel sensors also require media-control 
> (enabling/disable media links, etc.)?
> 
> >> But there are also other patches to rcar-vin applied to the tree mentioned
> >> by Eugeniu, which can broadly be described as:
> >>
> >> 1. If the rcar-csi2 sub-device's source pad format is changed via media-ctl,
> >> the connected VIN's crop bounds (vin->source rectangle) will be stale, since
> >> vin->source must always be equal to the source pad rectangle. So we have a
> >> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event sent
> >> from the rcar-csi2 sub-device when its source pad format is changed. In
> >> response, the VIN connected to that rcar-csi2 sub-device will reset the crop
> >> bounds to the new source pad format rectangle. In order to make this work
> >> however...
> > 
> > I'm no expert on when the crop/selection rectangles shall be reset and
> > have wrestled with this in the past for this driver. I have partial
> > rework of how formats especially crop/selection works planed. My goal of
> > that is to try and make it simpler and more straight forward taking
> > ideas from the vivid driver. There are some limitations in my
> > implementation of this in the current driver which prevents me from
> > adding sequential formats and enable the UDS scaler.
> 
> Ok. What I did for imx-media driver is to export a function from the 
> video capture interface that allows the source sub-device connected to 
> the capture interface to call it when the source pad format changes, 
> which gives the capture interface the chance to adjust compose window 

That sounds like a big hack, the two devices should be independent.
Let's not replicate this, and it should be fixed in the imx driver.

> (unlike rcar-vin, imx capture interface doesn't crop or scale, the 
> connected source subdev does that, but it does pad the compose window so 
> it needs to know the original compose window, sorry hope I haven't lost 
> you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is 
> maybe a cleaner way to update the crop bounds in rcar-vin than using an 
> exported function.
> 
> >> 2. Sub-device events need to be forwarded to the VIN's that have enabled
> >> media links to the reporting sub-device. Currently, sub-device events are
> >> only forwarded to VIN7, because the notifier is registered only from VIN7,
> >> and so the sub-devices are registered only to the v4l2_dev owned by VIN7. So
> >> we have a set of patches that fix this: sub-device events get forwarded to
> >> the VIN's that have connected media paths to that sub-device. Besides
> >> allowing to reset the VIN crop bounds rectangle asynchronously, this also
> >> seems to be logically the correct behavior. It also makes the user interface
> >> a little more intuitive: userland knows which VIN it is capturing on, and
> >> that is the same VIN that will be receiving sub-device events in the active
> >> pipeline.
> > 
> > This is a problem I ponder from time to time, happy to hear I'm not the
> > only one :-) My view is that events are somewhat not functional in the
> > media controller world and should be fixed at the framework level, maybe
> > by moving them from the vdev to the mdev :-)
> 
> How about embedding the 'struct v4l2_device' into 'struct rvin_group' 
> instead of 'struct rvin_dev'? That probably makes more sense, the 
> v4l2_dev keeps track of media-device-wide info such as the list of 
> sub-devices in the graph, so it seems more appropriate to create only 
> one instance of v4l2_dev. That will force rcar-vin to lookup the correct 
> VINs to forward the event to, since v4l2_dev is no longer attached to 
> the VINs.
> 
> But moving events from v4l2 to media (e.g. media entities send events to 
> the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev) 
> is also an interesting idea.
> 
> > I think it's great that you worked on the problem but I'm a bit
> > concerned with addressing this on a driver basis. Maybe this is
> > something to bring up at the next v4l2 summit? I might be wrong here and
> > there already is a consensus to address this as you describe in each
> > driver. Do you have any insights on the topic?
> 
> I haven't been tuned into that topic, but yes I agree that events need a 
> better framework.
> 
> >> 3. We have some patches under review that allow alternate -> none field
> >> mode. That is, source sub-device is sending alternate fields (top, bottom,
> >> top, ...), and userland is configured to receive those fields unmodified by
> >> setting field type to none. rcar-dma then detects and reports the field type
> >> of the captured field (top or bottom) in (struct v4l2_buffer)->field. Doing
> >> this allows for de-interlacing the captured fields using the FDP1 mem2mem
> >> device.
> >
> > Interesting, maybe I'm missing something but would it not be a better
> > solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
> > abusing V4L2_FIELD_NONE?
> 
> Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field 
> frames (top, bottom and alternate fields)"),
> 
> "There was never proper support in the VIN driver to deliver ALTERNATING 
> field format to user-space, remove this field option. The problem is 
> that ALTERNATING field order requires the sequence numbers of buffers 
> returned to userspace to reflect if fields were dropped or not, 
> something which is not possible with the VIN drivers capture logic."
> 
> Is that still a concern, or can alternate mode be brought back but with 
> possibly the above limitation?
> 
> Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it 
> sounds like using this field type in this way may not be too much abuse :)
> 
> "Images are in progressive format, not interlaced. The driver may also 
> indicate this order when it cannot distinguish 
> between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."
> 
> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html
> 
> >> There are some other miscellaneous patches that I can submit, but the above
> >> describes the bulk of the changes.
> > I'm happy to see work being done on the driver and would of course like
> > to help you get all changes upstream so that all your use-cases would
> > work out-of-the-box.
> >
> >> Before I start on porting these patches to the media-tree, do the above
> >> changes make general sense, or are there fundamental problems with those
> >> ideas?
> >>
> >>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
> >>>
> >>> Change the vin_to_source() macro to an inline function that will
> >>> retrieve the source subdevice for both media-control and non
> >>> media-control mode.
> >>>
> >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>> ---
> >>> [..]
> >>>
> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> >>> index 0b13b34d03e3..29d8c4a80c35 100644
> >>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> >>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> >>> @@ -217,7 +217,21 @@ struct rvin_dev {
> >>>    	v4l2_std_id std;
> >>>    };
> >>> -#define vin_to_source(vin)		((vin)->parallel->subdev)
> >>> +static inline struct v4l2_subdev *
> >>> +vin_to_source(struct rvin_dev *vin)
> >>> +{
> >>> +	if (vin->info->use_mc) {
> >>> +		struct media_pad *pad;
> >>> +
> >>> +		pad = media_entity_remote_pad(&vin->pad);
> >>> +		if (!pad)
> >>> +			return NULL;
> >>> +
> >>> +		return media_entity_to_v4l2_subdev(pad->entity);
> >>> +	}
> >>> +
> >>> +	return vin->parallel->subdev;
> >>> +}
> >>>    /* Debug */
> >>>    #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
> >>>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Steve Longerbeam]

Hi Laurent,

On 4/9/19 1:57 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> On Mon, Apr 08, 2019 at 08:58:25PM -0700, Steve Longerbeam wrote:
>> On 4/8/19 4:12 AM, Niklas Söderlund wrote:
>>> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
>>>> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
>>>>> Hi Niklas, Jacopo cc: Steve
>>>>>
>>>>> Apologize for reviving this old thread. Just one question below.
>>>>>
>>>>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
>>>>>
>>>>> [..]
>>>>>
>>>>>>> +
>>>>>>>     #define vin_to_source(vin)		((vin)->parallel->subdev)
>>>>>> This in particular I hate and at some point I hope to remove it or
>>>>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
>>>>>> related to  your patch-set.
>>>>> What about below patch excerpt (courtesy of Steve) which is currently
>>>>> under review in our tree? If we are on the same page here, we would
>>>>> happily contribute a patch to you based on below.
>>>> I can submit this patch to media-tree ML for formal review.
>>> I see no problem with the patch itself but as you point out bellow there
>>> are other patches which makes of the change I assume? The change to
>>> vin_to_source() on it's own would not add much value as vin_to_source()
>>> is currently only used in the none-mc parts of the driver where the
>>> change bellow would never be used.
>>>
>>> My dream would be to one day drop the none-mc mode of this driver, or
>>> split it out to a rcar-vin-legacy module or something ;-)
>> Yes, that's something that has been confusing me. Why does there need to
>> be a distinction between a media control mode non-mc mode in the
>> rcar-vin driver? I understand the "digital"/"parallel" mode is to handle
>> sensors with a parallel interface (as opposed to MIPI CSI-2), but why
>> not make the parallel sensors also require media-control
>> (enabling/disable media links, etc.)?
>>
>>>> But there are also other patches to rcar-vin applied to the tree mentioned
>>>> by Eugeniu, which can broadly be described as:
>>>>
>>>> 1. If the rcar-csi2 sub-device's source pad format is changed via media-ctl,
>>>> the connected VIN's crop bounds (vin->source rectangle) will be stale, since
>>>> vin->source must always be equal to the source pad rectangle. So we have a
>>>> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event sent
>>>> from the rcar-csi2 sub-device when its source pad format is changed. In
>>>> response, the VIN connected to that rcar-csi2 sub-device will reset the crop
>>>> bounds to the new source pad format rectangle. In order to make this work
>>>> however...
>>> I'm no expert on when the crop/selection rectangles shall be reset and
>>> have wrestled with this in the past for this driver. I have partial
>>> rework of how formats especially crop/selection works planed. My goal of
>>> that is to try and make it simpler and more straight forward taking
>>> ideas from the vivid driver. There are some limitations in my
>>> implementation of this in the current driver which prevents me from
>>> adding sequential formats and enable the UDS scaler.
>> Ok. What I did for imx-media driver is to export a function from the
>> video capture interface that allows the source sub-device connected to
>> the capture interface to call it when the source pad format changes,
>> which gives the capture interface the chance to adjust compose window
> That sounds like a big hack, the two devices should be independent.
> Let's not replicate this, and it should be fixed in the imx driver.

I admit it's a bit of a hack, but it is there to get around a limitation 
of media/v4l2 framework. And the two devices are not independent, they 
are connected the same way other media entities are connected. The 
media-ctl tool will take care of propagating formats from connected 
source -> sink sub-devices, but it stops at the sub-device -> 
video_device interface. First, this propagation should happen in the 
kernel at the time a source pad format is modified, rather than by a 
user tool, because it is a media/v4l2 core requirement that source -> 
sink formats must be equal before streaming starts. Second, a source pad 
format change should also be propagated to connected video devices. 
Userland is then free to make modifications to the video_device format 
if the latter supports cropping/scaling etc., but the default should be 
to reset the format at the time the connected sub-device format has changed.


Steve

>
>> (unlike rcar-vin, imx capture interface doesn't crop or scale, the
>> connected source subdev does that, but it does pad the compose window so
>> it needs to know the original compose window, sorry hope I haven't lost
>> you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is
>> maybe a cleaner way to update the crop bounds in rcar-vin than using an
>> exported function.
>>
>>>> 2. Sub-device events need to be forwarded to the VIN's that have enabled
>>>> media links to the reporting sub-device. Currently, sub-device events are
>>>> only forwarded to VIN7, because the notifier is registered only from VIN7,
>>>> and so the sub-devices are registered only to the v4l2_dev owned by VIN7. So
>>>> we have a set of patches that fix this: sub-device events get forwarded to
>>>> the VIN's that have connected media paths to that sub-device. Besides
>>>> allowing to reset the VIN crop bounds rectangle asynchronously, this also
>>>> seems to be logically the correct behavior. It also makes the user interface
>>>> a little more intuitive: userland knows which VIN it is capturing on, and
>>>> that is the same VIN that will be receiving sub-device events in the active
>>>> pipeline.
>>> This is a problem I ponder from time to time, happy to hear I'm not the
>>> only one :-) My view is that events are somewhat not functional in the
>>> media controller world and should be fixed at the framework level, maybe
>>> by moving them from the vdev to the mdev :-)
>> How about embedding the 'struct v4l2_device' into 'struct rvin_group'
>> instead of 'struct rvin_dev'? That probably makes more sense, the
>> v4l2_dev keeps track of media-device-wide info such as the list of
>> sub-devices in the graph, so it seems more appropriate to create only
>> one instance of v4l2_dev. That will force rcar-vin to lookup the correct
>> VINs to forward the event to, since v4l2_dev is no longer attached to
>> the VINs.
>>
>> But moving events from v4l2 to media (e.g. media entities send events to
>> the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev)
>> is also an interesting idea.
>>
>>> I think it's great that you worked on the problem but I'm a bit
>>> concerned with addressing this on a driver basis. Maybe this is
>>> something to bring up at the next v4l2 summit? I might be wrong here and
>>> there already is a consensus to address this as you describe in each
>>> driver. Do you have any insights on the topic?
>> I haven't been tuned into that topic, but yes I agree that events need a
>> better framework.
>>
>>>> 3. We have some patches under review that allow alternate -> none field
>>>> mode. That is, source sub-device is sending alternate fields (top, bottom,
>>>> top, ...), and userland is configured to receive those fields unmodified by
>>>> setting field type to none. rcar-dma then detects and reports the field type
>>>> of the captured field (top or bottom) in (struct v4l2_buffer)->field. Doing
>>>> this allows for de-interlacing the captured fields using the FDP1 mem2mem
>>>> device.
>>> Interesting, maybe I'm missing something but would it not be a better
>>> solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
>>> abusing V4L2_FIELD_NONE?
>> Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field
>> frames (top, bottom and alternate fields)"),
>>
>> "There was never proper support in the VIN driver to deliver ALTERNATING
>> field format to user-space, remove this field option. The problem is
>> that ALTERNATING field order requires the sequence numbers of buffers
>> returned to userspace to reflect if fields were dropped or not,
>> something which is not possible with the VIN drivers capture logic."
>>
>> Is that still a concern, or can alternate mode be brought back but with
>> possibly the above limitation?
>>
>> Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it
>> sounds like using this field type in this way may not be too much abuse :)
>>
>> "Images are in progressive format, not interlaced. The driver may also
>> indicate this order when it cannot distinguish
>> between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."
>>
>> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html
>>
>>>> There are some other miscellaneous patches that I can submit, but the above
>>>> describes the bulk of the changes.
>>> I'm happy to see work being done on the driver and would of course like
>>> to help you get all changes upstream so that all your use-cases would
>>> work out-of-the-box.
>>>
>>>> Before I start on porting these patches to the media-tree, do the above
>>>> changes make general sense, or are there fundamental problems with those
>>>> ideas?
>>>>
>>>>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
>>>>>
>>>>> Change the vin_to_source() macro to an inline function that will
>>>>> retrieve the source subdevice for both media-control and non
>>>>> media-control mode.
>>>>>
>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>> ---
>>>>> [..]
>>>>>
>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>> index 0b13b34d03e3..29d8c4a80c35 100644
>>>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>> @@ -217,7 +217,21 @@ struct rvin_dev {
>>>>>     	v4l2_std_id std;
>>>>>     };
>>>>> -#define vin_to_source(vin)		((vin)->parallel->subdev)
>>>>> +static inline struct v4l2_subdev *
>>>>> +vin_to_source(struct rvin_dev *vin)
>>>>> +{
>>>>> +	if (vin->info->use_mc) {
>>>>> +		struct media_pad *pad;
>>>>> +
>>>>> +		pad = media_entity_remote_pad(&vin->pad);
>>>>> +		if (!pad)
>>>>> +			return NULL;
>>>>> +
>>>>> +		return media_entity_to_v4l2_subdev(pad->entity);
>>>>> +	}
>>>>> +
>>>>> +	return vin->parallel->subdev;
>>>>> +}
>>>>>     /* Debug */
>>>>>     #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
>>>>>

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Steve Longerbeam]

On 4/9/19 6:25 AM, Niklas Söderlund wrote:
> Hi Steve, Kieran,
>
> On 2019-04-09 10:10:45 +0100, Kieran Bingham wrote:
>> Hi Steve, Niklas,
>>
>> Just a small 2-cents below:
>>
>> On 09/04/2019 04:58, Steve Longerbeam wrote:
>>>
>>> On 4/8/19 4:12 AM, Niklas Söderlund wrote:
>>>> Hi Steve, Eugeniu,
>>>>
>>>> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
>>>>> Hi Niklas, all,
>>>>>
>>>>>
>>>>> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
>>>>>> Hi Niklas, Jacopo cc: Steve
>>>>>>
>>>>>> Apologize for reviving this old thread. Just one question below.
>>>>>>
>>>>>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
>>>>>>
>>>>>> [..]
>>>>>>
>>>>>>>> +
>>>>>>>>     #define vin_to_source(vin)        ((vin)->parallel->subdev)
>>>>>>> This in particular I hate and at some point I hope to remove it or
>>>>>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
>>>>>>> related to  your patch-set.
>>>>>> What about below patch excerpt (courtesy of Steve) which is currently
>>>>>> under review in our tree? If we are on the same page here, we would
>>>>>> happily contribute a patch to you based on below.
>>>>> I can submit this patch to media-tree ML for formal review.
>>>> I see no problem with the patch itself but as you point out bellow there
>>>> are other patches which makes of the change I assume? The change to
>>>> vin_to_source() on it's own would not add much value as vin_to_source()
>>>> is currently only used in the none-mc parts of the driver where the
>>>> change bellow would never be used.
>>>>
>>>> My dream would be to one day drop the none-mc mode of this driver, or
>>>> split it out to a rcar-vin-legacy module or something ;-)
>>> Yes, that's something that has been confusing me. Why does there need to
>>> be a distinction between a media control mode non-mc mode in the
>>> rcar-vin driver? I understand the "digital"/"parallel" mode is to handle
>>> sensors with a parallel interface (as opposed to MIPI CSI-2), but why
>>> not make the parallel sensors also require media-control
>>> (enabling/disable media links, etc.)?
>>>
>>>
> The issue is backward compatibility. The original driver targeted
> Renesas Gen2 board which was relatively simple and might even pre-date
> the media controller api. When I extended the diver to support Gen3
> boards which are more complex I had to add media controller support to
> the driver. There is a huge overlap (rcar-dma.c) between Gen2 and Gen3
> and it seemed silly to have two drivers.

Ok, I suspected it was a backward compatibility issue, makes sense.

> As a side note Jacopo added support for the parallel node in the media
> controller for Gen3 boards which have the same pipeline as Gen2. So
> everything needed to remove the none MC code paths are in the driver but
> I'm afraid that would upset current users on Gen2 ;-)
>
>>>>> But there are also other patches to rcar-vin applied to the tree
>>>>> mentioned
>>>>> by Eugeniu, which can broadly be described as:
>>>>>
>>>>> 1. If the rcar-csi2 sub-device's source pad format is changed via
>>>>> media-ctl,
>>>>> the connected VIN's crop bounds (vin->source rectangle) will be
>>>>> stale, since
>>>>> vin->source must always be equal to the source pad rectangle. So we
>>>>> have a
>>>>> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event
>>>>> sent
>>>>> from the rcar-csi2 sub-device when its source pad format is changed. In
>>>>> response, the VIN connected to that rcar-csi2 sub-device will reset
>>>>> the crop
>>>>> bounds to the new source pad format rectangle. In order to make this
>>>>> work
>>>>> however...
>>>> I'm no expert on when the crop/selection rectangles shall be reset and
>>>> have wrestled with this in the past for this driver. I have partial
>>>> rework of how formats especially crop/selection works planed. My goal of
>>>> that is to try and make it simpler and more straight forward taking
>>>> ideas from the vivid driver. There are some limitations in my
>>>> implementation of this in the current driver which prevents me from
>>>> adding sequential formats and enable the UDS scaler.
>>> Ok. What I did for imx-media driver is to export a function from the
>>> video capture interface that allows the source sub-device connected to
>>> the capture interface to call it when the source pad format changes,
>>> which gives the capture interface the chance to adjust compose window
>>> (unlike rcar-vin, imx capture interface doesn't crop or scale, the
>>> connected source subdev does that, but it does pad the compose window so
>>> it needs to know the original compose window, sorry hope I haven't lost
>>> you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is
>>> maybe a cleaner way to update the crop bounds in rcar-vin than using an
>>> exported function.
>>>
> I agree V4L2_EVENT_SOURCE_CHANGE could be a nice way of doing this if
> the event routing is sorted out.

Ok.

But also, in my reply to Laurent, I brought the idea that there needs to 
be a mechanism in v4l2-core that will take care of this, e.g. 
automatically propagate formats from a sub-device source pad to a 
connected video device. I haven't looked into that idea in more detail 
so there are probably problems to overcome.

>>>
>>>>> 2. Sub-device events need to be forwarded to the VIN's that have enabled
>>>>> media links to the reporting sub-device. Currently, sub-device events
>>>>> are
>>>>> only forwarded to VIN7, because the notifier is registered only from
>>>>> VIN7,
>>>>> and so the sub-devices are registered only to the v4l2_dev owned by
>>>>> VIN7. So
>>>>> we have a set of patches that fix this: sub-device events get
>>>>> forwarded to
>>>>> the VIN's that have connected media paths to that sub-device. Besides
>>>>> allowing to reset the VIN crop bounds rectangle asynchronously, this
>>>>> also
>>>>> seems to be logically the correct behavior. It also makes the user
>>>>> interface
>>>>> a little more intuitive: userland knows which VIN it is capturing on,
>>>>> and
>>>>> that is the same VIN that will be receiving sub-device events in the
>>>>> active
>>>>> pipeline.
>>>> This is a problem I ponder from time to time, happy to hear I'm not the
>>>> only one :-) My view is that events are somewhat not functional in the
>>>> media controller world and should be fixed at the framework level, maybe
>>>> by moving them from the vdev to the mdev :-)
>>> How about embedding the 'struct v4l2_device' into 'struct rvin_group'
>>> instead of 'struct rvin_dev'? That probably makes more sense, the
>>> v4l2_dev keeps track of media-device-wide info such as the list of
>>> sub-devices in the graph, so it seems more appropriate to create only
>>> one instance of v4l2_dev. That will force rcar-vin to lookup the correct
>>> VINs to forward the event to, since v4l2_dev is no longer attached to
>>> the VINs.
> It might solve the event routing problem but will create others. For
> example each VIN instance can have a private notifier if it has a
> parallel subdevice and be part of VIN group for the ones coming from
> CSI-2.

Ah, ok, thanks for the warning. I would have run into that issue 
eventually, but now I won'r waste my time :)


>>> But moving events from v4l2 to media (e.g. media entities send events to
>>> the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev)
>>> is also an interesting idea.
> I think this is the right way to go.

Yes, especially because events are not limited to video, they are 
applicable to other kinds of media devices.

>
>>>> I think it's great that you worked on the problem but I'm a bit
>>>> concerned with addressing this on a driver basis. Maybe this is
>>>> something to bring up at the next v4l2 summit? I might be wrong here and
>>>> there already is a consensus to address this as you describe in each
>>>> driver. Do you have any insights on the topic?
>>> I haven't been tuned into that topic, but yes I agree that events need a
>>> better framework.
>>>
>>>
>>>>> 3. We have some patches under review that allow alternate -> none field
>>>>> mode. That is, source sub-device is sending alternate fields (top,
>>>>> bottom,
>>>>> top, ...), and userland is configured to receive those fields
>>>>> unmodified by
>>>>> setting field type to none. rcar-dma then detects and reports the
>>>>> field type
>>>>> of the captured field (top or bottom) in (struct v4l2_buffer)->field.
>>>>> Doing
>>>>> this allows for de-interlacing the captured fields using the FDP1
>>>>> mem2mem
>>>>> device.
>>>> Interesting, maybe I'm missing something but would it not be a better
>>>> solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
>>>> abusing V4L2_FIELD_NONE?
>>> Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field
>>> frames (top, bottom and alternate fields)"),
>>>
>>> "There was never proper support in the VIN driver to deliver ALTERNATING
>>> field format to user-space, remove this field option. The problem is
>>> that ALTERNATING field order requires the sequence numbers of buffers
>>> returned to userspace to reflect if fields were dropped or not,
>>> something which is not possible with the VIN drivers capture logic."
>>>
>>> Is that still a concern, or can alternate mode be brought back but with
>>> possibly the above limitation?
>> I think that might have been before we added the scratch buffer to
>> support continuous capture?
>>
>> Now that we have that - I think we should be able to correctly track
>> dropped frames right?
> I think you are correct Kieran, we now have the scratch buffer so it
> should be possible to add proper support for delivering ALTERNATING to
> userspace. I'm sure we find new issues on the way tho ;-)

Ok, but still it seems alternate -> none is a valid use-case? It allows 
rcar-vin to send unmodified fields to userland, for possible 
de-interlace by FDP1 mem2mem. Or does FDP1 support de-interlacing an 
INTERLACED stream (in addition to de-interlacing a sequential field stream)?


>
>>
>>> Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it
>>> sounds like using this field type in this way may not be too much abuse :)
> My bad I should have said using instead of abusing, sorry about that :-)
> I did not know that this was a valid use-case for FIELD_NONE so yes I
> think it could be used if we can't solve the requirements for adding
> ALTERNATING.

Ok, see above also.

Steve

>
>>> "Images are in progressive format, not interlaced. The driver may also
>>> indicate this order when it cannot distinguish
>>> between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."
>>>
>>> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html
>>>
>>>
>>> Steve
>>>
>>>>> There are some other miscellaneous patches that I can submit, but the
>>>>> above
>>>>> describes the bulk of the changes.
>>>> I'm happy to see work being done on the driver and would of course like
>>>> to help you get all changes upstream so that all your use-cases would
>>>> work out-of-the-box.
>>>>
>>>>> Before I start on porting these patches to the media-tree, do the above
>>>>> changes make general sense, or are there fundamental problems with those
>>>>> ideas?
>>>>>
>>>>> TIA,
>>>>> Steve
>>>>>
>>>>>
>>>>>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
>>>>>>
>>>>>> Change the vin_to_source() macro to an inline function that will
>>>>>> retrieve the source subdevice for both media-control and non
>>>>>> media-control mode.
>>>>>>
>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>> ---
>>>>>> [..]
>>>>>>
>>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>> b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>> index 0b13b34d03e3..29d8c4a80c35 100644
>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>> @@ -217,7 +217,21 @@ struct rvin_dev {
>>>>>>         v4l2_std_id std;
>>>>>>     };
>>>>>> -#define vin_to_source(vin)        ((vin)->parallel->subdev)
>>>>>> +static inline struct v4l2_subdev *
>>>>>> +vin_to_source(struct rvin_dev *vin)
>>>>>> +{
>>>>>> +    if (vin->info->use_mc) {
>>>>>> +        struct media_pad *pad;
>>>>>> +
>>>>>> +        pad = media_entity_remote_pad(&vin->pad);
>>>>>> +        if (!pad)
>>>>>> +            return NULL;
>>>>>> +
>>>>>> +        return media_entity_to_v4l2_subdev(pad->entity);
>>>>>> +    }
>>>>>> +
>>>>>> +    return vin->parallel->subdev;
>>>>>> +}
>>>>>>     /* Debug */
>>>>>>     #define vin_dbg(d, fmt, arg...)        dev_dbg(d->dev, fmt, ##arg)
>>>>>>
>>>>>> Best regards,
>>>>>> Eugeniu.
>> -- 
>> Regards
>> --
>> Kieran

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Laurent Pinchart]

Hi Steve,

On Tue, Apr 09, 2019 at 03:22:47PM -0700, Steve Longerbeam wrote:
> On 4/9/19 1:57 PM, Laurent Pinchart wrote:
> > On Mon, Apr 08, 2019 at 08:58:25PM -0700, Steve Longerbeam wrote:
> >> On 4/8/19 4:12 AM, Niklas Söderlund wrote:
> >>> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
> >>>> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
> >>>>> Hi Niklas, Jacopo cc: Steve
> >>>>>
> >>>>> Apologize for reviving this old thread. Just one question below.
> >>>>>
> >>>>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
> >>>>>
> >>>>> [..]
> >>>>>
> >>>>>>> +
> >>>>>>>     #define vin_to_source(vin)		((vin)->parallel->subdev)
> >>>>>> 
> >>>>>> This in particular I hate and at some point I hope to remove it or
> >>>>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
> >>>>>> related to  your patch-set.
> >>>>> 
> >>>>> What about below patch excerpt (courtesy of Steve) which is currently
> >>>>> under review in our tree? If we are on the same page here, we would
> >>>>> happily contribute a patch to you based on below.
> >>>> 
> >>>> I can submit this patch to media-tree ML for formal review.
> >>> 
> >>> I see no problem with the patch itself but as you point out bellow there
> >>> are other patches which makes of the change I assume? The change to
> >>> vin_to_source() on it's own would not add much value as vin_to_source()
> >>> is currently only used in the none-mc parts of the driver where the
> >>> change bellow would never be used.
> >>>
> >>> My dream would be to one day drop the none-mc mode of this driver, or
> >>> split it out to a rcar-vin-legacy module or something ;-)
> >> 
> >> Yes, that's something that has been confusing me. Why does there need to
> >> be a distinction between a media control mode non-mc mode in the
> >> rcar-vin driver? I understand the "digital"/"parallel" mode is to handle
> >> sensors with a parallel interface (as opposed to MIPI CSI-2), but why
> >> not make the parallel sensors also require media-control
> >> (enabling/disable media links, etc.)?
> >>
> >>>> But there are also other patches to rcar-vin applied to the tree mentioned
> >>>> by Eugeniu, which can broadly be described as:
> >>>>
> >>>> 1. If the rcar-csi2 sub-device's source pad format is changed via media-ctl,
> >>>> the connected VIN's crop bounds (vin->source rectangle) will be stale, since
> >>>> vin->source must always be equal to the source pad rectangle. So we have a
> >>>> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event sent
> >>>> from the rcar-csi2 sub-device when its source pad format is changed. In
> >>>> response, the VIN connected to that rcar-csi2 sub-device will reset the crop
> >>>> bounds to the new source pad format rectangle. In order to make this work
> >>>> however...
> >>> 
> >>> I'm no expert on when the crop/selection rectangles shall be reset and
> >>> have wrestled with this in the past for this driver. I have partial
> >>> rework of how formats especially crop/selection works planed. My goal of
> >>> that is to try and make it simpler and more straight forward taking
> >>> ideas from the vivid driver. There are some limitations in my
> >>> implementation of this in the current driver which prevents me from
> >>> adding sequential formats and enable the UDS scaler.
> >> 
> >> Ok. What I did for imx-media driver is to export a function from the
> >> video capture interface that allows the source sub-device connected to
> >> the capture interface to call it when the source pad format changes,
> >> which gives the capture interface the chance to adjust compose window
> > 
> > That sounds like a big hack, the two devices should be independent.
> > Let's not replicate this, and it should be fixed in the imx driver.
> 
> I admit it's a bit of a hack, but it is there to get around a limitation 
> of media/v4l2 framework. And the two devices are not independent, they 
> are connected the same way other media entities are connected. The 
> media-ctl tool will take care of propagating formats from connected 
> source -> sink sub-devices, but it stops at the sub-device -> 
> video_device interface. First, this propagation should happen in the 
> kernel at the time a source pad format is modified, rather than by a 
> user tool, because it is a media/v4l2 core requirement that source -> 
> sink formats must be equal before streaming starts.

That's not right. It's a requirement that the formats are compatible to
start streaming, but it's not a requirement *before* starting streaming.
That's why kernel drivers must not propagate formats between entities,
only within an entity. Doing in-kernel propagation between entities is
an API violation.

> Second, a source pad format change should also be propagated to
> connected video devices. Userland is then free to make modifications
> to the video_device format if the latter supports cropping/scaling
> etc., but the default should be to reset the format at the time the
> connected sub-device format has changed.

For the same reason above, the format on video nodes must be set by
applications. Could you please fix it in the imx driver ?

> >> (unlike rcar-vin, imx capture interface doesn't crop or scale, the
> >> connected source subdev does that, but it does pad the compose window so
> >> it needs to know the original compose window, sorry hope I haven't lost
> >> you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is
> >> maybe a cleaner way to update the crop bounds in rcar-vin than using an
> >> exported function.
> >>
> >>>> 2. Sub-device events need to be forwarded to the VIN's that have enabled
> >>>> media links to the reporting sub-device. Currently, sub-device events are
> >>>> only forwarded to VIN7, because the notifier is registered only from VIN7,
> >>>> and so the sub-devices are registered only to the v4l2_dev owned by VIN7. So
> >>>> we have a set of patches that fix this: sub-device events get forwarded to
> >>>> the VIN's that have connected media paths to that sub-device. Besides
> >>>> allowing to reset the VIN crop bounds rectangle asynchronously, this also
> >>>> seems to be logically the correct behavior. It also makes the user interface
> >>>> a little more intuitive: userland knows which VIN it is capturing on, and
> >>>> that is the same VIN that will be receiving sub-device events in the active
> >>>> pipeline.
> >>> 
> >>> This is a problem I ponder from time to time, happy to hear I'm not the
> >>> only one :-) My view is that events are somewhat not functional in the
> >>> media controller world and should be fixed at the framework level, maybe
> >>> by moving them from the vdev to the mdev :-)
> >> 
> >> How about embedding the 'struct v4l2_device' into 'struct rvin_group'
> >> instead of 'struct rvin_dev'? That probably makes more sense, the
> >> v4l2_dev keeps track of media-device-wide info such as the list of
> >> sub-devices in the graph, so it seems more appropriate to create only
> >> one instance of v4l2_dev. That will force rcar-vin to lookup the correct
> >> VINs to forward the event to, since v4l2_dev is no longer attached to
> >> the VINs.
> >>
> >> But moving events from v4l2 to media (e.g. media entities send events to
> >> the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev)
> >> is also an interesting idea.
> >>
> >>> I think it's great that you worked on the problem but I'm a bit
> >>> concerned with addressing this on a driver basis. Maybe this is
> >>> something to bring up at the next v4l2 summit? I might be wrong here and
> >>> there already is a consensus to address this as you describe in each
> >>> driver. Do you have any insights on the topic?
> >> 
> >> I haven't been tuned into that topic, but yes I agree that events need a
> >> better framework.
> >>
> >>>> 3. We have some patches under review that allow alternate -> none field
> >>>> mode. That is, source sub-device is sending alternate fields (top, bottom,
> >>>> top, ...), and userland is configured to receive those fields unmodified by
> >>>> setting field type to none. rcar-dma then detects and reports the field type
> >>>> of the captured field (top or bottom) in (struct v4l2_buffer)->field. Doing
> >>>> this allows for de-interlacing the captured fields using the FDP1 mem2mem
> >>>> device.
> >>> 
> >>> Interesting, maybe I'm missing something but would it not be a better
> >>> solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
> >>> abusing V4L2_FIELD_NONE?
> >> 
> >> Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field
> >> frames (top, bottom and alternate fields)"),
> >>
> >> "There was never proper support in the VIN driver to deliver ALTERNATING
> >> field format to user-space, remove this field option. The problem is
> >> that ALTERNATING field order requires the sequence numbers of buffers
> >> returned to userspace to reflect if fields were dropped or not,
> >> something which is not possible with the VIN drivers capture logic."
> >>
> >> Is that still a concern, or can alternate mode be brought back but with
> >> possibly the above limitation?
> >>
> >> Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it
> >> sounds like using this field type in this way may not be too much abuse :)
> >>
> >> "Images are in progressive format, not interlaced. The driver may also
> >> indicate this order when it cannot distinguish
> >> between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."
> >>
> >> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html
> >>
> >>>> There are some other miscellaneous patches that I can submit, but the above
> >>>> describes the bulk of the changes.
> >>> I'm happy to see work being done on the driver and would of course like
> >>> to help you get all changes upstream so that all your use-cases would
> >>> work out-of-the-box.
> >>>
> >>>> Before I start on porting these patches to the media-tree, do the above
> >>>> changes make general sense, or are there fundamental problems with those
> >>>> ideas?
> >>>>
> >>>>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
> >>>>>
> >>>>> Change the vin_to_source() macro to an inline function that will
> >>>>> retrieve the source subdevice for both media-control and non
> >>>>> media-control mode.
> >>>>>
> >>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>> ---
> >>>>> [..]
> >>>>>
> >>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> >>>>> index 0b13b34d03e3..29d8c4a80c35 100644
> >>>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> >>>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> >>>>> @@ -217,7 +217,21 @@ struct rvin_dev {
> >>>>>     	v4l2_std_id std;
> >>>>>     };
> >>>>> -#define vin_to_source(vin)		((vin)->parallel->subdev)
> >>>>> +static inline struct v4l2_subdev *
> >>>>> +vin_to_source(struct rvin_dev *vin)
> >>>>> +{
> >>>>> +	if (vin->info->use_mc) {
> >>>>> +		struct media_pad *pad;
> >>>>> +
> >>>>> +		pad = media_entity_remote_pad(&vin->pad);
> >>>>> +		if (!pad)
> >>>>> +			return NULL;
> >>>>> +
> >>>>> +		return media_entity_to_v4l2_subdev(pad->entity);
> >>>>> +	}
> >>>>> +
> >>>>> +	return vin->parallel->subdev;
> >>>>> +}
> >>>>>     /* Debug */
> >>>>>     #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
> >>>>>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Steve Longerbeam]

On 4/9/19 3:59 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> On Tue, Apr 09, 2019 at 03:22:47PM -0700, Steve Longerbeam wrote:
>> On 4/9/19 1:57 PM, Laurent Pinchart wrote:
>>> On Mon, Apr 08, 2019 at 08:58:25PM -0700, Steve Longerbeam wrote:
>>>> On 4/8/19 4:12 AM, Niklas Söderlund wrote:
>>>>> On 2019-04-05 17:04:50 -0700, Steve Longerbeam wrote:
>>>>>> On 4/5/19 9:16 AM, Eugeniu Rosca wrote:
>>>>>>> Hi Niklas, Jacopo cc: Steve
>>>>>>>
>>>>>>> Apologize for reviving this old thread. Just one question below.
>>>>>>>
>>>>>>> On 2018-05-24 10:14:52, Niklas Söderlund wrote:
>>>>>>>
>>>>>>> [..]
>>>>>>>
>>>>>>>>> +
>>>>>>>>>      #define vin_to_source(vin)		((vin)->parallel->subdev)
>>>>>>>> This in particular I hate and at some point I hope to remove it or
>>>>>>>> move  it to rcar-v4l2.c. :-) But that is a task for later and not
>>>>>>>> related to  your patch-set.
>>>>>>> What about below patch excerpt (courtesy of Steve) which is currently
>>>>>>> under review in our tree? If we are on the same page here, we would
>>>>>>> happily contribute a patch to you based on below.
>>>>>> I can submit this patch to media-tree ML for formal review.
>>>>> I see no problem with the patch itself but as you point out bellow there
>>>>> are other patches which makes of the change I assume? The change to
>>>>> vin_to_source() on it's own would not add much value as vin_to_source()
>>>>> is currently only used in the none-mc parts of the driver where the
>>>>> change bellow would never be used.
>>>>>
>>>>> My dream would be to one day drop the none-mc mode of this driver, or
>>>>> split it out to a rcar-vin-legacy module or something ;-)
>>>> Yes, that's something that has been confusing me. Why does there need to
>>>> be a distinction between a media control mode non-mc mode in the
>>>> rcar-vin driver? I understand the "digital"/"parallel" mode is to handle
>>>> sensors with a parallel interface (as opposed to MIPI CSI-2), but why
>>>> not make the parallel sensors also require media-control
>>>> (enabling/disable media links, etc.)?
>>>>
>>>>>> But there are also other patches to rcar-vin applied to the tree mentioned
>>>>>> by Eugeniu, which can broadly be described as:
>>>>>>
>>>>>> 1. If the rcar-csi2 sub-device's source pad format is changed via media-ctl,
>>>>>> the connected VIN's crop bounds (vin->source rectangle) will be stale, since
>>>>>> vin->source must always be equal to the source pad rectangle. So we have a
>>>>>> patch that will catch the asynchronous V4L2_EVENT_SOURCE_CHANGE event sent
>>>>>> from the rcar-csi2 sub-device when its source pad format is changed. In
>>>>>> response, the VIN connected to that rcar-csi2 sub-device will reset the crop
>>>>>> bounds to the new source pad format rectangle. In order to make this work
>>>>>> however...
>>>>> I'm no expert on when the crop/selection rectangles shall be reset and
>>>>> have wrestled with this in the past for this driver. I have partial
>>>>> rework of how formats especially crop/selection works planed. My goal of
>>>>> that is to try and make it simpler and more straight forward taking
>>>>> ideas from the vivid driver. There are some limitations in my
>>>>> implementation of this in the current driver which prevents me from
>>>>> adding sequential formats and enable the UDS scaler.
>>>> Ok. What I did for imx-media driver is to export a function from the
>>>> video capture interface that allows the source sub-device connected to
>>>> the capture interface to call it when the source pad format changes,
>>>> which gives the capture interface the chance to adjust compose window
>>> That sounds like a big hack, the two devices should be independent.
>>> Let's not replicate this, and it should be fixed in the imx driver.
>> I admit it's a bit of a hack, but it is there to get around a limitation
>> of media/v4l2 framework. And the two devices are not independent, they
>> are connected the same way other media entities are connected. The
>> media-ctl tool will take care of propagating formats from connected
>> source -> sink sub-devices, but it stops at the sub-device ->
>> video_device interface. First, this propagation should happen in the
>> kernel at the time a source pad format is modified, rather than by a
>> user tool, because it is a media/v4l2 core requirement that source ->
>> sink formats must be equal before streaming starts.
> That's not right. It's a requirement that the formats are compatible to
> start streaming, but it's not a requirement *before* starting streaming.
> That's why kernel drivers must not propagate formats between entities,
> only within an entity. Doing in-kernel propagation between entities is
> an API violation.
>
>> Second, a source pad format change should also be propagated to
>> connected video devices. Userland is then free to make modifications
>> to the video_device format if the latter supports cropping/scaling
>> etc., but the default should be to reset the format at the time the
>> connected sub-device format has changed.
> For the same reason above, the format on video nodes must be set by
> applications. Could you please fix it in the imx driver ?

Well, if it's an API violation then I'll remove it. But removing the 
propagation will put (more) burden on userland.  In imx the sub-device 
does the cropping and scaling, but the video_device connected  to it 
does not. Which means the compose rectangle in the video_device must 
always be the same as the sub-device compose rectangle.

Does sending the V4L2_EVENT_SOURCE_CHANGE event to automatically reset 
the video_device format when the sub-device format changes also violate 
the API? I don't see why it would, it's just making use of the event 
mechanism. If you agree then I will do the same in imx, and offer it as 
a patch to rcar-vin (to ensure the rcar video_device's crop bounds are 
always correct).

If you don't agree then perhaps imx and rcar-vin can at least do the 
equivalent of v4l2-core, which is to ensure the video_device formats are 
compatible with the connected sub-device before streaming can start.


Steve



>
>>>> (unlike rcar-vin, imx capture interface doesn't crop or scale, the
>>>> connected source subdev does that, but it does pad the compose window so
>>>> it needs to know the original compose window, sorry hope I haven't lost
>>>> you). Anyway I think catching the V4L2_EVENT_SOURCE_CHANGE event is
>>>> maybe a cleaner way to update the crop bounds in rcar-vin than using an
>>>> exported function.
>>>>
>>>>>> 2. Sub-device events need to be forwarded to the VIN's that have enabled
>>>>>> media links to the reporting sub-device. Currently, sub-device events are
>>>>>> only forwarded to VIN7, because the notifier is registered only from VIN7,
>>>>>> and so the sub-devices are registered only to the v4l2_dev owned by VIN7. So
>>>>>> we have a set of patches that fix this: sub-device events get forwarded to
>>>>>> the VIN's that have connected media paths to that sub-device. Besides
>>>>>> allowing to reset the VIN crop bounds rectangle asynchronously, this also
>>>>>> seems to be logically the correct behavior. It also makes the user interface
>>>>>> a little more intuitive: userland knows which VIN it is capturing on, and
>>>>>> that is the same VIN that will be receiving sub-device events in the active
>>>>>> pipeline.
>>>>> This is a problem I ponder from time to time, happy to hear I'm not the
>>>>> only one :-) My view is that events are somewhat not functional in the
>>>>> media controller world and should be fixed at the framework level, maybe
>>>>> by moving them from the vdev to the mdev :-)
>>>> How about embedding the 'struct v4l2_device' into 'struct rvin_group'
>>>> instead of 'struct rvin_dev'? That probably makes more sense, the
>>>> v4l2_dev keeps track of media-device-wide info such as the list of
>>>> sub-devices in the graph, so it seems more appropriate to create only
>>>> one instance of v4l2_dev. That will force rcar-vin to lookup the correct
>>>> VINs to forward the event to, since v4l2_dev is no longer attached to
>>>> the VINs.
>>>>
>>>> But moving events from v4l2 to media (e.g. media entities send events to
>>>> the mdev, rather than v4l2 sub-devices sending events to the v4l2_dev)
>>>> is also an interesting idea.
>>>>
>>>>> I think it's great that you worked on the problem but I'm a bit
>>>>> concerned with addressing this on a driver basis. Maybe this is
>>>>> something to bring up at the next v4l2 summit? I might be wrong here and
>>>>> there already is a consensus to address this as you describe in each
>>>>> driver. Do you have any insights on the topic?
>>>> I haven't been tuned into that topic, but yes I agree that events need a
>>>> better framework.
>>>>
>>>>>> 3. We have some patches under review that allow alternate -> none field
>>>>>> mode. That is, source sub-device is sending alternate fields (top, bottom,
>>>>>> top, ...), and userland is configured to receive those fields unmodified by
>>>>>> setting field type to none. rcar-dma then detects and reports the field type
>>>>>> of the captured field (top or bottom) in (struct v4l2_buffer)->field. Doing
>>>>>> this allows for de-interlacing the captured fields using the FDP1 mem2mem
>>>>>> device.
>>>>> Interesting, maybe I'm missing something but would it not be a better
>>>>> solution to add support for V4L2_FIELD_ALTERNATE to rcar-vin instead of
>>>>> abusing V4L2_FIELD_NONE?
>>>> Well, from 6c51f646f6 ("media: rcar-vin: fix handling of single field
>>>> frames (top, bottom and alternate fields)"),
>>>>
>>>> "There was never proper support in the VIN driver to deliver ALTERNATING
>>>> field format to user-space, remove this field option. The problem is
>>>> that ALTERNATING field order requires the sequence numbers of buffers
>>>> returned to userspace to reflect if fields were dropped or not,
>>>> something which is not possible with the VIN drivers capture logic."
>>>>
>>>> Is that still a concern, or can alternate mode be brought back but with
>>>> possibly the above limitation?
>>>>
>>>> Also, there is this paragraph in [1] regarding V4L2_FIELD_NONE, so it
>>>> sounds like using this field type in this way may not be too much abuse :)
>>>>
>>>> "Images are in progressive format, not interlaced. The driver may also
>>>> indicate this order when it cannot distinguish
>>>> between|V4L2_FIELD_TOP|and|V4L2_FIELD_BOTTOM|."
>>>>
>>>> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html
>>>>
>>>>>> There are some other miscellaneous patches that I can submit, but the above
>>>>>> describes the bulk of the changes.
>>>>> I'm happy to see work being done on the driver and would of course like
>>>>> to help you get all changes upstream so that all your use-cases would
>>>>> work out-of-the-box.
>>>>>
>>>>>> Before I start on porting these patches to the media-tree, do the above
>>>>>> changes make general sense, or are there fundamental problems with those
>>>>>> ideas?
>>>>>>
>>>>>>> Subject: [PATCH] media: rcar-vin: Generalize vin_to_source()
>>>>>>>
>>>>>>> Change the vin_to_source() macro to an inline function that will
>>>>>>> retrieve the source subdevice for both media-control and non
>>>>>>> media-control mode.
>>>>>>>
>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>> ---
>>>>>>> [..]
>>>>>>>
>>>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>>> index 0b13b34d03e3..29d8c4a80c35 100644
>>>>>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>>>>>>> @@ -217,7 +217,21 @@ struct rvin_dev {
>>>>>>>      	v4l2_std_id std;
>>>>>>>      };
>>>>>>> -#define vin_to_source(vin)		((vin)->parallel->subdev)
>>>>>>> +static inline struct v4l2_subdev *
>>>>>>> +vin_to_source(struct rvin_dev *vin)
>>>>>>> +{
>>>>>>> +	if (vin->info->use_mc) {
>>>>>>> +		struct media_pad *pad;
>>>>>>> +
>>>>>>> +		pad = media_entity_remote_pad(&vin->pad);
>>>>>>> +		if (!pad)
>>>>>>> +			return NULL;
>>>>>>> +
>>>>>>> +		return media_entity_to_v4l2_subdev(pad->entity);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return vin->parallel->subdev;
>>>>>>> +}
>>>>>>>      /* Debug */
>>>>>>>      #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
>>>>>>>

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Eugeniu Rosca]

Hi Niklas,

Two simple questions:
 - I can only clone git://git.ragnatech.se/linux, but can't access it on
   the web, does it have some web interface?
 - Browsing through your v4l2/next/csi2/upport branch, it looks to be
   targeted for upstream. Could we rely on those commits already or
   they are still in development/testing?

TIA,
Eugeniu.

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Niklas Söderlund]

Hi Eugenie,

On 2019-04-11 22:28:19 +0200, Eugeniu Rosca wrote:
> Hi Niklas,
> 
> Two simple questions:
>  - I can only clone git://git.ragnatech.se/linux, but can't access it on
>    the web, does it have some web interface?

There should be a gitweb instance running at https://git.ragnatech.se/ 
if not let me know since then something is wrong ;-)

>  - Browsing through your v4l2/next/csi2/upport branch, it looks to be
>    targeted for upstream. Could we rely on those commits already or
>    they are still in development/testing?

The patches in that branch are target for upstreaming but sometime 
changes due to review comments. Best check the media-tree of what have 
accepted and the patchwork instance if you want day fresh statuses. I 
also rebase the branch from time to time on-top of the media-tree so you 
should not depend on it much I'm afraid.

If you plan to do some work on the rcar-csi2 driver you can always reach 
out and check what the status is if you are unsure.

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Eugeniu Rosca]

Hi Niklas,

On Thu, Apr 11, 2019 at 10:42:37PM +0200, Niklas Söderlund wrote:
> Hi Eugenie,
> 
> On 2019-04-11 22:28:19 +0200, Eugeniu Rosca wrote:
> > Hi Niklas,
> > 
> > Two simple questions:
> >  - I can only clone git://git.ragnatech.se/linux, but can't access it on
> >    the web, does it have some web interface?
> 
> There should be a gitweb instance running at https://git.ragnatech.se/ 
> if not let me know since then something is wrong ;-)

Yes, I am able to access the gitweb instance too :)

> 
> >  - Browsing through your v4l2/next/csi2/upport branch, it looks to be
> >    targeted for upstream. Could we rely on those commits already or
> >    they are still in development/testing?
> 
> The patches in that branch are target for upstreaming but sometime 
> changes due to review comments. Best check the media-tree of what have 
> accepted and the patchwork instance if you want day fresh statuses. I 
> also rebase the branch from time to time on-top of the media-tree so you 
> should not depend on it much I'm afraid.
> 
> If you plan to do some work on the rcar-csi2 driver you can always reach 
> out and check what the status is if you are unsure.

Understood. Thanks. Since I've gone off-topic quite a bit already, may I
have your feedback on below questions sitting in the back of my mind for
some time:

 - There seem to be quite a number of non-mainlined rcar-vin patches
   sitting on top of v4.14.75-ltsi and part of rcar-3.9.x release. With
   some false positives, around 35-40 such commits can be identified as
   shown in [1]. Based on my checks, only a small subset of them are
   (actively) discussed in linux-media/linux-renesas-soc ML and for most
   of them I can't find any trace. I am just curious if there is any
   background force driving the upstreaming of those patches or the
   submissions/discussions happen rather by accident?

 - I am seeing two sets of rcar-vin tests [2-3] and I think there is a
   parent/fork relationship between them, but now they appear to be
   diverged. Which one would you recommend? What's the test coverage
   provided by these suites? Do you see room/chance for something like
   drivers/gpu/drm/selftests in rcar-vin in future (maybe using
   the upcoming kunit [4] approach?). BTW, we've been using the
   vsp-tests for quite some time and they've been extremely useful in
   pointing out regressions in our own patches. So, many thanks for
   sharing those too.

> 
> -- 
> Regards,
> Niklas Söderlund

[1] git log --oneline --cherry-pick --right-only \
    torvalds/master...rcar-3.9.4 -- drivers/media/platform/rcar-vin/
[2] https://git.ragnatech.se/vin-tests/
[3] http://jmondi.org/git/vin-tests/
[4] https://patchwork.kernel.org/cover/10704157/

Best regards,
Eugeniu.

Re: [PATCH v3 3/9] media: rcar-vin: Create a group notifier

[Niklas Söderlund]

Hi Eugeniu,

On 2019-04-12 16:12:05 +0200, Eugeniu Rosca wrote:
> Hi Niklas,
> 
> On Thu, Apr 11, 2019 at 10:42:37PM +0200, Niklas Söderlund wrote:
> > Hi Eugenie,
> > 
> > On 2019-04-11 22:28:19 +0200, Eugeniu Rosca wrote:
> > > Hi Niklas,
> > > 
> > > Two simple questions:
> > >  - I can only clone git://git.ragnatech.se/linux, but can't access it on
> > >    the web, does it have some web interface?
> > 
> > There should be a gitweb instance running at https://git.ragnatech.se/ 
> > if not let me know since then something is wrong ;-)
> 
> Yes, I am able to access the gitweb instance too :)
> 
> > 
> > >  - Browsing through your v4l2/next/csi2/upport branch, it looks to be
> > >    targeted for upstream. Could we rely on those commits already or
> > >    they are still in development/testing?
> > 
> > The patches in that branch are target for upstreaming but sometime 
> > changes due to review comments. Best check the media-tree of what have 
> > accepted and the patchwork instance if you want day fresh statuses. I 
> > also rebase the branch from time to time on-top of the media-tree so you 
> > should not depend on it much I'm afraid.
> > 
> > If you plan to do some work on the rcar-csi2 driver you can always reach 
> > out and check what the status is if you are unsure.
> 
> Understood. Thanks. Since I've gone off-topic quite a bit already, may I
> have your feedback on below questions sitting in the back of my mind for
> some time:

Off topic is good ;-)

> 
>  - There seem to be quite a number of non-mainlined rcar-vin patches
>    sitting on top of v4.14.75-ltsi and part of rcar-3.9.x release. With
>    some false positives, around 35-40 such commits can be identified as
>    shown in [1]. Based on my checks, only a small subset of them are
>    (actively) discussed in linux-media/linux-renesas-soc ML and for most
>    of them I can't find any trace. I am just curious if there is any
>    background force driving the upstreaming of those patches or the
>    submissions/discussions happen rather by accident?

Yes I'm driving the upstreaming of most of those out of tree patches.  
Currently a lot of them are blocked on the organic growth of the 
addition of Gen3 and my lack of understanding of v4l2 at the time. Now I 
know better and I'm in the process of cleaning up the driver to prepare 
upstreaming most of them. I will send out the first cleanup patch later 
today.

I have in the past tried to upstream all patches and then cleanup the 
feature complete driver but it's gotten to a point where that is more 
painful then doing the right thing (tm).

If there is any particular patch that would be helpful for you to have 
upstream earlier then others let me know and I can try to put it first 
in the queue.

> 
>  - I am seeing two sets of rcar-vin tests [2-3] and I think there is a
>    parent/fork relationship between them, but now they appear to be
>    diverged. Which one would you recommend? What's the test coverage
>    provided by these suites? Do you see room/chance for something like
>    drivers/gpu/drm/selftests in rcar-vin in future (maybe using
>    the upcoming kunit [4] approach?). BTW, we've been using the
>    vsp-tests for quite some time and they've been extremely useful in
>    pointing out regressions in our own patches. So, many thanks for
>    sharing those too.

The vin-tests at ragnatech.se is the parent one. It's not as nice as the 
vsp-tests and I would like to add more tests and rewrite it in python3.  
If you have additional tests pleas send me patches and I will be happy 
to include them.

One idea I would really like to do is a HDMI loopback test. Connecting 
the DU and VIN and running tests on that to allow automatic testing of 
multiple resolutions and in the future scaling. As it is now all 
vin-tests depends on external video sources. An alternate idea to this 
is to create a remote "video server" on a RPi as it can deliver both 
HDMI and CVBS video...

I see no issue with using a kernel test framework but I have little 
experience in that area and it's not something I'm planing on worknig on 
at the moment.

> 
> > 
> > -- 
> > Regards,
> > Niklas Söderlund
> 
> [1] git log --oneline --cherry-pick --right-only \
>     torvalds/master...rcar-3.9.4 -- drivers/media/platform/rcar-vin/
> [2] https://git.ragnatech.se/vin-tests/
> [3] http://jmondi.org/git/vin-tests/
> [4] https://patchwork.kernel.org/cover/10704157/
> 
> Best regards,
> Eugeniu.

-- 
Regards,
Niklas Söderlund