Re: [PATCH v12 11/17] media: cadence: csi2rx: add multistream support

[Rishikesh Donadkar <r-donadkar@ti.com>]

On 19/05/26 17:35, Sakari Ailus wrote:
> Hi Rishikesh,


Hi Sakari,

Thank you for the review !

>
> On Fri, Mar 13, 2026 at 02:36:55PM +0530, Rishikesh Donadkar wrote:
>> From: Jai Luthra <j-luthra@ti.com>
>>
>> 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@ti.com>
>> Reviewed-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Co-developed-by: Rishikesh Donadkar <r-donadkar@ti.com>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
>> ---
>>   drivers/media/platform/cadence/cdns-csi2rx.c | 279 +++++++++++++++----
>>   1 file changed, 227 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
>> index 52ca940acd7d0..f34df341a2cac 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,46 @@ 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;
>> +	struct v4l2_subdev_route *route;
>> +	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 {
>> +		route = &state->routing.routes[0];
>> +		framefmt = v4l2_subdev_state_get_format(state, CSI2RX_PAD_SINK,
>> +							route->sink_stream);
>> +		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 +411,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 +499,57 @@ 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;
>> +
>> +	ret = csi2rx_get_frame_desc_from_source(csi2rx, &fd);
>> +	if (ret || fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
>> +		dev_dbg(csi2rx->dev,
>> +			"Failed to get source frame desc, allowing only VC=0\n");
>> +		for (i = 0; i < CSI2RX_STREAMS_MAX; i++)
>> +			csi2rx->vc_select[i] = CSI2RX_STREAM_DATA_CFG_VC_SELECT(0);
>> +		return;
>> +	}
>> +
>> +	/* If source provides per-stream VC info, use it to filter by VC */
>> +	memset(csi2rx->vc_select, 0, sizeof(csi2rx->vc_select));
>> +
>> +	for_each_active_route(&state->routing, route) {
>> +		u32 cdns_stream = route->source_pad - CSI2RX_PAD_SOURCE_STREAM0;
>> +
>> +		for (i = 0; i < fd.num_entries; i++) {
>> +			if (fd.entry[i].stream != route->sink_stream)
>> +				continue;
>> +
>> +			csi2rx->vc_select[cdns_stream] |=
>> +				CSI2RX_STREAM_DATA_CFG_VC_SELECT(fd.entry[i].bus.csi2.vc);
>> +		}
>> +	}
>> +}
>> +
>>   static int csi2rx_enable_streams(struct v4l2_subdev *subdev,
>>   				 struct v4l2_subdev_state *state, u32 pad,
>>   				 u64 streams_mask)
>>   {
>>   	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> +	u64 sink_streams;
>>   	int ret;
>>   
>> +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad,
>> +						       CSI2RX_PAD_SINK,
>> +						       &streams_mask);
>> +
>>   	/*
>>   	 * If we're not the first users, there's no need to
>>   	 * enable the whole controller.
>>   	 */
>>   	if (!csi2rx->count) {
>> +		csi2rx_update_vc_select(csi2rx, state);
>>   		ret = csi2rx_start(csi2rx);
>>   		if (ret)
>>   			return ret;
>> @@ -505,10 +557,11 @@ static int csi2rx_enable_streams(struct v4l2_subdev *subdev,
>>   
>>   	/* Start streaming on the source */
>>   	ret = v4l2_subdev_enable_streams(csi2rx->source_subdev, csi2rx->source_pad,
>> -					 BIT_U64(0));
>> +					 sink_streams);
>>   	if (ret) {
>>   		dev_err(csi2rx->dev,
>> -			"Failed to start streams %d on subdev\n", 0);
>> +			"Failed to start streams %#llx on subdev\n",
>> +			sink_streams);
>>   		if (!csi2rx->count)
>>   			csi2rx_stop(csi2rx);
>>   		return ret;
>> @@ -523,9 +576,14 @@ static int csi2rx_disable_streams(struct v4l2_subdev *subdev,
>>   				  u64 streams_mask)
>>   {
>>   	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> +	u64 sink_streams;
>> +
>> +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad,
>> +						       CSI2RX_PAD_SINK,
>> +						       &streams_mask);
>>   
>>   	if (v4l2_subdev_disable_streams(csi2rx->source_subdev,
>> -					csi2rx->source_pad, BIT_U64(0))) {
>> +						 csi2rx->source_pad, sink_streams)) {
>>   		dev_err(csi2rx->dev, "Couldn't disable our subdev\n");
>>   	}
>>   
>> @@ -550,12 +608,56 @@ static int csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
>>   	return 0;
>>   }
>>   
>> +static int _csi2rx_set_routing(struct v4l2_subdev *subdev,
>> +			       struct v4l2_subdev_state *state,
>> +			       struct v4l2_subdev_krouting *routing)
>> +{
>> +	static const struct v4l2_mbus_framefmt format = {
>> +		.width = 640,
>> +		.height = 480,
>> +		.code = MEDIA_BUS_FMT_UYVY8_1X16,
>> +		.field = V4L2_FIELD_NONE,
>> +		.colorspace = V4L2_COLORSPACE_SRGB,
>> +		.ycbcr_enc = V4L2_YCBCR_ENC_601,
>> +		.quantization = V4L2_QUANTIZATION_LIM_RANGE,
>> +		.xfer_func = V4L2_XFER_FUNC_SRGB,
>> +	};
>> +	int ret;
>> +
>> +	if (routing->num_routes > V4L2_FRAME_DESC_ENTRY_MAX)
>> +		return -EINVAL;
> Is there a need for this check? The framework already checks the number of
> active routes isn't larger than V4L2_FRAME_DESC_ENTRY_MAX.


Thank you for pointing out, will remove this check.


>
>> +
>> +	ret = v4l2_subdev_routing_validate(subdev, routing,
>> +					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return v4l2_subdev_set_routing_with_fmt(subdev, state, routing, &format);
>> +}
>> +
>> +static int csi2rx_set_routing(struct v4l2_subdev *subdev,
>> +			      struct v4l2_subdev_state *state,
>> +			      enum v4l2_subdev_format_whence which,
>> +			      struct v4l2_subdev_krouting *routing)
>> +{
>> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> +	int ret;
>> +
>> +	if (which == V4L2_SUBDEV_FORMAT_ACTIVE && csi2rx->count)
>> +		return -EBUSY;
>> +
>> +	ret = _csi2rx_set_routing(subdev, state, routing);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>>   static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
>>   			  struct v4l2_subdev_state *state,
>>   			  struct v4l2_subdev_format *format)
>>   {
>>   	struct v4l2_mbus_framefmt *fmt;
>> -	unsigned int i;
>>   
>>   	/* No transcoding, source and sink formats must match. */
>>   	if (format->pad != CSI2RX_PAD_SINK)
>> @@ -567,14 +669,16 @@ static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
>>   	format->format.field = V4L2_FIELD_NONE;
>>   
>>   	/* Set sink format */
>> -	fmt = v4l2_subdev_state_get_format(state, format->pad);
>> +	fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream);
>>   	*fmt = format->format;
>>   
>> -	/* Propagate to source formats */
>> -	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) {
>> -		fmt = v4l2_subdev_state_get_format(state, i);
>> -		*fmt = format->format;
>> -	}
>> +	/* Propagate to source format */
>> +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
>> +							   format->stream);
>> +	if (!fmt)
>> +		return -EINVAL;
>> +
>> +	*fmt = format->format;
>>   
>>   	return 0;
>>   }
>> @@ -582,21 +686,22 @@ static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
>>   static int csi2rx_init_state(struct v4l2_subdev *subdev,
>>   			     struct v4l2_subdev_state *state)
>>   {
>> -	struct v4l2_subdev_format format = {
>> -		.pad = CSI2RX_PAD_SINK,
>> -		.format = {
>> -			.width = 640,
>> -			.height = 480,
>> -			.code = MEDIA_BUS_FMT_UYVY8_1X16,
>> -			.field = V4L2_FIELD_NONE,
>> -			.colorspace = V4L2_COLORSPACE_SRGB,
>> -			.ycbcr_enc = V4L2_YCBCR_ENC_601,
>> -			.quantization = V4L2_QUANTIZATION_LIM_RANGE,
>> -			.xfer_func = V4L2_XFER_FUNC_SRGB,
>> +	struct v4l2_subdev_route routes[] = {
>> +		{
>> +			.sink_pad = CSI2RX_PAD_SINK,
>> +			.sink_stream = 0,
>> +			.source_pad = CSI2RX_PAD_SOURCE_STREAM0,
>> +			.source_stream = 0,
>> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
>>   		},
>>   	};
>>   
>> -	return csi2rx_set_fmt(subdev, state, &format);
>> +	struct v4l2_subdev_krouting routing = {
>> +		.num_routes = ARRAY_SIZE(routes),
>> +		.routes = routes,
>> +	};
>> +
>> +	return _csi2rx_set_routing(subdev, state, &routing);
>>   }
>>   
>>   int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
>> @@ -604,25 +709,44 @@ int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
>>   {
>>   	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>>   	const struct csi2rx_fmt *csi_fmt;
>> +	struct v4l2_subdev_route *route;
>>   	struct v4l2_subdev_state *state;
>>   	struct v4l2_mbus_framefmt *fmt;
>> +	int ret = 0;
>>   
>>   	if (!ppc || pad < CSI2RX_PAD_SOURCE_STREAM0 || pad >= CSI2RX_PAD_MAX)
>>   		return -EINVAL;
>>   
>>   	state = v4l2_subdev_lock_and_get_active_state(subdev);
>> -	fmt = v4l2_subdev_state_get_format(state, pad);
>> -	csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
>> +	/* Check all streams on requested pad */
>> +	for_each_active_route(&state->routing, route) {
>> +		if (route->source_pad != pad)
>> +			continue;
>> +
>> +		fmt = v4l2_subdev_state_get_format(state, route->source_pad,
>> +						   route->source_stream);
>> +		if (!fmt) {
>> +			ret = -EPIPE;
>> +			*ppc = 1;
>> +			break;
>> +		}
>>   
>> -	/* Reduce requested PPC if it is too high */
>> -	*ppc = min(*ppc, csi_fmt->max_pixels);
>> +		csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
>> +		if (!csi_fmt) {
>> +			ret = -EINVAL;
>> +			*ppc = 1;
>> +			break;
>> +		}
>>   
>> +		/* Reduce requested PPC if it is too high for this stream */
>> +		*ppc = min(*ppc, csi_fmt->max_pixels);
>> +	}
>>   	v4l2_subdev_unlock_state(state);
>>   
>>   	csi2rx->num_pixels[pad - CSI2RX_PAD_SOURCE_STREAM0] =
>>   		CSI2RX_STREAM_CFG_NUM_PIXELS(*ppc);
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL_FOR_MODULES(cdns_csi2rx_negotiate_ppc, "j721e-csi2rx");
>>   
>> @@ -630,17 +754,66 @@ static int csi2rx_get_frame_desc(struct v4l2_subdev *subdev, unsigned int pad,
>>   				 struct v4l2_mbus_frame_desc *fd)
>>   {
>>   	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> +	struct v4l2_mbus_frame_desc source_fd = {0};
>> +	struct v4l2_subdev_route *route;
>> +	struct v4l2_subdev_state *state;
>> +	int ret;
>> +
>> +	ret = csi2rx_get_frame_desc_from_source(csi2rx, &source_fd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
>> +
>> +	state = v4l2_subdev_lock_and_get_active_state(subdev);
>> +
>> +	for_each_active_route(&state->routing, route) {
>> +		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
>> +		unsigned int i;
>> +
>> +		if (route->source_pad != pad)
>> +			continue;
>> +
>> +		for (i = 0; i < source_fd.num_entries; i++) {
>> +			if (source_fd.entry[i].stream == route->sink_stream) {
>> +				source_entry = &source_fd.entry[i];
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (!source_entry) {
>> +			dev_err(csi2rx->dev,
>> +				"Failed to find stream from source frame desc\n");
>> +			ret = -EPIPE;
>> +			goto err_missing_stream;
>> +		}
>> +
>> +		fd->entry[fd->num_entries].stream = route->source_stream;
>> +		fd->entry[fd->num_entries].flags = source_entry->flags;
>> +		fd->entry[fd->num_entries].length = source_entry->length;
>> +		fd->entry[fd->num_entries].pixelcode = source_entry->pixelcode;
>> +		fd->entry[fd->num_entries].bus.csi2.vc =
>> +			source_entry->bus.csi2.vc;
>> +		fd->entry[fd->num_entries].bus.csi2.dt =
>> +			source_entry->bus.csi2.dt;
>>   
>> -	return csi2rx_get_frame_desc_from_source(csi2rx, fd);
>> +		fd->num_entries++;
> Would v4l2_subdev_get_frame_desc_passthrough() be useful here?


Yes, will use v4l2_subdev_get_frame_desc_passthrough() helper instead of 
this.


Rishikesh

>
>> +	}
>> +
>> +err_missing_stream:
>> +	v4l2_subdev_unlock_state(state);
>> +
>> +	return ret;
>>   }
>>   
>>   static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
>> -	.enum_mbus_code	= csi2rx_enum_mbus_code,
>> -	.get_fmt	= v4l2_subdev_get_fmt,
>> -	.set_fmt	= csi2rx_set_fmt,
>> -	.enable_streams         = csi2rx_enable_streams,
>> -	.disable_streams        = csi2rx_disable_streams,
>> -	.get_frame_desc	= csi2rx_get_frame_desc,
>> +	.enum_mbus_code		= csi2rx_enum_mbus_code,
>> +	.get_fmt		= v4l2_subdev_get_fmt,
>> +	.set_fmt		= csi2rx_set_fmt,
>> +	.get_frame_desc		= csi2rx_get_frame_desc,
>> +	.set_routing		= csi2rx_set_routing,
>> +	.enable_streams		= csi2rx_enable_streams,
>> +	.disable_streams	= csi2rx_disable_streams,
>>   };
>>   
>>   static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
>> @@ -659,6 +832,7 @@ static const struct v4l2_subdev_internal_ops csi2rx_internal_ops = {
>>   static const struct media_entity_operations csi2rx_media_ops = {
>>   	.link_validate = v4l2_subdev_link_validate,
>>   	.get_fwnode_pad = v4l2_subdev_get_fwnode_pad_1_to_1,
>> +	.has_pad_interdep = v4l2_subdev_has_pad_interdep,
>>   };
>>   
>>   static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
>> @@ -876,7 +1050,8 @@ static int csi2rx_probe(struct platform_device *pdev)
>>   	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>   	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)
>>   		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
>> -	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	csi2rx->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE |
>> +		V4L2_SUBDEV_FL_STREAMS;
>>   	csi2rx->subdev.entity.ops = &csi2rx_media_ops;
>>   
>>   	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,