* [PATCH v4 1/9] media: rcar-vin: Rename 'digital' to 'parallel'
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 2/9] media: rcar-vin: Remove two empty lines Jacopo Mondi
` (7 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
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>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
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..16d3e01 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 2/9] media: rcar-vin: Remove two empty lines
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 1/9] media: rcar-vin: Rename 'digital' to 'parallel' Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 3/9] media: rcar-vin: Create a group notifier Jacopo Mondi
` (6 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
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 16d3e01..bcf02de 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 3/9] media: rcar-vin: Create a group notifier
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 1/9] media: rcar-vin: Rename 'digital' to 'parallel' Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 2/9] media: rcar-vin: Remove two empty lines Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 4/9] media: rcar-vin: Cache the mbus configuration flags Jacopo Mondi
` (5 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
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>
---
v3 -> v4:
- Unregister and cleanup group notifier when un-registering the VIN
instance whose v4l2_dev the notifier is associated to.
---
drivers/media/platform/rcar-vin/rcar-core.c | 38 ++++++++++++++---------------
drivers/media/platform/rcar-vin/rcar-vin.h | 5 ++--
2 files changed, 20 insertions(+), 23 deletions(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index bcf02de..d3aadf3 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -46,6 +46,8 @@
*/
#define rvin_group_id_to_master(vin) ((vin) < 4 ? 0 : 4)
+#define v4l2_dev_to_vin(d) container_of(d, struct rvin_dev, v4l2_dev)
+
/* -----------------------------------------------------------------------------
* Media Controller link notification
*/
@@ -583,7 +585,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 +651,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 +675,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 +736,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 +747,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 +767,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;
@@ -1114,8 +1110,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
if (vin->info->use_mc) {
mutex_lock(&vin->group->lock);
- if (vin->group->notifier == &vin->notifier)
- vin->group->notifier = NULL;
+ if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
+ v4l2_async_notifier_unregister(&vin->group->notifier);
+ v4l2_async_notifier_cleanup(&vin->group->notifier);
+ }
mutex_unlock(&vin->group->lock);
rvin_group_put(vin);
} else {
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 755ac3c..ebb480f7 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -225,8 +225,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 +237,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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 4/9] media: rcar-vin: Cache the mbus configuration flags
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
` (2 preceding siblings ...)
2018-05-24 22:02 ` [PATCH v4 3/9] media: rcar-vin: Create a group notifier Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3 Jacopo Mondi
` (4 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
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 | 21 ++++++++-----------
drivers/media/platform/rcar-vin/rcar-dma.c | 32 +++++++++++++++++++----------
drivers/media/platform/rcar-vin/rcar-vin.h | 22 ++++++++++++++------
3 files changed, 45 insertions(+), 30 deletions(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d3aadf3..a799684 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -526,30 +526,29 @@ 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->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;
}
@@ -559,7 +558,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;
@@ -785,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..d2b7002 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:
@@ -682,13 +690,15 @@ static int rvin_setup(struct rvin_dev *vin)
else
dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
- /* Hsync Signal Polarity Select */
- if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
- dmr2 |= VNDMR2_HPS;
+ if (!vin->is_csi) {
+ /* Hsync Signal Polarity Select */
+ if (!(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))
- dmr2 |= VNDMR2_VPS;
+ /* Vsync Signal Polarity Select */
+ if (!(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
+ dmr2 |= VNDMR2_VPS;
+ }
/*
* Output format
@@ -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 ebb480f7..8bc3704 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
` (3 preceding siblings ...)
2018-05-24 22:02 ` [PATCH v4 4/9] media: rcar-vin: Cache the mbus configuration flags Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
2018-05-24 22:29 ` Niklas Söderlund
2018-05-24 22:02 ` [PATCH v4 6/9] media: rcar-vin: Link parallel input media entities Jacopo Mondi
` (3 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
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.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
v3 -> v4:
- Change the mc/parallel initialization order. Initialize mc first, then
parallel
- As a consequence no need to delay parallel notifiers registration, the
media controller is set up already when parallel input got parsed,
this greatly simplify the group notifier complete callback.
---
drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
1 file changed, 35 insertions(+), 21 deletions(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index a799684..29619c2 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -399,6 +399,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;
+ if (vin->info->use_mc) {
+ vin->parallel->subdev = subdev;
+ return 0;
+ }
+
/* Find compatible subdevices mbus format */
vin->mbus_code = 0;
code.index = 0;
@@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(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->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)
@@ -552,18 +559,18 @@ 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));
@@ -784,14 +791,8 @@ 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,11 +1075,24 @@ 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_mc_init(vin);
+ if (ret)
+ return ret;
+
+ ret = rvin_parallel_init(vin);
+ if (ret < 0 && ret != -ENOTCONN)
goto error;
pm_suspend_ignore_children(&pdev->dev, true);
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3
2018-05-24 22:02 ` [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3 Jacopo Mondi
@ 2018-05-24 22:29 ` Niklas Söderlund
0 siblings, 0 replies; 16+ messages in thread
From: Niklas Söderlund @ 2018-05-24 22:29 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: laurent.pinchart, mchehab, linux-media, linux-renesas-soc
Hi Jacopo,
Thanks for your work.
I really like what you did with this patch in v4.
On 2018-05-25 00:02:15 +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.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> ---
> v3 -> v4:
> - Change the mc/parallel initialization order. Initialize mc first, then
> parallel
> - As a consequence no need to delay parallel notifiers registration, the
> media controller is set up already when parallel input got parsed,
> this greatly simplify the group notifier complete callback.
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index a799684..29619c2 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -399,6 +399,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;
>
> + if (vin->info->use_mc) {
> + vin->parallel->subdev = subdev;
> + return 0;
> + }
> +
> /* Find compatible subdevices mbus format */
> vin->mbus_code = 0;
> code.index = 0;
> @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(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->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)
> @@ -552,18 +559,18 @@ 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;
I think you still should return -ENODEV here if !vin->info->use_mc to
preserve Gen2 which runs without media controller behavior. How about:
return vin->info->use_mc ? -ENOTCONN : -ENODEV;
>
> vin_dbg(vin, "Found parallel subdevice %pOF\n",
> to_of_node(vin->parallel->asd.match.fwnode));
> @@ -784,14 +791,8 @@ 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,11 +1075,24 @@ 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;
> + }
I don't see why you need to move the media pad creation out of
rvin_mc_init(). With the reorder of the rvin_mc_init()
rvin_parallel_init() I would keep this in rvin_mc_init().
> +
> + ret = rvin_mc_init(vin);
> + if (ret)
> + return ret;
> +
> + ret = rvin_parallel_init(vin);
> + if (ret < 0 && ret != -ENOTCONN)
> goto error;
>
> pm_suspend_ignore_children(&pdev->dev, true);
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3
@ 2018-05-24 22:29 ` Niklas Söderlund
0 siblings, 0 replies; 16+ messages in thread
From: Niklas Söderlund @ 2018-05-24 22:29 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: laurent.pinchart, mchehab, linux-media, linux-renesas-soc
Hi Jacopo,
Thanks for your work.
I really like what you did with this patch in v4.
On 2018-05-25 00:02:15 +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.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> ---
> v3 -> v4:
> - Change the mc/parallel initialization order. Initialize mc first, then
> parallel
> - As a consequence no need to delay parallel notifiers registration, the
> media controller is set up already when parallel input got parsed,
> this greatly simplify the group notifier complete callback.
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index a799684..29619c2 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -399,6 +399,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;
>
> + if (vin->info->use_mc) {
> + vin->parallel->subdev = subdev;
> + return 0;
> + }
> +
> /* Find compatible subdevices mbus format */
> vin->mbus_code = 0;
> code.index = 0;
> @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(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->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)
> @@ -552,18 +559,18 @@ 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;
I think you still should return -ENODEV here if !vin->info->use_mc to
preserve Gen2 which runs without media controller behavior. How about:
return vin->info->use_mc ? -ENOTCONN : -ENODEV;
>
> vin_dbg(vin, "Found parallel subdevice %pOF\n",
> to_of_node(vin->parallel->asd.match.fwnode));
> @@ -784,14 +791,8 @@ 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,11 +1075,24 @@ 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;
> + }
I don't see why you need to move the media pad creation out of
rvin_mc_init(). With the reorder of the rvin_mc_init()
rvin_parallel_init() I would keep this in rvin_mc_init().
> +
> + ret = rvin_mc_init(vin);
> + if (ret)
> + return ret;
> +
> + ret = rvin_parallel_init(vin);
> + if (ret < 0 && ret != -ENOTCONN)
> goto error;
>
> pm_suspend_ignore_children(&pdev->dev, true);
> --
> 2.7.4
>
--
Regards,
Niklas S�derlund
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3
2018-05-24 22:29 ` Niklas Söderlund
(?)
@ 2018-05-25 7:16 ` jacopo mondi
-1 siblings, 0 replies; 16+ messages in thread
From: jacopo mondi @ 2018-05-25 7:16 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Jacopo Mondi, laurent.pinchart, mchehab, linux-media, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 5509 bytes --]
Hi Niklas,
On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> I really like what you did with this patch in v4.
Thanks for review and suggestions, what's there comes mostly from your
comments and guidance.
>
> On 2018-05-25 00:02:15 +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.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > v3 -> v4:
> > - Change the mc/parallel initialization order. Initialize mc first, then
> > parallel
> > - As a consequence no need to delay parallel notifiers registration, the
> > media controller is set up already when parallel input got parsed,
> > this greatly simplify the group notifier complete callback.
> > ---
> > drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> > 1 file changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index a799684..29619c2 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -399,6 +399,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;
> >
> > + if (vin->info->use_mc) {
> > + vin->parallel->subdev = subdev;
> > + return 0;
> > + }
> > +
> > /* Find compatible subdevices mbus format */
> > vin->mbus_code = 0;
> > code.index = 0;
> > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(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->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)
> > @@ -552,18 +559,18 @@ 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;
>
> I think you still should return -ENODEV here if !vin->info->use_mc to
> preserve Gen2 which runs without media controller behavior. How about:
>
> return vin->info->use_mc ? -ENOTCONN : -ENODEV;
Right, I wish I had some gen2 board to test. I wonder if that's not
better handled in probe though... I'll see how it looks like.
>
> >
> > vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > to_of_node(vin->parallel->asd.match.fwnode));
> > @@ -784,14 +791,8 @@ 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,11 +1075,24 @@ 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;
> > + }
>
> I don't see why you need to move the media pad creation out of
> rvin_mc_init(). With the reorder of the rvin_mc_init()
> rvin_parallel_init() I would keep this in rvin_mc_init().
>
Just because I've been lazy re-structuring that part of code I broke
up in previous versions. I'll move that part back to mc_init() and
possibly also remove the
+ if (!vin->info->use_mc)
+ return 0;
in that function and handle this in probe().
Thanks
j
> > +
> > + ret = rvin_mc_init(vin);
> > + if (ret)
> > + return ret;
> > +
> > + ret = rvin_parallel_init(vin);
> > + if (ret < 0 && ret != -ENOTCONN)
> > goto error;
> >
> > pm_suspend_ignore_children(&pdev->dev, true);
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3
2018-05-24 22:29 ` Niklas Söderlund
(?)
(?)
@ 2018-05-25 11:50 ` jacopo mondi
2018-05-28 13:50 ` Niklas Söderlund
-1 siblings, 1 reply; 16+ messages in thread
From: jacopo mondi @ 2018-05-25 11:50 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Jacopo Mondi, laurent.pinchart, mchehab, linux-media, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 7265 bytes --]
Hi Niklas,
I might have another question before sending v5.
On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> I really like what you did with this patch in v4.
>
> On 2018-05-25 00:02:15 +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.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > v3 -> v4:
> > - Change the mc/parallel initialization order. Initialize mc first, then
> > parallel
> > - As a consequence no need to delay parallel notifiers registration, the
> > media controller is set up already when parallel input got parsed,
> > this greatly simplify the group notifier complete callback.
> > ---
> > drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> > 1 file changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index a799684..29619c2 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -399,6 +399,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;
> >
> > + if (vin->info->use_mc) {
> > + vin->parallel->subdev = subdev;
> > + return 0;
> > + }
> > +
> > /* Find compatible subdevices mbus format */
> > vin->mbus_code = 0;
> > code.index = 0;
> > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(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->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)
> > @@ -552,18 +559,18 @@ 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;
>
> I think you still should return -ENODEV here if !vin->info->use_mc to
> preserve Gen2 which runs without media controller behavior. How about:
>
> return vin->info->use_mc ? -ENOTCONN : -ENODEV;
>
> >
> > vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > to_of_node(vin->parallel->asd.match.fwnode));
> > @@ -784,14 +791,8 @@ 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,11 +1075,24 @@ 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;
> > + }
>
> I don't see why you need to move the media pad creation out of
> rvin_mc_init(). With the reorder of the rvin_mc_init()
> rvin_parallel_init() I would keep this in rvin_mc_init().
>
> > +
> > + ret = rvin_mc_init(vin);
> > + if (ret)
> > + return ret;
> > +
> > + ret = rvin_parallel_init(vin);
> > + if (ret < 0 && ret != -ENOTCONN)
> > goto error;
> >
> > pm_suspend_ignore_children(&pdev->dev, true);
I've been looking at the error path handling now that the code looks
like this in the forthcoming v5:
if (vin->info->use_mc) {
ret = rvin_mc_init(vin);
if (ret)
goto error_dma_unregister;
}
ret = rvin_parallel_init(vin);
if (ret)
goto error_group_unregister;
...
error_group_unreg:
if (vin->info->use_mc) {
mutex_lock(&vin->group->lock);
if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
v4l2_async_notifier_unregister(&vin->group->notifier);
v4l2_async_notifier_cleanup(&vin->group->notifier);
}
mutex_unlock(&vin->group->lock);
rvin_group_put(vin);
}
error_dma_unreg:
rvin_dma_unregister(vin);
return ret;
Question is, do you think the group notifier should be unregistered
and cleaned up if the parallel input initialization of the VIN
instance whose v4l2_dev is used to register the group notifier fails?
I feel like it should, as the VIN instance whose v4l2_dev is used is
always the last probing one, so there should be no issues with other
VINs registering after it and finding themselves without a valid
notifier.
I felt like it was better anticipating this to you instead of adding
this part out of the blue in v5.
Also, I think in both parallel input and mc notifier registration, the
notifier should be cleaned up to release the parsed async subdevices
memory allocated by
v4l2_async_notifier_parse_fwnode_endpoints_by_port() if the
sub-sequent v4l2_async_notifier_register() fails.
That would be:
@@ -781,18 +782,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
&vin->group->notifier);
if (ret < 0) {
vin_err(vin, "Notifier registration failed\n");
- return ret;
+ goto error_clean_up_notifier;
}
return 0;
+
+error_clean_up_notifier:
+ v4l2_async_notifier_cleanup(&vin->group->notifier);
+
+ return ret;
}
in both mc and parallel initialization functions.
With your ack I can send two patches on top of the currently merged VIN
version, and rebase my series on top eventually.
Sorry for the long email.
Thanks
j
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3
2018-05-25 11:50 ` jacopo mondi
@ 2018-05-28 13:50 ` Niklas Söderlund
0 siblings, 0 replies; 16+ messages in thread
From: Niklas Söderlund @ 2018-05-28 13:50 UTC (permalink / raw)
To: jacopo mondi
Cc: Jacopo Mondi, laurent.pinchart, mchehab, linux-media, linux-renesas-soc
Hi Jacopo,
On 2018-05-25 13:50:08 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> I might have another question before sending v5.
>
> On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > I really like what you did with this patch in v4.
> >
> > On 2018-05-25 00:02:15 +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.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >
> > > ---
> > > v3 -> v4:
> > > - Change the mc/parallel initialization order. Initialize mc first, then
> > > parallel
> > > - As a consequence no need to delay parallel notifiers registration, the
> > > media controller is set up already when parallel input got parsed,
> > > this greatly simplify the group notifier complete callback.
> > > ---
> > > drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> > > 1 file changed, 35 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index a799684..29619c2 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -399,6 +399,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;
> > >
> > > + if (vin->info->use_mc) {
> > > + vin->parallel->subdev = subdev;
> > > + return 0;
> > > + }
> > > +
> > > /* Find compatible subdevices mbus format */
> > > vin->mbus_code = 0;
> > > code.index = 0;
> > > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(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->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)
> > > @@ -552,18 +559,18 @@ 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;
> >
> > I think you still should return -ENODEV here if !vin->info->use_mc to
> > preserve Gen2 which runs without media controller behavior. How about:
> >
> > return vin->info->use_mc ? -ENOTCONN : -ENODEV;
> >
> > >
> > > vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > > to_of_node(vin->parallel->asd.match.fwnode));
> > > @@ -784,14 +791,8 @@ 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,11 +1075,24 @@ 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;
> > > + }
> >
> > I don't see why you need to move the media pad creation out of
> > rvin_mc_init(). With the reorder of the rvin_mc_init()
> > rvin_parallel_init() I would keep this in rvin_mc_init().
> >
> > > +
> > > + ret = rvin_mc_init(vin);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = rvin_parallel_init(vin);
> > > + if (ret < 0 && ret != -ENOTCONN)
> > > goto error;
> > >
> > > pm_suspend_ignore_children(&pdev->dev, true);
>
> I've been looking at the error path handling now that the code looks
> like this in the forthcoming v5:
>
> if (vin->info->use_mc) {
> ret = rvin_mc_init(vin);
> if (ret)
> goto error_dma_unregister;
> }
>
> ret = rvin_parallel_init(vin);
> if (ret)
> goto error_group_unregister;
>
>
> ...
>
>
> error_group_unreg:
> if (vin->info->use_mc) {
> mutex_lock(&vin->group->lock);
> if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> v4l2_async_notifier_unregister(&vin->group->notifier);
> v4l2_async_notifier_cleanup(&vin->group->notifier);
> }
> mutex_unlock(&vin->group->lock);
> rvin_group_put(vin);
> }
>
> error_dma_unreg:
> rvin_dma_unregister(vin);
>
> return ret;
>
> Question is, do you think the group notifier should be unregistered
> and cleaned up if the parallel input initialization of the VIN
> instance whose v4l2_dev is used to register the group notifier fails?
>
> I feel like it should, as the VIN instance whose v4l2_dev is used is
> always the last probing one, so there should be no issues with other
> VINs registering after it and finding themselves without a valid
> notifier.
I agree with you. If the parallel initialization fails everything done
by that particular VIN probe should be undone. So if it have registered
the group notifier it should unregister it.
>
> I felt like it was better anticipating this to you instead of adding
> this part out of the blue in v5.
>
> Also, I think in both parallel input and mc notifier registration, the
> notifier should be cleaned up to release the parsed async subdevices
> memory allocated by
> v4l2_async_notifier_parse_fwnode_endpoints_by_port() if the
> sub-sequent v4l2_async_notifier_register() fails.
I agree with you here as well. That this memory should be cleaned up,
nice catch.
>
> That would be:
>
> @@ -781,18 +782,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
> &vin->group->notifier);
> if (ret < 0) {
> vin_err(vin, "Notifier registration failed\n");
> - return ret;
> + goto error_clean_up_notifier;
> }
>
> return 0;
> +
> +error_clean_up_notifier:
> + v4l2_async_notifier_cleanup(&vin->group->notifier);
> +
> + return ret;
> }
>
> in both mc and parallel initialization functions.
>
> With your ack I can send two patches on top of the currently merged VIN
> version, and rebase my series on top eventually.
>
> Sorry for the long email.
>
> Thanks
> j
>
>
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3
@ 2018-05-28 13:50 ` Niklas Söderlund
0 siblings, 0 replies; 16+ messages in thread
From: Niklas Söderlund @ 2018-05-28 13:50 UTC (permalink / raw)
To: jacopo mondi
Cc: Jacopo Mondi, laurent.pinchart, mchehab, linux-media, linux-renesas-soc
Hi Jacopo,
On 2018-05-25 13:50:08 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> I might have another question before sending v5.
>
> On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas S�derlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > I really like what you did with this patch in v4.
> >
> > On 2018-05-25 00:02:15 +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.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >
> > > ---
> > > v3 -> v4:
> > > - Change the mc/parallel initialization order. Initialize mc first, then
> > > parallel
> > > - As a consequence no need to delay parallel notifiers registration, the
> > > media controller is set up already when parallel input got parsed,
> > > this greatly simplify the group notifier complete callback.
> > > ---
> > > drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> > > 1 file changed, 35 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index a799684..29619c2 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -399,6 +399,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;
> > >
> > > + if (vin->info->use_mc) {
> > > + vin->parallel->subdev = subdev;
> > > + return 0;
> > > + }
> > > +
> > > /* Find compatible subdevices mbus format */
> > > vin->mbus_code = 0;
> > > code.index = 0;
> > > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(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->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)
> > > @@ -552,18 +559,18 @@ 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;
> >
> > I think you still should return -ENODEV here if !vin->info->use_mc to
> > preserve Gen2 which runs without media controller behavior. How about:
> >
> > return vin->info->use_mc ? -ENOTCONN : -ENODEV;
> >
> > >
> > > vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > > to_of_node(vin->parallel->asd.match.fwnode));
> > > @@ -784,14 +791,8 @@ 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,11 +1075,24 @@ 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;
> > > + }
> >
> > I don't see why you need to move the media pad creation out of
> > rvin_mc_init(). With the reorder of the rvin_mc_init()
> > rvin_parallel_init() I would keep this in rvin_mc_init().
> >
> > > +
> > > + ret = rvin_mc_init(vin);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = rvin_parallel_init(vin);
> > > + if (ret < 0 && ret != -ENOTCONN)
> > > goto error;
> > >
> > > pm_suspend_ignore_children(&pdev->dev, true);
>
> I've been looking at the error path handling now that the code looks
> like this in the forthcoming v5:
>
> if (vin->info->use_mc) {
> ret = rvin_mc_init(vin);
> if (ret)
> goto error_dma_unregister;
> }
>
> ret = rvin_parallel_init(vin);
> if (ret)
> goto error_group_unregister;
>
>
> ...
>
>
> error_group_unreg:
> if (vin->info->use_mc) {
> mutex_lock(&vin->group->lock);
> if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> v4l2_async_notifier_unregister(&vin->group->notifier);
> v4l2_async_notifier_cleanup(&vin->group->notifier);
> }
> mutex_unlock(&vin->group->lock);
> rvin_group_put(vin);
> }
>
> error_dma_unreg:
> rvin_dma_unregister(vin);
>
> return ret;
>
> Question is, do you think the group notifier should be unregistered
> and cleaned up if the parallel input initialization of the VIN
> instance whose v4l2_dev is used to register the group notifier fails?
>
> I feel like it should, as the VIN instance whose v4l2_dev is used is
> always the last probing one, so there should be no issues with other
> VINs registering after it and finding themselves without a valid
> notifier.
I agree with you. If the parallel initialization fails everything done
by that particular VIN probe should be undone. So if it have registered
the group notifier it should unregister it.
>
> I felt like it was better anticipating this to you instead of adding
> this part out of the blue in v5.
>
> Also, I think in both parallel input and mc notifier registration, the
> notifier should be cleaned up to release the parsed async subdevices
> memory allocated by
> v4l2_async_notifier_parse_fwnode_endpoints_by_port() if the
> sub-sequent v4l2_async_notifier_register() fails.
I agree with you here as well. That this memory should be cleaned up,
nice catch.
>
> That would be:
>
> @@ -781,18 +782,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
> &vin->group->notifier);
> if (ret < 0) {
> vin_err(vin, "Notifier registration failed\n");
> - return ret;
> + goto error_clean_up_notifier;
> }
>
> return 0;
> +
> +error_clean_up_notifier:
> + v4l2_async_notifier_cleanup(&vin->group->notifier);
> +
> + return ret;
> }
>
> in both mc and parallel initialization functions.
>
> With your ack I can send two patches on top of the currently merged VIN
> version, and rebase my series on top eventually.
>
> Sorry for the long email.
>
> Thanks
> j
>
>
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas S�derlund
--
Regards,
Niklas S�derlund
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 6/9] media: rcar-vin: Link parallel input media entities
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
` (4 preceding siblings ...)
2018-05-24 22:02 ` [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3 Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 7/9] media: rcar-vin: Handle parallel subdev in link_notify Jacopo Mondi
` (2 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
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 29619c2..b69b375 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -476,6 +476,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);
@@ -484,7 +486,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,
@@ -604,7 +625,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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 7/9] media: rcar-vin: Handle parallel subdev in link_notify
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
` (5 preceding siblings ...)
2018-05-24 22:02 ` [PATCH v4 6/9] media: rcar-vin: Link parallel input media entities Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 8/9] media: rcar-vin: Rename _rcar_info to rcar_info Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 9/9] media: rcar-vin: Add support for R-Car R8A77995 SoC Jacopo Mondi
8 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
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>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
drivers/media/platform/rcar-vin/rcar-core.c | 35 ++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index b69b375..8edf896 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -171,9 +171,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);
+
+ 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;
+ }
+
channel = rvin_group_csi_pad_to_channel(link->source->index);
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) {
@@ -183,6 +211,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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 8/9] media: rcar-vin: Rename _rcar_info to rcar_info
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
` (6 preceding siblings ...)
2018-05-24 22:02 ` [PATCH v4 7/9] media: rcar-vin: Handle parallel subdev in link_notify Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
2018-05-24 22:02 ` [PATCH v4 9/9] media: rcar-vin: Add support for R-Car R8A77995 SoC Jacopo Mondi
8 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
Remove leading underscore to align all rcar_group_route structure
declarations.
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 | 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 8edf896..30f6ea7 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1019,7 +1019,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) },
@@ -1035,7 +1035,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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 9/9] media: rcar-vin: Add support for R-Car R8A77995 SoC
2018-05-24 22:02 [PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3 Jacopo Mondi
` (7 preceding siblings ...)
2018-05-24 22:02 ` [PATCH v4 8/9] media: rcar-vin: Rename _rcar_info to rcar_info Jacopo Mondi
@ 2018-05-24 22:02 ` Jacopo Mondi
8 siblings, 0 replies; 16+ messages in thread
From: Jacopo Mondi @ 2018-05-24 22:02 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart
Cc: Jacopo Mondi, mchehab, linux-media, linux-renesas-soc
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 30f6ea7..223310f 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1038,6 +1038,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",
@@ -1079,6 +1091,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
^ permalink raw reply related [flat|nested] 16+ messages in thread