Re: [PATCH v9 12/19] media: cadence: csi2rx: add multistream support

[Rishikesh Donadkar]

On 15/01/26 17:31, Tomi Valkeinen wrote:
> Hi,

Hi Tomi,

Thank you for the review !

> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
> > From: Jai Luthra <j-luthra@xxxxxx>
> >
> > Cadence CSI-2 bridge IP supports capturing multiple virtual "streams"
> > of data over the same physical interface using MIPI Virtual Channels.
> >
> > While the hardware IP supports usecases where streams coming in the sink
> > pad can be broadcasted to multiple source pads, the driver will need
> > significant re-architecture to make that possible. The two users of this
> > IP in mainline linux are TI Shim and StarFive JH7110 CAMSS, and both
> > have only integrated the first source pad i.e stream0 of this IP. So for
> > now keep it simple and only allow 1-to-1 mapping of streams from sink to
> > source, without any broadcasting.
> >
> > Signed-off-by: Jai Luthra <j-luthra@xxxxxx>
> > Reviewed-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@xxxxxx>
> > Co-developed-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
> > Signed-off-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
> > ---
> >   drivers/media/platform/cadence/cdns-csi2rx.c | 248 +++++++++++++++----
> >   1 file changed, 201 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 65c6acb02f85b..5c16a2e509136 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -135,6 +135,7 @@ struct csi2rx_priv {
> >   	struct phy			*dphy;
> >   
> >   	u8				num_pixels[CSI2RX_STREAMS_MAX];
> > +	u32				vc_select[CSI2RX_STREAMS_MAX];
> >   	u8				lanes[CSI2RX_LANES_MAX];
> >   	u8				num_lanes;
> >   	u8				max_lanes;
> > @@ -273,30 +274,43 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> >   
> >   static int csi2rx_configure_ext_dphy(struct csi2rx_priv *csi2rx)
> >   {
> > -	struct media_pad *src_pad =
> > -		&csi2rx->source_subdev->entity.pads[csi2rx->source_pad];
> >   	union phy_configure_opts opts = { };
> >   	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
> > -	struct v4l2_subdev_state *state;
> >   	struct v4l2_mbus_framefmt *framefmt;
> > +	struct v4l2_subdev_state *state;
> >   	const struct csi2rx_fmt *fmt;
> > +	int source_pad = csi2rx->source_pad;
> > +	struct media_pad *pad = &csi2rx->source_subdev->entity.pads[source_pad];
> >   	s64 link_freq;
> >   	int ret;
> > +	u32 bpp;
> >   
> >   	state = v4l2_subdev_get_locked_active_state(&csi2rx->subdev);
> >   
> > -	framefmt = v4l2_subdev_state_get_format(state, CSI2RX_PAD_SINK, 0);
> > -	if (!framefmt) {
> > -		dev_err(csi2rx->dev, "Did not find active sink format\n");
> > -		return -EINVAL;
> > -	}
> > +	/*
> > +	 * For multi-stream transmitters there is no single pixel rate.
> > +	 *
> > +	 * In multistream usecase pass bpp as 0 so that v4l2_get_link_freq()
> > +	 * returns an error if it falls back to V4L2_CID_PIXEL_RATE.
> > +	 */
> > +	if (state->routing.num_routes > 1) {
> > +		bpp = 0;
> > +	} else {
> > +		framefmt = v4l2_subdev_state_get_format(state, CSI2RX_PAD_SINK, 0);
> > +		if (!framefmt) {
> > +			dev_err(csi2rx->dev, "Did not find active sink format\n");
> > +			return -EINVAL;
> > +		}
> >   
> > -	fmt = csi2rx_get_fmt_by_code(framefmt->code);
> > +		fmt = csi2rx_get_fmt_by_code(framefmt->code);
> > +		bpp = fmt->bpp;
> > +	}
> >   
> > -	link_freq = v4l2_get_link_freq(src_pad,
> > -				       fmt->bpp, 2 * csi2rx->num_lanes);
> > -	if (link_freq < 0)
> > +	link_freq = v4l2_get_link_freq(pad, bpp, 2 * csi2rx->num_lanes);
> > +	if (link_freq < 0) {
> > +		dev_err(csi2rx->dev, "Unable to calculate link frequency\n");
> >   		return link_freq;
> > +	}
> >   
> >   	ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq,
> >   							 csi2rx->num_lanes, cfg);
> > @@ -394,11 +408,7 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> >   					  csi2rx->num_pixels[i]),
> >   		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> >   
> > -		/*
> > -		 * Enable one virtual channel. When multiple virtual channels
> > -		 * are supported this will have to be changed.
> > -		 */
> > -		writel(CSI2RX_STREAM_DATA_CFG_VC_SELECT(0),
> > +		writel(csi2rx->vc_select[i],
> >   		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
> >   
> >   		writel(CSI2RX_STREAM_CTRL_START,
> > @@ -486,18 +496,59 @@ static int csi2rx_log_status(struct v4l2_subdev *sd)
> >   	return 0;
> >   }
> >   
> > +static void csi2rx_update_vc_select(struct csi2rx_priv *csi2rx,
> > +				    struct v4l2_subdev_state *state)
> > +{
> > +	struct v4l2_mbus_frame_desc fd = {0};
> > +	struct v4l2_subdev_route *route;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/* Capture VC=0 by default */
> > +	for (i = 0; i < CSI2RX_STREAMS_MAX; i++)
> > +		csi2rx->vc_select[i] = CSI2RX_STREAM_DATA_CFG_VC_SELECT(0);
> This should be inside the if-block below, as in the other code path you
> just memset the whole vc_select.

Will do

Rishikesh

> With that fixed:
>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>
>   Tomi