Message-ID: <82c1a585-7e49-4db8-a832-d7a8bf35e75d@ti.com>
Date: Tue, 20 Jan 2026 14:18:01 +0530
From: Rishikesh Donadkar <r-donadkar@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
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>, <jai.luthra@...ux.dev>,
	<laurent.pinchart@...asonboard.com>, <mripard@...nel.org>
Subject: Re: [PATCH v9 13/19] media: ti: j721e-csi2rx: add multistream support


On 15/01/26 17:57, Tomi Valkeinen wrote:
> Hi,
>
> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
>> From: Jai Luthra <j-luthra@...com>
>>
>> Each CSI2 stream can be multiplexed into 4 independent streams, each
>> 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   | 259 +++++++++++++-----
>>   1 file changed, 189 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> index 6f9f34aa26f1b..4a063364f893e 100644
>> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> @@ -137,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)
>> @@ -144,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,
>> @@ -567,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);
>> +	}
> I think this is a bit messy. Here we have ti_csi2rx_setup_shim() which
> is passed ctx as a parameter, but it also does non-context stuff, if
> this is the first enable.
Right, I will shift the pixel reset code from ti_csi2rx_setup_shim() to 
ti_csi2rx_sd_enable_streams() function to make things symmetric in the 
start and stop path.
>
> In ti_csi2rx_sd_enable_streams(), we check !csi->vc_cached (which,
> afaics, is essentially also "is this the first enable"), and do stuff.
>
> The structure should be so that we have a clear
> setup-things-on-the-first-enable function, and
> setup-things-for-a-context function. Now these two things are mixed in
> together, also with the 'vc_cached' field which I think is not needed at
> all.


Yes, Thanks for pointing out, the vc_cached field can be removed entirely.

