Re: [PATCH 2/3] media: rcar-vin: Breakout media link creation

From: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jacopo Mondi <jacopo@jmondi.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Jacopo Mondi <jacopo+renesas@jmondi.org>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 2/3] media: rcar-vin: Breakout media link creation
Date: Sat, 27 Nov 2021 16:49:41 +0100	[thread overview]
Message-ID: <YaJTlR9DXTxe7tQe@bismarck.dyn.berto.se> (raw)
In-Reply-To: <YYwG8/4qeOuV7cDG@pendragon.ideasonboard.com>

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

-- 
Kind Regards,
Niklas Söderlund