| Message ID | 20211020200225.1956048-3-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: rcar-{csi2,vin}: Move to full Virtual Channel routing per CSI-2 IP | expand |
Hi Niklas On Wed, Oct 20, 2021 at 10:02:24PM +0200, Niklas Söderlund wrote: > In preparation of creating more links to allow for full Virtual Channel > routing within the CSI-2 block break out the link creation logic to a > helper function as the logic will grow in future work. > > There is no functional change. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 38 ++++++++++----------- > 1 file changed, 18 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index bd960c348ba5228c..65ab66a072e9d635 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -909,6 +909,22 @@ static const struct media_device_ops rvin_csi2_media_ops = { > .link_notify = rvin_csi2_link_notify, > }; > > +static int rvin_csi2_add_route(struct rvin_group *group, How about rvin_csi2_create_link() ? > + const struct rvin_group_route *route) > + > +{ > + struct media_entity *source = &group->remotes[route->csi].subdev->entity; > + unsigned int source_idx = rvin_group_csi_channel_to_pad(route->channel); > + struct media_entity *sink = &group->vin[route->vin]->vdev.entity; > + struct media_pad *source_pad = &source->pads[source_idx]; > + struct media_pad *sink_pad = &sink->pads[0]; > + And keep the comment here to re-state that if the linke existed already is not a fatal error Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > + if (media_entity_find_link(source_pad, sink_pad)) > + return 0; > + > + return media_create_pad_link(source, source_idx, sink, 0, 0); > +} > + > static int rvin_csi2_setup_links(struct rvin_dev *vin) > { > const struct rvin_group_route *route; > @@ -917,10 +933,6 @@ static int rvin_csi2_setup_links(struct rvin_dev *vin) > /* Create all media device links between VINs and CSI-2's. */ > mutex_lock(&vin->group->lock); > for (route = vin->info->routes; route->mask; route++) { > - struct media_pad *source_pad, *sink_pad; > - struct media_entity *source, *sink; > - unsigned int source_idx; > - > /* Check that VIN is part of the group. */ > if (!vin->group->vin[route->vin]) > continue; > @@ -933,23 +945,9 @@ static int rvin_csi2_setup_links(struct rvin_dev *vin) > if (!vin->group->remotes[route->csi].subdev) > continue; > > - source = &vin->group->remotes[route->csi].subdev->entity; > - source_idx = rvin_group_csi_channel_to_pad(route->channel); > - source_pad = &source->pads[source_idx]; > - > - sink = &vin->group->vin[route->vin]->vdev.entity; > - sink_pad = &sink->pads[0]; > - > - /* Skip if link already exists. */ > - if (media_entity_find_link(source_pad, sink_pad)) > - continue; > - > - ret = media_create_pad_link(source, source_idx, sink, 0, 0); > - if (ret) { > - vin_err(vin, "Error adding link from %s to %s\n", > - source->name, sink->name); > + ret = rvin_csi2_add_route(vin->group, route); > + if (ret) > break; > - } > } > mutex_unlock(&vin->group->lock); > > -- > 2.33.1 >
Hello, On Thu, Nov 04, 2021 at 05:43:06PM +0100, Jacopo Mondi wrote: > On Wed, Oct 20, 2021 at 10:02:24PM +0200, Niklas Söderlund wrote: > > In preparation of creating more links to allow for full Virtual Channel > > routing within the CSI-2 block break out the link creation logic to a > > helper function as the logic will grow in future work. Are links the right option, should we switch to subdev internal routing configuration ? > > There is no functional change. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 38 ++++++++++----------- > > 1 file changed, 18 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index bd960c348ba5228c..65ab66a072e9d635 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -909,6 +909,22 @@ static const struct media_device_ops rvin_csi2_media_ops = { > > .link_notify = rvin_csi2_link_notify, > > }; > > > > +static int rvin_csi2_add_route(struct rvin_group *group, > > How about rvin_csi2_create_link() ? > > > + const struct rvin_group_route *route) > > + > > +{ > > + struct media_entity *source = &group->remotes[route->csi].subdev->entity; > > + unsigned int source_idx = rvin_group_csi_channel_to_pad(route->channel); > > + struct media_entity *sink = &group->vin[route->vin]->vdev.entity; > > + struct media_pad *source_pad = &source->pads[source_idx]; > > + struct media_pad *sink_pad = &sink->pads[0]; > > + > > And keep the comment here to re-state that if the linke existed > already is not a fatal error > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> With those comments addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + if (media_entity_find_link(source_pad, sink_pad)) > > + return 0; > > + > > + return media_create_pad_link(source, source_idx, sink, 0, 0); > > +} > > + > > static int rvin_csi2_setup_links(struct rvin_dev *vin) > > { > > const struct rvin_group_route *route; > > @@ -917,10 +933,6 @@ static int rvin_csi2_setup_links(struct rvin_dev *vin) > > /* Create all media device links between VINs and CSI-2's. */ > > mutex_lock(&vin->group->lock); > > for (route = vin->info->routes; route->mask; route++) { > > - struct media_pad *source_pad, *sink_pad; > > - struct media_entity *source, *sink; > > - unsigned int source_idx; > > - > > /* Check that VIN is part of the group. */ > > if (!vin->group->vin[route->vin]) > > continue; > > @@ -933,23 +945,9 @@ static int rvin_csi2_setup_links(struct rvin_dev *vin) > > if (!vin->group->remotes[route->csi].subdev) > > continue; > > > > - source = &vin->group->remotes[route->csi].subdev->entity; > > - source_idx = rvin_group_csi_channel_to_pad(route->channel); > > - source_pad = &source->pads[source_idx]; > > - > > - sink = &vin->group->vin[route->vin]->vdev.entity; > > - sink_pad = &sink->pads[0]; > > - > > - /* Skip if link already exists. */ > > - if (media_entity_find_link(source_pad, sink_pad)) > > - continue; > > - > > - ret = media_create_pad_link(source, source_idx, sink, 0, 0); > > - if (ret) { > > - vin_err(vin, "Error adding link from %s to %s\n", > > - source->name, sink->name); > > + ret = rvin_csi2_add_route(vin->group, route); > > + if (ret) > > break; > > - } > > } > > mutex_unlock(&vin->group->lock); > >
Hello Laurent, Thanks for your review. On 2021-11-10 19:52:51 +0200, Laurent Pinchart wrote: > Hello, > > On Thu, Nov 04, 2021 at 05:43:06PM +0100, Jacopo Mondi wrote: > > On Wed, Oct 20, 2021 at 10:02:24PM +0200, Niklas Söderlund wrote: > > > In preparation of creating more links to allow for full Virtual Channel > > > routing within the CSI-2 block break out the link creation logic to a > > > helper function as the logic will grow in future work. > > Are links the right option, should we switch to subdev internal routing > configuration ? That is an interesting question I thought about it but decided against it, at lest for now. The design we have is that each source pad of the R-Car CSI-2 subdevice is fixed to a specific VC (source pad 0 -> VC0, source pad 1 - > VC1, etc). And with this patch we preserve this behavior. Once we have the internal routing and multiplexed stream API upstream we can evolve this and still keep the API consistent. As a first step we expose the internal routing true the new API, read-only as that how it is implemented today and then on-top of that we can decide if we want to make it configurable from user-space, or not. > > > > There is no functional change. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > drivers/media/platform/rcar-vin/rcar-core.c | 38 ++++++++++----------- > > > 1 file changed, 18 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > > index bd960c348ba5228c..65ab66a072e9d635 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > > @@ -909,6 +909,22 @@ static const struct media_device_ops rvin_csi2_media_ops = { > > > .link_notify = rvin_csi2_link_notify, > > > }; > > > > > > +static int rvin_csi2_add_route(struct rvin_group *group, > > > > How about rvin_csi2_create_link() ? > > > > > + const struct rvin_group_route *route) > > > + > > > +{ > > > + struct media_entity *source = &group->remotes[route->csi].subdev->entity; > > > + unsigned int source_idx = rvin_group_csi_channel_to_pad(route->channel); > > > + struct media_entity *sink = &group->vin[route->vin]->vdev.entity; > > > + struct media_pad *source_pad = &source->pads[source_idx]; > > > + struct media_pad *sink_pad = &sink->pads[0]; > > > + > > > > And keep the comment here to re-state that if the linke existed > > already is not a fatal error > > > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > With those comments addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > + if (media_entity_find_link(source_pad, sink_pad)) > > > + return 0; > > > + > > > + return media_create_pad_link(source, source_idx, sink, 0, 0); > > > +} > > > + > > > static int rvin_csi2_setup_links(struct rvin_dev *vin) > > > { > > > const struct rvin_group_route *route; > > > @@ -917,10 +933,6 @@ static int rvin_csi2_setup_links(struct rvin_dev *vin) > > > /* Create all media device links between VINs and CSI-2's. */ > > > mutex_lock(&vin->group->lock); > > > for (route = vin->info->routes; route->mask; route++) { > > > - struct media_pad *source_pad, *sink_pad; > > > - struct media_entity *source, *sink; > > > - unsigned int source_idx; > > > - > > > /* Check that VIN is part of the group. */ > > > if (!vin->group->vin[route->vin]) > > > continue; > > > @@ -933,23 +945,9 @@ static int rvin_csi2_setup_links(struct rvin_dev *vin) > > > if (!vin->group->remotes[route->csi].subdev) > > > continue; > > > > > > - source = &vin->group->remotes[route->csi].subdev->entity; > > > - source_idx = rvin_group_csi_channel_to_pad(route->channel); > > > - source_pad = &source->pads[source_idx]; > > > - > > > - sink = &vin->group->vin[route->vin]->vdev.entity; > > > - sink_pad = &sink->pads[0]; > > > - > > > - /* Skip if link already exists. */ > > > - if (media_entity_find_link(source_pad, sink_pad)) > > > - continue; > > > - > > > - ret = media_create_pad_link(source, source_idx, sink, 0, 0); > > > - if (ret) { > > > - vin_err(vin, "Error adding link from %s to %s\n", > > > - source->name, sink->name); > > > + ret = rvin_csi2_add_route(vin->group, route); > > > + if (ret) > > > break; > > > - } > > > } > > > mutex_unlock(&vin->group->lock); > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index bd960c348ba5228c..65ab66a072e9d635 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -909,6 +909,22 @@ static const struct media_device_ops rvin_csi2_media_ops = { .link_notify = rvin_csi2_link_notify, }; +static int rvin_csi2_add_route(struct rvin_group *group, + const struct rvin_group_route *route) + +{ + struct media_entity *source = &group->remotes[route->csi].subdev->entity; + unsigned int source_idx = rvin_group_csi_channel_to_pad(route->channel); + struct media_entity *sink = &group->vin[route->vin]->vdev.entity; + struct media_pad *source_pad = &source->pads[source_idx]; + struct media_pad *sink_pad = &sink->pads[0]; + + if (media_entity_find_link(source_pad, sink_pad)) + return 0; + + return media_create_pad_link(source, source_idx, sink, 0, 0); +} + static int rvin_csi2_setup_links(struct rvin_dev *vin) { const struct rvin_group_route *route; @@ -917,10 +933,6 @@ static int rvin_csi2_setup_links(struct rvin_dev *vin) /* Create all media device links between VINs and CSI-2's. */ mutex_lock(&vin->group->lock); for (route = vin->info->routes; route->mask; route++) { - struct media_pad *source_pad, *sink_pad; - struct media_entity *source, *sink; - unsigned int source_idx; - /* Check that VIN is part of the group. */ if (!vin->group->vin[route->vin]) continue; @@ -933,23 +945,9 @@ static int rvin_csi2_setup_links(struct rvin_dev *vin) if (!vin->group->remotes[route->csi].subdev) continue; - source = &vin->group->remotes[route->csi].subdev->entity; - source_idx = rvin_group_csi_channel_to_pad(route->channel); - source_pad = &source->pads[source_idx]; - - sink = &vin->group->vin[route->vin]->vdev.entity; - sink_pad = &sink->pads[0]; - - /* Skip if link already exists. */ - if (media_entity_find_link(source_pad, sink_pad)) - continue; - - ret = media_create_pad_link(source, source_idx, sink, 0, 0); - if (ret) { - vin_err(vin, "Error adding link from %s to %s\n", - source->name, sink->name); + ret = rvin_csi2_add_route(vin->group, route); + if (ret) break; - } } mutex_unlock(&vin->group->lock);
In preparation of creating more links to allow for full Virtual Channel routing within the CSI-2 block break out the link creation logic to a helper function as the logic will grow in future work. There is no functional change. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-core.c | 38 ++++++++++----------- 1 file changed, 18 insertions(+), 20 deletions(-)