>
> Actually, maybe I'm mixing the operation of the video dev and the subdev
> here, as the single file contains both. It still looks confusing: in
> ti_csi2rx_stop_streaming() we assert reset, but we don't de-assert in
> ti_csi2rx_start_streaming(). Instead we deassert in
> ti_csi2rx_setup_shim() which is called from ti_csi2rx_sd_enable_streams().
>
> Please check these out. There most likely should be more symmetry here
> wrt. enabling and disabling things.
>
>>   
>>   	/* Negotiate pixel count from the source */
>>   	ti_csi2rx_request_max_ppc(csi);
>> @@ -889,29 +881,69 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb)
>>   	}
>>   }
>>   
>> +static int ti_csi2rx_get_stream(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_get_locked_active_state(&csi->subdev);
>> +
>> +	for_each_active_route(&state->routing, r) {
>> +		if (r->source_pad == pad->index) {
>> +			ctx->stream = r->sink_stream;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	/* No route found for this ctx */
>> +	return -ENODEV;
>> +}
>> +
>>   static int ti_csi2rx_get_vc_and_dt(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;
>> +	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) {
>> -			ctx->vc = fd.entry[i].bus.csi2.vc;
>> -			ctx->dt = fd.entry[i].bus.csi2.dt;
>> -		}
>> +	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_stream(curr_ctx);
>> +		if (ret)
>> +			continue;
>> +
>> +		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;
>> +				curr_ctx->dt = fd.entry[j].bus.csi2.dt;
>> +			}
>>   	}
>>   
>>   	return 0;
>> @@ -922,8 +954,6 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   	struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vq);
>>   	struct ti_csi2rx_dev *csi = ctx->csi;
>>   	struct ti_csi2rx_dma *dma = &ctx->dma;
>> -	struct ti_csi2rx_buffer *buf;
>> -	const struct ti_csi2rx_fmt *fmt;
>>   	unsigned long flags;
>>   	int ret = 0;
>>   
>> @@ -938,35 +968,9 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   	if (ret)
>>   		goto err;
>>   
>> -	ret = ti_csi2rx_get_vc_and_dt(ctx);
>> -	if (ret == -ENOIOCTLCMD) {
>> -		ctx->vc = 0;
>> -		fmt = find_format_by_fourcc(ctx->v_fmt.fmt.pix.pixelformat);
>> -		ctx->dt = fmt->csi_dt;
>> -	} else if (ret < 0) {
>> -		goto err;
>> -	}
>> -
>> -	ti_csi2rx_setup_shim(ctx);
>> -
>> -	ctx->sequence = 0;
>> -
>> -	spin_lock_irqsave(&dma->lock, flags);
>> -	buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer, list);
>> -
>> -	ret = ti_csi2rx_start_dma(ctx, buf);
>> -	if (ret) {
>> -		dev_err(csi->dev, "Failed to start DMA: %d\n", ret);
>> -		spin_unlock_irqrestore(&dma->lock, flags);
>> -		goto err_pipeline;
>> -	}
>> -
>> -	list_move_tail(&buf->list, &dma->submitted);
>> -	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_U64(0));
>>   	if (ret)
>>   		goto err_dma;
>> @@ -975,7 +979,6 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   
>>   err_dma:
>>   	ti_csi2rx_stop_dma(ctx);
>> -err_pipeline:
>>   	video_device_pipeline_stop(&ctx->vdev);
>>   	writel(0, csi->shim + SHIM_CNTL);
>>   	writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
>> @@ -990,17 +993,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_U64(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);
>>   }
>> @@ -1043,25 +1055,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;
>> +
>> +	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,
>> +	};
>>   
>> -	fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_SINK);
>> -	*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);
>>   
>> -	fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE);
>> -	*fmt = ti_csi2rx_default_fmt;
>> +	if (ret)
>> +		return ret;
>>   
>> -	return 0;
>> +	/* 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,
>> @@ -1069,15 +1140,58 @@ static int ti_csi2rx_sd_enable_streams(struct v4l2_subdev *sd,
>>   				       u32 pad, u64 streams_mask)
>>   {
>>   	struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
>> +	struct ti_csi2rx_ctx *ctx = &csi->ctx[pad - TI_CSI2RX_PAD_FIRST_SOURCE];
>> +	struct ti_csi2rx_dma *dma = &ctx->dma;
>>   	struct media_pad *remote_pad;
>> +	struct ti_csi2rx_buffer *buf;
>> +	const struct ti_csi2rx_fmt *fmt;
>> +	unsigned long flags;
>> +	u64 sink_streams;
>>   	int ret = 0;
>>   
>> +	ret = ti_csi2rx_get_stream(ctx);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Get the VC and DT for all enabled ctx on first stream start */
>> +	if (!csi->vc_cached) {
>> +		ret = ti_csi2rx_get_vc_and_dt(ctx);
>> +		if (ret == -ENOIOCTLCMD) {
>> +			ctx->vc = 0;
>> +			fmt = find_format_by_fourcc(ctx->v_fmt.fmt.pix.pixelformat);
>> +			ctx->dt = fmt->csi_dt;
>> +		} else if (ret < 0) {
>> +			return ret;
>> +		}
>> +		csi->vc_cached = true;
>> +	}
>> +
>> +	ti_csi2rx_setup_shim(ctx);
>> +	ctx->sequence = 0;
>> +
>> +	spin_lock_irqsave(&dma->lock, flags);
>> +	buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer, list);
>> +
>> +	ret = ti_csi2rx_start_dma(ctx, buf);
>> +	if (ret) {
>> +		dev_err(csi->dev, "Failed to start DMA: %d\n", ret);
>> +		spin_unlock_irqrestore(&dma->lock, flags);
>> +		return ret;
>> +	}
>> +
>> +	list_move_tail(&buf->list, &dma->submitted);
>> +	dma->state = TI_CSI2RX_DMA_ACTIVE;
>> +	spin_unlock_irqrestore(&dma->lock, flags);
>> +
>>   	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_U64(0));
>> +					 sink_streams);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -1092,17 +1206,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;
> Correct me if I'm wrong, but I don't think this should ever happen. If
> you want, you can place it in the beginning of the func, with a WARN_ON().


Will do.


Rishikesh

>
>>   
>>   	ret = v4l2_subdev_disable_streams(csi->source, remote_pad->index,
>> -					  BIT_U64(0));
>> +					  sink_streams);
>>   	if (!ret)
>>   		--csi->enable_count;
>>   
>> @@ -1111,6 +1229,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,
>> @@ -1289,7 +1408,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;
>

Powered by blists - more mailing lists