Re: [PATCH v8 13/18] media: ti: j721e-csi2rx: add multistream support
From: Rishikesh Donadkar
Date: Tue Dec 09 2025 - 05:16:48 EST
On 01/12/25 18:33, Tomi Valkeinen wrote:
Hi,
Hi Tomi,
Thank you for the review !
On 12/11/2025 13:54, Rishikesh Donadkar wrote:
From: Jai Luthra <j-luthra@xxxxxx>
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?
Yes, The commit message talks about how TI CSI does the multiplexing of
CSI stream from the sensor into 4 streams as show in the Figure 12-388
in AM62A TRM[1]. I will modify the commit message to mention that this
is TI CSI specific.
[1]:
https://www.ti.com/lit/ug/spruj16c/spruj16c.pdf?ts=1765273774405&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FAM62A7
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@xxxxxx>
Co-developed-by: Pratyush Yadav <p.yadav@xxxxxx>
Signed-off-by: Pratyush Yadav <p.yadav@xxxxxx>
Signed-off-by: Jai Luthra <j-luthra@xxxxxx>
Co-developed-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
Signed-off-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
---
.../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.
Thank you for pointing out, I will store the stream number instead of
storing the pointer for route, here we don't allow anyone to update the
routes when we are in streaming (i.e. if csi->enable_count > 0, return
-EBUSY). So, the stream number is not likely to change.
+
+ for_each_active_route(&state->routing, r) {
+ if (!(r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
for_each_active_route() already checks this ("_active_").
Okay, I will remove the check for active route.
+ continue;
+ if (r->source_pad != pad->index)
+ continue;
+
+ ctx->route = r;
Shouldn't you break here?
Right, I will add a 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?
Yes, here we pass a single context to ti_csi2rx_get_vc(), but we loop
over all the ctx on first stream start.
I will modify this function to also deal with DT.
+ 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"
Okay
Rishikesh
+ .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);