Message-ID: <1e7be0c3-b7da-4eac-9a39-147c1e627cbb@ideasonboard.com>
Date: Mon, 1 Dec 2025 15:03:42 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Rishikesh Donadkar <r-donadkar@...com>, jai.luthra@...ux.dev,
 laurent.pinchart@...asonboard.com, mripard@...nel.org
Cc: y-abhilashchandra@...com, devarsht@...com, s-jain1@...com,
 vigneshr@...com, mchehab@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
 p.zabel@...gutronix.de, conor+dt@...nel.org, sakari.ailus@...ux.intel.com,
 hverkuil-cisco@...all.nl, jai.luthra@...asonboard.com,
 changhuang.liang@...rfivetech.com, jack.zhu@...rfivetech.com,
 sjoerd@...labora.com, dan.carpenter@...aro.org, hverkuil+cisco@...nel.org,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v8 13/18] media: ti: j721e-csi2rx: add multistream support

Hi,

On 12/11/2025 13:54, Rishikesh Donadkar wrote:
> From: Jai Luthra <j-luthra@...com>
> 
> Each CSI2 stream can be multiplexed into 4 independent streams, each

Well, that's not true, at least generally speaking (there can be more
than 4). Is that specific to TI hardware?

> identified by its virtual channel number and data type. The incoming
> data from these streams can be filtered on the basis of either the
> virtual channel or the data type.
> 
> To capture this multiplexed stream, the application needs to tell
> the driver how it wants to route the data. It needs to specify
> which context should process which stream. This is done via the
> new routing APIs.
> 
> Add ioctls to accept routing information from the application and save
> that in the driver. This can be used when starting streaming on a
> context to determine which route and consequently which virtual channel
> it should process.
> 
> De-assert the pixel interface reset on first start_streaming() and assert
> it on the last stop_streaming().
> 
> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@...com>
> Co-developed-by: Pratyush Yadav <p.yadav@...com>
> Signed-off-by: Pratyush Yadav <p.yadav@...com>
> Signed-off-by: Jai Luthra <j-luthra@...com>
> Co-developed-by: Rishikesh Donadkar <r-donadkar@...com>
> Signed-off-by: Rishikesh Donadkar <r-donadkar@...com>
> ---
>  .../platform/ti/j721e-csi2rx/j721e-csi2rx.c   | 224 ++++++++++++++----
>  1 file changed, 179 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index 126a62fa2c4c4..c8e0e761f2802 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> @@ -103,6 +103,7 @@ struct ti_csi2rx_dev;
>  
>  struct ti_csi2rx_ctx {
>  	struct ti_csi2rx_dev		*csi;
> +	struct v4l2_subdev_route        *route;
>  	struct video_device		vdev;
>  	struct vb2_queue		vidq;
>  	struct mutex			mutex; /* To serialize ioctls. */
> @@ -136,6 +137,7 @@ struct ti_csi2rx_dev {
>  		dma_addr_t		paddr;
>  		size_t			len;
>  	} drain;
> +	bool                            vc_cached;
>  };
>  
>  static inline struct ti_csi2rx_dev *to_csi2rx_dev(struct v4l2_subdev *sd)
> @@ -143,17 +145,6 @@ static inline struct ti_csi2rx_dev *to_csi2rx_dev(struct v4l2_subdev *sd)
>  	return container_of(sd, struct ti_csi2rx_dev, subdev);
>  }
>  
> -static const struct v4l2_mbus_framefmt ti_csi2rx_default_fmt = {
> -	.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,
> -};
> -
>  static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = {
>  	{
>  		.fourcc			= V4L2_PIX_FMT_YUYV,
> @@ -566,8 +557,10 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
>  	fmt = find_format_by_fourcc(ctx->v_fmt.fmt.pix.pixelformat);
>  
>  	/* De-assert the pixel interface reset. */
> -	reg = SHIM_CNTL_PIX_RST;
> -	writel(reg, csi->shim + SHIM_CNTL);
> +	if (!csi->enable_count) {
> +		reg = SHIM_CNTL_PIX_RST;
> +		writel(reg, csi->shim + SHIM_CNTL);
> +	}
>  
>  	/* Negotiate pixel count from the source */
>  	ti_csi2rx_request_max_ppc(csi);
> @@ -888,30 +881,80 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb)
>  	}
>  }
>  
> +static int ti_csi2rx_get_route(struct ti_csi2rx_ctx *ctx)
> +{
> +	struct ti_csi2rx_dev *csi = ctx->csi;
> +	struct media_pad *pad;
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_subdev_route *r;
> +
> +	/* Get the source pad connected to this ctx */
> +	pad = media_entity_remote_source_pad_unique(ctx->pad.entity);
> +	if (!pad) {
> +		dev_err(csi->dev, "No pad connected to ctx %d\n", ctx->idx);
> +		return -ENODEV;
> +	}
> +
> +	state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);

