* [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2
@ 2025-05-21 13:20 Niklas Söderlund
2025-05-21 13:20 ` [PATCH v4 1/6] media: rcar-vin: Use correct count of remote subdevices Niklas Söderlund
` (6 more replies)
0 siblings, 7 replies; 22+ messages in thread
From: Niklas Söderlund @ 2025-05-21 13:20 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen,
Laurent Pinchart, linux-media, linux-renesas-soc
Cc: Niklas Söderlund
Hello,
This series completes the conversion of the soc_camera VIN driver to a
full fledge media-graph enabled driver for R-Car Gen2 devices, Gen3 and
later have been media-graph centric from the start. Having a single
driver supporting both MC and non-MC operation have lead do odd design
decisions in the driver, and it have prevented improving the driver over
all.
New features and bug fixes have always been more important then fixing
this old generation to be MC-centric. But in order to start to play with
libcamera support for the R-Car pipeline it have become more pressing to
make take the time to make this driver MC-only, and more importantly
test it to make sure nothing really breaks.
Patch 1/6 and 2/6 are drive-by fixes correcting issues in the existing
design. Patch 3/6 prepares for Gen2 MC by making sure each VIN instance
on Gen2 gets a unique ID which will be needed to support VIN groups.
Compared to Gen3 and later the group ID does not match what it can do in
the group and does not need to be set from DT, all that matters is that
each VIN instance have a unique ID.
Patch 4/6 and 5/6 uses the fact that VIN instances on Gen2 now have
unique IDs and greatly simplifies the unnecessarily complex vl4-async
notifier usage in the VIN driver. This have in the past lead to some
subtle bugs and having only a single notifier for all VIN will remove a
lot of possibilities for this to go wrong in the future.
Finally patch 6/6 removes all non MC code paths and have the Gen2
devices register a media device and configure links.
This have been tested on Gen3 and Gen4 devices without any regressions.
And on Gen2 to make sure the media-graph behaves as it should. As a
bonus the Gen2 devices can now join the VIN CI and any future issues
should be caught as they are for Gen3 and Gen4.
See individual patches for changelog.
Niklas Söderlund (6):
media: rcar-vin: Use correct count of remote subdevices
media: rcar-vin: Change link setup argument
media: rcar-vin: Generate a VIN group ID for Gen2
media: rcar-vin: Prepare for unifying all v4l-async notifiers
media: rcar-vin: Merge all notifiers
media: rcar-vin: Enable media-graph on Gen2
.../platform/renesas/rcar-vin/rcar-core.c | 706 +++++++-----------
.../platform/renesas/rcar-vin/rcar-dma.c | 16 +-
.../platform/renesas/rcar-vin/rcar-v4l2.c | 488 +-----------
.../platform/renesas/rcar-vin/rcar-vin.h | 14 +-
4 files changed, 305 insertions(+), 919 deletions(-)
--
2.49.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 1/6] media: rcar-vin: Use correct count of remote subdevices
2025-05-21 13:20 [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2 Niklas Söderlund
@ 2025-05-21 13:20 ` Niklas Söderlund
2025-05-27 11:11 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 2/6] media: rcar-vin: Change link setup argument Niklas Söderlund
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Niklas Söderlund @ 2025-05-21 13:20 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen,
Laurent Pinchart, linux-media, linux-renesas-soc
Cc: Niklas Söderlund
When extending the driver with Gen4 support the iteration of over
possible remote subdevices changed from being R-Car CSI-2 Rx only to
also cover R-Car CSISP instances. In two loops updating the bounds
variable was missed.
This had no ill effect as the count the two values have always been the
same in the past. Fix it before the two diverge.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
drivers/media/platform/renesas/rcar-vin/rcar-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
index 846ae7989b1d..b9ea5b8db559 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
@@ -262,7 +262,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
mutex_lock(&vin->group->lock);
- for (i = 0; i < RVIN_CSI_MAX; i++) {
+ for (i = 0; i < RVIN_REMOTES_MAX; i++) {
if (vin->group->remotes[i].asc != asc)
continue;
vin->group->remotes[i].subdev = NULL;
@@ -284,7 +284,7 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
mutex_lock(&vin->group->lock);
- for (i = 0; i < RVIN_CSI_MAX; i++) {
+ for (i = 0; i < RVIN_REMOTES_MAX; i++) {
if (vin->group->remotes[i].asc != asc)
continue;
vin->group->remotes[i].subdev = subdev;
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 2/6] media: rcar-vin: Change link setup argument
2025-05-21 13:20 [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2 Niklas Söderlund
2025-05-21 13:20 ` [PATCH v4 1/6] media: rcar-vin: Use correct count of remote subdevices Niklas Söderlund
@ 2025-05-21 13:20 ` Niklas Söderlund
2025-05-27 11:19 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 3/6] media: rcar-vin: Generate a VIN group ID for Gen2 Niklas Söderlund
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Niklas Söderlund @ 2025-05-21 13:20 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen,
Laurent Pinchart, linux-media, linux-renesas-soc
Cc: Niklas Söderlund
The link setup callback once acted on each VIN instance, and expected to
be called once for each VIN instance. This have changed as the driver
grew support for later hardware generations and the callback is now
expected to setup links for all VIN in the group.
The argument to the callback have however remained a pointer to a single
VIN instance. This pointer was then used to get the group structure. Fix
this and pass the group as the single argument to the link setup
callback making the expectation of the function clear.
There is no intentional change in behavior.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
.../platform/renesas/rcar-vin/rcar-core.c | 52 ++++++++++++-------
.../platform/renesas/rcar-vin/rcar-vin.h | 2 +-
2 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
index b9ea5b8db559..1be408d6c508 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
@@ -65,7 +65,7 @@ static void rvin_group_cleanup(struct rvin_group *group)
}
static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
- int (*link_setup)(struct rvin_dev *),
+ int (*link_setup)(struct rvin_group *),
const struct media_device_ops *ops)
{
struct media_device *mdev = &group->mdev;
@@ -115,7 +115,7 @@ static void rvin_group_release(struct kref *kref)
}
static int rvin_group_get(struct rvin_dev *vin,
- int (*link_setup)(struct rvin_dev *),
+ int (*link_setup)(struct rvin_group *),
const struct media_device_ops *ops)
{
struct rvin_group *group;
@@ -246,7 +246,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
}
}
- return vin->group->link_setup(vin);
+ return vin->group->link_setup(vin->group);
}
static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
@@ -909,35 +909,46 @@ static int rvin_csi2_create_link(struct rvin_group *group, unsigned int id,
return 0;
}
-static int rvin_csi2_setup_links(struct rvin_dev *vin)
+static int rvin_csi2_setup_links(struct rvin_group *group)
{
- const struct rvin_group_route *route;
+ const struct rvin_group_route *routes, *route;
unsigned int id;
int ret = -EINVAL;
+ /* Find any VIN in group to get route info. */
+ routes = NULL;
+ for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
+ if (group->vin[i]) {
+ routes = group->vin[i]->info->routes;
+ break;
+ }
+ }
+ if (!routes)
+ return -ENODEV;
+
/* Create all media device links between VINs and CSI-2's. */
- mutex_lock(&vin->group->lock);
- for (route = vin->info->routes; route->chsel; route++) {
+ mutex_lock(&group->lock);
+ for (route = routes; route->chsel; route++) {
/* Check that VIN' master is part of the group. */
- if (!vin->group->vin[route->master])
+ if (!group->vin[route->master])
continue;
/* Check that CSI-2 is part of the group. */
- if (!vin->group->remotes[route->csi].subdev)
+ if (!group->remotes[route->csi].subdev)
continue;
for (id = route->master; id < route->master + 4; id++) {
/* Check that VIN is part of the group. */
- if (!vin->group->vin[id])
+ if (!group->vin[id])
continue;
- ret = rvin_csi2_create_link(vin->group, id, route);
+ ret = rvin_csi2_create_link(group, id, route);
if (ret)
goto out;
}
}
out:
- mutex_unlock(&vin->group->lock);
+ mutex_unlock(&group->lock);
return ret;
}
@@ -991,30 +1002,33 @@ static int rvin_csi2_init(struct rvin_dev *vin)
* ISP
*/
-static int rvin_isp_setup_links(struct rvin_dev *vin)
+static int rvin_isp_setup_links(struct rvin_group *group)
{
unsigned int i;
int ret = -EINVAL;
/* Create all media device links between VINs and ISP's. */
- mutex_lock(&vin->group->lock);
+ mutex_lock(&group->lock);
for (i = 0; i < RCAR_VIN_NUM; i++) {
struct media_pad *source_pad, *sink_pad;
struct media_entity *source, *sink;
unsigned int source_slot = i / 8;
unsigned int source_idx = i % 8 + 1;
+ struct rvin_dev *vin;
- if (!vin->group->vin[i])
+ vin = group->vin[i];
+
+ if (!vin)
continue;
/* Check that ISP is part of the group. */
- if (!vin->group->remotes[source_slot].subdev)
+ if (!group->remotes[source_slot].subdev)
continue;
- source = &vin->group->remotes[source_slot].subdev->entity;
+ source = &group->remotes[source_slot].subdev->entity;
source_pad = &source->pads[source_idx];
- sink = &vin->group->vin[i]->vdev.entity;
+ sink = &vin->vdev.entity;
sink_pad = &sink->pads[0];
/* Skip if link already exists. */
@@ -1030,7 +1044,7 @@ static int rvin_isp_setup_links(struct rvin_dev *vin)
break;
}
}
- mutex_unlock(&vin->group->lock);
+ mutex_unlock(&group->lock);
return ret;
}
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
index 83d1b2734c41..7d4fce248976 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
@@ -257,7 +257,7 @@ struct rvin_group {
struct v4l2_async_notifier notifier;
struct rvin_dev *vin[RCAR_VIN_NUM];
- int (*link_setup)(struct rvin_dev *vin);
+ int (*link_setup)(struct rvin_group *group);
struct {
struct v4l2_async_connection *asc;
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 3/6] media: rcar-vin: Generate a VIN group ID for Gen2
2025-05-21 13:20 [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2 Niklas Söderlund
2025-05-21 13:20 ` [PATCH v4 1/6] media: rcar-vin: Use correct count of remote subdevices Niklas Söderlund
2025-05-21 13:20 ` [PATCH v4 2/6] media: rcar-vin: Change link setup argument Niklas Söderlund
@ 2025-05-21 13:20 ` Niklas Söderlund
2025-05-27 11:31 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers Niklas Söderlund
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Niklas Söderlund @ 2025-05-21 13:20 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen,
Laurent Pinchart, linux-media, linux-renesas-soc
Cc: Niklas Söderlund
Prepare to move Gen2 and earlier models to media controller by
generating a unique VIN group id for each VIN instance. On Gen3 and Gen4
it is important to have a specific id in the group as media graph routes
depend on this. On Gen2 and earlier models all that will matter is to
have a unique id in the range.
Break out the id generation to a own function keeping the logic for Gen3
and Gen4 while generating a sequential id for Gen2 models.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Move ID allocation to probe.
- Use ida_alloc_range() instead of implementing our own schema by
counting DT nodes.
---
.../platform/renesas/rcar-vin/rcar-core.c | 78 ++++++++++++++-----
1 file changed, 59 insertions(+), 19 deletions(-)
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
index 1be408d6c508..d9ad56fb2aa9 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
@@ -10,6 +10,7 @@
* Based on the soc-camera rcar_vin driver
*/
+#include <linux/idr.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_graph.h>
@@ -55,6 +56,7 @@
* be only one group for all instances.
*/
+static DEFINE_IDA(rvin_ida);
static DEFINE_MUTEX(rvin_group_lock);
static struct rvin_group *rvin_group_data;
@@ -119,23 +121,8 @@ static int rvin_group_get(struct rvin_dev *vin,
const struct media_device_ops *ops)
{
struct rvin_group *group;
- u32 id;
int ret;
- /* Make sure VIN id is present and sane */
- ret = of_property_read_u32(vin->dev->of_node, "renesas,id", &id);
- if (ret) {
- vin_err(vin, "%pOF: No renesas,id property found\n",
- vin->dev->of_node);
- return -EINVAL;
- }
-
- if (id >= RCAR_VIN_NUM) {
- vin_err(vin, "%pOF: Invalid renesas,id '%u'\n",
- vin->dev->of_node, id);
- return -EINVAL;
- }
-
/* Join or create a VIN group */
mutex_lock(&rvin_group_lock);
if (rvin_group_data) {
@@ -164,16 +151,15 @@ static int rvin_group_get(struct rvin_dev *vin,
/* Add VIN to group */
mutex_lock(&group->lock);
- if (group->vin[id]) {
- vin_err(vin, "Duplicate renesas,id property value %u\n", id);
+ if (group->vin[vin->id]) {
+ vin_err(vin, "Duplicate renesas,id property value %u\n", vin->id);
mutex_unlock(&group->lock);
kref_put(&group->refcount, rvin_group_release);
return -EINVAL;
}
- group->vin[id] = vin;
+ group->vin[vin->id] = vin;
- vin->id = id;
vin->group = group;
vin->v4l2_dev.mdev = &group->mdev;
@@ -1375,6 +1361,54 @@ static const struct of_device_id rvin_of_id_table[] = {
};
MODULE_DEVICE_TABLE(of, rvin_of_id_table);
+static int rvin_id_get(struct rvin_dev *vin)
+{
+ u32 oid;
+ int id;
+
+ switch (vin->info->model) {
+ case RCAR_GEN3:
+ case RCAR_GEN4:
+ if (of_property_read_u32(vin->dev->of_node, "renesas,id", &oid))
+ break;
+
+ if (oid < 0 || oid >= RCAR_VIN_NUM) {
+ vin_err(vin, "%pOF: Invalid renesas,id '%u'\n",
+ vin->dev->of_node, oid);
+ return -EINVAL;
+ }
+
+ vin->id = oid;
+
+ return 0;
+ default:
+ id = ida_alloc_range(&rvin_ida, 0, RCAR_VIN_NUM - 1,
+ GFP_KERNEL);
+ if (id < 0)
+ break;
+
+ vin->id = id;
+
+ return 0;
+ }
+
+ vin_err(vin, "Can't figure out VIN id\n");
+
+ return -EINVAL;
+}
+
+static void rvin_id_put(struct rvin_dev *vin)
+{
+ switch (vin->info->model) {
+ case RCAR_GEN3:
+ case RCAR_GEN4:
+ break;
+ default:
+ ida_free(&rvin_ida, vin->id);
+ break;
+ }
+}
+
static int rcar_vin_probe(struct platform_device *pdev)
{
struct rvin_dev *vin;
@@ -1402,6 +1436,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vin);
+ if (rvin_id_get(vin))
+ return -EINVAL;
+
if (vin->info->use_isp) {
ret = rvin_isp_init(vin);
} else if (vin->info->use_mc) {
@@ -1419,6 +1456,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
if (ret) {
rvin_dma_unregister(vin);
+ rvin_id_put(vin);
return ret;
}
@@ -1443,6 +1481,8 @@ static void rcar_vin_remove(struct platform_device *pdev)
else
rvin_parallel_cleanup(vin);
+ rvin_id_put(vin);
+
rvin_dma_unregister(vin);
}
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers
2025-05-21 13:20 [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2 Niklas Söderlund
` (2 preceding siblings ...)
2025-05-21 13:20 ` [PATCH v4 3/6] media: rcar-vin: Generate a VIN group ID for Gen2 Niklas Söderlund
@ 2025-05-21 13:20 ` Niklas Söderlund
2025-05-27 7:01 ` Sakari Ailus
2025-05-27 11:32 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 5/6] media: rcar-vin: Merge all notifiers Niklas Söderlund
` (2 subsequent siblings)
6 siblings, 2 replies; 22+ messages in thread
From: Niklas Söderlund @ 2025-05-21 13:20 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen,
Laurent Pinchart, linux-media, linux-renesas-soc
Cc: Niklas Söderlund
The R-Car VIN driver is needless complex and uses more then one
v4l-async notifier to attach to all its subdevices. Prepare for unifying
them by moving rvin_parallel_parse_of() to where it needs to be when
they are unified.
The function is moved verbatim and there is no change in behavior.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
.../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
1 file changed, 53 insertions(+), 53 deletions(-)
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
index d9ad56fb2aa9..60ec57d73a12 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
@@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
}
}
+static int rvin_parallel_parse_of(struct rvin_dev *vin)
+{
+ struct fwnode_handle *ep, *fwnode;
+ struct v4l2_fwnode_endpoint vep = {
+ .bus_type = V4L2_MBUS_UNKNOWN,
+ };
+ struct v4l2_async_connection *asc;
+ int ret;
+
+ ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
+ if (!ep)
+ return 0;
+
+ fwnode = fwnode_graph_get_remote_endpoint(ep);
+ ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+ fwnode_handle_put(ep);
+ if (ret) {
+ vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
+ ret = -EINVAL;
+ goto out;
+ }
+
+ switch (vep.bus_type) {
+ case V4L2_MBUS_PARALLEL:
+ case V4L2_MBUS_BT656:
+ vin_dbg(vin, "Found %s media bus\n",
+ vep.bus_type == V4L2_MBUS_PARALLEL ?
+ "PARALLEL" : "BT656");
+ vin->parallel.mbus_type = vep.bus_type;
+ vin->parallel.bus = vep.bus.parallel;
+ break;
+ default:
+ vin_err(vin, "Unknown media bus type\n");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
+ struct v4l2_async_connection);
+ if (IS_ERR(asc)) {
+ ret = PTR_ERR(asc);
+ goto out;
+ }
+
+ vin->parallel.asc = asc;
+
+ vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
+out:
+ fwnode_handle_put(fwnode);
+
+ return ret;
+}
+
static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
unsigned int max_id)
{
@@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
.complete = rvin_parallel_notify_complete,
};
-static int rvin_parallel_parse_of(struct rvin_dev *vin)
-{
- struct fwnode_handle *ep, *fwnode;
- struct v4l2_fwnode_endpoint vep = {
- .bus_type = V4L2_MBUS_UNKNOWN,
- };
- struct v4l2_async_connection *asc;
- int ret;
-
- ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
- if (!ep)
- return 0;
-
- fwnode = fwnode_graph_get_remote_endpoint(ep);
- ret = v4l2_fwnode_endpoint_parse(ep, &vep);
- fwnode_handle_put(ep);
- if (ret) {
- vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
- ret = -EINVAL;
- goto out;
- }
-
- switch (vep.bus_type) {
- case V4L2_MBUS_PARALLEL:
- case V4L2_MBUS_BT656:
- vin_dbg(vin, "Found %s media bus\n",
- vep.bus_type == V4L2_MBUS_PARALLEL ?
- "PARALLEL" : "BT656");
- vin->parallel.mbus_type = vep.bus_type;
- vin->parallel.bus = vep.bus.parallel;
- break;
- default:
- vin_err(vin, "Unknown media bus type\n");
- ret = -EINVAL;
- goto out;
- }
-
- asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
- struct v4l2_async_connection);
- if (IS_ERR(asc)) {
- ret = PTR_ERR(asc);
- goto out;
- }
-
- vin->parallel.asc = asc;
-
- vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
-out:
- fwnode_handle_put(fwnode);
-
- return ret;
-}
-
static void rvin_parallel_cleanup(struct rvin_dev *vin)
{
v4l2_async_nf_unregister(&vin->notifier);
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 5/6] media: rcar-vin: Merge all notifiers
2025-05-21 13:20 [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2 Niklas Söderlund
` (3 preceding siblings ...)
2025-05-21 13:20 ` [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers Niklas Söderlund
@ 2025-05-21 13:20 ` Niklas Söderlund
2025-05-27 11:45 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 6/6] media: rcar-vin: Enable media-graph on Gen2 Niklas Söderlund
2025-05-30 12:41 ` [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC " Tomi Valkeinen
6 siblings, 1 reply; 22+ messages in thread
From: Niklas Söderlund @ 2025-05-21 13:20 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen,
Laurent Pinchart, linux-media, linux-renesas-soc
Cc: Niklas Söderlund
The VIN usage of v4l-async is complex and stems from organic growth of
the driver of supporting both private local subdevices (Gen2, Gen3) and
subdevices shared between all VIN instances (Gen3 and Gen4).
The driver used a separate notifier for each VIN for the private local
ones, and a shared group notifier for the shared ones. This was complex
and lead to subtle bugs when unbinding and later rebinding subdevices in
on of the notifiers having to handle different edge cases depending on
if it also had subdevices in the other notifiers etc.
To simplify this have the Gen2 devices allocate and form a VIN group
too. This way all subdevices on all models can be collect in a
single group notifier. Then there is only a single complete callback for
all where the video devices and subdevice nodes can be registered etc.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
.../platform/renesas/rcar-vin/rcar-core.c | 263 ++++++++----------
.../platform/renesas/rcar-vin/rcar-vin.h | 2 -
2 files changed, 114 insertions(+), 151 deletions(-)
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
index 60ec57d73a12..b0727e98dac6 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
@@ -43,6 +43,9 @@
#define v4l2_dev_to_vin(d) container_of(d, struct rvin_dev, v4l2_dev)
+static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
+ struct v4l2_subdev *subdev);
+
/* -----------------------------------------------------------------------------
* Gen3 Group Allocator
*/
@@ -232,7 +235,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
}
}
- return vin->group->link_setup(vin->group);
+ if (vin->group->link_setup)
+ return vin->group->link_setup(vin->group);
+
+ return 0;
}
static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
@@ -240,22 +246,31 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
struct v4l2_async_connection *asc)
{
struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
- unsigned int i;
+ struct rvin_group *group = vin->group;
- for (i = 0; i < RCAR_VIN_NUM; i++)
- if (vin->group->vin[i])
- rvin_v4l2_unregister(vin->group->vin[i]);
+ for (unsigned int i = 0; i < RCAR_VIN_NUM; i++)
+ if (group->vin[i])
+ rvin_v4l2_unregister(group->vin[i]);
mutex_lock(&vin->group->lock);
+ for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
+ if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
+ continue;
+
+ group->vin[i]->parallel.subdev = NULL;
+
+ vin_dbg(group->vin[i], "Unbind parallel subdev %s\n",
+ subdev->name);
+ }
- for (i = 0; i < RVIN_REMOTES_MAX; i++) {
- if (vin->group->remotes[i].asc != asc)
+ for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
+ if (group->remotes[i].asc != asc)
continue;
- vin->group->remotes[i].subdev = NULL;
+
+ group->remotes[i].subdev = NULL;
+
vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
- break;
}
-
mutex_unlock(&vin->group->lock);
media_device_unregister(&vin->group->mdev);
@@ -266,21 +281,38 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
struct v4l2_async_connection *asc)
{
struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
- unsigned int i;
+ struct rvin_group *group = vin->group;
- mutex_lock(&vin->group->lock);
+ guard(mutex)(&group->lock);
- for (i = 0; i < RVIN_REMOTES_MAX; i++) {
+ for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
+ int ret;
+
+ if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
+ continue;
+
+ ret = rvin_parallel_subdevice_attach(group->vin[i], subdev);
+ if (ret)
+ return ret;
+
+ v4l2_set_subdev_hostdata(subdev, group->vin[i]);
+
+ vin_dbg(group->vin[i], "Bound subdev %s\n", subdev->name);
+
+ return 0;
+ }
+
+ for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
if (vin->group->remotes[i].asc != asc)
continue;
+
vin->group->remotes[i].subdev = subdev;
vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
- break;
+
+ return 0;
}
- mutex_unlock(&vin->group->lock);
-
- return 0;
+ return -ENODEV;
}
static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
@@ -374,7 +406,7 @@ static int rvin_parallel_parse_of(struct rvin_dev *vin)
goto out;
}
- asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
+ asc = v4l2_async_nf_add_fwnode(&vin->group->notifier, fwnode,
struct v4l2_async_connection);
if (IS_ERR(asc)) {
ret = PTR_ERR(asc);
@@ -424,6 +456,12 @@ static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
if (!(vin_mask & BIT(i)))
continue;
+ /* Parse local subdevice. */
+ ret = rvin_parallel_parse_of(vin->group->vin[i]);
+ if (ret)
+ return ret;
+
+ /* Prase shared subdevices. */
for (id = 0; id < max_id; id++) {
if (vin->group->remotes[id].asc)
continue;
@@ -603,124 +641,6 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
return 0;
}
-static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
-{
- rvin_v4l2_unregister(vin);
- vin->parallel.subdev = NULL;
-
- if (!vin->info->use_mc)
- rvin_free_controls(vin);
-}
-
-static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
-{
- struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
- struct media_entity *source;
- struct media_entity *sink;
- int ret;
-
- ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
- if (ret < 0) {
- vin_err(vin, "Failed to register subdev nodes\n");
- return ret;
- }
-
- 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,
- struct v4l2_subdev *subdev,
- struct v4l2_async_connection *asc)
-{
- struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
-
- vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
-
- mutex_lock(&vin->lock);
- rvin_parallel_subdevice_detach(vin);
- mutex_unlock(&vin->lock);
-}
-
-static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
- struct v4l2_subdev *subdev,
- struct v4l2_async_connection *asc)
-{
- struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
- int ret;
-
- mutex_lock(&vin->lock);
- ret = rvin_parallel_subdevice_attach(vin, subdev);
- mutex_unlock(&vin->lock);
- if (ret)
- return ret;
-
- v4l2_set_subdev_hostdata(subdev, vin);
-
- vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
- subdev->name, vin->parallel.source_pad,
- vin->parallel.sink_pad);
-
- return 0;
-}
-
-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 void rvin_parallel_cleanup(struct rvin_dev *vin)
-{
- v4l2_async_nf_unregister(&vin->notifier);
- v4l2_async_nf_cleanup(&vin->notifier);
-}
-
-static int rvin_parallel_init(struct rvin_dev *vin)
-{
- int ret;
-
- v4l2_async_nf_init(&vin->notifier, &vin->v4l2_dev);
-
- ret = rvin_parallel_parse_of(vin);
- if (ret)
- return ret;
-
- if (!vin->parallel.asc)
- return -ENODEV;
-
- vin_dbg(vin, "Found parallel subdevice %pOF\n",
- to_of_node(vin->parallel.asc->match.fwnode));
-
- vin->notifier.ops = &rvin_parallel_notify_ops;
- ret = v4l2_async_nf_register(&vin->notifier);
- if (ret < 0) {
- vin_err(vin, "Notifier registration failed\n");
- v4l2_async_nf_cleanup(&vin->notifier);
- return ret;
- }
-
- return 0;
-}
-
/* -----------------------------------------------------------------------------
* CSI-2
*/
@@ -895,11 +815,63 @@ static int rvin_csi2_create_link(struct rvin_group *group, unsigned int id,
return 0;
}
+static int rvin_parallel_setup_links(struct rvin_group *group)
+{
+ u32 flags = MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE;
+ int ret = 0;
+
+ mutex_lock(&group->lock);
+ /* If the group also have links don't enable the link. */
+ for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
+ if (group->remotes[i].subdev) {
+ flags = 0;
+ break;
+ }
+ }
+
+ /* Create links */
+ for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
+ struct rvin_dev *vin = group->vin[i];
+ struct media_entity *source;
+ struct media_entity *sink;
+
+ /* Noting to do if their is no VIN or parallel subdev. */
+ if (!vin || !vin->parallel.subdev)
+ continue;
+
+ source = &vin->parallel.subdev->entity;
+ sink = &vin->vdev.entity;
+
+ ret = media_create_pad_link(source, vin->parallel.source_pad,
+ sink, 0, flags);
+ if (ret)
+ break;
+ }
+ mutex_unlock(&group->lock);
+
+ return ret;
+}
+
static int rvin_csi2_setup_links(struct rvin_group *group)
{
const struct rvin_group_route *routes, *route;
unsigned int id;
- int ret = -EINVAL;
+ int ret;
+
+ /* Find any VIN in group to get route info. */
+ routes = NULL;
+ for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
+ if (group->vin[i]) {
+ routes = group->vin[i]->info->routes;
+ break;
+ }
+ }
+ if (!routes)
+ return -ENODEV;
+
+ ret = rvin_parallel_setup_links(group);
+ if (ret)
+ return ret;
/* Find any VIN in group to get route info. */
routes = NULL;
@@ -914,6 +886,7 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
/* Create all media device links between VINs and CSI-2's. */
mutex_lock(&group->lock);
+ ret = -EINVAL;
for (route = routes; route->chsel; route++) {
/* Check that VIN' master is part of the group. */
if (!group->vin[route->master])
@@ -941,7 +914,6 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
static void rvin_csi2_cleanup(struct rvin_dev *vin)
{
- rvin_parallel_cleanup(vin);
rvin_group_notifier_cleanup(vin);
rvin_group_put(vin);
rvin_free_controls(vin);
@@ -964,18 +936,11 @@ static int rvin_csi2_init(struct rvin_dev *vin)
if (ret)
goto err_controls;
- /* It's OK to not have a parallel subdevice. */
- ret = rvin_parallel_init(vin);
- if (ret && ret != -ENODEV)
- goto err_group;
-
ret = rvin_group_notifier_init(vin, 1, RVIN_CSI_MAX);
if (ret)
- goto err_parallel;
+ goto err_group;
return 0;
-err_parallel:
- rvin_parallel_cleanup(vin);
err_group:
rvin_group_put(vin);
err_controls:
@@ -1448,7 +1413,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
rvin_group_id_to_master(vin->id) == vin->id)
vin->scaler = vin->info->scaler;
} else {
- ret = rvin_parallel_init(vin);
+ ret = rvin_group_get(vin, NULL, NULL);
+ if (!ret)
+ ret = rvin_group_notifier_init(vin, 0, 0);
if (vin->info->scaler)
vin->scaler = vin->info->scaler;
@@ -1478,8 +1445,6 @@ static void rcar_vin_remove(struct platform_device *pdev)
rvin_isp_cleanup(vin);
else if (vin->info->use_mc)
rvin_csi2_cleanup(vin);
- else
- rvin_parallel_cleanup(vin);
rvin_id_put(vin);
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
index 7d4fce248976..a577f4fe4a6c 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
@@ -149,7 +149,6 @@ struct rvin_info {
* @vdev: V4L2 video device associated with VIN
* @v4l2_dev: V4L2 device
* @ctrl_handler: V4L2 control handler
- * @notifier: V4L2 asynchronous subdevs notifier
*
* @parallel: parallel input subdevice descriptor
*
@@ -189,7 +188,6 @@ struct rvin_dev {
struct video_device vdev;
struct v4l2_device v4l2_dev;
struct v4l2_ctrl_handler ctrl_handler;
- struct v4l2_async_notifier notifier;
struct rvin_parallel_entity parallel;
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 6/6] media: rcar-vin: Enable media-graph on Gen2
2025-05-21 13:20 [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2 Niklas Söderlund
` (4 preceding siblings ...)
2025-05-21 13:20 ` [PATCH v4 5/6] media: rcar-vin: Merge all notifiers Niklas Söderlund
@ 2025-05-21 13:20 ` Niklas Söderlund
2025-05-27 11:48 ` Laurent Pinchart
2025-05-30 12:41 ` [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC " Tomi Valkeinen
6 siblings, 1 reply; 22+ messages in thread
From: Niklas Söderlund @ 2025-05-21 13:20 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen,
Laurent Pinchart, linux-media, linux-renesas-soc
Cc: Niklas Söderlund
Complete the conversion from soc_camera to a full fledge media
controller enabled devices for all supported generations of the device.
All work is already done as this is already supported on Gen3, and
later.
All that is missing is some small touch ups of the unified v4l-notifier,
unconditionally creation of a media pad for each VIN and removing all
special cases for the non media-graph call paths.
Mark the completion of converting from soc_camera by injecting an
attribution of myself in the header.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v3
- Resolve conflicts with other VIN work merged a head of this series.
---
.../platform/renesas/rcar-vin/rcar-core.c | 249 ++-------
.../platform/renesas/rcar-vin/rcar-dma.c | 16 +-
.../platform/renesas/rcar-vin/rcar-v4l2.c | 488 +-----------------
.../platform/renesas/rcar-vin/rcar-vin.h | 10 +-
4 files changed, 66 insertions(+), 697 deletions(-)
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
index b0727e98dac6..5958bf89e64c 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
@@ -2,12 +2,11 @@
/*
* Driver for Renesas R-Car VIN
*
+ * Copyright (C) 2025 Niklas Söderlund <niklas.soderlund@ragnatech.se>
* Copyright (C) 2016 Renesas Electronics Corp.
* Copyright (C) 2011-2013 Renesas Solutions Corp.
* Copyright (C) 2013 Cogent Embedded, Inc., <source@cogentembedded.com>
* Copyright (C) 2008 Magnus Damm
- *
- * Based on the soc-camera rcar_vin driver
*/
#include <linux/idr.h>
@@ -43,9 +42,6 @@
#define v4l2_dev_to_vin(d) container_of(d, struct rvin_dev, v4l2_dev)
-static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
- struct v4l2_subdev *subdev);
-
/* -----------------------------------------------------------------------------
* Gen3 Group Allocator
*/
@@ -235,10 +231,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
}
}
- if (vin->group->link_setup)
- return vin->group->link_setup(vin->group);
-
- return 0;
+ return vin->group->link_setup(vin->group);
}
static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
@@ -286,17 +279,15 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
guard(mutex)(&group->lock);
for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
- int ret;
-
if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
continue;
- ret = rvin_parallel_subdevice_attach(group->vin[i], subdev);
- if (ret)
- return ret;
-
- v4l2_set_subdev_hostdata(subdev, group->vin[i]);
+ group->vin[i]->parallel.source_pad = 0;
+ for (unsigned int pad = 0; pad < subdev->entity.num_pads; pad++)
+ if (subdev->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)
+ group->vin[i]->parallel.source_pad = pad;
+ group->vin[i]->parallel.subdev = subdev;
vin_dbg(group->vin[i], "Bound subdev %s\n", subdev->name);
return 0;
@@ -361,14 +352,6 @@ static int rvin_group_parse_of(struct rvin_dev *vin, unsigned int port,
return ret;
}
-static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
-{
- if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
- v4l2_async_nf_unregister(&vin->group->notifier);
- v4l2_async_nf_cleanup(&vin->group->notifier);
- }
-}
-
static int rvin_parallel_parse_of(struct rvin_dev *vin)
{
struct fwnode_handle *ep, *fwnode;
@@ -514,7 +497,7 @@ static void rvin_free_controls(struct rvin_dev *vin)
vin->vdev.ctrl_handler = NULL;
}
-static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev)
+static int rvin_create_controls(struct rvin_dev *vin)
{
int ret;
@@ -532,115 +515,11 @@ static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev
return ret;
}
- /* For the non-MC mode add controls from the subdevice. */
- if (subdev) {
- ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
- subdev->ctrl_handler, NULL, true);
- if (ret < 0) {
- rvin_free_controls(vin);
- return ret;
- }
- }
-
vin->vdev.ctrl_handler = &vin->ctrl_handler;
return 0;
}
-/* -----------------------------------------------------------------------------
- * Async notifier
- */
-
-static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
-{
- unsigned int pad;
-
- if (sd->entity.num_pads <= 1)
- return 0;
-
- for (pad = 0; pad < sd->entity.num_pads; pad++)
- if (sd->entity.pads[pad].flags & direction)
- return pad;
-
- return -EINVAL;
-}
-
-/* -----------------------------------------------------------------------------
- * Parallel async notifier
- */
-
-/* The vin lock should be held when calling the subdevice attach and detach */
-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,
- };
- int ret;
-
- /* Find source and sink pad of remote subdevice */
- ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
- if (ret < 0)
- return ret;
- vin->parallel.source_pad = ret;
-
- 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;
- code.pad = vin->parallel.source_pad;
- while (!vin->mbus_code &&
- !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
- code.index++;
- switch (code.code) {
- case MEDIA_BUS_FMT_YUYV8_1X16:
- case MEDIA_BUS_FMT_UYVY8_1X16:
- case MEDIA_BUS_FMT_UYVY8_2X8:
- case MEDIA_BUS_FMT_UYVY10_2X10:
- case MEDIA_BUS_FMT_RGB888_1X24:
- vin->mbus_code = code.code;
- vin_dbg(vin, "Found media bus format for %s: %d\n",
- subdev->name, vin->mbus_code);
- break;
- default:
- break;
- }
- }
-
- if (!vin->mbus_code) {
- vin_err(vin, "Unsupported media bus format for %s\n",
- subdev->name);
- return -EINVAL;
- }
-
- /* Read tvnorms */
- ret = v4l2_subdev_call(subdev, video, g_tvnorms, &vin->vdev.tvnorms);
- if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
- return ret;
-
- /* Read standard */
- vin->std = V4L2_STD_UNKNOWN;
- ret = v4l2_subdev_call(subdev, video, g_std, &vin->std);
- if (ret < 0 && ret != -ENOIOCTLCMD)
- return ret;
-
- /* Add the controls */
- ret = rvin_create_controls(vin, subdev);
- if (ret < 0)
- return ret;
-
- vin->parallel.subdev = subdev;
-
- return 0;
-}
-
/* -----------------------------------------------------------------------------
* CSI-2
*/
@@ -912,39 +791,17 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
return ret;
}
-static void rvin_csi2_cleanup(struct rvin_dev *vin)
-{
- rvin_group_notifier_cleanup(vin);
- rvin_group_put(vin);
- rvin_free_controls(vin);
-}
-
static int rvin_csi2_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_create_controls(vin, NULL);
- if (ret < 0)
- return ret;
-
ret = rvin_group_get(vin, rvin_csi2_setup_links, &rvin_csi2_media_ops);
if (ret)
- goto err_controls;
+ return ret;
ret = rvin_group_notifier_init(vin, 1, RVIN_CSI_MAX);
if (ret)
- goto err_group;
-
- return 0;
-err_group:
- rvin_group_put(vin);
-err_controls:
- rvin_free_controls(vin);
+ rvin_group_put(vin);
return ret;
}
@@ -1000,41 +857,19 @@ static int rvin_isp_setup_links(struct rvin_group *group)
return ret;
}
-static void rvin_isp_cleanup(struct rvin_dev *vin)
-{
- rvin_group_notifier_cleanup(vin);
- rvin_group_put(vin);
- rvin_free_controls(vin);
-}
-
static int rvin_isp_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_create_controls(vin, NULL);
- if (ret < 0)
- return ret;
-
ret = rvin_group_get(vin, rvin_isp_setup_links, NULL);
if (ret)
- goto err_controls;
+ return ret;
ret = rvin_group_notifier_init(vin, 2, RVIN_ISP_MAX);
if (ret)
- goto err_group;
+ rvin_group_put(vin);
return 0;
-err_group:
- rvin_group_put(vin);
-err_controls:
- rvin_free_controls(vin);
-
- return ret;
}
/* -----------------------------------------------------------------------------
@@ -1067,7 +902,7 @@ static int __maybe_unused rvin_resume(struct device *dev)
* as we don't know if and in which order the master VINs will
* be resumed.
*/
- if (vin->info->use_mc) {
+ if (vin->info->model == RCAR_GEN3) {
unsigned int master_id = rvin_group_id_to_master(vin->id);
struct rvin_dev *master = vin->group->vin[master_id];
int ret;
@@ -1089,7 +924,6 @@ static int __maybe_unused rvin_resume(struct device *dev)
static const struct rvin_info rcar_info_h1 = {
.model = RCAR_H1,
- .use_mc = false,
.max_width = 2048,
.max_height = 2048,
.scaler = rvin_scaler_gen2,
@@ -1097,7 +931,6 @@ static const struct rvin_info rcar_info_h1 = {
static const struct rvin_info rcar_info_m1 = {
.model = RCAR_M1,
- .use_mc = false,
.max_width = 2048,
.max_height = 2048,
.scaler = rvin_scaler_gen2,
@@ -1105,7 +938,6 @@ static const struct rvin_info rcar_info_m1 = {
static const struct rvin_info rcar_info_gen2 = {
.model = RCAR_GEN2,
- .use_mc = false,
.max_width = 2048,
.max_height = 2048,
.scaler = rvin_scaler_gen2,
@@ -1120,7 +952,6 @@ static const struct rvin_group_route rcar_info_r8a774e1_routes[] = {
static const struct rvin_info rcar_info_r8a774e1 = {
.model = RCAR_GEN3,
- .use_mc = true,
.max_width = 4096,
.max_height = 4096,
.routes = rcar_info_r8a774e1_routes,
@@ -1136,7 +967,6 @@ static const struct rvin_group_route rcar_info_r8a7795_routes[] = {
static const struct rvin_info rcar_info_r8a7795 = {
.model = RCAR_GEN3,
- .use_mc = true,
.nv12 = true,
.max_width = 4096,
.max_height = 4096,
@@ -1154,7 +984,6 @@ static const struct rvin_group_route rcar_info_r8a7796_routes[] = {
static const struct rvin_info rcar_info_r8a7796 = {
.model = RCAR_GEN3,
- .use_mc = true,
.nv12 = true,
.max_width = 4096,
.max_height = 4096,
@@ -1172,7 +1001,6 @@ static const struct rvin_group_route rcar_info_r8a77965_routes[] = {
static const struct rvin_info rcar_info_r8a77965 = {
.model = RCAR_GEN3,
- .use_mc = true,
.nv12 = true,
.max_width = 4096,
.max_height = 4096,
@@ -1187,7 +1015,6 @@ static const struct rvin_group_route rcar_info_r8a77970_routes[] = {
static const struct rvin_info rcar_info_r8a77970 = {
.model = RCAR_GEN3,
- .use_mc = true,
.max_width = 4096,
.max_height = 4096,
.routes = rcar_info_r8a77970_routes,
@@ -1201,7 +1028,6 @@ static const struct rvin_group_route rcar_info_r8a77980_routes[] = {
static const struct rvin_info rcar_info_r8a77980 = {
.model = RCAR_GEN3,
- .use_mc = true,
.nv12 = true,
.max_width = 4096,
.max_height = 4096,
@@ -1215,7 +1041,6 @@ static const struct rvin_group_route rcar_info_r8a77990_routes[] = {
static const struct rvin_info rcar_info_r8a77990 = {
.model = RCAR_GEN3,
- .use_mc = true,
.nv12 = true,
.max_width = 4096,
.max_height = 4096,
@@ -1229,7 +1054,6 @@ static const struct rvin_group_route rcar_info_r8a77995_routes[] = {
static const struct rvin_info rcar_info_r8a77995 = {
.model = RCAR_GEN3,
- .use_mc = true,
.nv12 = true,
.max_width = 4096,
.max_height = 4096,
@@ -1239,7 +1063,6 @@ static const struct rvin_info rcar_info_r8a77995 = {
static const struct rvin_info rcar_info_gen4 = {
.model = RCAR_GEN4,
- .use_mc = true,
.use_isp = true,
.nv12 = true,
.raw10 = true,
@@ -1404,24 +1227,40 @@ static int rcar_vin_probe(struct platform_device *pdev)
if (rvin_id_get(vin))
return -EINVAL;
- if (vin->info->use_isp) {
- ret = rvin_isp_init(vin);
- } else if (vin->info->use_mc) {
- ret = rvin_csi2_init(vin);
+ vin->pad.flags = MEDIA_PAD_FL_SINK;
+ ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
+ if (ret)
+ return ret;
- if (vin->info->scaler &&
- rvin_group_id_to_master(vin->id) == vin->id)
- vin->scaler = vin->info->scaler;
- } else {
- ret = rvin_group_get(vin, NULL, NULL);
+ ret = rvin_create_controls(vin);
+ if (ret < 0)
+ return ret;
+
+ switch (vin->info->model) {
+ case RCAR_GEN3:
+ case RCAR_GEN4:
+ if (vin->info->use_isp) {
+ ret = rvin_isp_init(vin);
+ } else {
+ ret = rvin_csi2_init(vin);
+
+ if (vin->info->scaler &&
+ rvin_group_id_to_master(vin->id) == vin->id)
+ vin->scaler = vin->info->scaler;
+ }
+ break;
+ default:
+ ret = rvin_group_get(vin, rvin_parallel_setup_links, NULL);
if (!ret)
ret = rvin_group_notifier_init(vin, 0, 0);
if (vin->info->scaler)
vin->scaler = vin->info->scaler;
+ break;
}
if (ret) {
+ rvin_free_controls(vin);
rvin_dma_unregister(vin);
rvin_id_put(vin);
return ret;
@@ -1441,10 +1280,14 @@ static void rcar_vin_remove(struct platform_device *pdev)
rvin_v4l2_unregister(vin);
- if (vin->info->use_isp)
- rvin_isp_cleanup(vin);
- else if (vin->info->use_mc)
- rvin_csi2_cleanup(vin);
+ if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
+ v4l2_async_nf_unregister(&vin->group->notifier);
+ v4l2_async_nf_cleanup(&vin->group->notifier);
+ }
+
+ rvin_group_put(vin);
+
+ rvin_free_controls(vin);
rvin_id_put(vin);
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
index 5c08ee2c9807..f0f2dba19eea 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
@@ -2,12 +2,11 @@
/*
* Driver for Renesas R-Car VIN
*
+ * Copyright (C) 2025 Niklas Söderlund <niklas.soderlund@ragnatech.se>
* Copyright (C) 2016 Renesas Electronics Corp.
* Copyright (C) 2011-2013 Renesas Solutions Corp.
* Copyright (C) 2013 Cogent Embedded, Inc., <source@cogentembedded.com>
* Copyright (C) 2008 Magnus Damm
- *
- * Based on the soc-camera rcar_vin driver
*/
#include <linux/delay.h>
@@ -560,8 +559,6 @@ void rvin_scaler_gen2(struct rvin_dev *vin)
/* Set scaling coefficient */
crop_height = vin->crop.height;
- if (V4L2_FIELD_HAS_BOTH(vin->format.field))
- crop_height *= 2;
ys = 0;
if (crop_height != vin->compose.height)
@@ -700,9 +697,6 @@ static int rvin_setup(struct rvin_dev *vin)
case V4L2_FIELD_INTERLACED:
/* Default to TB */
vnmc = VNMC_IM_FULL;
- /* Use BT if video standard can be read and is 60 Hz format */
- if (!vin->info->use_mc && vin->std & V4L2_STD_525_60)
- vnmc = VNMC_IM_FULL | VNMC_FOC;
break;
case V4L2_FIELD_INTERLACED_TB:
vnmc = VNMC_IM_FULL;
@@ -1297,14 +1291,6 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
struct media_pad *pad;
int ret;
- /* No media controller used, simply pass operation to subdevice. */
- if (!vin->info->use_mc) {
- ret = v4l2_subdev_call(vin->parallel.subdev, video, s_stream,
- on);
-
- return ret == -ENOIOCTLCMD ? 0 : ret;
- }
-
pad = media_pad_remote_pad_first(&vin->pad);
if (!pad)
return -EPIPE;
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
index db091af57c19..59b01cb0628a 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
@@ -2,12 +2,11 @@
/*
* Driver for Renesas R-Car VIN
*
+ * Copyright (C) 2025 Niklas Söderlund <niklas.soderlund@ragnatech.se>
* Copyright (C) 2016 Renesas Electronics Corp.
* Copyright (C) 2011-2013 Renesas Solutions Corp.
* Copyright (C) 2013 Cogent Embedded, Inc., <source@cogentembedded.com>
* Copyright (C) 2008 Magnus Damm
- *
- * Based on the soc-camera rcar_vin driver
*/
#include <linux/pm_runtime.h>
@@ -230,101 +229,6 @@ static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix)
* V4L2
*/
-static int rvin_reset_format(struct rvin_dev *vin)
-{
- struct v4l2_subdev_format fmt = {
- .which = V4L2_SUBDEV_FORMAT_ACTIVE,
- .pad = vin->parallel.source_pad,
- };
- int ret;
-
- ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
- if (ret)
- return ret;
-
- v4l2_fill_pix_format(&vin->format, &fmt.format);
-
- vin->crop.top = 0;
- vin->crop.left = 0;
- vin->crop.width = vin->format.width;
- vin->crop.height = vin->format.height;
-
- /* Make use of the hardware interlacer by default. */
- if (vin->format.field == V4L2_FIELD_ALTERNATE) {
- vin->format.field = V4L2_FIELD_INTERLACED;
- vin->format.height *= 2;
- }
-
- rvin_format_align(vin, &vin->format);
-
- vin->compose.top = 0;
- vin->compose.left = 0;
- vin->compose.width = vin->format.width;
- vin->compose.height = vin->format.height;
-
- return 0;
-}
-
-static int rvin_try_format(struct rvin_dev *vin, u32 which,
- struct v4l2_pix_format *pix,
- struct v4l2_rect *src_rect)
-{
- struct v4l2_subdev *sd = vin_to_source(vin);
- struct v4l2_subdev_state *sd_state;
- static struct lock_class_key key;
- struct v4l2_subdev_format format = {
- .which = which,
- .pad = vin->parallel.source_pad,
- };
- enum v4l2_field field;
- u32 width, height;
- int ret;
-
- /*
- * FIXME: Drop this call, drivers are not supposed to use
- * __v4l2_subdev_state_alloc().
- */
- sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
- if (IS_ERR(sd_state))
- return PTR_ERR(sd_state);
-
- if (!rvin_format_from_pixel(vin, pix->pixelformat))
- pix->pixelformat = RVIN_DEFAULT_FORMAT;
-
- v4l2_fill_mbus_format(&format.format, pix, vin->mbus_code);
-
- /* Allow the video device to override field and to scale */
- field = pix->field;
- width = pix->width;
- height = pix->height;
-
- ret = v4l2_subdev_call(sd, pad, set_fmt, sd_state, &format);
- if (ret < 0 && ret != -ENOIOCTLCMD)
- goto done;
- ret = 0;
-
- v4l2_fill_pix_format(pix, &format.format);
-
- if (src_rect) {
- src_rect->top = 0;
- src_rect->left = 0;
- src_rect->width = pix->width;
- src_rect->height = pix->height;
- }
-
- if (field != V4L2_FIELD_ANY)
- pix->field = field;
-
- pix->width = width;
- pix->height = height;
-
- rvin_format_align(vin, pix);
-done:
- __v4l2_subdev_state_free(sd_state);
-
- return ret;
-}
-
static int rvin_querycap(struct file *file, void *priv,
struct v4l2_capability *cap)
{
@@ -333,42 +237,6 @@ static int rvin_querycap(struct file *file, void *priv,
return 0;
}
-static int rvin_try_fmt_vid_cap(struct file *file, void *priv,
- struct v4l2_format *f)
-{
- struct rvin_dev *vin = video_drvdata(file);
-
- return rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, &f->fmt.pix, NULL);
-}
-
-static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
- struct v4l2_format *f)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_rect fmt_rect, src_rect;
- int ret;
-
- if (vb2_is_busy(&vin->queue))
- return -EBUSY;
-
- ret = rvin_try_format(vin, V4L2_SUBDEV_FORMAT_ACTIVE, &f->fmt.pix,
- &src_rect);
- if (ret)
- return ret;
-
- vin->format = f->fmt.pix;
-
- fmt_rect.top = 0;
- fmt_rect.left = 0;
- fmt_rect.width = vin->format.width;
- fmt_rect.height = vin->format.height;
-
- v4l2_rect_map_inside(&vin->crop, &src_rect);
- v4l2_rect_map_inside(&vin->compose, &fmt_rect);
-
- return 0;
-}
-
static int rvin_g_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_format *f)
{
@@ -465,6 +333,7 @@ static int rvin_enum_fmt_vid_cap(struct file *file, void *priv,
static int rvin_remote_rectangle(struct rvin_dev *vin, struct v4l2_rect *rect)
{
+ struct media_pad *pad = media_pad_remote_pad_first(&vin->pad);
struct v4l2_subdev_format fmt = {
.which = V4L2_SUBDEV_FORMAT_ACTIVE,
};
@@ -472,18 +341,11 @@ static int rvin_remote_rectangle(struct rvin_dev *vin, struct v4l2_rect *rect)
unsigned int index;
int ret;
- if (vin->info->use_mc) {
- struct media_pad *pad = media_pad_remote_pad_first(&vin->pad);
+ if (!pad)
+ return -EINVAL;
- if (!pad)
- return -EINVAL;
-
- sd = media_entity_to_v4l2_subdev(pad->entity);
- index = pad->index;
- } else {
- sd = vin_to_source(vin);
- index = vin->parallel.source_pad;
- }
+ sd = media_entity_to_v4l2_subdev(pad->entity);
+ index = pad->index;
fmt.pad = index;
ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
@@ -623,113 +485,6 @@ static int rvin_s_selection(struct file *file, void *fh,
return 0;
}
-static int rvin_g_parm(struct file *file, void *priv,
- struct v4l2_streamparm *parm)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
-
- return v4l2_g_parm_cap(&vin->vdev, sd, parm);
-}
-
-static int rvin_s_parm(struct file *file, void *priv,
- struct v4l2_streamparm *parm)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
-
- return v4l2_s_parm_cap(&vin->vdev, sd, parm);
-}
-
-static int rvin_g_pixelaspect(struct file *file, void *priv,
- int type, struct v4l2_fract *f)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
-
- if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
- return -EINVAL;
-
- return v4l2_subdev_call(sd, video, g_pixelaspect, f);
-}
-
-static int rvin_enum_input(struct file *file, void *priv,
- struct v4l2_input *i)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
- int ret;
-
- if (i->index != 0)
- return -EINVAL;
-
- ret = v4l2_subdev_call(sd, video, g_input_status, &i->status);
- if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
- return ret;
-
- i->type = V4L2_INPUT_TYPE_CAMERA;
-
- if (v4l2_subdev_has_op(sd, pad, dv_timings_cap)) {
- i->capabilities = V4L2_IN_CAP_DV_TIMINGS;
- i->std = 0;
- } else {
- i->capabilities = V4L2_IN_CAP_STD;
- i->std = vin->vdev.tvnorms;
- }
-
- strscpy(i->name, "Camera", sizeof(i->name));
-
- return 0;
-}
-
-static int rvin_g_input(struct file *file, void *priv, unsigned int *i)
-{
- *i = 0;
- return 0;
-}
-
-static int rvin_s_input(struct file *file, void *priv, unsigned int i)
-{
- if (i > 0)
- return -EINVAL;
- return 0;
-}
-
-static int rvin_querystd(struct file *file, void *priv, v4l2_std_id *a)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
-
- return v4l2_subdev_call(sd, video, querystd, a);
-}
-
-static int rvin_s_std(struct file *file, void *priv, v4l2_std_id a)
-{
- struct rvin_dev *vin = video_drvdata(file);
- int ret;
-
- ret = v4l2_subdev_call(vin_to_source(vin), video, s_std, a);
- if (ret < 0)
- return ret;
-
- vin->std = a;
-
- /* Changing the standard will change the width/height */
- return rvin_reset_format(vin);
-}
-
-static int rvin_g_std(struct file *file, void *priv, v4l2_std_id *a)
-{
- struct rvin_dev *vin = video_drvdata(file);
-
- if (v4l2_subdev_has_op(vin_to_source(vin), pad, dv_timings_cap))
- return -ENOIOCTLCMD;
-
- *a = vin->std;
-
- return 0;
-}
-
static int rvin_subscribe_event(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub)
{
@@ -740,167 +495,6 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
return v4l2_ctrl_subscribe_event(fh, sub);
}
-static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
- struct v4l2_enum_dv_timings *timings)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
- int ret;
-
- if (timings->pad)
- return -EINVAL;
-
- timings->pad = vin->parallel.sink_pad;
-
- ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
-
- timings->pad = 0;
-
- return ret;
-}
-
-static int rvin_s_dv_timings(struct file *file, void *priv_fh,
- struct v4l2_dv_timings *timings)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
- int ret;
-
- ret = v4l2_subdev_call(sd, pad, s_dv_timings,
- vin->parallel.sink_pad, timings);
- if (ret)
- return ret;
-
- /* Changing the timings will change the width/height */
- return rvin_reset_format(vin);
-}
-
-static int rvin_g_dv_timings(struct file *file, void *priv_fh,
- struct v4l2_dv_timings *timings)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
-
- return v4l2_subdev_call(sd, pad, g_dv_timings,
- vin->parallel.sink_pad, timings);
-}
-
-static int rvin_query_dv_timings(struct file *file, void *priv_fh,
- struct v4l2_dv_timings *timings)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
-
- return v4l2_subdev_call(sd, pad, query_dv_timings,
- vin->parallel.sink_pad, timings);
-}
-
-static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
- struct v4l2_dv_timings_cap *cap)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
- int ret;
-
- if (cap->pad)
- return -EINVAL;
-
- cap->pad = vin->parallel.sink_pad;
-
- ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
-
- cap->pad = 0;
-
- return ret;
-}
-
-static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
- int ret;
-
- if (edid->pad)
- return -EINVAL;
-
- edid->pad = vin->parallel.sink_pad;
-
- ret = v4l2_subdev_call(sd, pad, get_edid, edid);
-
- edid->pad = 0;
-
- return ret;
-}
-
-static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
-{
- struct rvin_dev *vin = video_drvdata(file);
- struct v4l2_subdev *sd = vin_to_source(vin);
- int ret;
-
- if (edid->pad)
- return -EINVAL;
-
- edid->pad = vin->parallel.sink_pad;
-
- ret = v4l2_subdev_call(sd, pad, set_edid, edid);
-
- edid->pad = 0;
-
- return ret;
-}
-
-static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
- .vidioc_querycap = rvin_querycap,
- .vidioc_try_fmt_vid_cap = rvin_try_fmt_vid_cap,
- .vidioc_g_fmt_vid_cap = rvin_g_fmt_vid_cap,
- .vidioc_s_fmt_vid_cap = rvin_s_fmt_vid_cap,
- .vidioc_enum_fmt_vid_cap = rvin_enum_fmt_vid_cap,
-
- .vidioc_g_selection = rvin_g_selection,
- .vidioc_s_selection = rvin_s_selection,
-
- .vidioc_g_parm = rvin_g_parm,
- .vidioc_s_parm = rvin_s_parm,
-
- .vidioc_g_pixelaspect = rvin_g_pixelaspect,
-
- .vidioc_enum_input = rvin_enum_input,
- .vidioc_g_input = rvin_g_input,
- .vidioc_s_input = rvin_s_input,
-
- .vidioc_dv_timings_cap = rvin_dv_timings_cap,
- .vidioc_enum_dv_timings = rvin_enum_dv_timings,
- .vidioc_g_dv_timings = rvin_g_dv_timings,
- .vidioc_s_dv_timings = rvin_s_dv_timings,
- .vidioc_query_dv_timings = rvin_query_dv_timings,
-
- .vidioc_g_edid = rvin_g_edid,
- .vidioc_s_edid = rvin_s_edid,
-
- .vidioc_querystd = rvin_querystd,
- .vidioc_g_std = rvin_g_std,
- .vidioc_s_std = rvin_s_std,
-
- .vidioc_reqbufs = vb2_ioctl_reqbufs,
- .vidioc_create_bufs = vb2_ioctl_create_bufs,
- .vidioc_querybuf = vb2_ioctl_querybuf,
- .vidioc_qbuf = vb2_ioctl_qbuf,
- .vidioc_dqbuf = vb2_ioctl_dqbuf,
- .vidioc_expbuf = vb2_ioctl_expbuf,
- .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
- .vidioc_streamon = vb2_ioctl_streamon,
- .vidioc_streamoff = vb2_ioctl_streamoff,
-
- .vidioc_log_status = v4l2_ctrl_log_status,
- .vidioc_subscribe_event = rvin_subscribe_event,
- .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
-};
-
-/* -----------------------------------------------------------------------------
- * V4L2 Media Controller
- */
-
static void rvin_mc_try_format(struct rvin_dev *vin,
struct v4l2_pix_format *pix)
{
@@ -979,19 +573,6 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
* File Operations
*/
-static int rvin_power_parallel(struct rvin_dev *vin, bool on)
-{
- struct v4l2_subdev *sd = vin_to_source(vin);
- int power = on ? 1 : 0;
- int ret;
-
- ret = v4l2_subdev_call(sd, core, s_power, power);
- if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
- return ret;
-
- return 0;
-}
-
static int rvin_open(struct file *file)
{
struct rvin_dev *vin = video_drvdata(file);
@@ -1011,11 +592,7 @@ static int rvin_open(struct file *file)
if (ret)
goto err_unlock;
- if (vin->info->use_mc)
- ret = v4l2_pipeline_pm_get(&vin->vdev.entity);
- else if (v4l2_fh_is_singular_file(file))
- ret = rvin_power_parallel(vin, true);
-
+ ret = v4l2_pipeline_pm_get(&vin->vdev.entity);
if (ret < 0)
goto err_open;
@@ -1027,10 +604,7 @@ static int rvin_open(struct file *file)
return 0;
err_power:
- if (vin->info->use_mc)
- v4l2_pipeline_pm_put(&vin->vdev.entity);
- else if (v4l2_fh_is_singular_file(file))
- rvin_power_parallel(vin, false);
+ v4l2_pipeline_pm_put(&vin->vdev.entity);
err_open:
v4l2_fh_release(file);
err_unlock:
@@ -1044,23 +618,14 @@ static int rvin_open(struct file *file)
static int rvin_release(struct file *file)
{
struct rvin_dev *vin = video_drvdata(file);
- bool fh_singular;
int ret;
mutex_lock(&vin->lock);
- /* Save the singular status before we call the clean-up helper */
- fh_singular = v4l2_fh_is_singular_file(file);
-
/* the release helper will cleanup any on-going streaming */
ret = _vb2_fop_release(file, NULL);
- if (vin->info->use_mc) {
- v4l2_pipeline_pm_put(&vin->vdev.entity);
- } else {
- if (fh_singular)
- rvin_power_parallel(vin, false);
- }
+ v4l2_pipeline_pm_put(&vin->vdev.entity);
mutex_unlock(&vin->lock);
@@ -1091,18 +656,6 @@ void rvin_v4l2_unregister(struct rvin_dev *vin)
video_unregister_device(&vin->vdev);
}
-static void rvin_notify_video_device(struct rvin_dev *vin,
- unsigned int notification, void *arg)
-{
- switch (notification) {
- case V4L2_DEVICE_NOTIFY_EVENT:
- v4l2_event_queue(&vin->vdev, arg);
- break;
- default:
- break;
- }
-}
-
static void rvin_notify(struct v4l2_subdev *sd,
unsigned int notification, void *arg)
{
@@ -1113,12 +666,6 @@ static void rvin_notify(struct v4l2_subdev *sd,
container_of(sd->v4l2_dev, struct rvin_dev, v4l2_dev);
unsigned int i;
- /* If no media controller, no need to route the event. */
- if (!vin->info->use_mc) {
- rvin_notify_video_device(vin, notification, arg);
- return;
- }
-
group = vin->group;
for (i = 0; i < RCAR_VIN_NUM; i++) {
@@ -1134,7 +681,13 @@ static void rvin_notify(struct v4l2_subdev *sd,
if (remote != sd)
continue;
- rvin_notify_video_device(vin, notification, arg);
+ switch (notification) {
+ case V4L2_DEVICE_NOTIFY_EVENT:
+ v4l2_event_queue(&vin->vdev, arg);
+ break;
+ default:
+ break;
+ }
}
}
@@ -1162,13 +715,8 @@ int rvin_v4l2_register(struct rvin_dev *vin)
vin->format.field = RVIN_DEFAULT_FIELD;
vin->format.colorspace = RVIN_DEFAULT_COLORSPACE;
- if (vin->info->use_mc) {
- vdev->device_caps |= V4L2_CAP_IO_MC;
- vdev->ioctl_ops = &rvin_mc_ioctl_ops;
- } else {
- vdev->ioctl_ops = &rvin_ioctl_ops;
- rvin_reset_format(vin);
- }
+ vdev->device_caps |= V4L2_CAP_IO_MC;
+ vdev->ioctl_ops = &rvin_mc_ioctl_ops;
rvin_format_align(vin, &vin->format);
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
index a577f4fe4a6c..a9bb530174aa 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
@@ -2,12 +2,11 @@
/*
* Driver for Renesas R-Car VIN
*
+ * Copyright (C) 2025 Niklas Söderlund <niklas.soderlund@ragnatech.se>
* Copyright (C) 2016 Renesas Electronics Corp.
* Copyright (C) 2011-2013 Renesas Solutions Corp.
* Copyright (C) 2013 Cogent Embedded, Inc., <source@cogentembedded.com>
* Copyright (C) 2008 Magnus Damm
- *
- * Based on the soc-camera rcar_vin driver
*/
#ifndef __RCAR_VIN__
@@ -79,8 +78,6 @@ struct rvin_video_format {
* @mbus_type: media bus type
* @bus: media bus parallel configuration
* @source_pad: source pad of remote subdevice
- * @sink_pad: sink pad of remote subdevice
- *
*/
struct rvin_parallel_entity {
struct v4l2_async_connection *asc;
@@ -90,7 +87,6 @@ struct rvin_parallel_entity {
struct v4l2_mbus_config_parallel bus;
unsigned int source_pad;
- unsigned int sink_pad;
};
/**
@@ -117,7 +113,6 @@ struct rvin_group_route {
/**
* struct rvin_info - Information about the particular VIN implementation
* @model: VIN model
- * @use_mc: use media controller instead of controlling subdevice
* @use_isp: the VIN is connected to the ISP and not to the CSI-2
* @nv12: support outputting NV12 pixel format
* @raw10: support outputting RAW10 pixel format
@@ -129,7 +124,6 @@ struct rvin_group_route {
*/
struct rvin_info {
enum model_id model;
- bool use_mc;
bool use_isp;
bool nv12;
bool raw10;
@@ -176,7 +170,6 @@ struct rvin_info {
* @crop: active cropping
* @compose: active composing
* @scaler: Optional scaler
- * @std: active video standard of the video source
*
* @alpha: Alpha component to fill in for supported pixel formats
*/
@@ -218,7 +211,6 @@ struct rvin_dev {
struct v4l2_rect crop;
struct v4l2_rect compose;
void (*scaler)(struct rvin_dev *vin);
- v4l2_std_id std;
unsigned int alpha;
};
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers
2025-05-21 13:20 ` [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers Niklas Söderlund
@ 2025-05-27 7:01 ` Sakari Ailus
2025-05-27 11:06 ` Laurent Pinchart
2025-06-06 13:50 ` Niklas Söderlund
2025-05-27 11:32 ` Laurent Pinchart
1 sibling, 2 replies; 22+ messages in thread
From: Sakari Ailus @ 2025-05-27 7:01 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Mauro Carvalho Chehab, Tomi Valkeinen, Laurent Pinchart,
linux-media, linux-renesas-soc
Hej Niklas,
On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> The R-Car VIN driver is needless complex and uses more then one
s/needless\K/ly/
> v4l-async notifier to attach to all its subdevices. Prepare for unifying
> them by moving rvin_parallel_parse_of() to where it needs to be when
> they are unified.
>
> The function is moved verbatim and there is no change in behavior.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
> 1 file changed, 53 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index d9ad56fb2aa9..60ec57d73a12 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> }
> }
>
> +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> +{
> + struct fwnode_handle *ep, *fwnode;
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_UNKNOWN,
> + };
> + struct v4l2_async_connection *asc;
> + int ret;
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> + if (!ep)
> + return 0;
> +
> + fwnode = fwnode_graph_get_remote_endpoint(ep);
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + fwnode_handle_put(ep);
> + if (ret) {
> + vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + switch (vep.bus_type) {
> + case V4L2_MBUS_PARALLEL:
> + case V4L2_MBUS_BT656:
> + vin_dbg(vin, "Found %s media bus\n",
> + vep.bus_type == V4L2_MBUS_PARALLEL ?
> + "PARALLEL" : "BT656");
> + vin->parallel.mbus_type = vep.bus_type;
> + vin->parallel.bus = vep.bus.parallel;
> + break;
> + default:
> + vin_err(vin, "Unknown media bus type\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> + struct v4l2_async_connection);
If you use v4l2_async_nf_add_fwnode_remote() here, you can omit
fwnode_graph_get_remote_endpoint() call above. Also the error handling
becomes more simple.
> + if (IS_ERR(asc)) {
> + ret = PTR_ERR(asc);
> + goto out;
> + }
> +
> + vin->parallel.asc = asc;
> +
> + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> +out:
> + fwnode_handle_put(fwnode);
> +
> + return ret;
> +}
> +
> static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> unsigned int max_id)
> {
> @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> .complete = rvin_parallel_notify_complete,
> };
>
> -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> -{
> - struct fwnode_handle *ep, *fwnode;
> - struct v4l2_fwnode_endpoint vep = {
> - .bus_type = V4L2_MBUS_UNKNOWN,
> - };
> - struct v4l2_async_connection *asc;
> - int ret;
> -
> - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> - if (!ep)
> - return 0;
> -
> - fwnode = fwnode_graph_get_remote_endpoint(ep);
> - ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> - fwnode_handle_put(ep);
> - if (ret) {
> - vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - switch (vep.bus_type) {
> - case V4L2_MBUS_PARALLEL:
> - case V4L2_MBUS_BT656:
> - vin_dbg(vin, "Found %s media bus\n",
> - vep.bus_type == V4L2_MBUS_PARALLEL ?
> - "PARALLEL" : "BT656");
> - vin->parallel.mbus_type = vep.bus_type;
> - vin->parallel.bus = vep.bus.parallel;
> - break;
> - default:
> - vin_err(vin, "Unknown media bus type\n");
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> - struct v4l2_async_connection);
> - if (IS_ERR(asc)) {
> - ret = PTR_ERR(asc);
> - goto out;
> - }
> -
> - vin->parallel.asc = asc;
> -
> - vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> -out:
> - fwnode_handle_put(fwnode);
> -
> - return ret;
> -}
> -
> static void rvin_parallel_cleanup(struct rvin_dev *vin)
> {
> v4l2_async_nf_unregister(&vin->notifier);
> --
> 2.49.0
>
--
Sakari Ailus
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers
2025-05-27 7:01 ` Sakari Ailus
@ 2025-05-27 11:06 ` Laurent Pinchart
2025-05-28 10:27 ` Sakari Ailus
2025-06-06 13:50 ` Niklas Söderlund
1 sibling, 1 reply; 22+ messages in thread
From: Laurent Pinchart @ 2025-05-27 11:06 UTC (permalink / raw)
To: Sakari Ailus
Cc: Niklas Söderlund, Mauro Carvalho Chehab, Tomi Valkeinen,
linux-media, linux-renesas-soc
On Tue, May 27, 2025 at 07:01:47AM +0000, Sakari Ailus wrote:
> On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> > The R-Car VIN driver is needless complex and uses more then one
>
> s/needless\K/ly/
>
> > v4l-async notifier to attach to all its subdevices. Prepare for unifying
> > them by moving rvin_parallel_parse_of() to where it needs to be when
> > they are unified.
> >
> > The function is moved verbatim and there is no change in behavior.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > .../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
> > 1 file changed, 53 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > index d9ad56fb2aa9..60ec57d73a12 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> > }
> > }
> >
> > +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > +{
> > + struct fwnode_handle *ep, *fwnode;
> > + struct v4l2_fwnode_endpoint vep = {
> > + .bus_type = V4L2_MBUS_UNKNOWN,
> > + };
> > + struct v4l2_async_connection *asc;
> > + int ret;
> > +
> > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > + if (!ep)
> > + return 0;
> > +
> > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > + fwnode_handle_put(ep);
> > + if (ret) {
> > + vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
I just noticed that this error message isn't correct. The endpoint
before parsed is ep, not fwnode, so you should write
vin_err(vin, "Failed to parse %pOF\n", to_of_node(ep));
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + switch (vep.bus_type) {
> > + case V4L2_MBUS_PARALLEL:
> > + case V4L2_MBUS_BT656:
> > + vin_dbg(vin, "Found %s media bus\n",
> > + vep.bus_type == V4L2_MBUS_PARALLEL ?
> > + "PARALLEL" : "BT656");
> > + vin->parallel.mbus_type = vep.bus_type;
> > + vin->parallel.bus = vep.bus.parallel;
> > + break;
> > + default:
> > + vin_err(vin, "Unknown media bus type\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > + struct v4l2_async_connection);
>
> If you use v4l2_async_nf_add_fwnode_remote() here, you can omit
> fwnode_graph_get_remote_endpoint() call above. Also the error handling
> becomes more simple.
That would contradict the commit message that indicates the function is
moved without being modified. I'd rather keep the patch as-is, and then
improve the function in a separate patch.
Regarding improvements, declaring ep and fwnode as
struct fwnode_handle __free(fwnode_handle) *ep = NULL;
struct fwnode_handle __free(fwnode_handle) *fwnode = NULL;
would also simplify error handling.
> > + if (IS_ERR(asc)) {
> > + ret = PTR_ERR(asc);
> > + goto out;
> > + }
> > +
> > + vin->parallel.asc = asc;
> > +
> > + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > +out:
> > + fwnode_handle_put(fwnode);
> > +
> > + return ret;
> > +}
> > +
> > static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > unsigned int max_id)
> > {
> > @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> > .complete = rvin_parallel_notify_complete,
> > };
> >
> > -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > -{
> > - struct fwnode_handle *ep, *fwnode;
> > - struct v4l2_fwnode_endpoint vep = {
> > - .bus_type = V4L2_MBUS_UNKNOWN,
> > - };
> > - struct v4l2_async_connection *asc;
> > - int ret;
> > -
> > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > - if (!ep)
> > - return 0;
> > -
> > - fwnode = fwnode_graph_get_remote_endpoint(ep);
> > - ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > - fwnode_handle_put(ep);
> > - if (ret) {
> > - vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > - switch (vep.bus_type) {
> > - case V4L2_MBUS_PARALLEL:
> > - case V4L2_MBUS_BT656:
> > - vin_dbg(vin, "Found %s media bus\n",
> > - vep.bus_type == V4L2_MBUS_PARALLEL ?
> > - "PARALLEL" : "BT656");
> > - vin->parallel.mbus_type = vep.bus_type;
> > - vin->parallel.bus = vep.bus.parallel;
> > - break;
> > - default:
> > - vin_err(vin, "Unknown media bus type\n");
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > - struct v4l2_async_connection);
> > - if (IS_ERR(asc)) {
> > - ret = PTR_ERR(asc);
> > - goto out;
> > - }
> > -
> > - vin->parallel.asc = asc;
> > -
> > - vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > -out:
> > - fwnode_handle_put(fwnode);
> > -
> > - return ret;
> > -}
> > -
> > static void rvin_parallel_cleanup(struct rvin_dev *vin)
> > {
> > v4l2_async_nf_unregister(&vin->notifier);
> > --
> > 2.49.0
> >
>
> --
> Sakari Ailus
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/6] media: rcar-vin: Use correct count of remote subdevices
2025-05-21 13:20 ` [PATCH v4 1/6] media: rcar-vin: Use correct count of remote subdevices Niklas Söderlund
@ 2025-05-27 11:11 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2025-05-27 11:11 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen, linux-media,
linux-renesas-soc
Hi Niklas,
Thank you for the patch.
On Wed, May 21, 2025 at 03:20:32PM +0200, Niklas Söderlund wrote:
> When extending the driver with Gen4 support the iteration of over
> possible remote subdevices changed from being R-Car CSI-2 Rx only to
> also cover R-Car CSISP instances. In two loops updating the bounds
> variable was missed.
>
> This had no ill effect as the count the two values have always been the
> same in the past. Fix it before the two diverge.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> drivers/media/platform/renesas/rcar-vin/rcar-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index 846ae7989b1d..b9ea5b8db559 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -262,7 +262,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>
> mutex_lock(&vin->group->lock);
>
> - for (i = 0; i < RVIN_CSI_MAX; i++) {
> + for (i = 0; i < RVIN_REMOTES_MAX; i++) {
Using ARRAY_SIZE would be even better I think.
for (i = 0; i < ARRAY_SIZE(vin->group->remotes); i++) {
Same below.
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> if (vin->group->remotes[i].asc != asc)
> continue;
> vin->group->remotes[i].subdev = NULL;
> @@ -284,7 +284,7 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>
> mutex_lock(&vin->group->lock);
>
> - for (i = 0; i < RVIN_CSI_MAX; i++) {
> + for (i = 0; i < RVIN_REMOTES_MAX; i++) {
> if (vin->group->remotes[i].asc != asc)
> continue;
> vin->group->remotes[i].subdev = subdev;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/6] media: rcar-vin: Change link setup argument
2025-05-21 13:20 ` [PATCH v4 2/6] media: rcar-vin: Change link setup argument Niklas Söderlund
@ 2025-05-27 11:19 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2025-05-27 11:19 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen, linux-media,
linux-renesas-soc
Hi Niklas,
Thank you for the patch.
On Wed, May 21, 2025 at 03:20:33PM +0200, Niklas Söderlund wrote:
> The link setup callback once acted on each VIN instance, and expected to
> be called once for each VIN instance. This have changed as the driver
> grew support for later hardware generations and the callback is now
> expected to setup links for all VIN in the group.
>
> The argument to the callback have however remained a pointer to a single
> VIN instance. This pointer was then used to get the group structure. Fix
> this and pass the group as the single argument to the link setup
> callback making the expectation of the function clear.
>
> There is no intentional change in behavior.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 52 ++++++++++++-------
> .../platform/renesas/rcar-vin/rcar-vin.h | 2 +-
> 2 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index b9ea5b8db559..1be408d6c508 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -65,7 +65,7 @@ static void rvin_group_cleanup(struct rvin_group *group)
> }
>
> static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
> - int (*link_setup)(struct rvin_dev *),
> + int (*link_setup)(struct rvin_group *),
> const struct media_device_ops *ops)
> {
> struct media_device *mdev = &group->mdev;
> @@ -115,7 +115,7 @@ static void rvin_group_release(struct kref *kref)
> }
>
> static int rvin_group_get(struct rvin_dev *vin,
> - int (*link_setup)(struct rvin_dev *),
> + int (*link_setup)(struct rvin_group *),
> const struct media_device_ops *ops)
> {
> struct rvin_group *group;
> @@ -246,7 +246,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> }
> }
>
> - return vin->group->link_setup(vin);
> + return vin->group->link_setup(vin->group);
> }
>
> static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -909,35 +909,46 @@ static int rvin_csi2_create_link(struct rvin_group *group, unsigned int id,
> return 0;
> }
>
> -static int rvin_csi2_setup_links(struct rvin_dev *vin)
> +static int rvin_csi2_setup_links(struct rvin_group *group)
> {
> - const struct rvin_group_route *route;
> + const struct rvin_group_route *routes, *route;
const struct rvin_group_route *routes = NULL;
const struct rvin_group_route *route;
> unsigned int id;
> int ret = -EINVAL;
>
> + /* Find any VIN in group to get route info. */
> + routes = NULL;
> + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
Use ARRAY_SIZE()
> + if (group->vin[i]) {
> + routes = group->vin[i]->info->routes;
> + break;
> + }
> + }
> + if (!routes)
> + return -ENODEV;
I wonder if the info pointer should also be stored in the rvin_group
structure.
> +
> /* Create all media device links between VINs and CSI-2's. */
> - mutex_lock(&vin->group->lock);
> - for (route = vin->info->routes; route->chsel; route++) {
> + mutex_lock(&group->lock);
> + for (route = routes; route->chsel; route++) {
> /* Check that VIN' master is part of the group. */
> - if (!vin->group->vin[route->master])
> + if (!group->vin[route->master])
> continue;
>
> /* Check that CSI-2 is part of the group. */
> - if (!vin->group->remotes[route->csi].subdev)
> + if (!group->remotes[route->csi].subdev)
> continue;
>
> for (id = route->master; id < route->master + 4; id++) {
> /* Check that VIN is part of the group. */
> - if (!vin->group->vin[id])
> + if (!group->vin[id])
> continue;
>
> - ret = rvin_csi2_create_link(vin->group, id, route);
> + ret = rvin_csi2_create_link(group, id, route);
> if (ret)
> goto out;
> }
> }
> out:
> - mutex_unlock(&vin->group->lock);
> + mutex_unlock(&group->lock);
>
> return ret;
> }
> @@ -991,30 +1002,33 @@ static int rvin_csi2_init(struct rvin_dev *vin)
> * ISP
> */
>
> -static int rvin_isp_setup_links(struct rvin_dev *vin)
> +static int rvin_isp_setup_links(struct rvin_group *group)
> {
> unsigned int i;
> int ret = -EINVAL;
>
> /* Create all media device links between VINs and ISP's. */
> - mutex_lock(&vin->group->lock);
> + mutex_lock(&group->lock);
> for (i = 0; i < RCAR_VIN_NUM; i++) {
> struct media_pad *source_pad, *sink_pad;
> struct media_entity *source, *sink;
> unsigned int source_slot = i / 8;
> unsigned int source_idx = i % 8 + 1;
> + struct rvin_dev *vin;
struct rvin_dev *vin = group->vin[i];
>
> - if (!vin->group->vin[i])
> + vin = group->vin[i];
> +
> + if (!vin)
> continue;
>
> /* Check that ISP is part of the group. */
> - if (!vin->group->remotes[source_slot].subdev)
> + if (!group->remotes[source_slot].subdev)
> continue;
>
> - source = &vin->group->remotes[source_slot].subdev->entity;
> + source = &group->remotes[source_slot].subdev->entity;
> source_pad = &source->pads[source_idx];
>
> - sink = &vin->group->vin[i]->vdev.entity;
> + sink = &vin->vdev.entity;
> sink_pad = &sink->pads[0];
>
> /* Skip if link already exists. */
> @@ -1030,7 +1044,7 @@ static int rvin_isp_setup_links(struct rvin_dev *vin)
> break;
> }
> }
> - mutex_unlock(&vin->group->lock);
> + mutex_unlock(&group->lock);
>
> return ret;
> }
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> index 83d1b2734c41..7d4fce248976 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> @@ -257,7 +257,7 @@ struct rvin_group {
> struct v4l2_async_notifier notifier;
> struct rvin_dev *vin[RCAR_VIN_NUM];
>
> - int (*link_setup)(struct rvin_dev *vin);
> + int (*link_setup)(struct rvin_group *group);
>
> struct {
> struct v4l2_async_connection *asc;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/6] media: rcar-vin: Generate a VIN group ID for Gen2
2025-05-21 13:20 ` [PATCH v4 3/6] media: rcar-vin: Generate a VIN group ID for Gen2 Niklas Söderlund
@ 2025-05-27 11:31 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2025-05-27 11:31 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen, linux-media,
linux-renesas-soc
Hi Niklas,
Thank you for the patch.
On Wed, May 21, 2025 at 03:20:34PM +0200, Niklas Söderlund wrote:
> Prepare to move Gen2 and earlier models to media controller by
> generating a unique VIN group id for each VIN instance. On Gen3 and Gen4
> it is important to have a specific id in the group as media graph routes
> depend on this. On Gen2 and earlier models all that will matter is to
> have a unique id in the range.
>
> Break out the id generation to a own function keeping the logic for Gen3
> and Gen4 while generating a sequential id for Gen2 models.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Move ID allocation to probe.
> - Use ida_alloc_range() instead of implementing our own schema by
> counting DT nodes.
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 78 ++++++++++++++-----
> 1 file changed, 59 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index 1be408d6c508..d9ad56fb2aa9 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -10,6 +10,7 @@
> * Based on the soc-camera rcar_vin driver
> */
>
> +#include <linux/idr.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_graph.h>
> @@ -55,6 +56,7 @@
> * be only one group for all instances.
> */
>
> +static DEFINE_IDA(rvin_ida);
> static DEFINE_MUTEX(rvin_group_lock);
> static struct rvin_group *rvin_group_data;
>
> @@ -119,23 +121,8 @@ static int rvin_group_get(struct rvin_dev *vin,
> const struct media_device_ops *ops)
> {
> struct rvin_group *group;
> - u32 id;
> int ret;
>
> - /* Make sure VIN id is present and sane */
> - ret = of_property_read_u32(vin->dev->of_node, "renesas,id", &id);
> - if (ret) {
> - vin_err(vin, "%pOF: No renesas,id property found\n",
> - vin->dev->of_node);
> - return -EINVAL;
> - }
> -
> - if (id >= RCAR_VIN_NUM) {
> - vin_err(vin, "%pOF: Invalid renesas,id '%u'\n",
> - vin->dev->of_node, id);
> - return -EINVAL;
> - }
> -
> /* Join or create a VIN group */
> mutex_lock(&rvin_group_lock);
> if (rvin_group_data) {
> @@ -164,16 +151,15 @@ static int rvin_group_get(struct rvin_dev *vin,
> /* Add VIN to group */
> mutex_lock(&group->lock);
>
> - if (group->vin[id]) {
> - vin_err(vin, "Duplicate renesas,id property value %u\n", id);
> + if (group->vin[vin->id]) {
> + vin_err(vin, "Duplicate renesas,id property value %u\n", vin->id);
> mutex_unlock(&group->lock);
> kref_put(&group->refcount, rvin_group_release);
> return -EINVAL;
> }
>
> - group->vin[id] = vin;
> + group->vin[vin->id] = vin;
>
> - vin->id = id;
> vin->group = group;
> vin->v4l2_dev.mdev = &group->mdev;
>
> @@ -1375,6 +1361,54 @@ static const struct of_device_id rvin_of_id_table[] = {
> };
> MODULE_DEVICE_TABLE(of, rvin_of_id_table);
>
> +static int rvin_id_get(struct rvin_dev *vin)
> +{
> + u32 oid;
> + int id;
> +
> + switch (vin->info->model) {
> + case RCAR_GEN3:
> + case RCAR_GEN4:
> + if (of_property_read_u32(vin->dev->of_node, "renesas,id", &oid))
I would keep the original error message here:
vin_err(vin, "%pOF: No renesas,id property found\n",
vin->dev->of_node);
> + break;
And you can return -EINVAL directly.
> +
> + if (oid < 0 || oid >= RCAR_VIN_NUM) {
> + vin_err(vin, "%pOF: Invalid renesas,id '%u'\n",
> + vin->dev->of_node, oid);
> + return -EINVAL;
> + }
> +
> + vin->id = oid;
> +
> + return 0;
> + default:
> + id = ida_alloc_range(&rvin_ida, 0, RCAR_VIN_NUM - 1,
> + GFP_KERNEL);
> + if (id < 0)
> + break;
Same here, I'd add a specific error message and return:
vin_err(vin, "Can't allocate group ID\n");
return -EINVAL;
> +
> + vin->id = id;
> +
> + return 0;
> + }
> +
> + vin_err(vin, "Can't figure out VIN id\n");
> +
> + return -EINVAL;
And you can drop this.
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> +}
> +
> +static void rvin_id_put(struct rvin_dev *vin)
> +{
> + switch (vin->info->model) {
> + case RCAR_GEN3:
> + case RCAR_GEN4:
> + break;
> + default:
> + ida_free(&rvin_ida, vin->id);
> + break;
> + }
> +}
> +
> static int rcar_vin_probe(struct platform_device *pdev)
> {
> struct rvin_dev *vin;
> @@ -1402,6 +1436,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, vin);
>
> + if (rvin_id_get(vin))
> + return -EINVAL;
> +
> if (vin->info->use_isp) {
> ret = rvin_isp_init(vin);
> } else if (vin->info->use_mc) {
> @@ -1419,6 +1456,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
>
> if (ret) {
> rvin_dma_unregister(vin);
> + rvin_id_put(vin);
> return ret;
> }
>
> @@ -1443,6 +1481,8 @@ static void rcar_vin_remove(struct platform_device *pdev)
> else
> rvin_parallel_cleanup(vin);
>
> + rvin_id_put(vin);
> +
> rvin_dma_unregister(vin);
> }
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers
2025-05-21 13:20 ` [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers Niklas Söderlund
2025-05-27 7:01 ` Sakari Ailus
@ 2025-05-27 11:32 ` Laurent Pinchart
1 sibling, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2025-05-27 11:32 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen, linux-media,
linux-renesas-soc
Hi Niklas,
On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> The R-Car VIN driver is needless complex and uses more then one
> v4l-async notifier to attach to all its subdevices. Prepare for unifying
> them by moving rvin_parallel_parse_of() to where it needs to be when
> they are unified.
>
> The function is moved verbatim and there is no change in behavior.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
> 1 file changed, 53 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index d9ad56fb2aa9..60ec57d73a12 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> }
> }
>
> +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> +{
> + struct fwnode_handle *ep, *fwnode;
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_UNKNOWN,
> + };
> + struct v4l2_async_connection *asc;
> + int ret;
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> + if (!ep)
> + return 0;
> +
> + fwnode = fwnode_graph_get_remote_endpoint(ep);
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + fwnode_handle_put(ep);
> + if (ret) {
> + vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + switch (vep.bus_type) {
> + case V4L2_MBUS_PARALLEL:
> + case V4L2_MBUS_BT656:
> + vin_dbg(vin, "Found %s media bus\n",
> + vep.bus_type == V4L2_MBUS_PARALLEL ?
> + "PARALLEL" : "BT656");
> + vin->parallel.mbus_type = vep.bus_type;
> + vin->parallel.bus = vep.bus.parallel;
> + break;
> + default:
> + vin_err(vin, "Unknown media bus type\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> + struct v4l2_async_connection);
> + if (IS_ERR(asc)) {
> + ret = PTR_ERR(asc);
> + goto out;
> + }
> +
> + vin->parallel.asc = asc;
> +
> + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> +out:
> + fwnode_handle_put(fwnode);
> +
> + return ret;
> +}
> +
> static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> unsigned int max_id)
> {
> @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> .complete = rvin_parallel_notify_complete,
> };
>
> -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> -{
> - struct fwnode_handle *ep, *fwnode;
> - struct v4l2_fwnode_endpoint vep = {
> - .bus_type = V4L2_MBUS_UNKNOWN,
> - };
> - struct v4l2_async_connection *asc;
> - int ret;
> -
> - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> - if (!ep)
> - return 0;
> -
> - fwnode = fwnode_graph_get_remote_endpoint(ep);
> - ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> - fwnode_handle_put(ep);
> - if (ret) {
> - vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - switch (vep.bus_type) {
> - case V4L2_MBUS_PARALLEL:
> - case V4L2_MBUS_BT656:
> - vin_dbg(vin, "Found %s media bus\n",
> - vep.bus_type == V4L2_MBUS_PARALLEL ?
> - "PARALLEL" : "BT656");
> - vin->parallel.mbus_type = vep.bus_type;
> - vin->parallel.bus = vep.bus.parallel;
> - break;
> - default:
> - vin_err(vin, "Unknown media bus type\n");
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> - struct v4l2_async_connection);
> - if (IS_ERR(asc)) {
> - ret = PTR_ERR(asc);
> - goto out;
> - }
> -
> - vin->parallel.asc = asc;
> -
> - vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> -out:
> - fwnode_handle_put(fwnode);
> -
> - return ret;
> -}
> -
> static void rvin_parallel_cleanup(struct rvin_dev *vin)
> {
> v4l2_async_nf_unregister(&vin->notifier);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 5/6] media: rcar-vin: Merge all notifiers
2025-05-21 13:20 ` [PATCH v4 5/6] media: rcar-vin: Merge all notifiers Niklas Söderlund
@ 2025-05-27 11:45 ` Laurent Pinchart
2025-06-06 14:09 ` Niklas Söderlund
0 siblings, 1 reply; 22+ messages in thread
From: Laurent Pinchart @ 2025-05-27 11:45 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen, linux-media,
linux-renesas-soc
Hi Niklas,
Thank you for the patch.
On Wed, May 21, 2025 at 03:20:36PM +0200, Niklas Söderlund wrote:
> The VIN usage of v4l-async is complex and stems from organic growth of
> the driver of supporting both private local subdevices (Gen2, Gen3) and
> subdevices shared between all VIN instances (Gen3 and Gen4).
>
> The driver used a separate notifier for each VIN for the private local
> ones, and a shared group notifier for the shared ones. This was complex
> and lead to subtle bugs when unbinding and later rebinding subdevices in
> on of the notifiers having to handle different edge cases depending on
s/on of/one of/ (I think)
> if it also had subdevices in the other notifiers etc.
>
> To simplify this have the Gen2 devices allocate and form a VIN group
> too. This way all subdevices on all models can be collect in a
> single group notifier. Then there is only a single complete callback for
> all where the video devices and subdevice nodes can be registered etc.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 263 ++++++++----------
> .../platform/renesas/rcar-vin/rcar-vin.h | 2 -
> 2 files changed, 114 insertions(+), 151 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index 60ec57d73a12..b0727e98dac6 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -43,6 +43,9 @@
>
> #define v4l2_dev_to_vin(d) container_of(d, struct rvin_dev, v4l2_dev)
>
> +static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> + struct v4l2_subdev *subdev);
Any chance you could move the function instead of forward-declaring it ?
> +
> /* -----------------------------------------------------------------------------
> * Gen3 Group Allocator
> */
> @@ -232,7 +235,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> }
> }
>
> - return vin->group->link_setup(vin->group);
> + if (vin->group->link_setup)
> + return vin->group->link_setup(vin->group);
> +
> + return 0;
> }
>
> static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -240,22 +246,31 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> struct v4l2_async_connection *asc)
> {
> struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> - unsigned int i;
> + struct rvin_group *group = vin->group;
>
> - for (i = 0; i < RCAR_VIN_NUM; i++)
> - if (vin->group->vin[i])
> - rvin_v4l2_unregister(vin->group->vin[i]);
> + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++)
While at it, you can use ARRAY_SIZE().
> + if (group->vin[i])
> + rvin_v4l2_unregister(group->vin[i]);
And please use curly braces for the for statement.
>
> mutex_lock(&vin->group->lock);
Add a blank line.
> + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
ARRAY_SIZE() here too. Same below where applicable.
> + if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
> + continue;
> +
> + group->vin[i]->parallel.subdev = NULL;
> +
> + vin_dbg(group->vin[i], "Unbind parallel subdev %s\n",
> + subdev->name);
> + }
>
> - for (i = 0; i < RVIN_REMOTES_MAX; i++) {
> - if (vin->group->remotes[i].asc != asc)
> + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> + if (group->remotes[i].asc != asc)
> continue;
> - vin->group->remotes[i].subdev = NULL;
> +
> + group->remotes[i].subdev = NULL;
> +
> vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
> - break;
> }
> -
You can keep this blank line :-)
> mutex_unlock(&vin->group->lock);
>
> media_device_unregister(&vin->group->mdev);
> @@ -266,21 +281,38 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> struct v4l2_async_connection *asc)
> {
> struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> - unsigned int i;
> + struct rvin_group *group = vin->group;
>
> - mutex_lock(&vin->group->lock);
> + guard(mutex)(&group->lock);
>
> - for (i = 0; i < RVIN_REMOTES_MAX; i++) {
> + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> + int ret;
> +
> + if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
> + continue;
> +
> + ret = rvin_parallel_subdevice_attach(group->vin[i], subdev);
> + if (ret)
> + return ret;
> +
> + v4l2_set_subdev_hostdata(subdev, group->vin[i]);
> +
> + vin_dbg(group->vin[i], "Bound subdev %s\n", subdev->name);
> +
> + return 0;
> + }
> +
> + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> if (vin->group->remotes[i].asc != asc)
> continue;
> +
> vin->group->remotes[i].subdev = subdev;
> vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
> - break;
> +
> + return 0;
> }
>
> - mutex_unlock(&vin->group->lock);
> -
> - return 0;
> + return -ENODEV;
> }
>
> static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
> @@ -374,7 +406,7 @@ static int rvin_parallel_parse_of(struct rvin_dev *vin)
> goto out;
> }
>
> - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> + asc = v4l2_async_nf_add_fwnode(&vin->group->notifier, fwnode,
> struct v4l2_async_connection);
> if (IS_ERR(asc)) {
> ret = PTR_ERR(asc);
> @@ -424,6 +456,12 @@ static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> if (!(vin_mask & BIT(i)))
> continue;
>
> + /* Parse local subdevice. */
> + ret = rvin_parallel_parse_of(vin->group->vin[i]);
> + if (ret)
> + return ret;
> +
> + /* Prase shared subdevices. */
> for (id = 0; id < max_id; id++) {
> if (vin->group->remotes[id].asc)
> continue;
> @@ -603,124 +641,6 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> return 0;
> }
>
> -static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> -{
> - rvin_v4l2_unregister(vin);
> - vin->parallel.subdev = NULL;
> -
> - if (!vin->info->use_mc)
> - rvin_free_controls(vin);
> -}
> -
> -static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> -{
> - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> - struct media_entity *source;
> - struct media_entity *sink;
> - int ret;
> -
> - ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> - if (ret < 0) {
> - vin_err(vin, "Failed to register subdev nodes\n");
> - return ret;
> - }
> -
> - 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,
> - struct v4l2_subdev *subdev,
> - struct v4l2_async_connection *asc)
> -{
> - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> -
> - vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
> -
> - mutex_lock(&vin->lock);
> - rvin_parallel_subdevice_detach(vin);
> - mutex_unlock(&vin->lock);
> -}
> -
> -static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> - struct v4l2_subdev *subdev,
> - struct v4l2_async_connection *asc)
> -{
> - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> - int ret;
> -
> - mutex_lock(&vin->lock);
> - ret = rvin_parallel_subdevice_attach(vin, subdev);
> - mutex_unlock(&vin->lock);
> - if (ret)
> - return ret;
> -
> - v4l2_set_subdev_hostdata(subdev, vin);
> -
> - vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
> - subdev->name, vin->parallel.source_pad,
> - vin->parallel.sink_pad);
> -
> - return 0;
> -}
> -
> -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 void rvin_parallel_cleanup(struct rvin_dev *vin)
> -{
> - v4l2_async_nf_unregister(&vin->notifier);
> - v4l2_async_nf_cleanup(&vin->notifier);
> -}
> -
> -static int rvin_parallel_init(struct rvin_dev *vin)
> -{
> - int ret;
> -
> - v4l2_async_nf_init(&vin->notifier, &vin->v4l2_dev);
> -
> - ret = rvin_parallel_parse_of(vin);
> - if (ret)
> - return ret;
> -
> - if (!vin->parallel.asc)
> - return -ENODEV;
> -
> - vin_dbg(vin, "Found parallel subdevice %pOF\n",
> - to_of_node(vin->parallel.asc->match.fwnode));
> -
> - vin->notifier.ops = &rvin_parallel_notify_ops;
> - ret = v4l2_async_nf_register(&vin->notifier);
> - if (ret < 0) {
> - vin_err(vin, "Notifier registration failed\n");
> - v4l2_async_nf_cleanup(&vin->notifier);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> /* -----------------------------------------------------------------------------
> * CSI-2
> */
> @@ -895,11 +815,63 @@ static int rvin_csi2_create_link(struct rvin_group *group, unsigned int id,
> return 0;
> }
>
> +static int rvin_parallel_setup_links(struct rvin_group *group)
> +{
> + u32 flags = MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE;
> + int ret = 0;
> +
> + mutex_lock(&group->lock);
Use a guard.
> + /* If the group also have links don't enable the link. */
s/have/has/
> + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> + if (group->remotes[i].subdev) {
> + flags = 0;
> + break;
> + }
> + }
> +
> + /* Create links */
s/links/links./
> + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> + struct rvin_dev *vin = group->vin[i];
> + struct media_entity *source;
> + struct media_entity *sink;
> +
> + /* Noting to do if their is no VIN or parallel subdev. */
s/Noting/Nothing/
s/their/there/
> + if (!vin || !vin->parallel.subdev)
> + continue;
> +
> + source = &vin->parallel.subdev->entity;
> + sink = &vin->vdev.entity;
> +
> + ret = media_create_pad_link(source, vin->parallel.source_pad,
> + sink, 0, flags);
> + if (ret)
> + break;
return ret;
(thanks to the guard above)
> + }
> + mutex_unlock(&group->lock);
> +
> + return ret;
return 0;
> +}
> +
> static int rvin_csi2_setup_links(struct rvin_group *group)
> {
> const struct rvin_group_route *routes, *route;
> unsigned int id;
> - int ret = -EINVAL;
> + int ret;
> +
> + /* Find any VIN in group to get route info. */
> + routes = NULL;
> + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> + if (group->vin[i]) {
> + routes = group->vin[i]->info->routes;
> + break;
> + }
> + }
> + if (!routes)
> + return -ENODEV;
Storing the info pointer in the group as proposed in the review of a
previous patch from the same series would help here too.
> +
> + ret = rvin_parallel_setup_links(group);
> + if (ret)
> + return ret;
>
> /* Find any VIN in group to get route info. */
> routes = NULL;
> @@ -914,6 +886,7 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
>
> /* Create all media device links between VINs and CSI-2's. */
> mutex_lock(&group->lock);
> + ret = -EINVAL;
> for (route = routes; route->chsel; route++) {
> /* Check that VIN' master is part of the group. */
> if (!group->vin[route->master])
> @@ -941,7 +914,6 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
>
> static void rvin_csi2_cleanup(struct rvin_dev *vin)
> {
> - rvin_parallel_cleanup(vin);
> rvin_group_notifier_cleanup(vin);
> rvin_group_put(vin);
> rvin_free_controls(vin);
> @@ -964,18 +936,11 @@ static int rvin_csi2_init(struct rvin_dev *vin)
> if (ret)
> goto err_controls;
>
> - /* It's OK to not have a parallel subdevice. */
> - ret = rvin_parallel_init(vin);
> - if (ret && ret != -ENODEV)
> - goto err_group;
> -
> ret = rvin_group_notifier_init(vin, 1, RVIN_CSI_MAX);
> if (ret)
> - goto err_parallel;
> + goto err_group;
>
> return 0;
> -err_parallel:
> - rvin_parallel_cleanup(vin);
> err_group:
> rvin_group_put(vin);
> err_controls:
> @@ -1448,7 +1413,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
> rvin_group_id_to_master(vin->id) == vin->id)
> vin->scaler = vin->info->scaler;
> } else {
> - ret = rvin_parallel_init(vin);
> + ret = rvin_group_get(vin, NULL, NULL);
> + if (!ret)
> + ret = rvin_group_notifier_init(vin, 0, 0);
>
> if (vin->info->scaler)
> vin->scaler = vin->info->scaler;
> @@ -1478,8 +1445,6 @@ static void rcar_vin_remove(struct platform_device *pdev)
> rvin_isp_cleanup(vin);
> else if (vin->info->use_mc)
> rvin_csi2_cleanup(vin);
> - else
> - rvin_parallel_cleanup(vin);
>
> rvin_id_put(vin);
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> index 7d4fce248976..a577f4fe4a6c 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> @@ -149,7 +149,6 @@ struct rvin_info {
> * @vdev: V4L2 video device associated with VIN
> * @v4l2_dev: V4L2 device
> * @ctrl_handler: V4L2 control handler
> - * @notifier: V4L2 asynchronous subdevs notifier
> *
> * @parallel: parallel input subdevice descriptor
> *
> @@ -189,7 +188,6 @@ struct rvin_dev {
> struct video_device vdev;
> struct v4l2_device v4l2_dev;
> struct v4l2_ctrl_handler ctrl_handler;
> - struct v4l2_async_notifier notifier;
>
> struct rvin_parallel_entity parallel;
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 6/6] media: rcar-vin: Enable media-graph on Gen2
2025-05-21 13:20 ` [PATCH v4 6/6] media: rcar-vin: Enable media-graph on Gen2 Niklas Söderlund
@ 2025-05-27 11:48 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2025-05-27 11:48 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen, linux-media,
linux-renesas-soc
Hi Niklas,
Thank you for the patch.
On Wed, May 21, 2025 at 03:20:37PM +0200, Niklas Söderlund wrote:
> Complete the conversion from soc_camera to a full fledge media
> controller enabled devices for all supported generations of the device.
> All work is already done as this is already supported on Gen3, and
> later.
>
> All that is missing is some small touch ups of the unified v4l-notifier,
> unconditionally creation of a media pad for each VIN and removing all
> special cases for the non media-graph call paths.
Would it be possible to split the first two items from the last ?
Removal of all special cases and dead code is large, and makes it
difficult to spot the other changes.
> Mark the completion of converting from soc_camera by injecting an
> attribution of myself in the header.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v3
> - Resolve conflicts with other VIN work merged a head of this series.
> ---
> .../platform/renesas/rcar-vin/rcar-core.c | 249 ++-------
> .../platform/renesas/rcar-vin/rcar-dma.c | 16 +-
> .../platform/renesas/rcar-vin/rcar-v4l2.c | 488 +-----------------
> .../platform/renesas/rcar-vin/rcar-vin.h | 10 +-
> 4 files changed, 66 insertions(+), 697 deletions(-)
[snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers
2025-05-27 11:06 ` Laurent Pinchart
@ 2025-05-28 10:27 ` Sakari Ailus
0 siblings, 0 replies; 22+ messages in thread
From: Sakari Ailus @ 2025-05-28 10:27 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Niklas Söderlund, Mauro Carvalho Chehab, Tomi Valkeinen,
linux-media, linux-renesas-soc
Hi Laurent,
On Tue, May 27, 2025 at 01:06:47PM +0200, Laurent Pinchart wrote:
> On Tue, May 27, 2025 at 07:01:47AM +0000, Sakari Ailus wrote:
> > On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> > > The R-Car VIN driver is needless complex and uses more then one
> >
> > s/needless\K/ly/
> >
> > > v4l-async notifier to attach to all its subdevices. Prepare for unifying
> > > them by moving rvin_parallel_parse_of() to where it needs to be when
> > > they are unified.
> > >
> > > The function is moved verbatim and there is no change in behavior.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > > .../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
> > > 1 file changed, 53 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > index d9ad56fb2aa9..60ec57d73a12 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> > > }
> > > }
> > >
> > > +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > +{
> > > + struct fwnode_handle *ep, *fwnode;
> > > + struct v4l2_fwnode_endpoint vep = {
> > > + .bus_type = V4L2_MBUS_UNKNOWN,
> > > + };
> > > + struct v4l2_async_connection *asc;
> > > + int ret;
> > > +
> > > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > + if (!ep)
> > > + return 0;
> > > +
> > > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > + fwnode_handle_put(ep);
> > > + if (ret) {
> > > + vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
>
> I just noticed that this error message isn't correct. The endpoint
> before parsed is ep, not fwnode, so you should write
>
> vin_err(vin, "Failed to parse %pOF\n", to_of_node(ep));
>
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + switch (vep.bus_type) {
> > > + case V4L2_MBUS_PARALLEL:
> > > + case V4L2_MBUS_BT656:
> > > + vin_dbg(vin, "Found %s media bus\n",
> > > + vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > + "PARALLEL" : "BT656");
> > > + vin->parallel.mbus_type = vep.bus_type;
> > > + vin->parallel.bus = vep.bus.parallel;
> > > + break;
> > > + default:
> > > + vin_err(vin, "Unknown media bus type\n");
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > + struct v4l2_async_connection);
> >
> > If you use v4l2_async_nf_add_fwnode_remote() here, you can omit
> > fwnode_graph_get_remote_endpoint() call above. Also the error handling
> > becomes more simple.
>
> That would contradict the commit message that indicates the function is
> moved without being modified. I'd rather keep the patch as-is, and then
> improve the function in a separate patch.
Sounds like a good idea.
>
> Regarding improvements, declaring ep and fwnode as
>
> struct fwnode_handle __free(fwnode_handle) *ep = NULL;
> struct fwnode_handle __free(fwnode_handle) *fwnode = NULL;
>
> would also simplify error handling.
>
> > > + if (IS_ERR(asc)) {
> > > + ret = PTR_ERR(asc);
> > > + goto out;
> > > + }
> > > +
> > > + vin->parallel.asc = asc;
> > > +
> > > + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > > +out:
> > > + fwnode_handle_put(fwnode);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > > unsigned int max_id)
> > > {
> > > @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> > > .complete = rvin_parallel_notify_complete,
> > > };
> > >
> > > -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > -{
> > > - struct fwnode_handle *ep, *fwnode;
> > > - struct v4l2_fwnode_endpoint vep = {
> > > - .bus_type = V4L2_MBUS_UNKNOWN,
> > > - };
> > > - struct v4l2_async_connection *asc;
> > > - int ret;
> > > -
> > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > - if (!ep)
> > > - return 0;
> > > -
> > > - fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > - ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > - fwnode_handle_put(ep);
> > > - if (ret) {
> > > - vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > -
> > > - switch (vep.bus_type) {
> > > - case V4L2_MBUS_PARALLEL:
> > > - case V4L2_MBUS_BT656:
> > > - vin_dbg(vin, "Found %s media bus\n",
> > > - vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > - "PARALLEL" : "BT656");
> > > - vin->parallel.mbus_type = vep.bus_type;
> > > - vin->parallel.bus = vep.bus.parallel;
> > > - break;
> > > - default:
> > > - vin_err(vin, "Unknown media bus type\n");
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > -
> > > - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > - struct v4l2_async_connection);
> > > - if (IS_ERR(asc)) {
> > > - ret = PTR_ERR(asc);
> > > - goto out;
> > > - }
> > > -
> > > - vin->parallel.asc = asc;
> > > -
> > > - vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > > -out:
> > > - fwnode_handle_put(fwnode);
> > > -
> > > - return ret;
> > > -}
> > > -
> > > static void rvin_parallel_cleanup(struct rvin_dev *vin)
> > > {
> > > v4l2_async_nf_unregister(&vin->notifier);
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2
2025-05-21 13:20 [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2 Niklas Söderlund
` (5 preceding siblings ...)
2025-05-21 13:20 ` [PATCH v4 6/6] media: rcar-vin: Enable media-graph on Gen2 Niklas Söderlund
@ 2025-05-30 12:41 ` Tomi Valkeinen
6 siblings, 0 replies; 22+ messages in thread
From: Tomi Valkeinen @ 2025-05-30 12:41 UTC (permalink / raw)
To: Niklas Söderlund, Sakari Ailus, Mauro Carvalho Chehab,
Laurent Pinchart, linux-media, linux-renesas-soc
Hi,
On 21/05/2025 16:20, Niklas Söderlund wrote:
> Hello,
>
> This series completes the conversion of the soc_camera VIN driver to a
> full fledge media-graph enabled driver for R-Car Gen2 devices, Gen3 and
> later have been media-graph centric from the start. Having a single
> driver supporting both MC and non-MC operation have lead do odd design
> decisions in the driver, and it have prevented improving the driver over
> all.
>
> New features and bug fixes have always been more important then fixing
> this old generation to be MC-centric. But in order to start to play with
> libcamera support for the R-Car pipeline it have become more pressing to
> make take the time to make this driver MC-only, and more importantly
> test it to make sure nothing really breaks.
>
> Patch 1/6 and 2/6 are drive-by fixes correcting issues in the existing
> design. Patch 3/6 prepares for Gen2 MC by making sure each VIN instance
> on Gen2 gets a unique ID which will be needed to support VIN groups.
> Compared to Gen3 and later the group ID does not match what it can do in
> the group and does not need to be set from DT, all that matters is that
> each VIN instance have a unique ID.
>
> Patch 4/6 and 5/6 uses the fact that VIN instances on Gen2 now have
> unique IDs and greatly simplifies the unnecessarily complex vl4-async
> notifier usage in the VIN driver. This have in the past lead to some
> subtle bugs and having only a single notifier for all VIN will remove a
> lot of possibilities for this to go wrong in the future.
>
> Finally patch 6/6 removes all non MC code paths and have the Gen2
> devices register a media device and configure links.
>
> This have been tested on Gen3 and Gen4 devices without any regressions.
> And on Gen2 to make sure the media-graph behaves as it should. As a
> bonus the Gen2 devices can now join the VIN CI and any future issues
> should be caught as they are for Gen3 and Gen4.
>
> See individual patches for changelog.
>
> Niklas Söderlund (6):
> media: rcar-vin: Use correct count of remote subdevices
> media: rcar-vin: Change link setup argument
> media: rcar-vin: Generate a VIN group ID for Gen2
> media: rcar-vin: Prepare for unifying all v4l-async notifiers
> media: rcar-vin: Merge all notifiers
> media: rcar-vin: Enable media-graph on Gen2
>
> .../platform/renesas/rcar-vin/rcar-core.c | 706 +++++++-----------
> .../platform/renesas/rcar-vin/rcar-dma.c | 16 +-
> .../platform/renesas/rcar-vin/rcar-v4l2.c | 488 +-----------
> .../platform/renesas/rcar-vin/rcar-vin.h | 14 +-
> 4 files changed, 305 insertions(+), 919 deletions(-)
>
On my V4H board, GMSL2, multistreams branch:
Tested-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
Tomi
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers
2025-05-27 7:01 ` Sakari Ailus
2025-05-27 11:06 ` Laurent Pinchart
@ 2025-06-06 13:50 ` Niklas Söderlund
2025-06-07 11:13 ` Sakari Ailus
1 sibling, 1 reply; 22+ messages in thread
From: Niklas Söderlund @ 2025-06-06 13:50 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Tomi Valkeinen, Laurent Pinchart,
linux-media, linux-renesas-soc
Hej Sakari,
Thanks for your feedback.
On 2025-05-27 07:01:47 +0000, Sakari Ailus wrote:
> Hej Niklas,
>
> On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> > The R-Car VIN driver is needless complex and uses more then one
>
> s/needless\K/ly/
>
> > v4l-async notifier to attach to all its subdevices. Prepare for unifying
> > them by moving rvin_parallel_parse_of() to where it needs to be when
> > they are unified.
> >
> > The function is moved verbatim and there is no change in behavior.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > .../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
> > 1 file changed, 53 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > index d9ad56fb2aa9..60ec57d73a12 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> > }
> > }
> >
> > +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > +{
> > + struct fwnode_handle *ep, *fwnode;
> > + struct v4l2_fwnode_endpoint vep = {
> > + .bus_type = V4L2_MBUS_UNKNOWN,
> > + };
> > + struct v4l2_async_connection *asc;
> > + int ret;
> > +
> > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > + if (!ep)
> > + return 0;
> > +
> > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > + fwnode_handle_put(ep);
> > + if (ret) {
> > + vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + switch (vep.bus_type) {
> > + case V4L2_MBUS_PARALLEL:
> > + case V4L2_MBUS_BT656:
> > + vin_dbg(vin, "Found %s media bus\n",
> > + vep.bus_type == V4L2_MBUS_PARALLEL ?
> > + "PARALLEL" : "BT656");
> > + vin->parallel.mbus_type = vep.bus_type;
> > + vin->parallel.bus = vep.bus.parallel;
> > + break;
> > + default:
> > + vin_err(vin, "Unknown media bus type\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > + struct v4l2_async_connection);
>
> If you use v4l2_async_nf_add_fwnode_remote() here, you can omit
> fwnode_graph_get_remote_endpoint() call above. Also the error handling
> becomes more simple.
Indeed it would, but I do use fwnode in the debug print at the end of
the function. And I do find that print out use-full when debugging, so I
would like to keep it.
Laurent's suggestion of using __free(fwnode_handle) can instead be used
to make error handling easier, and since it would be needed for the ep
variable anyhow I think I will try that.
>
> > + if (IS_ERR(asc)) {
> > + ret = PTR_ERR(asc);
> > + goto out;
> > + }
> > +
> > + vin->parallel.asc = asc;
> > +
> > + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > +out:
> > + fwnode_handle_put(fwnode);
> > +
> > + return ret;
> > +}
> > +
> > static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > unsigned int max_id)
> > {
> > @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> > .complete = rvin_parallel_notify_complete,
> > };
> >
> > -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > -{
> > - struct fwnode_handle *ep, *fwnode;
> > - struct v4l2_fwnode_endpoint vep = {
> > - .bus_type = V4L2_MBUS_UNKNOWN,
> > - };
> > - struct v4l2_async_connection *asc;
> > - int ret;
> > -
> > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > - if (!ep)
> > - return 0;
> > -
> > - fwnode = fwnode_graph_get_remote_endpoint(ep);
> > - ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > - fwnode_handle_put(ep);
> > - if (ret) {
> > - vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > - switch (vep.bus_type) {
> > - case V4L2_MBUS_PARALLEL:
> > - case V4L2_MBUS_BT656:
> > - vin_dbg(vin, "Found %s media bus\n",
> > - vep.bus_type == V4L2_MBUS_PARALLEL ?
> > - "PARALLEL" : "BT656");
> > - vin->parallel.mbus_type = vep.bus_type;
> > - vin->parallel.bus = vep.bus.parallel;
> > - break;
> > - default:
> > - vin_err(vin, "Unknown media bus type\n");
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > - struct v4l2_async_connection);
> > - if (IS_ERR(asc)) {
> > - ret = PTR_ERR(asc);
> > - goto out;
> > - }
> > -
> > - vin->parallel.asc = asc;
> > -
> > - vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > -out:
> > - fwnode_handle_put(fwnode);
> > -
> > - return ret;
> > -}
> > -
> > static void rvin_parallel_cleanup(struct rvin_dev *vin)
> > {
> > v4l2_async_nf_unregister(&vin->notifier);
> > --
> > 2.49.0
> >
>
> --
> Sakari Ailus
--
Kind Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 5/6] media: rcar-vin: Merge all notifiers
2025-05-27 11:45 ` Laurent Pinchart
@ 2025-06-06 14:09 ` Niklas Söderlund
2025-06-07 14:02 ` Laurent Pinchart
0 siblings, 1 reply; 22+ messages in thread
From: Niklas Söderlund @ 2025-06-06 14:09 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen, linux-media,
linux-renesas-soc
Hi Laurent,
Thanks for your feedback!
On 2025-05-27 13:45:13 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Wed, May 21, 2025 at 03:20:36PM +0200, Niklas Söderlund wrote:
> > The VIN usage of v4l-async is complex and stems from organic growth of
> > the driver of supporting both private local subdevices (Gen2, Gen3) and
> > subdevices shared between all VIN instances (Gen3 and Gen4).
> >
> > The driver used a separate notifier for each VIN for the private local
> > ones, and a shared group notifier for the shared ones. This was complex
> > and lead to subtle bugs when unbinding and later rebinding subdevices in
> > on of the notifiers having to handle different edge cases depending on
>
> s/on of/one of/ (I think)
>
> > if it also had subdevices in the other notifiers etc.
> >
> > To simplify this have the Gen2 devices allocate and form a VIN group
> > too. This way all subdevices on all models can be collect in a
> > single group notifier. Then there is only a single complete callback for
> > all where the video devices and subdevice nodes can be registered etc.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > .../platform/renesas/rcar-vin/rcar-core.c | 263 ++++++++----------
> > .../platform/renesas/rcar-vin/rcar-vin.h | 2 -
> > 2 files changed, 114 insertions(+), 151 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > index 60ec57d73a12..b0727e98dac6 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > @@ -43,6 +43,9 @@
> >
> > #define v4l2_dev_to_vin(d) container_of(d, struct rvin_dev, v4l2_dev)
> >
> > +static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > + struct v4l2_subdev *subdev);
>
> Any chance you could move the function instead of forward-declaring it ?
I could move it in a separate commit prior to this patch. But the last
patch in the series removes the function all together so I thought that
to be more churn then needed.
>
> > +
> > /* -----------------------------------------------------------------------------
> > * Gen3 Group Allocator
> > */
> > @@ -232,7 +235,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> > }
> > }
> >
> > - return vin->group->link_setup(vin->group);
> > + if (vin->group->link_setup)
> > + return vin->group->link_setup(vin->group);
> > +
> > + return 0;
> > }
> >
> > static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> > @@ -240,22 +246,31 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> > struct v4l2_async_connection *asc)
> > {
> > struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > - unsigned int i;
> > + struct rvin_group *group = vin->group;
> >
> > - for (i = 0; i < RCAR_VIN_NUM; i++)
> > - if (vin->group->vin[i])
> > - rvin_v4l2_unregister(vin->group->vin[i]);
> > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++)
>
> While at it, you can use ARRAY_SIZE().
The use of RCAR_VIN_NUM is everywhere in the driver. I do however agree
using ARRAY_SIZE() is better. I will convert the whole driver to
ARRAY_SIZE() in a follow up patch outside this series.
I think it's important that all loops look the same and I'm worried this
series is growing too large with all nice, but unrelated, cleanups
unearth by it. I hope this is OK.
I will however keep the conversion to ARRAY_SIZE() for the
RVIN_REMOTES_MAX define as this was already something this series needed
to address in patch 1 and it's only used in one location that was not
already touched by this series.
>
> > + if (group->vin[i])
> > + rvin_v4l2_unregister(group->vin[i]);
>
> And please use curly braces for the for statement.
>
> >
> > mutex_lock(&vin->group->lock);
>
> Add a blank line.
>
> > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
>
> ARRAY_SIZE() here too. Same below where applicable.
>
> > + if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
> > + continue;
> > +
> > + group->vin[i]->parallel.subdev = NULL;
> > +
> > + vin_dbg(group->vin[i], "Unbind parallel subdev %s\n",
> > + subdev->name);
> > + }
> >
> > - for (i = 0; i < RVIN_REMOTES_MAX; i++) {
> > - if (vin->group->remotes[i].asc != asc)
> > + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> > + if (group->remotes[i].asc != asc)
> > continue;
> > - vin->group->remotes[i].subdev = NULL;
> > +
> > + group->remotes[i].subdev = NULL;
> > +
> > vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
> > - break;
> > }
> > -
>
> You can keep this blank line :-)
>
> > mutex_unlock(&vin->group->lock);
> >
> > media_device_unregister(&vin->group->mdev);
> > @@ -266,21 +281,38 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> > struct v4l2_async_connection *asc)
> > {
> > struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > - unsigned int i;
> > + struct rvin_group *group = vin->group;
> >
> > - mutex_lock(&vin->group->lock);
> > + guard(mutex)(&group->lock);
> >
> > - for (i = 0; i < RVIN_REMOTES_MAX; i++) {
> > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> > + int ret;
> > +
> > + if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
> > + continue;
> > +
> > + ret = rvin_parallel_subdevice_attach(group->vin[i], subdev);
> > + if (ret)
> > + return ret;
> > +
> > + v4l2_set_subdev_hostdata(subdev, group->vin[i]);
> > +
> > + vin_dbg(group->vin[i], "Bound subdev %s\n", subdev->name);
> > +
> > + return 0;
> > + }
> > +
> > + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> > if (vin->group->remotes[i].asc != asc)
> > continue;
> > +
> > vin->group->remotes[i].subdev = subdev;
> > vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
> > - break;
> > +
> > + return 0;
> > }
> >
> > - mutex_unlock(&vin->group->lock);
> > -
> > - return 0;
> > + return -ENODEV;
> > }
> >
> > static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
> > @@ -374,7 +406,7 @@ static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > goto out;
> > }
> >
> > - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > + asc = v4l2_async_nf_add_fwnode(&vin->group->notifier, fwnode,
> > struct v4l2_async_connection);
> > if (IS_ERR(asc)) {
> > ret = PTR_ERR(asc);
> > @@ -424,6 +456,12 @@ static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > if (!(vin_mask & BIT(i)))
> > continue;
> >
> > + /* Parse local subdevice. */
> > + ret = rvin_parallel_parse_of(vin->group->vin[i]);
> > + if (ret)
> > + return ret;
> > +
> > + /* Prase shared subdevices. */
> > for (id = 0; id < max_id; id++) {
> > if (vin->group->remotes[id].asc)
> > continue;
> > @@ -603,124 +641,6 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > return 0;
> > }
> >
> > -static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> > -{
> > - rvin_v4l2_unregister(vin);
> > - vin->parallel.subdev = NULL;
> > -
> > - if (!vin->info->use_mc)
> > - rvin_free_controls(vin);
> > -}
> > -
> > -static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > -{
> > - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > - struct media_entity *source;
> > - struct media_entity *sink;
> > - int ret;
> > -
> > - ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> > - if (ret < 0) {
> > - vin_err(vin, "Failed to register subdev nodes\n");
> > - return ret;
> > - }
> > -
> > - 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,
> > - struct v4l2_subdev *subdev,
> > - struct v4l2_async_connection *asc)
> > -{
> > - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > -
> > - vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
> > -
> > - mutex_lock(&vin->lock);
> > - rvin_parallel_subdevice_detach(vin);
> > - mutex_unlock(&vin->lock);
> > -}
> > -
> > -static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> > - struct v4l2_subdev *subdev,
> > - struct v4l2_async_connection *asc)
> > -{
> > - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > - int ret;
> > -
> > - mutex_lock(&vin->lock);
> > - ret = rvin_parallel_subdevice_attach(vin, subdev);
> > - mutex_unlock(&vin->lock);
> > - if (ret)
> > - return ret;
> > -
> > - v4l2_set_subdev_hostdata(subdev, vin);
> > -
> > - vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
> > - subdev->name, vin->parallel.source_pad,
> > - vin->parallel.sink_pad);
> > -
> > - return 0;
> > -}
> > -
> > -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 void rvin_parallel_cleanup(struct rvin_dev *vin)
> > -{
> > - v4l2_async_nf_unregister(&vin->notifier);
> > - v4l2_async_nf_cleanup(&vin->notifier);
> > -}
> > -
> > -static int rvin_parallel_init(struct rvin_dev *vin)
> > -{
> > - int ret;
> > -
> > - v4l2_async_nf_init(&vin->notifier, &vin->v4l2_dev);
> > -
> > - ret = rvin_parallel_parse_of(vin);
> > - if (ret)
> > - return ret;
> > -
> > - if (!vin->parallel.asc)
> > - return -ENODEV;
> > -
> > - vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > - to_of_node(vin->parallel.asc->match.fwnode));
> > -
> > - vin->notifier.ops = &rvin_parallel_notify_ops;
> > - ret = v4l2_async_nf_register(&vin->notifier);
> > - if (ret < 0) {
> > - vin_err(vin, "Notifier registration failed\n");
> > - v4l2_async_nf_cleanup(&vin->notifier);
> > - return ret;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > /* -----------------------------------------------------------------------------
> > * CSI-2
> > */
> > @@ -895,11 +815,63 @@ static int rvin_csi2_create_link(struct rvin_group *group, unsigned int id,
> > return 0;
> > }
> >
> > +static int rvin_parallel_setup_links(struct rvin_group *group)
> > +{
> > + u32 flags = MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE;
> > + int ret = 0;
> > +
> > + mutex_lock(&group->lock);
>
> Use a guard.
>
> > + /* If the group also have links don't enable the link. */
>
> s/have/has/
>
> > + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> > + if (group->remotes[i].subdev) {
> > + flags = 0;
> > + break;
> > + }
> > + }
> > +
> > + /* Create links */
>
> s/links/links./
>
> > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> > + struct rvin_dev *vin = group->vin[i];
> > + struct media_entity *source;
> > + struct media_entity *sink;
> > +
> > + /* Noting to do if their is no VIN or parallel subdev. */
>
> s/Noting/Nothing/
> s/their/there/
>
> > + if (!vin || !vin->parallel.subdev)
> > + continue;
> > +
> > + source = &vin->parallel.subdev->entity;
> > + sink = &vin->vdev.entity;
> > +
> > + ret = media_create_pad_link(source, vin->parallel.source_pad,
> > + sink, 0, flags);
> > + if (ret)
> > + break;
>
> return ret;
>
> (thanks to the guard above)
>
> > + }
> > + mutex_unlock(&group->lock);
> > +
> > + return ret;
>
> return 0;
>
> > +}
> > +
> > static int rvin_csi2_setup_links(struct rvin_group *group)
> > {
> > const struct rvin_group_route *routes, *route;
> > unsigned int id;
> > - int ret = -EINVAL;
> > + int ret;
> > +
> > + /* Find any VIN in group to get route info. */
> > + routes = NULL;
> > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> > + if (group->vin[i]) {
> > + routes = group->vin[i]->info->routes;
> > + break;
> > + }
> > + }
> > + if (!routes)
> > + return -ENODEV;
>
> Storing the info pointer in the group as proposed in the review of a
> previous patch from the same series would help here too.
I agree. I have added a patch that introduce this change separatly and
all new code uses the info from the group structure. I have added to my
ever growing todo file to remove the info struct all together from the
private VIN data structure in a follow up patch. This change would
depend on this series as Gen2 currently do not participate in the group
concept.
>
> > +
> > + ret = rvin_parallel_setup_links(group);
> > + if (ret)
> > + return ret;
> >
> > /* Find any VIN in group to get route info. */
> > routes = NULL;
> > @@ -914,6 +886,7 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
> >
> > /* Create all media device links between VINs and CSI-2's. */
> > mutex_lock(&group->lock);
> > + ret = -EINVAL;
> > for (route = routes; route->chsel; route++) {
> > /* Check that VIN' master is part of the group. */
> > if (!group->vin[route->master])
> > @@ -941,7 +914,6 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
> >
> > static void rvin_csi2_cleanup(struct rvin_dev *vin)
> > {
> > - rvin_parallel_cleanup(vin);
> > rvin_group_notifier_cleanup(vin);
> > rvin_group_put(vin);
> > rvin_free_controls(vin);
> > @@ -964,18 +936,11 @@ static int rvin_csi2_init(struct rvin_dev *vin)
> > if (ret)
> > goto err_controls;
> >
> > - /* It's OK to not have a parallel subdevice. */
> > - ret = rvin_parallel_init(vin);
> > - if (ret && ret != -ENODEV)
> > - goto err_group;
> > -
> > ret = rvin_group_notifier_init(vin, 1, RVIN_CSI_MAX);
> > if (ret)
> > - goto err_parallel;
> > + goto err_group;
> >
> > return 0;
> > -err_parallel:
> > - rvin_parallel_cleanup(vin);
> > err_group:
> > rvin_group_put(vin);
> > err_controls:
> > @@ -1448,7 +1413,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > rvin_group_id_to_master(vin->id) == vin->id)
> > vin->scaler = vin->info->scaler;
> > } else {
> > - ret = rvin_parallel_init(vin);
> > + ret = rvin_group_get(vin, NULL, NULL);
> > + if (!ret)
> > + ret = rvin_group_notifier_init(vin, 0, 0);
> >
> > if (vin->info->scaler)
> > vin->scaler = vin->info->scaler;
> > @@ -1478,8 +1445,6 @@ static void rcar_vin_remove(struct platform_device *pdev)
> > rvin_isp_cleanup(vin);
> > else if (vin->info->use_mc)
> > rvin_csi2_cleanup(vin);
> > - else
> > - rvin_parallel_cleanup(vin);
> >
> > rvin_id_put(vin);
> >
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > index 7d4fce248976..a577f4fe4a6c 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > @@ -149,7 +149,6 @@ struct rvin_info {
> > * @vdev: V4L2 video device associated with VIN
> > * @v4l2_dev: V4L2 device
> > * @ctrl_handler: V4L2 control handler
> > - * @notifier: V4L2 asynchronous subdevs notifier
> > *
> > * @parallel: parallel input subdevice descriptor
> > *
> > @@ -189,7 +188,6 @@ struct rvin_dev {
> > struct video_device vdev;
> > struct v4l2_device v4l2_dev;
> > struct v4l2_ctrl_handler ctrl_handler;
> > - struct v4l2_async_notifier notifier;
> >
> > struct rvin_parallel_entity parallel;
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Kind Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers
2025-06-06 13:50 ` Niklas Söderlund
@ 2025-06-07 11:13 ` Sakari Ailus
2025-06-07 11:59 ` Niklas Söderlund
0 siblings, 1 reply; 22+ messages in thread
From: Sakari Ailus @ 2025-06-07 11:13 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Mauro Carvalho Chehab, Tomi Valkeinen, Laurent Pinchart,
linux-media, linux-renesas-soc
Hejssan!
On Fri, Jun 06, 2025 at 03:50:00PM +0200, Niklas Söderlund wrote:
> Hej Sakari,
>
> Thanks for your feedback.
Var så god!
>
> On 2025-05-27 07:01:47 +0000, Sakari Ailus wrote:
> > Hej Niklas,
> >
> > On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> > > The R-Car VIN driver is needless complex and uses more then one
> >
> > s/needless\K/ly/
> >
> > > v4l-async notifier to attach to all its subdevices. Prepare for unifying
> > > them by moving rvin_parallel_parse_of() to where it needs to be when
> > > they are unified.
> > >
> > > The function is moved verbatim and there is no change in behavior.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > > .../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
> > > 1 file changed, 53 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > index d9ad56fb2aa9..60ec57d73a12 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> > > }
> > > }
> > >
> > > +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > +{
> > > + struct fwnode_handle *ep, *fwnode;
> > > + struct v4l2_fwnode_endpoint vep = {
> > > + .bus_type = V4L2_MBUS_UNKNOWN,
> > > + };
> > > + struct v4l2_async_connection *asc;
> > > + int ret;
> > > +
> > > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > + if (!ep)
> > > + return 0;
> > > +
> > > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > + fwnode_handle_put(ep);
> > > + if (ret) {
> > > + vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + switch (vep.bus_type) {
> > > + case V4L2_MBUS_PARALLEL:
> > > + case V4L2_MBUS_BT656:
> > > + vin_dbg(vin, "Found %s media bus\n",
> > > + vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > + "PARALLEL" : "BT656");
> > > + vin->parallel.mbus_type = vep.bus_type;
> > > + vin->parallel.bus = vep.bus.parallel;
> > > + break;
> > > + default:
> > > + vin_err(vin, "Unknown media bus type\n");
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > + struct v4l2_async_connection);
> >
> > If you use v4l2_async_nf_add_fwnode_remote() here, you can omit
> > fwnode_graph_get_remote_endpoint() call above. Also the error handling
> > becomes more simple.
>
> Indeed it would, but I do use fwnode in the debug print at the end of
> the function. And I do find that print out use-full when debugging, so I
> would like to keep it.
The drivers really shouldn't have a need for this. How about adding that
debug print to the V4L2 async framework instead? I think it might be useful
for other drivers as well even though the information is available via
debugfs (or sysfs?) already.
>
> Laurent's suggestion of using __free(fwnode_handle) can instead be used
> to make error handling easier, and since it would be needed for the ep
> variable anyhow I think I will try that.
Sounds good to me.
>
> >
> > > + if (IS_ERR(asc)) {
> > > + ret = PTR_ERR(asc);
> > > + goto out;
> > > + }
> > > +
> > > + vin->parallel.asc = asc;
> > > +
> > > + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
Please use %pfw instead for the fwnode (at least for the possible V4L2
async patch).
> > > +out:
> > > + fwnode_handle_put(fwnode);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > > unsigned int max_id)
> > > {
> > > @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> > > .complete = rvin_parallel_notify_complete,
> > > };
> > >
> > > -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > -{
> > > - struct fwnode_handle *ep, *fwnode;
> > > - struct v4l2_fwnode_endpoint vep = {
> > > - .bus_type = V4L2_MBUS_UNKNOWN,
> > > - };
> > > - struct v4l2_async_connection *asc;
> > > - int ret;
> > > -
> > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > - if (!ep)
> > > - return 0;
> > > -
> > > - fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > - ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > - fwnode_handle_put(ep);
> > > - if (ret) {
> > > - vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > -
> > > - switch (vep.bus_type) {
> > > - case V4L2_MBUS_PARALLEL:
> > > - case V4L2_MBUS_BT656:
> > > - vin_dbg(vin, "Found %s media bus\n",
> > > - vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > - "PARALLEL" : "BT656");
> > > - vin->parallel.mbus_type = vep.bus_type;
> > > - vin->parallel.bus = vep.bus.parallel;
> > > - break;
> > > - default:
> > > - vin_err(vin, "Unknown media bus type\n");
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > -
> > > - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > - struct v4l2_async_connection);
> > > - if (IS_ERR(asc)) {
> > > - ret = PTR_ERR(asc);
> > > - goto out;
> > > - }
> > > -
> > > - vin->parallel.asc = asc;
> > > -
> > > - vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > > -out:
> > > - fwnode_handle_put(fwnode);
> > > -
> > > - return ret;
> > > -}
> > > -
> > > static void rvin_parallel_cleanup(struct rvin_dev *vin)
> > > {
> > > v4l2_async_nf_unregister(&vin->notifier);
--
Med vänliga hälsningar,
Sakari Ailus
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers
2025-06-07 11:13 ` Sakari Ailus
@ 2025-06-07 11:59 ` Niklas Söderlund
0 siblings, 0 replies; 22+ messages in thread
From: Niklas Söderlund @ 2025-06-07 11:59 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Tomi Valkeinen, Laurent Pinchart,
linux-media, linux-renesas-soc
Hej Sakari,
On 2025-06-07 11:13:55 +0000, Sakari Ailus wrote:
> Hejssan!
>
> On Fri, Jun 06, 2025 at 03:50:00PM +0200, Niklas Söderlund wrote:
> > Hej Sakari,
> >
> > Thanks for your feedback.
>
> Var så god!
>
> >
> > On 2025-05-27 07:01:47 +0000, Sakari Ailus wrote:
> > > Hej Niklas,
> > >
> > > On Wed, May 21, 2025 at 03:20:35PM +0200, Niklas Söderlund wrote:
> > > > The R-Car VIN driver is needless complex and uses more then one
> > >
> > > s/needless\K/ly/
> > >
> > > > v4l-async notifier to attach to all its subdevices. Prepare for unifying
> > > > them by moving rvin_parallel_parse_of() to where it needs to be when
> > > > they are unified.
> > > >
> > > > The function is moved verbatim and there is no change in behavior.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > ---
> > > > .../platform/renesas/rcar-vin/rcar-core.c | 106 +++++++++---------
> > > > 1 file changed, 53 insertions(+), 53 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > > index d9ad56fb2aa9..60ec57d73a12 100644
> > > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > > @@ -337,6 +337,59 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> > > > }
> > > > }
> > > >
> > > > +static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > > +{
> > > > + struct fwnode_handle *ep, *fwnode;
> > > > + struct v4l2_fwnode_endpoint vep = {
> > > > + .bus_type = V4L2_MBUS_UNKNOWN,
> > > > + };
> > > > + struct v4l2_async_connection *asc;
> > > > + int ret;
> > > > +
> > > > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > > + if (!ep)
> > > > + return 0;
> > > > +
> > > > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > > + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > > + fwnode_handle_put(ep);
> > > > + if (ret) {
> > > > + vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > > + ret = -EINVAL;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + switch (vep.bus_type) {
> > > > + case V4L2_MBUS_PARALLEL:
> > > > + case V4L2_MBUS_BT656:
> > > > + vin_dbg(vin, "Found %s media bus\n",
> > > > + vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > > + "PARALLEL" : "BT656");
> > > > + vin->parallel.mbus_type = vep.bus_type;
> > > > + vin->parallel.bus = vep.bus.parallel;
> > > > + break;
> > > > + default:
> > > > + vin_err(vin, "Unknown media bus type\n");
> > > > + ret = -EINVAL;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > > + struct v4l2_async_connection);
> > >
> > > If you use v4l2_async_nf_add_fwnode_remote() here, you can omit
> > > fwnode_graph_get_remote_endpoint() call above. Also the error handling
> > > becomes more simple.
> >
> > Indeed it would, but I do use fwnode in the debug print at the end of
> > the function. And I do find that print out use-full when debugging, so I
> > would like to keep it.
>
> The drivers really shouldn't have a need for this. How about adding that
> debug print to the V4L2 async framework instead? I think it might be useful
> for other drivers as well even though the information is available via
> debugfs (or sysfs?) already.
That is a good idea, I will try to find time and move the debug prints
(I have other similar ones in the driver) to the framework in follow up
work.
Or if debugfs is enough for my needs I can drop them all together! My
primary use-case is to get a grips of what's going on from bug reports
sent to me so I can't always poke around on a live system.
>
> >
> > Laurent's suggestion of using __free(fwnode_handle) can instead be used
> > to make error handling easier, and since it would be needed for the ep
> > variable anyhow I think I will try that.
>
> Sounds good to me.
>
> >
> > >
> > > > + if (IS_ERR(asc)) {
> > > > + ret = PTR_ERR(asc);
> > > > + goto out;
> > > > + }
> > > > +
> > > > + vin->parallel.asc = asc;
> > > > +
> > > > + vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
>
> Please use %pfw instead for the fwnode (at least for the possible V4L2
> async patch).
ack.
>
> > > > +out:
> > > > + fwnode_handle_put(fwnode);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > > > unsigned int max_id)
> > > > {
> > > > @@ -635,59 +688,6 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
> > > > .complete = rvin_parallel_notify_complete,
> > > > };
> > > >
> > > > -static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > > -{
> > > > - struct fwnode_handle *ep, *fwnode;
> > > > - struct v4l2_fwnode_endpoint vep = {
> > > > - .bus_type = V4L2_MBUS_UNKNOWN,
> > > > - };
> > > > - struct v4l2_async_connection *asc;
> > > > - int ret;
> > > > -
> > > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
> > > > - if (!ep)
> > > > - return 0;
> > > > -
> > > > - fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > > - ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > > - fwnode_handle_put(ep);
> > > > - if (ret) {
> > > > - vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> > > > - ret = -EINVAL;
> > > > - goto out;
> > > > - }
> > > > -
> > > > - switch (vep.bus_type) {
> > > > - case V4L2_MBUS_PARALLEL:
> > > > - case V4L2_MBUS_BT656:
> > > > - vin_dbg(vin, "Found %s media bus\n",
> > > > - vep.bus_type == V4L2_MBUS_PARALLEL ?
> > > > - "PARALLEL" : "BT656");
> > > > - vin->parallel.mbus_type = vep.bus_type;
> > > > - vin->parallel.bus = vep.bus.parallel;
> > > > - break;
> > > > - default:
> > > > - vin_err(vin, "Unknown media bus type\n");
> > > > - ret = -EINVAL;
> > > > - goto out;
> > > > - }
> > > > -
> > > > - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > > - struct v4l2_async_connection);
> > > > - if (IS_ERR(asc)) {
> > > > - ret = PTR_ERR(asc);
> > > > - goto out;
> > > > - }
> > > > -
> > > > - vin->parallel.asc = asc;
> > > > -
> > > > - vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
> > > > -out:
> > > > - fwnode_handle_put(fwnode);
> > > > -
> > > > - return ret;
> > > > -}
> > > > -
> > > > static void rvin_parallel_cleanup(struct rvin_dev *vin)
> > > > {
> > > > v4l2_async_nf_unregister(&vin->notifier);
>
> --
> Med vänliga hälsningar,
>
> Sakari Ailus
--
Kind Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 5/6] media: rcar-vin: Merge all notifiers
2025-06-06 14:09 ` Niklas Söderlund
@ 2025-06-07 14:02 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2025-06-07 14:02 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, Mauro Carvalho Chehab, Tomi Valkeinen, linux-media,
linux-renesas-soc
On Fri, Jun 06, 2025 at 04:09:33PM +0200, Niklas Söderlund wrote:
> Hi Laurent,
>
> Thanks for your feedback!
>
> On 2025-05-27 13:45:13 +0200, Laurent Pinchart wrote:
> > Hi Niklas,
> >
> > Thank you for the patch.
> >
> > On Wed, May 21, 2025 at 03:20:36PM +0200, Niklas Söderlund wrote:
> > > The VIN usage of v4l-async is complex and stems from organic growth of
> > > the driver of supporting both private local subdevices (Gen2, Gen3) and
> > > subdevices shared between all VIN instances (Gen3 and Gen4).
> > >
> > > The driver used a separate notifier for each VIN for the private local
> > > ones, and a shared group notifier for the shared ones. This was complex
> > > and lead to subtle bugs when unbinding and later rebinding subdevices in
> > > on of the notifiers having to handle different edge cases depending on
> >
> > s/on of/one of/ (I think)
> >
> > > if it also had subdevices in the other notifiers etc.
> > >
> > > To simplify this have the Gen2 devices allocate and form a VIN group
> > > too. This way all subdevices on all models can be collect in a
> > > single group notifier. Then there is only a single complete callback for
> > > all where the video devices and subdevice nodes can be registered etc.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > > .../platform/renesas/rcar-vin/rcar-core.c | 263 ++++++++----------
> > > .../platform/renesas/rcar-vin/rcar-vin.h | 2 -
> > > 2 files changed, 114 insertions(+), 151 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > index 60ec57d73a12..b0727e98dac6 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > > @@ -43,6 +43,9 @@
> > >
> > > #define v4l2_dev_to_vin(d) container_of(d, struct rvin_dev, v4l2_dev)
> > >
> > > +static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > > + struct v4l2_subdev *subdev);
> >
> > Any chance you could move the function instead of forward-declaring it ?
>
> I could move it in a separate commit prior to this patch. But the last
> patch in the series removes the function all together so I thought that
> to be more churn then needed.
OK it's fine then.
> > > +
> > > /* -----------------------------------------------------------------------------
> > > * Gen3 Group Allocator
> > > */
> > > @@ -232,7 +235,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> > > }
> > > }
> > >
> > > - return vin->group->link_setup(vin->group);
> > > + if (vin->group->link_setup)
> > > + return vin->group->link_setup(vin->group);
> > > +
> > > + return 0;
> > > }
> > >
> > > static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> > > @@ -240,22 +246,31 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> > > struct v4l2_async_connection *asc)
> > > {
> > > struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > > - unsigned int i;
> > > + struct rvin_group *group = vin->group;
> > >
> > > - for (i = 0; i < RCAR_VIN_NUM; i++)
> > > - if (vin->group->vin[i])
> > > - rvin_v4l2_unregister(vin->group->vin[i]);
> > > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++)
> >
> > While at it, you can use ARRAY_SIZE().
>
> The use of RCAR_VIN_NUM is everywhere in the driver. I do however agree
> using ARRAY_SIZE() is better. I will convert the whole driver to
> ARRAY_SIZE() in a follow up patch outside this series.
OK.
> I think it's important that all loops look the same and I'm worried this
> series is growing too large with all nice, but unrelated, cleanups
> unearth by it. I hope this is OK.
>
> I will however keep the conversion to ARRAY_SIZE() for the
> RVIN_REMOTES_MAX define as this was already something this series needed
> to address in patch 1 and it's only used in one location that was not
> already touched by this series.
>
> >
> > > + if (group->vin[i])
> > > + rvin_v4l2_unregister(group->vin[i]);
> >
> > And please use curly braces for the for statement.
> >
> > >
> > > mutex_lock(&vin->group->lock);
> >
> > Add a blank line.
> >
> > > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> >
> > ARRAY_SIZE() here too. Same below where applicable.
> >
> > > + if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
> > > + continue;
> > > +
> > > + group->vin[i]->parallel.subdev = NULL;
> > > +
> > > + vin_dbg(group->vin[i], "Unbind parallel subdev %s\n",
> > > + subdev->name);
> > > + }
> > >
> > > - for (i = 0; i < RVIN_REMOTES_MAX; i++) {
> > > - if (vin->group->remotes[i].asc != asc)
> > > + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> > > + if (group->remotes[i].asc != asc)
> > > continue;
> > > - vin->group->remotes[i].subdev = NULL;
> > > +
> > > + group->remotes[i].subdev = NULL;
> > > +
> > > vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
> > > - break;
> > > }
> > > -
> >
> > You can keep this blank line :-)
> >
> > > mutex_unlock(&vin->group->lock);
> > >
> > > media_device_unregister(&vin->group->mdev);
> > > @@ -266,21 +281,38 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> > > struct v4l2_async_connection *asc)
> > > {
> > > struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > > - unsigned int i;
> > > + struct rvin_group *group = vin->group;
> > >
> > > - mutex_lock(&vin->group->lock);
> > > + guard(mutex)(&group->lock);
> > >
> > > - for (i = 0; i < RVIN_REMOTES_MAX; i++) {
> > > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> > > + int ret;
> > > +
> > > + if (!group->vin[i] || group->vin[i]->parallel.asc != asc)
> > > + continue;
> > > +
> > > + ret = rvin_parallel_subdevice_attach(group->vin[i], subdev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + v4l2_set_subdev_hostdata(subdev, group->vin[i]);
> > > +
> > > + vin_dbg(group->vin[i], "Bound subdev %s\n", subdev->name);
> > > +
> > > + return 0;
> > > + }
> > > +
> > > + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> > > if (vin->group->remotes[i].asc != asc)
> > > continue;
> > > +
> > > vin->group->remotes[i].subdev = subdev;
> > > vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
> > > - break;
> > > +
> > > + return 0;
> > > }
> > >
> > > - mutex_unlock(&vin->group->lock);
> > > -
> > > - return 0;
> > > + return -ENODEV;
> > > }
> > >
> > > static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
> > > @@ -374,7 +406,7 @@ static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > > goto out;
> > > }
> > >
> > > - asc = v4l2_async_nf_add_fwnode(&vin->notifier, fwnode,
> > > + asc = v4l2_async_nf_add_fwnode(&vin->group->notifier, fwnode,
> > > struct v4l2_async_connection);
> > > if (IS_ERR(asc)) {
> > > ret = PTR_ERR(asc);
> > > @@ -424,6 +456,12 @@ static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> > > if (!(vin_mask & BIT(i)))
> > > continue;
> > >
> > > + /* Parse local subdevice. */
> > > + ret = rvin_parallel_parse_of(vin->group->vin[i]);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Prase shared subdevices. */
> > > for (id = 0; id < max_id; id++) {
> > > if (vin->group->remotes[id].asc)
> > > continue;
> > > @@ -603,124 +641,6 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > > return 0;
> > > }
> > >
> > > -static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> > > -{
> > > - rvin_v4l2_unregister(vin);
> > > - vin->parallel.subdev = NULL;
> > > -
> > > - if (!vin->info->use_mc)
> > > - rvin_free_controls(vin);
> > > -}
> > > -
> > > -static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > > -{
> > > - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > > - struct media_entity *source;
> > > - struct media_entity *sink;
> > > - int ret;
> > > -
> > > - ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> > > - if (ret < 0) {
> > > - vin_err(vin, "Failed to register subdev nodes\n");
> > > - return ret;
> > > - }
> > > -
> > > - 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,
> > > - struct v4l2_subdev *subdev,
> > > - struct v4l2_async_connection *asc)
> > > -{
> > > - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > > -
> > > - vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
> > > -
> > > - mutex_lock(&vin->lock);
> > > - rvin_parallel_subdevice_detach(vin);
> > > - mutex_unlock(&vin->lock);
> > > -}
> > > -
> > > -static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> > > - struct v4l2_subdev *subdev,
> > > - struct v4l2_async_connection *asc)
> > > -{
> > > - struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > > - int ret;
> > > -
> > > - mutex_lock(&vin->lock);
> > > - ret = rvin_parallel_subdevice_attach(vin, subdev);
> > > - mutex_unlock(&vin->lock);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - v4l2_set_subdev_hostdata(subdev, vin);
> > > -
> > > - vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
> > > - subdev->name, vin->parallel.source_pad,
> > > - vin->parallel.sink_pad);
> > > -
> > > - return 0;
> > > -}
> > > -
> > > -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 void rvin_parallel_cleanup(struct rvin_dev *vin)
> > > -{
> > > - v4l2_async_nf_unregister(&vin->notifier);
> > > - v4l2_async_nf_cleanup(&vin->notifier);
> > > -}
> > > -
> > > -static int rvin_parallel_init(struct rvin_dev *vin)
> > > -{
> > > - int ret;
> > > -
> > > - v4l2_async_nf_init(&vin->notifier, &vin->v4l2_dev);
> > > -
> > > - ret = rvin_parallel_parse_of(vin);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - if (!vin->parallel.asc)
> > > - return -ENODEV;
> > > -
> > > - vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > > - to_of_node(vin->parallel.asc->match.fwnode));
> > > -
> > > - vin->notifier.ops = &rvin_parallel_notify_ops;
> > > - ret = v4l2_async_nf_register(&vin->notifier);
> > > - if (ret < 0) {
> > > - vin_err(vin, "Notifier registration failed\n");
> > > - v4l2_async_nf_cleanup(&vin->notifier);
> > > - return ret;
> > > - }
> > > -
> > > - return 0;
> > > -}
> > > -
> > > /* -----------------------------------------------------------------------------
> > > * CSI-2
> > > */
> > > @@ -895,11 +815,63 @@ static int rvin_csi2_create_link(struct rvin_group *group, unsigned int id,
> > > return 0;
> > > }
> > >
> > > +static int rvin_parallel_setup_links(struct rvin_group *group)
> > > +{
> > > + u32 flags = MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE;
> > > + int ret = 0;
> > > +
> > > + mutex_lock(&group->lock);
> >
> > Use a guard.
> >
> > > + /* If the group also have links don't enable the link. */
> >
> > s/have/has/
> >
> > > + for (unsigned int i = 0; i < RVIN_REMOTES_MAX; i++) {
> > > + if (group->remotes[i].subdev) {
> > > + flags = 0;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + /* Create links */
> >
> > s/links/links./
> >
> > > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> > > + struct rvin_dev *vin = group->vin[i];
> > > + struct media_entity *source;
> > > + struct media_entity *sink;
> > > +
> > > + /* Noting to do if their is no VIN or parallel subdev. */
> >
> > s/Noting/Nothing/
> > s/their/there/
> >
> > > + if (!vin || !vin->parallel.subdev)
> > > + continue;
> > > +
> > > + source = &vin->parallel.subdev->entity;
> > > + sink = &vin->vdev.entity;
> > > +
> > > + ret = media_create_pad_link(source, vin->parallel.source_pad,
> > > + sink, 0, flags);
> > > + if (ret)
> > > + break;
> >
> > return ret;
> >
> > (thanks to the guard above)
> >
> > > + }
> > > + mutex_unlock(&group->lock);
> > > +
> > > + return ret;
> >
> > return 0;
> >
> > > +}
> > > +
> > > static int rvin_csi2_setup_links(struct rvin_group *group)
> > > {
> > > const struct rvin_group_route *routes, *route;
> > > unsigned int id;
> > > - int ret = -EINVAL;
> > > + int ret;
> > > +
> > > + /* Find any VIN in group to get route info. */
> > > + routes = NULL;
> > > + for (unsigned int i = 0; i < RCAR_VIN_NUM; i++) {
> > > + if (group->vin[i]) {
> > > + routes = group->vin[i]->info->routes;
> > > + break;
> > > + }
> > > + }
> > > + if (!routes)
> > > + return -ENODEV;
> >
> > Storing the info pointer in the group as proposed in the review of a
> > previous patch from the same series would help here too.
>
> I agree. I have added a patch that introduce this change separatly and
> all new code uses the info from the group structure. I have added to my
> ever growing todo file to remove the info struct all together from the
> private VIN data structure in a follow up patch. This change would
> depend on this series as Gen2 currently do not participate in the group
> concept.
>
> > > +
> > > + ret = rvin_parallel_setup_links(group);
> > > + if (ret)
> > > + return ret;
> > >
> > > /* Find any VIN in group to get route info. */
> > > routes = NULL;
> > > @@ -914,6 +886,7 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
> > >
> > > /* Create all media device links between VINs and CSI-2's. */
> > > mutex_lock(&group->lock);
> > > + ret = -EINVAL;
> > > for (route = routes; route->chsel; route++) {
> > > /* Check that VIN' master is part of the group. */
> > > if (!group->vin[route->master])
> > > @@ -941,7 +914,6 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
> > >
> > > static void rvin_csi2_cleanup(struct rvin_dev *vin)
> > > {
> > > - rvin_parallel_cleanup(vin);
> > > rvin_group_notifier_cleanup(vin);
> > > rvin_group_put(vin);
> > > rvin_free_controls(vin);
> > > @@ -964,18 +936,11 @@ static int rvin_csi2_init(struct rvin_dev *vin)
> > > if (ret)
> > > goto err_controls;
> > >
> > > - /* It's OK to not have a parallel subdevice. */
> > > - ret = rvin_parallel_init(vin);
> > > - if (ret && ret != -ENODEV)
> > > - goto err_group;
> > > -
> > > ret = rvin_group_notifier_init(vin, 1, RVIN_CSI_MAX);
> > > if (ret)
> > > - goto err_parallel;
> > > + goto err_group;
> > >
> > > return 0;
> > > -err_parallel:
> > > - rvin_parallel_cleanup(vin);
> > > err_group:
> > > rvin_group_put(vin);
> > > err_controls:
> > > @@ -1448,7 +1413,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > > rvin_group_id_to_master(vin->id) == vin->id)
> > > vin->scaler = vin->info->scaler;
> > > } else {
> > > - ret = rvin_parallel_init(vin);
> > > + ret = rvin_group_get(vin, NULL, NULL);
> > > + if (!ret)
> > > + ret = rvin_group_notifier_init(vin, 0, 0);
> > >
> > > if (vin->info->scaler)
> > > vin->scaler = vin->info->scaler;
> > > @@ -1478,8 +1445,6 @@ static void rcar_vin_remove(struct platform_device *pdev)
> > > rvin_isp_cleanup(vin);
> > > else if (vin->info->use_mc)
> > > rvin_csi2_cleanup(vin);
> > > - else
> > > - rvin_parallel_cleanup(vin);
> > >
> > > rvin_id_put(vin);
> > >
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > > index 7d4fce248976..a577f4fe4a6c 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > > @@ -149,7 +149,6 @@ struct rvin_info {
> > > * @vdev: V4L2 video device associated with VIN
> > > * @v4l2_dev: V4L2 device
> > > * @ctrl_handler: V4L2 control handler
> > > - * @notifier: V4L2 asynchronous subdevs notifier
> > > *
> > > * @parallel: parallel input subdevice descriptor
> > > *
> > > @@ -189,7 +188,6 @@ struct rvin_dev {
> > > struct video_device vdev;
> > > struct v4l2_device v4l2_dev;
> > > struct v4l2_ctrl_handler ctrl_handler;
> > > - struct v4l2_async_notifier notifier;
> > >
> > > struct rvin_parallel_entity parallel;
> > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2025-06-07 14:02 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-21 13:20 [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC on Gen2 Niklas Söderlund
2025-05-21 13:20 ` [PATCH v4 1/6] media: rcar-vin: Use correct count of remote subdevices Niklas Söderlund
2025-05-27 11:11 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 2/6] media: rcar-vin: Change link setup argument Niklas Söderlund
2025-05-27 11:19 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 3/6] media: rcar-vin: Generate a VIN group ID for Gen2 Niklas Söderlund
2025-05-27 11:31 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 4/6] media: rcar-vin: Prepare for unifying all v4l-async notifiers Niklas Söderlund
2025-05-27 7:01 ` Sakari Ailus
2025-05-27 11:06 ` Laurent Pinchart
2025-05-28 10:27 ` Sakari Ailus
2025-06-06 13:50 ` Niklas Söderlund
2025-06-07 11:13 ` Sakari Ailus
2025-06-07 11:59 ` Niklas Söderlund
2025-05-27 11:32 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 5/6] media: rcar-vin: Merge all notifiers Niklas Söderlund
2025-05-27 11:45 ` Laurent Pinchart
2025-06-06 14:09 ` Niklas Söderlund
2025-06-07 14:02 ` Laurent Pinchart
2025-05-21 13:20 ` [PATCH v4 6/6] media: rcar-vin: Enable media-graph on Gen2 Niklas Söderlund
2025-05-27 11:48 ` Laurent Pinchart
2025-05-30 12:41 ` [PATCH v4 0/6] media: rcar-vin: Unify notifiers and enable MC " Tomi Valkeinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).