[2/3] media: rcar-vin: Breakout media link creation
Commit Message
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(-)
Comments
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
@@ -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);