This looks a bit concerning. You lock the state, find the correct route
and store a pointer to the route into the ctx, then unlock the state...
Alarm bells should be ringing here.

> +
> +	for_each_active_route(&state->routing, r) {
> +		if (!(r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))

for_each_active_route() already checks this ("_active_").

> +			continue;
> +		if (r->source_pad != pad->index)
> +			continue;
> +
> +		ctx->route = r;

Shouldn't you break here?

> +	}
> +
> +	v4l2_subdev_unlock_state(state);
> +
> +	if (!ctx->route)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
>  static int ti_csi2rx_get_vc(struct ti_csi2rx_ctx *ctx)
>  {
>  	struct ti_csi2rx_dev *csi = ctx->csi;
> +	struct ti_csi2rx_ctx *curr_ctx;
>  	struct v4l2_mbus_frame_desc fd;
> -	struct media_pad *pad;
> -	int ret, i;
> +	struct media_pad *source_pad;
> +	struct v4l2_subdev_route *curr_route;
> +	int ret;
> +	unsigned int i, j;
>  
> -	pad = media_entity_remote_pad_unique(&csi->subdev.entity, MEDIA_PAD_FL_SOURCE);
> -	if (!pad)
> +	/* Get the frame desc form source */
> +	source_pad = media_entity_remote_pad_unique(&csi->subdev.entity, MEDIA_PAD_FL_SOURCE);
> +	if (!source_pad)
>  		return -ENODEV;
>  
> -	ret = v4l2_subdev_call(csi->source, pad, get_frame_desc, pad->index, &fd);
> +	ret = v4l2_subdev_call(csi->source, pad, get_frame_desc, source_pad->index, &fd);
>  	if (ret)
>  		return ret;
>  
>  	if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
>  		return -EINVAL;
>  
> -	for (i = 0; i < fd.num_entries; i++) {
> -		if (ctx->stream == fd.entry[i].stream)
> -			return fd.entry[i].bus.csi2.vc;
> +	for (i = 0; i < csi->num_ctx; i++) {
> +		curr_ctx = &csi->ctx[i];
> +
> +		/* Capture VC 0 by default */
> +		curr_ctx->vc = 0;
> +
> +		ret = ti_csi2rx_get_route(curr_ctx);
> +		if (ret)
> +			continue;
> +
> +		curr_route = curr_ctx->route;
> +		curr_ctx->stream = curr_route->sink_stream;
> +
> +		for (j = 0; j < fd.num_entries; j++)
> +			if (curr_ctx->stream == fd.entry[j].stream)
> +				curr_ctx->vc = fd.entry[j].bus.csi2.vc;
>  	}
>  
> -	return -ENODEV;
> +	return 0;
>  }
>  
>  static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
> @@ -934,13 +977,24 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (ret)
>  		goto err;
>  
> -	ret = ti_csi2rx_get_vc(ctx);
> -	if (ret == -ENOIOCTLCMD)
> -		ctx->vc = 0;
> -	else if (ret < 0)
> +	/* If no stream is routed to this ctx, exit early */
> +	ret = ti_csi2rx_get_route(ctx);
> +	if (ret)
>  		goto err;
> -	else
> -		ctx->vc = ret;
> +
> +	/* Get the VC for all enabled ctx on first stream start */

Is the comment right? How does this work? ti_csi2rx_get_vc() is passed a
single context, but... it gets VCs for all contexts? And shouldn't these
also be dealing with DT at the same time with VC?

> +	mutex_lock(&csi->mutex);
> +	if (!csi->vc_cached) {
> +		ret = ti_csi2rx_get_vc(ctx);
> +		if (ret == -ENOIOCTLCMD) {
> +			ctx->vc = 0;
> +		} else if (ret < 0) {
> +			mutex_unlock(&csi->mutex);
> +			goto err;
> +		}
> +		csi->vc_cached = true;
> +	}
> +	mutex_unlock(&csi->mutex);
>  
>  	ti_csi2rx_setup_shim(ctx);
>  
> @@ -960,8 +1014,9 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	dma->state = TI_CSI2RX_DMA_ACTIVE;
>  	spin_unlock_irqrestore(&dma->lock, flags);
>  
> +	/* Start stream 0, we don't allow multiple streams on the source pad */
>  	ret = v4l2_subdev_enable_streams(&csi->subdev,
> -					 TI_CSI2RX_PAD_FIRST_SOURCE,
> +					 TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx,
>  					 BIT(0));
>  	if (ret)
>  		goto err_dma;
> @@ -985,17 +1040,26 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq)
>  	struct ti_csi2rx_dev *csi = ctx->csi;
>  	int ret;
>  
> -	video_device_pipeline_stop(&ctx->vdev);
> +	mutex_lock(&csi->mutex);
>  
> -	writel(0, csi->shim + SHIM_CNTL);
>  	writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
>  
> +	/* assert pixel reset to prevent stale data */
> +	if (csi->enable_count == 1) {
> +		writel(0, csi->shim + SHIM_CNTL);
> +		csi->vc_cached = false;
> +	}
> +
> +	video_device_pipeline_stop(&ctx->vdev);
> +
>  	ret = v4l2_subdev_disable_streams(&csi->subdev,
> -					  TI_CSI2RX_PAD_FIRST_SOURCE,
> +					  TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx,
>  					  BIT(0));
>  	if (ret)
>  		dev_err(csi->dev, "Failed to stop subdev stream\n");
>  
> +	mutex_unlock(&csi->mutex);
> +
>  	ti_csi2rx_stop_dma(ctx);
>  	ti_csi2rx_cleanup_buffers(ctx, VB2_BUF_STATE_ERROR);
>  }
> @@ -1038,25 +1102,84 @@ static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd,
>  	fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream);
>  	*fmt = format->format;
>  
> -	fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE,
> -					   format->stream);
> +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> +							   format->stream);
> +	if (!fmt)
> +		return -EINVAL;
> +
>  	*fmt = format->format;
>  
>  	return 0;
>  }
>  
> -static int ti_csi2rx_sd_init_state(struct v4l2_subdev *sd,
> -				   struct v4l2_subdev_state *state)
> +static int _ti_csi2rx_sd_set_routing(struct v4l2_subdev *sd,
> +				     struct v4l2_subdev_state *state,
> +				     struct v4l2_subdev_krouting *routing)
>  {
> -	struct v4l2_mbus_framefmt *fmt;
> +	int ret;
>  
> -	fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_SINK);
> -	*fmt = ti_csi2rx_default_fmt;
> +	const struct v4l2_mbus_framefmt format = {

"static const"

> +		.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,
> +	};
>  
> -	fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE);
> -	*fmt = ti_csi2rx_default_fmt;
> +	ret = v4l2_subdev_routing_validate(sd, routing,
> +					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1 |
> +					   V4L2_SUBDEV_ROUTING_NO_SOURCE_MULTIPLEXING);
>  
> -	return 0;
> +	if (ret)
> +		return ret;
> +
> +	/* Only stream ID 0 allowed on source pads */
> +	for (unsigned int i = 0; i < routing->num_routes; ++i) {
> +		const struct v4l2_subdev_route *route = &routing->routes[i];
> +
> +		if (route->source_stream != 0)
> +			return -EINVAL;
> +	}
> +
> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format);
> +
> +	return ret;
> +}
> +
> +static int ti_csi2rx_sd_set_routing(struct v4l2_subdev *sd,
> +				    struct v4l2_subdev_state *state,
> +				    enum v4l2_subdev_format_whence which,
> +				    struct v4l2_subdev_krouting *routing)
> +{
> +	struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
> +
> +	if (csi->enable_count > 0)
> +		return -EBUSY;
> +
> +	return _ti_csi2rx_sd_set_routing(sd, state, routing);
> +}
> +
> +static int ti_csi2rx_sd_init_state(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_subdev_route routes[] = { {
> +		.sink_pad = 0,
> +		.sink_stream = 0,
> +		.source_pad = TI_CSI2RX_PAD_FIRST_SOURCE,
> +		.source_stream = 0,
> +		.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +	} };
> +
> +	struct v4l2_subdev_krouting routing = {
> +		.num_routes = 1,
> +		.routes = routes,
> +	};
> +
> +	/* Initialize routing to single route to the fist source pad */
> +	return _ti_csi2rx_sd_set_routing(sd, state, &routing);
>  }
>  
>  static int ti_csi2rx_sd_enable_streams(struct v4l2_subdev *sd,
> @@ -1065,14 +1188,18 @@ static int ti_csi2rx_sd_enable_streams(struct v4l2_subdev *sd,
>  {
>  	struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
>  	struct media_pad *remote_pad;
> +	u64 sink_streams;
>  	int ret = 0;
>  
>  	remote_pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
>  	if (!remote_pad)
>  		return -ENODEV;
> +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad,
> +						       TI_CSI2RX_PAD_SINK,
> +						       &streams_mask);
>  
>  	ret = v4l2_subdev_enable_streams(csi->source, remote_pad->index,
> -					 BIT(0));
> +					 sink_streams);
>  	if (ret)
>  		return ret;
>  
> @@ -1087,17 +1214,21 @@ static int ti_csi2rx_sd_disable_streams(struct v4l2_subdev *sd,
>  {
>  	struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
>  	struct media_pad *remote_pad;
> +	u64 sink_streams;
>  	int ret = 0;
>  
>  	remote_pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
>  	if (!remote_pad)
>  		return -ENODEV;
> +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad,
> +						       TI_CSI2RX_PAD_SINK,
> +						       &streams_mask);
>  
>  	if (csi->enable_count == 0)
>  		return -EINVAL;
>  
>  	ret = v4l2_subdev_disable_streams(csi->source, remote_pad->index,
> -					  BIT(0));
> +					  sink_streams);
>  	if (!ret)
>  		--csi->enable_count;
>  
> @@ -1106,6 +1237,7 @@ static int ti_csi2rx_sd_disable_streams(struct v4l2_subdev *sd,
>  
>  static const struct v4l2_subdev_pad_ops ti_csi2rx_subdev_pad_ops = {
>  	.enum_mbus_code	= ti_csi2rx_enum_mbus_code,
> +	.set_routing = ti_csi2rx_sd_set_routing,
>  	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = ti_csi2rx_sd_set_fmt,
>  	.enable_streams = ti_csi2rx_sd_enable_streams,
> @@ -1284,7 +1416,7 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
>  	v4l2_subdev_init(sd, &ti_csi2rx_subdev_ops);
>  	sd->internal_ops = &ti_csi2rx_internal_ops;
>  	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> -	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
>  	strscpy(sd->name, dev_name(csi->dev), sizeof(sd->name));
>  	sd->dev = csi->dev;
>  	sd->entity.ops = &ti_csi2rx_subdev_entity_ops;
> @@ -1347,6 +1479,8 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
>  
>  	ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt);
>  
> +	ctx->route = NULL;
> +
>  	ctx->pad.flags = MEDIA_PAD_FL_SINK;
>  	vdev->entity.ops = &ti_csi2rx_video_entity_ops;
>  	ret = media_entity_pads_init(&ctx->vdev.entity, 1, &ctx->pad);

Powered by blists - more mailing lists