From: Jai Luthra <j-luthra@ti.com>
With single stream capture, it was simpler to use the video device as
the media entity representing the main TI CSI2RX device. Now with multi
stream capture coming into the picture, the model has shifted to each
video device having a link to the main device's subdev. The routing
would then be set on this subdev.
Add this subdev, link each context to this subdev's entity and link the
subdev's entity to the source. Also add an array of media pads. It will
have one sink pad and source pads equal to the number of contexts.
Support the new enable_stream()/disable_stream() APIs in the subdev
instead of s_stream() hook.
Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
Signed-off-by: Jai Luthra <j-luthra@ti.com>
Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
---
.../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 292 +++++++++++++++---
1 file changed, 248 insertions(+), 44 deletions(-)
diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index f66d68edcd57a..8f49ea2638585 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -54,6 +54,11 @@
#define MAX_WIDTH_BYTES SZ_16K
#define MAX_HEIGHT_LINES SZ_16K
+#define TI_CSI2RX_PAD_SINK 0
+#define TI_CSI2RX_PAD_FIRST_SOURCE 1
+#define TI_CSI2RX_NUM_SOURCE_PADS 1
+#define TI_CSI2RX_NUM_PADS (1 + TI_CSI2RX_NUM_SOURCE_PADS)
+
#define DRAIN_TIMEOUT_MS 50
#define DRAIN_BUFFER_SIZE SZ_32K
@@ -102,6 +107,7 @@ struct ti_csi2rx_ctx {
struct mutex mutex; /* To serialize ioctls. */
struct v4l2_format v_fmt;
struct ti_csi2rx_dma dma;
+ struct media_pad pad;
u32 sequence;
u32 idx;
};
@@ -109,12 +115,15 @@ struct ti_csi2rx_ctx {
struct ti_csi2rx_dev {
struct device *dev;
void __iomem *shim;
+ struct mutex mutex; /* To serialize ioctls. */
+ unsigned int enable_count;
struct v4l2_device v4l2_dev;
struct media_device mdev;
struct media_pipeline pipe;
- struct media_pad pad;
+ struct media_pad pads[TI_CSI2RX_NUM_PADS];
struct v4l2_async_notifier notifier;
struct v4l2_subdev *source;
+ struct v4l2_subdev subdev;
struct ti_csi2rx_ctx ctx[TI_CSI2RX_NUM_CTX];
u8 pix_per_clk;
/* Buffer to drain stale data from PSI-L endpoint */
@@ -125,6 +134,22 @@ struct ti_csi2rx_dev {
} drain;
};
+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,
@@ -422,6 +447,18 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev);
int ret, i;
+ /* Create link from source to subdev */
+ ret = media_create_pad_link(&csi->source->entity,
+ CSI2RX_BRIDGE_SOURCE_PAD,
+ &csi->subdev.entity,
+ TI_CSI2RX_PAD_SINK,
+ MEDIA_LNK_FL_IMMUTABLE |
+ MEDIA_LNK_FL_ENABLED);
+
+ if (ret)
+ return ret;
+
+ /* Create and link video nodes for all DMA contexts */
for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) {
struct ti_csi2rx_ctx *ctx = &csi->ctx[i];
struct video_device *vdev = &ctx->vdev;
@@ -429,15 +466,17 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
if (ret)
goto unregister_dev;
- }
- ret = media_create_pad_link(&csi->source->entity,
- CSI2RX_BRIDGE_SOURCE_PAD,
- &csi->ctx[0].vdev.entity, csi->pad.index,
- MEDIA_LNK_FL_IMMUTABLE |
- MEDIA_LNK_FL_ENABLED);
- if (ret)
- goto unregister_dev;
+ ret = media_create_pad_link(&csi->subdev.entity,
+ TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx,
+ &vdev->entity, 0,
+ MEDIA_LNK_FL_IMMUTABLE |
+ MEDIA_LNK_FL_ENABLED);
+ if (ret) {
+ video_unregister_device(vdev);
+ goto unregister_dev;
+ }
+ }
ret = v4l2_device_register_subdev_nodes(&csi->v4l2_dev);
if (ret)
@@ -447,8 +486,10 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
unregister_dev:
i--;
- for (; i >= 0; i--)
+ for (; i >= 0; i--) {
+ media_entity_remove_links(&csi->ctx[i].vdev.entity);
video_unregister_device(&csi->ctx[i].vdev);
+ }
return ret;
}
@@ -493,14 +534,13 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi)
}
/* Request maximum possible pixels per clock from the bridge */
-static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_ctx *ctx)
+static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_dev *csi)
{
- struct ti_csi2rx_dev *csi = ctx->csi;
u8 ppc = TI_CSI2RX_MAX_PIX_PER_CLK;
struct media_pad *pad;
int ret;
- pad = media_entity_remote_source_pad_unique(&ctx->vdev.entity);
+ pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
if (IS_ERR(pad))
return;
@@ -526,7 +566,7 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
writel(reg, csi->shim + SHIM_CNTL);
/* Negotiate pixel count from the source */
- ti_csi2rx_request_max_ppc(ctx);
+ ti_csi2rx_request_max_ppc(csi);
reg = SHIM_DMACNTX_EN;
reg |= FIELD_PREP(SHIM_DMACNTX_FMT, fmt->csi_dt);
@@ -881,7 +921,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);
- ret = v4l2_subdev_call(csi->source, video, s_stream, 1);
+ ret = v4l2_subdev_enable_streams(&csi->subdev,
+ TI_CSI2RX_PAD_FIRST_SOURCE,
+ BIT_U64(0));
if (ret)
goto err_dma;
@@ -909,7 +951,9 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq)
writel(0, csi->shim + SHIM_CNTL);
writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
- ret = v4l2_subdev_call(csi->source, video, s_stream, 0);
+ ret = v4l2_subdev_disable_streams(&csi->subdev,
+ TI_CSI2RX_PAD_FIRST_SOURCE,
+ BIT_U64(0));
if (ret)
dev_err(csi->dev, "Failed to stop subdev stream\n");
@@ -925,8 +969,121 @@ static const struct vb2_ops csi_vb2_qops = {
.stop_streaming = ti_csi2rx_stop_streaming,
};
+static int ti_csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_mbus_code_enum *code_enum)
+{
+ if (code_enum->index >= ARRAY_SIZE(ti_csi2rx_formats))
+ return -EINVAL;
+
+ code_enum->code = ti_csi2rx_formats[code_enum->index].code;
+
+ return 0;
+}
+
+static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_format *format)
+{
+ struct v4l2_mbus_framefmt *fmt;
+
+ /* No transcoding, don't allow setting source fmt */
+ if (format->pad > TI_CSI2RX_PAD_SINK)
+ return v4l2_subdev_get_fmt(sd, state, format);
+
+ if (!find_format_by_code(format->format.code))
+ format->format.code = ti_csi2rx_formats[0].code;
+
+ format->format.field = V4L2_FIELD_NONE;
+
+ 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 = format->format;
+
+ return 0;
+}
+
+static int ti_csi2rx_sd_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state)
+{
+ struct v4l2_mbus_framefmt *fmt;
+
+ fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_SINK);
+ *fmt = ti_csi2rx_default_fmt;
+
+ fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE);
+ *fmt = ti_csi2rx_default_fmt;
+
+ return 0;
+}
+
+static int ti_csi2rx_sd_enable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ u32 pad, u64 streams_mask)
+{
+ struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
+ struct media_pad *remote_pad;
+ int ret = 0;
+
+ remote_pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
+ if (!remote_pad)
+ return -ENODEV;
+
+ ret = v4l2_subdev_enable_streams(csi->source, remote_pad->index,
+ BIT_U64(0));
+ if (ret)
+ return ret;
+
+ csi->enable_count++;
+
+ return 0;
+}
+
+static int ti_csi2rx_sd_disable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ u32 pad, u64 streams_mask)
+{
+ struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
+ struct media_pad *remote_pad;
+ int ret = 0;
+
+ remote_pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
+ if (!remote_pad)
+ return -ENODEV;
+
+ if (csi->enable_count == 0)
+ return -EINVAL;
+
+ ret = v4l2_subdev_disable_streams(csi->source, remote_pad->index,
+ BIT_U64(0));
+ if (!ret)
+ --csi->enable_count;
+
+ return 0;
+}
+
+static const struct v4l2_subdev_pad_ops ti_csi2rx_subdev_pad_ops = {
+ .enum_mbus_code = ti_csi2rx_enum_mbus_code,
+ .get_fmt = v4l2_subdev_get_fmt,
+ .set_fmt = ti_csi2rx_sd_set_fmt,
+ .enable_streams = ti_csi2rx_sd_enable_streams,
+ .disable_streams = ti_csi2rx_sd_disable_streams,
+};
+
+static const struct v4l2_subdev_ops ti_csi2rx_subdev_ops = {
+ .pad = &ti_csi2rx_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops ti_csi2rx_internal_ops = {
+ .init_state = ti_csi2rx_sd_init_state,
+};
+
static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi)
{
+ v4l2_subdev_cleanup(&csi->subdev);
media_device_unregister(&csi->mdev);
v4l2_device_unregister(&csi->v4l2_dev);
media_device_cleanup(&csi->mdev);
@@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
struct ti_csi2rx_dev *csi = ctx->csi;
struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
- struct v4l2_subdev_format source_fmt = {
- .which = V4L2_SUBDEV_FORMAT_ACTIVE,
- .pad = link->source->index,
- };
+ struct v4l2_mbus_framefmt *format;
+ struct v4l2_subdev_state *state;
const struct ti_csi2rx_fmt *ti_fmt;
- int ret;
- ret = v4l2_subdev_call_state_active(csi->source, pad,
- get_fmt, &source_fmt);
- if (ret)
- return ret;
+ state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
+ format = v4l2_subdev_state_get_format(state, link->source->index, 0);
+ v4l2_subdev_unlock_state(state);
- if (source_fmt.format.width != csi_fmt->width) {
+ if (!format) {
+ dev_dbg(csi->dev,
+ "Skipping validation as no format present on \"%s\":%u:0\n",
+ link->source->entity->name, link->source->index);
+ return 0;
+ }
+
+ if (format->width != csi_fmt->width) {
dev_dbg(csi->dev, "Width does not match (source %u, sink %u)\n",
- source_fmt.format.width, csi_fmt->width);
+ format->width, csi_fmt->width);
return -EPIPE;
}
- if (source_fmt.format.height != csi_fmt->height) {
+ if (format->height != csi_fmt->height) {
dev_dbg(csi->dev, "Height does not match (source %u, sink %u)\n",
- source_fmt.format.height, csi_fmt->height);
+ format->height, csi_fmt->height);
return -EPIPE;
}
- if (source_fmt.format.field != csi_fmt->field &&
+ if (format->field != csi_fmt->field &&
csi_fmt->field != V4L2_FIELD_NONE) {
dev_dbg(csi->dev, "Field does not match (source %u, sink %u)\n",
- source_fmt.format.field, csi_fmt->field);
+ format->field, csi_fmt->field);
return -EPIPE;
}
- ti_fmt = find_format_by_code(source_fmt.format.code);
+ ti_fmt = find_format_by_code(format->code);
if (!ti_fmt) {
dev_dbg(csi->dev, "Media bus format 0x%x not supported\n",
- source_fmt.format.code);
+ format->code);
return -EPIPE;
}
if (ti_fmt->fourcc != csi_fmt->pixelformat) {
dev_dbg(csi->dev,
- "Cannot transform source fmt 0x%x to sink fmt 0x%x\n",
- ti_fmt->fourcc, csi_fmt->pixelformat);
+ "Cannot transform \"%s\":%u format %p4cc to %p4cc\n",
+ link->source->entity->name, link->source->index,
+ &ti_fmt->fourcc, &csi_fmt->pixelformat);
return -EPIPE;
}
@@ -1033,6 +1194,10 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = {
.link_validate = ti_csi2rx_link_validate,
};
+static const struct media_entity_operations ti_csi2rx_subdev_entity_ops = {
+ .link_validate = v4l2_subdev_link_validate,
+};
+
static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx)
{
struct dma_slave_config cfg = {
@@ -1058,6 +1223,7 @@ static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx)
static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
{
struct media_device *mdev = &csi->mdev;
+ struct v4l2_subdev *sd = &csi->subdev;
int ret;
mdev->dev = csi->dev;
@@ -1070,16 +1236,51 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
ret = v4l2_device_register(csi->dev, &csi->v4l2_dev);
if (ret)
- return ret;
+ goto cleanup_media;
ret = media_device_register(mdev);
- if (ret) {
- v4l2_device_unregister(&csi->v4l2_dev);
- media_device_cleanup(mdev);
- return ret;
- }
+ if (ret)
+ goto unregister_v4l2;
+
+ 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;
+ strscpy(sd->name, dev_name(csi->dev), sizeof(sd->name));
+ sd->dev = csi->dev;
+ sd->entity.ops = &ti_csi2rx_subdev_entity_ops;
+
+ csi->pads[TI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+
+ for (unsigned int i = TI_CSI2RX_PAD_FIRST_SOURCE;
+ i < TI_CSI2RX_NUM_PADS; i++)
+ csi->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+
+ ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(csi->pads),
+ csi->pads);
+ if (ret)
+ goto unregister_media;
+
+ ret = v4l2_subdev_init_finalize(sd);
+ if (ret)
+ goto unregister_media;
+
+ ret = v4l2_device_register_subdev(&csi->v4l2_dev, sd);
+ if (ret)
+ goto cleanup_subdev;
return 0;
+
+cleanup_subdev:
+ v4l2_subdev_cleanup(sd);
+unregister_media:
+ media_device_unregister(mdev);
+unregister_v4l2:
+ v4l2_device_unregister(&csi->v4l2_dev);
+cleanup_media:
+ media_device_cleanup(mdev);
+
+ return ret;
}
static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
@@ -1106,9 +1307,9 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt);
- csi->pad.flags = MEDIA_PAD_FL_SINK;
+ ctx->pad.flags = MEDIA_PAD_FL_SINK;
vdev->entity.ops = &ti_csi2rx_video_entity_ops;
- ret = media_entity_pads_init(&ctx->vdev.entity, 1, &csi->pad);
+ ret = media_entity_pads_init(&ctx->vdev.entity, 1, &ctx->pad);
if (ret)
return ret;
@@ -1169,6 +1370,8 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
if (!csi->drain.vaddr)
return -ENOMEM;
+ mutex_init(&csi->mutex);
+
ret = ti_csi2rx_v4l2_init(csi);
if (ret)
goto err_v4l2;
@@ -1201,6 +1404,7 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
ti_csi2rx_cleanup_ctx(&csi->ctx[i]);
ti_csi2rx_cleanup_v4l2(csi);
err_v4l2:
+ mutex_destroy(&csi->mutex);
dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
csi->drain.paddr);
return ret;
@@ -1216,7 +1420,7 @@ static void ti_csi2rx_remove(struct platform_device *pdev)
ti_csi2rx_cleanup_notifier(csi);
ti_csi2rx_cleanup_v4l2(csi);
-
+ mutex_destroy(&csi->mutex);
dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
csi->drain.paddr);
}
--
2.34.1Hi,
On 30/12/2025 10:32, Rishikesh Donadkar wrote:
> From: Jai Luthra <j-luthra@ti.com>
>
> With single stream capture, it was simpler to use the video device as
> the media entity representing the main TI CSI2RX device. Now with multi
> stream capture coming into the picture, the model has shifted to each
> video device having a link to the main device's subdev. The routing
> would then be set on this subdev.
>
> Add this subdev, link each context to this subdev's entity and link the
> subdev's entity to the source. Also add an array of media pads. It will
> have one sink pad and source pads equal to the number of contexts.
>
> Support the new enable_stream()/disable_stream() APIs in the subdev
> instead of s_stream() hook.
>
> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> ---
> .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 292 +++++++++++++++---
> 1 file changed, 248 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index f66d68edcd57a..8f49ea2638585 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> @@ -54,6 +54,11 @@
> #define MAX_WIDTH_BYTES SZ_16K
> #define MAX_HEIGHT_LINES SZ_16K
>
> +#define TI_CSI2RX_PAD_SINK 0
> +#define TI_CSI2RX_PAD_FIRST_SOURCE 1
> +#define TI_CSI2RX_NUM_SOURCE_PADS 1
> +#define TI_CSI2RX_NUM_PADS (1 + TI_CSI2RX_NUM_SOURCE_PADS)
> +
> #define DRAIN_TIMEOUT_MS 50
> #define DRAIN_BUFFER_SIZE SZ_32K
>
> @@ -102,6 +107,7 @@ struct ti_csi2rx_ctx {
> struct mutex mutex; /* To serialize ioctls. */
> struct v4l2_format v_fmt;
> struct ti_csi2rx_dma dma;
> + struct media_pad pad;
> u32 sequence;
> u32 idx;
> };
> @@ -109,12 +115,15 @@ struct ti_csi2rx_ctx {
> struct ti_csi2rx_dev {
> struct device *dev;
> void __iomem *shim;
> + struct mutex mutex; /* To serialize ioctls. */
The mutex is not used (in this patch at least).
> + unsigned int enable_count;
> struct v4l2_device v4l2_dev;
> struct media_device mdev;
> struct media_pipeline pipe;
> - struct media_pad pad;
> + struct media_pad pads[TI_CSI2RX_NUM_PADS];
> struct v4l2_async_notifier notifier;
> struct v4l2_subdev *source;
> + struct v4l2_subdev subdev;
> struct ti_csi2rx_ctx ctx[TI_CSI2RX_NUM_CTX];
> u8 pix_per_clk;
> /* Buffer to drain stale data from PSI-L endpoint */
> @@ -125,6 +134,22 @@ struct ti_csi2rx_dev {
> } drain;
> };
>
> +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,
> @@ -422,6 +447,18 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
> struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev);
> int ret, i;
>
> + /* Create link from source to subdev */
> + ret = media_create_pad_link(&csi->source->entity,
> + CSI2RX_BRIDGE_SOURCE_PAD,
> + &csi->subdev.entity,
> + TI_CSI2RX_PAD_SINK,
> + MEDIA_LNK_FL_IMMUTABLE |
> + MEDIA_LNK_FL_ENABLED);
> +
> + if (ret)
> + return ret;
> +
> + /* Create and link video nodes for all DMA contexts */
> for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) {
> struct ti_csi2rx_ctx *ctx = &csi->ctx[i];
> struct video_device *vdev = &ctx->vdev;
> @@ -429,15 +466,17 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
> ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> if (ret)
> goto unregister_dev;
> - }
>
> - ret = media_create_pad_link(&csi->source->entity,
> - CSI2RX_BRIDGE_SOURCE_PAD,
> - &csi->ctx[0].vdev.entity, csi->pad.index,
> - MEDIA_LNK_FL_IMMUTABLE |
> - MEDIA_LNK_FL_ENABLED);
> - if (ret)
> - goto unregister_dev;
> + ret = media_create_pad_link(&csi->subdev.entity,
> + TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx,
> + &vdev->entity, 0,
> + MEDIA_LNK_FL_IMMUTABLE |
> + MEDIA_LNK_FL_ENABLED);
> + if (ret) {
> + video_unregister_device(vdev);
> + goto unregister_dev;
> + }
> + }
>
> ret = v4l2_device_register_subdev_nodes(&csi->v4l2_dev);
> if (ret)
> @@ -447,8 +486,10 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
>
> unregister_dev:
> i--;
> - for (; i >= 0; i--)
> + for (; i >= 0; i--) {
> + media_entity_remove_links(&csi->ctx[i].vdev.entity);
> video_unregister_device(&csi->ctx[i].vdev);
> + }
> return ret;
> }
>
> @@ -493,14 +534,13 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi)
> }
>
> /* Request maximum possible pixels per clock from the bridge */
> -static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_ctx *ctx)
> +static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_dev *csi)
> {
> - struct ti_csi2rx_dev *csi = ctx->csi;
> u8 ppc = TI_CSI2RX_MAX_PIX_PER_CLK;
> struct media_pad *pad;
> int ret;
>
> - pad = media_entity_remote_source_pad_unique(&ctx->vdev.entity);
> + pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
> if (IS_ERR(pad))
> return;
>
> @@ -526,7 +566,7 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
> writel(reg, csi->shim + SHIM_CNTL);
>
> /* Negotiate pixel count from the source */
> - ti_csi2rx_request_max_ppc(ctx);
> + ti_csi2rx_request_max_ppc(csi);
>
> reg = SHIM_DMACNTX_EN;
> reg |= FIELD_PREP(SHIM_DMACNTX_FMT, fmt->csi_dt);
> @@ -881,7 +921,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);
>
> - ret = v4l2_subdev_call(csi->source, video, s_stream, 1);
> + ret = v4l2_subdev_enable_streams(&csi->subdev,
> + TI_CSI2RX_PAD_FIRST_SOURCE,
> + BIT_U64(0));
> if (ret)
> goto err_dma;
>
> @@ -909,7 +951,9 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq)
> writel(0, csi->shim + SHIM_CNTL);
> writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
>
> - ret = v4l2_subdev_call(csi->source, video, s_stream, 0);
> + ret = v4l2_subdev_disable_streams(&csi->subdev,
> + TI_CSI2RX_PAD_FIRST_SOURCE,
> + BIT_U64(0));
> if (ret)
> dev_err(csi->dev, "Failed to stop subdev stream\n");
>
> @@ -925,8 +969,121 @@ static const struct vb2_ops csi_vb2_qops = {
> .stop_streaming = ti_csi2rx_stop_streaming,
> };
>
> +static int ti_csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_mbus_code_enum *code_enum)
> +{
> + if (code_enum->index >= ARRAY_SIZE(ti_csi2rx_formats))
> + return -EINVAL;
> +
> + code_enum->code = ti_csi2rx_formats[code_enum->index].code;
> +
> + return 0;
> +}
> +
> +static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_format *format)
> +{
> + struct v4l2_mbus_framefmt *fmt;
> +
> + /* No transcoding, don't allow setting source fmt */
> + if (format->pad > TI_CSI2RX_PAD_SINK)
> + return v4l2_subdev_get_fmt(sd, state, format);
> +
> + if (!find_format_by_code(format->format.code))
> + format->format.code = ti_csi2rx_formats[0].code;
> +
> + format->format.field = V4L2_FIELD_NONE;
> +
> + 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 = format->format;
> +
> + return 0;
> +}
> +
> +static int ti_csi2rx_sd_init_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + struct v4l2_mbus_framefmt *fmt;
> +
> + fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_SINK);
> + *fmt = ti_csi2rx_default_fmt;
> +
> + fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE);
> + *fmt = ti_csi2rx_default_fmt;
> +
> + return 0;
> +}
> +
> +static int ti_csi2rx_sd_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + u32 pad, u64 streams_mask)
> +{
> + struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
> + struct media_pad *remote_pad;
> + int ret = 0;
> +
> + remote_pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
> + if (!remote_pad)
> + return -ENODEV;
> +
> + ret = v4l2_subdev_enable_streams(csi->source, remote_pad->index,
> + BIT_U64(0));
> + if (ret)
> + return ret;
> +
> + csi->enable_count++;
> +
> + return 0;
> +}
> +
> +static int ti_csi2rx_sd_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + u32 pad, u64 streams_mask)
> +{
> + struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
> + struct media_pad *remote_pad;
> + int ret = 0;
> +
> + remote_pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
> + if (!remote_pad)
> + return -ENODEV;
> +
> + if (csi->enable_count == 0)
> + return -EINVAL;
> +
> + ret = v4l2_subdev_disable_streams(csi->source, remote_pad->index,
> + BIT_U64(0));
> + if (!ret)
> + --csi->enable_count;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops ti_csi2rx_subdev_pad_ops = {
> + .enum_mbus_code = ti_csi2rx_enum_mbus_code,
> + .get_fmt = v4l2_subdev_get_fmt,
> + .set_fmt = ti_csi2rx_sd_set_fmt,
> + .enable_streams = ti_csi2rx_sd_enable_streams,
> + .disable_streams = ti_csi2rx_sd_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_ops ti_csi2rx_subdev_ops = {
> + .pad = &ti_csi2rx_subdev_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops ti_csi2rx_internal_ops = {
> + .init_state = ti_csi2rx_sd_init_state,
> +};
> +
> static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi)
> {
> + v4l2_subdev_cleanup(&csi->subdev);
> media_device_unregister(&csi->mdev);
> v4l2_device_unregister(&csi->v4l2_dev);
> media_device_cleanup(&csi->mdev);
> @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
> struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
> struct ti_csi2rx_dev *csi = ctx->csi;
> struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
> - struct v4l2_subdev_format source_fmt = {
> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> - .pad = link->source->index,
> - };
> + struct v4l2_mbus_framefmt *format;
> + struct v4l2_subdev_state *state;
> const struct ti_csi2rx_fmt *ti_fmt;
> - int ret;
>
> - ret = v4l2_subdev_call_state_active(csi->source, pad,
> - get_fmt, &source_fmt);
> - if (ret)
> - return ret;
> + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
> + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
> + v4l2_subdev_unlock_state(state);
>
> - if (source_fmt.format.width != csi_fmt->width) {
> + if (!format) {
> + dev_dbg(csi->dev,
> + "Skipping validation as no format present on \"%s\":%u:0\n",
> + link->source->entity->name, link->source->index);
> + return 0;
Isn't this an error?
Tomi
> + }
> +
> + if (format->width != csi_fmt->width) {
> dev_dbg(csi->dev, "Width does not match (source %u, sink %u)\n",
> - source_fmt.format.width, csi_fmt->width);
> + format->width, csi_fmt->width);
> return -EPIPE;
> }
>
> - if (source_fmt.format.height != csi_fmt->height) {
> + if (format->height != csi_fmt->height) {
> dev_dbg(csi->dev, "Height does not match (source %u, sink %u)\n",
> - source_fmt.format.height, csi_fmt->height);
> + format->height, csi_fmt->height);
> return -EPIPE;
> }
>
> - if (source_fmt.format.field != csi_fmt->field &&
> + if (format->field != csi_fmt->field &&
> csi_fmt->field != V4L2_FIELD_NONE) {
> dev_dbg(csi->dev, "Field does not match (source %u, sink %u)\n",
> - source_fmt.format.field, csi_fmt->field);
> + format->field, csi_fmt->field);
> return -EPIPE;
> }
>
> - ti_fmt = find_format_by_code(source_fmt.format.code);
> + ti_fmt = find_format_by_code(format->code);
> if (!ti_fmt) {
> dev_dbg(csi->dev, "Media bus format 0x%x not supported\n",
> - source_fmt.format.code);
> + format->code);
> return -EPIPE;
> }
>
> if (ti_fmt->fourcc != csi_fmt->pixelformat) {
> dev_dbg(csi->dev,
> - "Cannot transform source fmt 0x%x to sink fmt 0x%x\n",
> - ti_fmt->fourcc, csi_fmt->pixelformat);
> + "Cannot transform \"%s\":%u format %p4cc to %p4cc\n",
> + link->source->entity->name, link->source->index,
> + &ti_fmt->fourcc, &csi_fmt->pixelformat);
> return -EPIPE;
> }
>
> @@ -1033,6 +1194,10 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = {
> .link_validate = ti_csi2rx_link_validate,
> };
>
> +static const struct media_entity_operations ti_csi2rx_subdev_entity_ops = {
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx)
> {
> struct dma_slave_config cfg = {
> @@ -1058,6 +1223,7 @@ static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx)
> static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
> {
> struct media_device *mdev = &csi->mdev;
> + struct v4l2_subdev *sd = &csi->subdev;
> int ret;
>
> mdev->dev = csi->dev;
> @@ -1070,16 +1236,51 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
>
> ret = v4l2_device_register(csi->dev, &csi->v4l2_dev);
> if (ret)
> - return ret;
> + goto cleanup_media;
>
> ret = media_device_register(mdev);
> - if (ret) {
> - v4l2_device_unregister(&csi->v4l2_dev);
> - media_device_cleanup(mdev);
> - return ret;
> - }
> + if (ret)
> + goto unregister_v4l2;
> +
> + 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;
> + strscpy(sd->name, dev_name(csi->dev), sizeof(sd->name));
> + sd->dev = csi->dev;
> + sd->entity.ops = &ti_csi2rx_subdev_entity_ops;
> +
> + csi->pads[TI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +
> + for (unsigned int i = TI_CSI2RX_PAD_FIRST_SOURCE;
> + i < TI_CSI2RX_NUM_PADS; i++)
> + csi->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(csi->pads),
> + csi->pads);
> + if (ret)
> + goto unregister_media;
> +
> + ret = v4l2_subdev_init_finalize(sd);
> + if (ret)
> + goto unregister_media;
> +
> + ret = v4l2_device_register_subdev(&csi->v4l2_dev, sd);
> + if (ret)
> + goto cleanup_subdev;
>
> return 0;
> +
> +cleanup_subdev:
> + v4l2_subdev_cleanup(sd);
> +unregister_media:
> + media_device_unregister(mdev);
> +unregister_v4l2:
> + v4l2_device_unregister(&csi->v4l2_dev);
> +cleanup_media:
> + media_device_cleanup(mdev);
> +
> + return ret;
> }
>
> static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
> @@ -1106,9 +1307,9 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
>
> ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt);
>
> - csi->pad.flags = MEDIA_PAD_FL_SINK;
> + ctx->pad.flags = MEDIA_PAD_FL_SINK;
> vdev->entity.ops = &ti_csi2rx_video_entity_ops;
> - ret = media_entity_pads_init(&ctx->vdev.entity, 1, &csi->pad);
> + ret = media_entity_pads_init(&ctx->vdev.entity, 1, &ctx->pad);
> if (ret)
> return ret;
>
> @@ -1169,6 +1370,8 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
> if (!csi->drain.vaddr)
> return -ENOMEM;
>
> + mutex_init(&csi->mutex);
> +
> ret = ti_csi2rx_v4l2_init(csi);
> if (ret)
> goto err_v4l2;
> @@ -1201,6 +1404,7 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
> ti_csi2rx_cleanup_ctx(&csi->ctx[i]);
> ti_csi2rx_cleanup_v4l2(csi);
> err_v4l2:
> + mutex_destroy(&csi->mutex);
> dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
> csi->drain.paddr);
> return ret;
> @@ -1216,7 +1420,7 @@ static void ti_csi2rx_remove(struct platform_device *pdev)
>
> ti_csi2rx_cleanup_notifier(csi);
> ti_csi2rx_cleanup_v4l2(csi);
> -
> + mutex_destroy(&csi->mutex);
> dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
> csi->drain.paddr);
> }
On 14/01/26 20:51, 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@ti.com>
>>
>> With single stream capture, it was simpler to use the video device as
>> the media entity representing the main TI CSI2RX device. Now with multi
>> stream capture coming into the picture, the model has shifted to each
>> video device having a link to the main device's subdev. The routing
>> would then be set on this subdev.
>>
>> Add this subdev, link each context to this subdev's entity and link the
>> subdev's entity to the source. Also add an array of media pads. It will
>> have one sink pad and source pads equal to the number of contexts.
>>
>> Support the new enable_stream()/disable_stream() APIs in the subdev
>> instead of s_stream() hook.
>>
>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
>> ---
>> .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 292 +++++++++++++++---
>> 1 file changed, 248 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> index f66d68edcd57a..8f49ea2638585 100644
>> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> @@ -54,6 +54,11 @@
>> #define MAX_WIDTH_BYTES SZ_16K
>> #define MAX_HEIGHT_LINES SZ_16K
>>
>> +#define TI_CSI2RX_PAD_SINK 0
>> +#define TI_CSI2RX_PAD_FIRST_SOURCE 1
>> +#define TI_CSI2RX_NUM_SOURCE_PADS 1
>> +#define TI_CSI2RX_NUM_PADS (1 + TI_CSI2RX_NUM_SOURCE_PADS)
>> +
>> #define DRAIN_TIMEOUT_MS 50
>> #define DRAIN_BUFFER_SIZE SZ_32K
>>
>> @@ -102,6 +107,7 @@ struct ti_csi2rx_ctx {
>> struct mutex mutex; /* To serialize ioctls. */
>> struct v4l2_format v_fmt;
>> struct ti_csi2rx_dma dma;
>> + struct media_pad pad;
>> u32 sequence;
>> u32 idx;
>> };
>> @@ -109,12 +115,15 @@ struct ti_csi2rx_ctx {
>> struct ti_csi2rx_dev {
>> struct device *dev;
>> void __iomem *shim;
>> + struct mutex mutex; /* To serialize ioctls. */
> The mutex is not used (in this patch at least).
Yes, now since in the later patches that add multi stream support, we
access all the shared variables in the enable/disable_streams call which
are serialized by the framework, I think this mutex is note required. I
will remove it entirely.
Rishikesh
>
>> + unsigned int enable_count;
>> struct v4l2_device v4l2_dev;
>> struct media_device mdev;
>> struct media_pipeline pipe;
>> - struct media_pad pad;
>> + struct media_pad pads[TI_CSI2RX_NUM_PADS];
>> struct v4l2_async_notifier notifier;
>> struct v4l2_subdev *source;
>> + struct v4l2_subdev subdev;
>> struct ti_csi2rx_ctx ctx[TI_CSI2RX_NUM_CTX];
>> u8 pix_per_clk;
>> /* Buffer to drain stale data from PSI-L endpoint */
>> @@ -125,6 +134,22 @@ struct ti_csi2rx_dev {
>> } drain;
>> };
>>
>> +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,
>> @@ -422,6 +447,18 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
>> struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev);
>> int ret, i;
>>
>> + /* Create link from source to subdev */
>> + ret = media_create_pad_link(&csi->source->entity,
>> + CSI2RX_BRIDGE_SOURCE_PAD,
>> + &csi->subdev.entity,
>> + TI_CSI2RX_PAD_SINK,
>> + MEDIA_LNK_FL_IMMUTABLE |
>> + MEDIA_LNK_FL_ENABLED);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + /* Create and link video nodes for all DMA contexts */
>> for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) {
>> struct ti_csi2rx_ctx *ctx = &csi->ctx[i];
>> struct video_device *vdev = &ctx->vdev;
>> @@ -429,15 +466,17 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
>> ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>> if (ret)
>> goto unregister_dev;
>> - }
>>
>> - ret = media_create_pad_link(&csi->source->entity,
>> - CSI2RX_BRIDGE_SOURCE_PAD,
>> - &csi->ctx[0].vdev.entity, csi->pad.index,
>> - MEDIA_LNK_FL_IMMUTABLE |
>> - MEDIA_LNK_FL_ENABLED);
>> - if (ret)
>> - goto unregister_dev;
>> + ret = media_create_pad_link(&csi->subdev.entity,
>> + TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx,
>> + &vdev->entity, 0,
>> + MEDIA_LNK_FL_IMMUTABLE |
>> + MEDIA_LNK_FL_ENABLED);
>> + if (ret) {
>> + video_unregister_device(vdev);
>> + goto unregister_dev;
>> + }
>> + }
>>
>> ret = v4l2_device_register_subdev_nodes(&csi->v4l2_dev);
>> if (ret)
>> @@ -447,8 +486,10 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
>>
>> unregister_dev:
>> i--;
>> - for (; i >= 0; i--)
>> + for (; i >= 0; i--) {
>> + media_entity_remove_links(&csi->ctx[i].vdev.entity);
>> video_unregister_device(&csi->ctx[i].vdev);
>> + }
>> return ret;
>> }
>>
>> @@ -493,14 +534,13 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi)
>> }
>>
>> /* Request maximum possible pixels per clock from the bridge */
>> -static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_ctx *ctx)
>> +static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_dev *csi)
>> {
>> - struct ti_csi2rx_dev *csi = ctx->csi;
>> u8 ppc = TI_CSI2RX_MAX_PIX_PER_CLK;
>> struct media_pad *pad;
>> int ret;
>>
>> - pad = media_entity_remote_source_pad_unique(&ctx->vdev.entity);
>> + pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
>> if (IS_ERR(pad))
>> return;
>>
>> @@ -526,7 +566,7 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
>> writel(reg, csi->shim + SHIM_CNTL);
>>
>> /* Negotiate pixel count from the source */
>> - ti_csi2rx_request_max_ppc(ctx);
>> + ti_csi2rx_request_max_ppc(csi);
>>
>> reg = SHIM_DMACNTX_EN;
>> reg |= FIELD_PREP(SHIM_DMACNTX_FMT, fmt->csi_dt);
>> @@ -881,7 +921,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);
>>
>> - ret = v4l2_subdev_call(csi->source, video, s_stream, 1);
>> + ret = v4l2_subdev_enable_streams(&csi->subdev,
>> + TI_CSI2RX_PAD_FIRST_SOURCE,
>> + BIT_U64(0));
>> if (ret)
>> goto err_dma;
>>
>> @@ -909,7 +951,9 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq)
>> writel(0, csi->shim + SHIM_CNTL);
>> writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
>>
>> - ret = v4l2_subdev_call(csi->source, video, s_stream, 0);
>> + ret = v4l2_subdev_disable_streams(&csi->subdev,
>> + TI_CSI2RX_PAD_FIRST_SOURCE,
>> + BIT_U64(0));
>> if (ret)
>> dev_err(csi->dev, "Failed to stop subdev stream\n");
>>
>> @@ -925,8 +969,121 @@ static const struct vb2_ops csi_vb2_qops = {
>> .stop_streaming = ti_csi2rx_stop_streaming,
>> };
>>
>> +static int ti_csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_mbus_code_enum *code_enum)
>> +{
>> + if (code_enum->index >= ARRAY_SIZE(ti_csi2rx_formats))
>> + return -EINVAL;
>> +
>> + code_enum->code = ti_csi2rx_formats[code_enum->index].code;
>> +
>> + return 0;
>> +}
>> +
>> +static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_format *format)
>> +{
>> + struct v4l2_mbus_framefmt *fmt;
>> +
>> + /* No transcoding, don't allow setting source fmt */
>> + if (format->pad > TI_CSI2RX_PAD_SINK)
>> + return v4l2_subdev_get_fmt(sd, state, format);
>> +
>> + if (!find_format_by_code(format->format.code))
>> + format->format.code = ti_csi2rx_formats[0].code;
>> +
>> + format->format.field = V4L2_FIELD_NONE;
>> +
>> + 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 = format->format;
>> +
>> + return 0;
>> +}
>> +
>> +static int ti_csi2rx_sd_init_state(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state)
>> +{
>> + struct v4l2_mbus_framefmt *fmt;
>> +
>> + fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_SINK);
>> + *fmt = ti_csi2rx_default_fmt;
>> +
>> + fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE);
>> + *fmt = ti_csi2rx_default_fmt;
>> +
>> + return 0;
>> +}
>> +
>> +static int ti_csi2rx_sd_enable_streams(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + u32 pad, u64 streams_mask)
>> +{
>> + struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
>> + struct media_pad *remote_pad;
>> + int ret = 0;
>> +
>> + remote_pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
>> + if (!remote_pad)
>> + return -ENODEV;
>> +
>> + ret = v4l2_subdev_enable_streams(csi->source, remote_pad->index,
>> + BIT_U64(0));
>> + if (ret)
>> + return ret;
>> +
>> + csi->enable_count++;
>> +
>> + return 0;
>> +}
>> +
>> +static int ti_csi2rx_sd_disable_streams(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + u32 pad, u64 streams_mask)
>> +{
>> + struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd);
>> + struct media_pad *remote_pad;
>> + int ret = 0;
>> +
>> + remote_pad = media_entity_remote_source_pad_unique(&csi->subdev.entity);
>> + if (!remote_pad)
>> + return -ENODEV;
>> +
>> + if (csi->enable_count == 0)
>> + return -EINVAL;
>> +
>> + ret = v4l2_subdev_disable_streams(csi->source, remote_pad->index,
>> + BIT_U64(0));
>> + if (!ret)
>> + --csi->enable_count;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_pad_ops ti_csi2rx_subdev_pad_ops = {
>> + .enum_mbus_code = ti_csi2rx_enum_mbus_code,
>> + .get_fmt = v4l2_subdev_get_fmt,
>> + .set_fmt = ti_csi2rx_sd_set_fmt,
>> + .enable_streams = ti_csi2rx_sd_enable_streams,
>> + .disable_streams = ti_csi2rx_sd_disable_streams,
>> +};
>> +
>> +static const struct v4l2_subdev_ops ti_csi2rx_subdev_ops = {
>> + .pad = &ti_csi2rx_subdev_pad_ops,
>> +};
>> +
>> +static const struct v4l2_subdev_internal_ops ti_csi2rx_internal_ops = {
>> + .init_state = ti_csi2rx_sd_init_state,
>> +};
>> +
>> static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi)
>> {
>> + v4l2_subdev_cleanup(&csi->subdev);
>> media_device_unregister(&csi->mdev);
>> v4l2_device_unregister(&csi->v4l2_dev);
>> media_device_cleanup(&csi->mdev);
>> @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
>> struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
>> struct ti_csi2rx_dev *csi = ctx->csi;
>> struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
>> - struct v4l2_subdev_format source_fmt = {
>> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> - .pad = link->source->index,
>> - };
>> + struct v4l2_mbus_framefmt *format;
>> + struct v4l2_subdev_state *state;
>> const struct ti_csi2rx_fmt *ti_fmt;
>> - int ret;
>>
>> - ret = v4l2_subdev_call_state_active(csi->source, pad,
>> - get_fmt, &source_fmt);
>> - if (ret)
>> - return ret;
>> + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
>> + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
>> + v4l2_subdev_unlock_state(state);
>>
>> - if (source_fmt.format.width != csi_fmt->width) {
>> + if (!format) {
>> + dev_dbg(csi->dev,
>> + "Skipping validation as no format present on \"%s\":%u:0\n",
>> + link->source->entity->name, link->source->index);
>> + return 0;
> Isn't this an error?
>
> Tomi
>
>
>> + }
>> +
>> + if (format->width != csi_fmt->width) {
>> dev_dbg(csi->dev, "Width does not match (source %u, sink %u)\n",
>> - source_fmt.format.width, csi_fmt->width);
>> + format->width, csi_fmt->width);
>> return -EPIPE;
>> }
>>
>> - if (source_fmt.format.height != csi_fmt->height) {
>> + if (format->height != csi_fmt->height) {
>> dev_dbg(csi->dev, "Height does not match (source %u, sink %u)\n",
>> - source_fmt.format.height, csi_fmt->height);
>> + format->height, csi_fmt->height);
>> return -EPIPE;
>> }
>>
>> - if (source_fmt.format.field != csi_fmt->field &&
>> + if (format->field != csi_fmt->field &&
>> csi_fmt->field != V4L2_FIELD_NONE) {
>> dev_dbg(csi->dev, "Field does not match (source %u, sink %u)\n",
>> - source_fmt.format.field, csi_fmt->field);
>> + format->field, csi_fmt->field);
>> return -EPIPE;
>> }
>>
>> - ti_fmt = find_format_by_code(source_fmt.format.code);
>> + ti_fmt = find_format_by_code(format->code);
>> if (!ti_fmt) {
>> dev_dbg(csi->dev, "Media bus format 0x%x not supported\n",
>> - source_fmt.format.code);
>> + format->code);
>> return -EPIPE;
>> }
>>
>> if (ti_fmt->fourcc != csi_fmt->pixelformat) {
>> dev_dbg(csi->dev,
>> - "Cannot transform source fmt 0x%x to sink fmt 0x%x\n",
>> - ti_fmt->fourcc, csi_fmt->pixelformat);
>> + "Cannot transform \"%s\":%u format %p4cc to %p4cc\n",
>> + link->source->entity->name, link->source->index,
>> + &ti_fmt->fourcc, &csi_fmt->pixelformat);
>> return -EPIPE;
>> }
>>
>> @@ -1033,6 +1194,10 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = {
>> .link_validate = ti_csi2rx_link_validate,
>> };
>>
>> +static const struct media_entity_operations ti_csi2rx_subdev_entity_ops = {
>> + .link_validate = v4l2_subdev_link_validate,
>> +};
>> +
>> static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx)
>> {
>> struct dma_slave_config cfg = {
>> @@ -1058,6 +1223,7 @@ static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx)
>> static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
>> {
>> struct media_device *mdev = &csi->mdev;
>> + struct v4l2_subdev *sd = &csi->subdev;
>> int ret;
>>
>> mdev->dev = csi->dev;
>> @@ -1070,16 +1236,51 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi)
>>
>> ret = v4l2_device_register(csi->dev, &csi->v4l2_dev);
>> if (ret)
>> - return ret;
>> + goto cleanup_media;
>>
>> ret = media_device_register(mdev);
>> - if (ret) {
>> - v4l2_device_unregister(&csi->v4l2_dev);
>> - media_device_cleanup(mdev);
>> - return ret;
>> - }
>> + if (ret)
>> + goto unregister_v4l2;
>> +
>> + 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;
>> + strscpy(sd->name, dev_name(csi->dev), sizeof(sd->name));
>> + sd->dev = csi->dev;
>> + sd->entity.ops = &ti_csi2rx_subdev_entity_ops;
>> +
>> + csi->pads[TI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>> +
>> + for (unsigned int i = TI_CSI2RX_PAD_FIRST_SOURCE;
>> + i < TI_CSI2RX_NUM_PADS; i++)
>> + csi->pads[i].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> + ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(csi->pads),
>> + csi->pads);
>> + if (ret)
>> + goto unregister_media;
>> +
>> + ret = v4l2_subdev_init_finalize(sd);
>> + if (ret)
>> + goto unregister_media;
>> +
>> + ret = v4l2_device_register_subdev(&csi->v4l2_dev, sd);
>> + if (ret)
>> + goto cleanup_subdev;
>>
>> return 0;
>> +
>> +cleanup_subdev:
>> + v4l2_subdev_cleanup(sd);
>> +unregister_media:
>> + media_device_unregister(mdev);
>> +unregister_v4l2:
>> + v4l2_device_unregister(&csi->v4l2_dev);
>> +cleanup_media:
>> + media_device_cleanup(mdev);
>> +
>> + return ret;
>> }
>>
>> static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
>> @@ -1106,9 +1307,9 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
>>
>> ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt);
>>
>> - csi->pad.flags = MEDIA_PAD_FL_SINK;
>> + ctx->pad.flags = MEDIA_PAD_FL_SINK;
>> vdev->entity.ops = &ti_csi2rx_video_entity_ops;
>> - ret = media_entity_pads_init(&ctx->vdev.entity, 1, &csi->pad);
>> + ret = media_entity_pads_init(&ctx->vdev.entity, 1, &ctx->pad);
>> if (ret)
>> return ret;
>>
>> @@ -1169,6 +1370,8 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
>> if (!csi->drain.vaddr)
>> return -ENOMEM;
>>
>> + mutex_init(&csi->mutex);
>> +
>> ret = ti_csi2rx_v4l2_init(csi);
>> if (ret)
>> goto err_v4l2;
>> @@ -1201,6 +1404,7 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
>> ti_csi2rx_cleanup_ctx(&csi->ctx[i]);
>> ti_csi2rx_cleanup_v4l2(csi);
>> err_v4l2:
>> + mutex_destroy(&csi->mutex);
>> dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
>> csi->drain.paddr);
>> return ret;
>> @@ -1216,7 +1420,7 @@ static void ti_csi2rx_remove(struct platform_device *pdev)
>>
>> ti_csi2rx_cleanup_notifier(csi);
>> ti_csi2rx_cleanup_v4l2(csi);
>> -
>> + mutex_destroy(&csi->mutex);
>> dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
>> csi->drain.paddr);
>> }
>
Hi Tomi,
+Sakari, Laurent
Quoting Tomi Valkeinen (2026-01-14 20:51:49)
> Hi,
>
> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
> > From: Jai Luthra <j-luthra@ti.com>
> >
> > With single stream capture, it was simpler to use the video device as
> > the media entity representing the main TI CSI2RX device. Now with multi
> > stream capture coming into the picture, the model has shifted to each
> > video device having a link to the main device's subdev. The routing
> > would then be set on this subdev.
> >
> > Add this subdev, link each context to this subdev's entity and link the
> > subdev's entity to the source. Also add an array of media pads. It will
> > have one sink pad and source pads equal to the number of contexts.
> >
> > Support the new enable_stream()/disable_stream() APIs in the subdev
> > instead of s_stream() hook.
> >
> > Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> > Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > Signed-off-by: Jai Luthra <j-luthra@ti.com>
> > Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> > ---
[...]
> > @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
> > struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
> > struct ti_csi2rx_dev *csi = ctx->csi;
> > struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
> > - struct v4l2_subdev_format source_fmt = {
> > - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > - .pad = link->source->index,
> > - };
> > + struct v4l2_mbus_framefmt *format;
> > + struct v4l2_subdev_state *state;
> > const struct ti_csi2rx_fmt *ti_fmt;
> > - int ret;
> >
> > - ret = v4l2_subdev_call_state_active(csi->source, pad,
> > - get_fmt, &source_fmt);
> > - if (ret)
> > - return ret;
> > + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
> > + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
> > + v4l2_subdev_unlock_state(state);
> >
> > - if (source_fmt.format.width != csi_fmt->width) {
> > + if (!format) {
> > + dev_dbg(csi->dev,
> > + "Skipping validation as no format present on \"%s\":%u:0\n",
> > + link->source->entity->name, link->source->index);
> > + return 0;
>
> Isn't this an error?
Well, the j7 shim subdev introduced here has immutable and active links to
all the video nodes, for each DMA channel (taken from DT), many of which
may be unused for certain setups, and thus there might not be any valid
format on the subdev source pad corresponding to an unused video node.
Jacopo had a similar comment on v2, see this discussion (grep for Mali):
https://lore.kernel.org/linux-media/4mnlnsj4co3agvln4qsasmgvgwiyoo7yu2h5wyh4rmzzafhm5u@avhnbw7iknms/
I know other drivers use a different approach with mutable links, so it
would be good if you/Laurent/Sakari can give your opinions on if only one
of these two approaches should be taken for multi-stream pipelines.
>
> Tomi
>
Thanks,
Jai
[...]
Hi,
On 15/01/2026 08:36, Jai Luthra wrote:
> Hi Tomi,
>
> +Sakari, Laurent
>
> Quoting Tomi Valkeinen (2026-01-14 20:51:49)
>> Hi,
>>
>> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
>>> From: Jai Luthra <j-luthra@ti.com>
>>>
>>> With single stream capture, it was simpler to use the video device as
>>> the media entity representing the main TI CSI2RX device. Now with multi
>>> stream capture coming into the picture, the model has shifted to each
>>> video device having a link to the main device's subdev. The routing
>>> would then be set on this subdev.
>>>
>>> Add this subdev, link each context to this subdev's entity and link the
>>> subdev's entity to the source. Also add an array of media pads. It will
>>> have one sink pad and source pads equal to the number of contexts.
>>>
>>> Support the new enable_stream()/disable_stream() APIs in the subdev
>>> instead of s_stream() hook.
>>>
>>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>>> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
>>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
>>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
>>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
>>> ---
>
> [...]
>
>>> @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
>>> struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
>>> struct ti_csi2rx_dev *csi = ctx->csi;
>>> struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
>>> - struct v4l2_subdev_format source_fmt = {
>>> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>> - .pad = link->source->index,
>>> - };
>>> + struct v4l2_mbus_framefmt *format;
>>> + struct v4l2_subdev_state *state;
>>> const struct ti_csi2rx_fmt *ti_fmt;
>>> - int ret;
>>>
>>> - ret = v4l2_subdev_call_state_active(csi->source, pad,
>>> - get_fmt, &source_fmt);
>>> - if (ret)
>>> - return ret;
>>> + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
>>> + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
>>> + v4l2_subdev_unlock_state(state);
>>>
>>> - if (source_fmt.format.width != csi_fmt->width) {
>>> + if (!format) {
>>> + dev_dbg(csi->dev,
>>> + "Skipping validation as no format present on \"%s\":%u:0\n",
>>> + link->source->entity->name, link->source->index);
>>> + return 0;
>>
>> Isn't this an error?
>
> Well, the j7 shim subdev introduced here has immutable and active links to
> all the video nodes, for each DMA channel (taken from DT), many of which
> may be unused for certain setups, and thus there might not be any valid
> format on the subdev source pad corresponding to an unused video node.
>
> Jacopo had a similar comment on v2, see this discussion (grep for Mali):
> https://lore.kernel.org/linux-media/4mnlnsj4co3agvln4qsasmgvgwiyoo7yu2h5wyh4rmzzafhm5u@avhnbw7iknms/
>
> I know other drivers use a different approach with mutable links, so it
> would be good if you/Laurent/Sakari can give your opinions on if only one
> of these two approaches should be taken for multi-stream pipelines.
I see.
Well, I don't have a definite answer. With some thinking both options
make certain sense. It makes sense to keep the links immutable and
always enabled, as there's no configuration that can be done. On the
other hand, it makes sense to require the unused links to be disabled,
as, well, they are not used.
Tomi
On Thu, Jan 15, 2026 at 02:56:21PM +0200, Tomi Valkeinen wrote:
> On 15/01/2026 08:36, Jai Luthra wrote:
> > Quoting Tomi Valkeinen (2026-01-14 20:51:49)
> >> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
> >>> From: Jai Luthra <j-luthra@ti.com>
> >>>
> >>> With single stream capture, it was simpler to use the video device as
> >>> the media entity representing the main TI CSI2RX device. Now with multi
> >>> stream capture coming into the picture, the model has shifted to each
> >>> video device having a link to the main device's subdev. The routing
> >>> would then be set on this subdev.
> >>>
> >>> Add this subdev, link each context to this subdev's entity and link the
> >>> subdev's entity to the source. Also add an array of media pads. It will
> >>> have one sink pad and source pads equal to the number of contexts.
> >>>
> >>> Support the new enable_stream()/disable_stream() APIs in the subdev
> >>> instead of s_stream() hook.
> >>>
> >>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> >>> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
> >>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> >>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> >>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> >>> ---
> >
> > [...]
> >
> >>> @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
> >>> struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
> >>> struct ti_csi2rx_dev *csi = ctx->csi;
> >>> struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
> >>> - struct v4l2_subdev_format source_fmt = {
> >>> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>> - .pad = link->source->index,
> >>> - };
> >>> + struct v4l2_mbus_framefmt *format;
> >>> + struct v4l2_subdev_state *state;
> >>> const struct ti_csi2rx_fmt *ti_fmt;
> >>> - int ret;
> >>>
> >>> - ret = v4l2_subdev_call_state_active(csi->source, pad,
> >>> - get_fmt, &source_fmt);
> >>> - if (ret)
> >>> - return ret;
> >>> + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
> >>> + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
> >>> + v4l2_subdev_unlock_state(state);
> >>>
> >>> - if (source_fmt.format.width != csi_fmt->width) {
> >>> + if (!format) {
> >>> + dev_dbg(csi->dev,
> >>> + "Skipping validation as no format present on \"%s\":%u:0\n",
> >>> + link->source->entity->name, link->source->index);
> >>> + return 0;
> >>
> >> Isn't this an error?
> >
> > Well, the j7 shim subdev introduced here has immutable and active links to
> > all the video nodes, for each DMA channel (taken from DT), many of which
> > may be unused for certain setups, and thus there might not be any valid
> > format on the subdev source pad corresponding to an unused video node.
> >
> > Jacopo had a similar comment on v2, see this discussion (grep for Mali):
> > https://lore.kernel.org/linux-media/4mnlnsj4co3agvln4qsasmgvgwiyoo7yu2h5wyh4rmzzafhm5u@avhnbw7iknms/
> >
> > I know other drivers use a different approach with mutable links, so it
> > would be good if you/Laurent/Sakari can give your opinions on if only one
> > of these two approaches should be taken for multi-stream pipelines.
>
> I see.
>
> Well, I don't have a definite answer. With some thinking both options
> make certain sense. It makes sense to keep the links immutable and
> always enabled, as there's no configuration that can be done. On the
> other hand, it makes sense to require the unused links to be disabled,
> as, well, they are not used.
I'm not familiar with the implications this would have on this driver,
but generally speaking, if a stream is added to the media pipeline by
the pipeline build algorithm, then it is expected that applications
would have configured it correctly. Streams that are not used are
expected to be disabled if they would otherwise be added to the
pipeline.
--
Regards,
Laurent Pinchart
Hi,
On 21/01/2026 01:25, Laurent Pinchart wrote:
> On Thu, Jan 15, 2026 at 02:56:21PM +0200, Tomi Valkeinen wrote:
>> On 15/01/2026 08:36, Jai Luthra wrote:
>>> Quoting Tomi Valkeinen (2026-01-14 20:51:49)
>>>> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
>>>>> From: Jai Luthra <j-luthra@ti.com>
>>>>>
>>>>> With single stream capture, it was simpler to use the video device as
>>>>> the media entity representing the main TI CSI2RX device. Now with multi
>>>>> stream capture coming into the picture, the model has shifted to each
>>>>> video device having a link to the main device's subdev. The routing
>>>>> would then be set on this subdev.
>>>>>
>>>>> Add this subdev, link each context to this subdev's entity and link the
>>>>> subdev's entity to the source. Also add an array of media pads. It will
>>>>> have one sink pad and source pads equal to the number of contexts.
>>>>>
>>>>> Support the new enable_stream()/disable_stream() APIs in the subdev
>>>>> instead of s_stream() hook.
>>>>>
>>>>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>>>>> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
>>>>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
>>>>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
>>>>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
>>>>> ---
>>>
>>> [...]
>>>
>>>>> @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
>>>>> struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
>>>>> struct ti_csi2rx_dev *csi = ctx->csi;
>>>>> struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
>>>>> - struct v4l2_subdev_format source_fmt = {
>>>>> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>>> - .pad = link->source->index,
>>>>> - };
>>>>> + struct v4l2_mbus_framefmt *format;
>>>>> + struct v4l2_subdev_state *state;
>>>>> const struct ti_csi2rx_fmt *ti_fmt;
>>>>> - int ret;
>>>>>
>>>>> - ret = v4l2_subdev_call_state_active(csi->source, pad,
>>>>> - get_fmt, &source_fmt);
>>>>> - if (ret)
>>>>> - return ret;
>>>>> + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
>>>>> + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
>>>>> + v4l2_subdev_unlock_state(state);
>>>>>
>>>>> - if (source_fmt.format.width != csi_fmt->width) {
>>>>> + if (!format) {
>>>>> + dev_dbg(csi->dev,
>>>>> + "Skipping validation as no format present on \"%s\":%u:0\n",
>>>>> + link->source->entity->name, link->source->index);
>>>>> + return 0;
>>>>
>>>> Isn't this an error?
>>>
>>> Well, the j7 shim subdev introduced here has immutable and active links to
>>> all the video nodes, for each DMA channel (taken from DT), many of which
>>> may be unused for certain setups, and thus there might not be any valid
>>> format on the subdev source pad corresponding to an unused video node.
>>>
>>> Jacopo had a similar comment on v2, see this discussion (grep for Mali):
>>> https://lore.kernel.org/linux-media/4mnlnsj4co3agvln4qsasmgvgwiyoo7yu2h5wyh4rmzzafhm5u@avhnbw7iknms/
>>>
>>> I know other drivers use a different approach with mutable links, so it
>>> would be good if you/Laurent/Sakari can give your opinions on if only one
>>> of these two approaches should be taken for multi-stream pipelines.
>>
>> I see.
>>
>> Well, I don't have a definite answer. With some thinking both options
>> make certain sense. It makes sense to keep the links immutable and
>> always enabled, as there's no configuration that can be done. On the
>> other hand, it makes sense to require the unused links to be disabled,
>> as, well, they are not used.
>
> I'm not familiar with the implications this would have on this driver,
> but generally speaking, if a stream is added to the media pipeline by
> the pipeline build algorithm, then it is expected that applications
> would have configured it correctly. Streams that are not used are
> expected to be disabled if they would otherwise be added to the
> pipeline.
>
I think the thing here is that the driver creates immutable
always-enabled media links between the videodevs and the first subdev.
Then, say, if only one stream is being used, only one of those links is
actually used, and for every other link the above check fails as there's
no stream, so no format.
In TI CAL driver the links were mutable, and unused links had to be
disabled. There it made sense as the links had to be configurable (there
were two PHYs). Here, there's no configuration needed, so immutable
links make sense, but then they're enabled even when actually not used.
Tomi
On Wed, Jan 21, 2026 at 09:38:29AM +0200, Tomi Valkeinen wrote:
> On 21/01/2026 01:25, Laurent Pinchart wrote:
> > On Thu, Jan 15, 2026 at 02:56:21PM +0200, Tomi Valkeinen wrote:
> >> On 15/01/2026 08:36, Jai Luthra wrote:
> >>> Quoting Tomi Valkeinen (2026-01-14 20:51:49)
> >>>> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
> >>>>> From: Jai Luthra <j-luthra@ti.com>
> >>>>>
> >>>>> With single stream capture, it was simpler to use the video device as
> >>>>> the media entity representing the main TI CSI2RX device. Now with multi
> >>>>> stream capture coming into the picture, the model has shifted to each
> >>>>> video device having a link to the main device's subdev. The routing
> >>>>> would then be set on this subdev.
> >>>>>
> >>>>> Add this subdev, link each context to this subdev's entity and link the
> >>>>> subdev's entity to the source. Also add an array of media pads. It will
> >>>>> have one sink pad and source pads equal to the number of contexts.
> >>>>>
> >>>>> Support the new enable_stream()/disable_stream() APIs in the subdev
> >>>>> instead of s_stream() hook.
> >>>>>
> >>>>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> >>>>> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
> >>>>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> >>>>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> >>>>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> >>>>> ---
> >>>
> >>> [...]
> >>>
> >>>>> @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
> >>>>> struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
> >>>>> struct ti_csi2rx_dev *csi = ctx->csi;
> >>>>> struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
> >>>>> - struct v4l2_subdev_format source_fmt = {
> >>>>> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>>>> - .pad = link->source->index,
> >>>>> - };
> >>>>> + struct v4l2_mbus_framefmt *format;
> >>>>> + struct v4l2_subdev_state *state;
> >>>>> const struct ti_csi2rx_fmt *ti_fmt;
> >>>>> - int ret;
> >>>>>
> >>>>> - ret = v4l2_subdev_call_state_active(csi->source, pad,
> >>>>> - get_fmt, &source_fmt);
> >>>>> - if (ret)
> >>>>> - return ret;
> >>>>> + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
> >>>>> + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
> >>>>> + v4l2_subdev_unlock_state(state);
> >>>>>
> >>>>> - if (source_fmt.format.width != csi_fmt->width) {
> >>>>> + if (!format) {
> >>>>> + dev_dbg(csi->dev,
> >>>>> + "Skipping validation as no format present on \"%s\":%u:0\n",
> >>>>> + link->source->entity->name, link->source->index);
> >>>>> + return 0;
> >>>>
> >>>> Isn't this an error?
> >>>
> >>> Well, the j7 shim subdev introduced here has immutable and active links to
> >>> all the video nodes, for each DMA channel (taken from DT), many of which
> >>> may be unused for certain setups, and thus there might not be any valid
> >>> format on the subdev source pad corresponding to an unused video node.
> >>>
> >>> Jacopo had a similar comment on v2, see this discussion (grep for Mali):
> >>> https://lore.kernel.org/linux-media/4mnlnsj4co3agvln4qsasmgvgwiyoo7yu2h5wyh4rmzzafhm5u@avhnbw7iknms/
> >>>
> >>> I know other drivers use a different approach with mutable links, so it
> >>> would be good if you/Laurent/Sakari can give your opinions on if only one
> >>> of these two approaches should be taken for multi-stream pipelines.
> >>
> >> I see.
> >>
> >> Well, I don't have a definite answer. With some thinking both options
> >> make certain sense. It makes sense to keep the links immutable and
> >> always enabled, as there's no configuration that can be done. On the
> >> other hand, it makes sense to require the unused links to be disabled,
> >> as, well, they are not used.
> >
> > I'm not familiar with the implications this would have on this driver,
> > but generally speaking, if a stream is added to the media pipeline by
> > the pipeline build algorithm, then it is expected that applications
> > would have configured it correctly. Streams that are not used are
> > expected to be disabled if they would otherwise be added to the
> > pipeline.
>
> I think the thing here is that the driver creates immutable
> always-enabled media links between the videodevs and the first subdev.
> Then, say, if only one stream is being used, only one of those links is
> actually used, and for every other link the above check fails as there's
> no stream, so no format.
>
> In TI CAL driver the links were mutable, and unused links had to be
> disabled. There it made sense as the links had to be configurable (there
> were two PHYs). Here, there's no configuration needed, so immutable
> links make sense, but then they're enabled even when actually not used.
If the routing table in the subdev does not contain any route that goes
towards a video node, then that video node should not be added to the
pipeline by the validation code, and no validation will be attempted. At
least that's the theory.
I see that this driver implements .link_validate() as a
media_entity_operations, not a subdev operation. I wonder if that could
explain the issue.
--
Regards,
Laurent Pinchart
Hi Laurent,
Quoting Laurent Pinchart (2026-01-21 16:22:32)
> On Wed, Jan 21, 2026 at 09:38:29AM +0200, Tomi Valkeinen wrote:
> > On 21/01/2026 01:25, Laurent Pinchart wrote:
> > > On Thu, Jan 15, 2026 at 02:56:21PM +0200, Tomi Valkeinen wrote:
> > >> On 15/01/2026 08:36, Jai Luthra wrote:
> > >>> Quoting Tomi Valkeinen (2026-01-14 20:51:49)
> > >>>> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
> > >>>>> From: Jai Luthra <j-luthra@ti.com>
> > >>>>>
> > >>>>> With single stream capture, it was simpler to use the video device as
> > >>>>> the media entity representing the main TI CSI2RX device. Now with multi
> > >>>>> stream capture coming into the picture, the model has shifted to each
> > >>>>> video device having a link to the main device's subdev. The routing
> > >>>>> would then be set on this subdev.
> > >>>>>
> > >>>>> Add this subdev, link each context to this subdev's entity and link the
> > >>>>> subdev's entity to the source. Also add an array of media pads. It will
> > >>>>> have one sink pad and source pads equal to the number of contexts.
> > >>>>>
> > >>>>> Support the new enable_stream()/disable_stream() APIs in the subdev
> > >>>>> instead of s_stream() hook.
> > >>>>>
> > >>>>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> > >>>>> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
> > >>>>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > >>>>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> > >>>>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> > >>>>> ---
> > >>>
> > >>> [...]
> > >>>
> > >>>>> @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
> > >>>>> struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
> > >>>>> struct ti_csi2rx_dev *csi = ctx->csi;
> > >>>>> struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
> > >>>>> - struct v4l2_subdev_format source_fmt = {
> > >>>>> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > >>>>> - .pad = link->source->index,
> > >>>>> - };
> > >>>>> + struct v4l2_mbus_framefmt *format;
> > >>>>> + struct v4l2_subdev_state *state;
> > >>>>> const struct ti_csi2rx_fmt *ti_fmt;
> > >>>>> - int ret;
> > >>>>>
> > >>>>> - ret = v4l2_subdev_call_state_active(csi->source, pad,
> > >>>>> - get_fmt, &source_fmt);
> > >>>>> - if (ret)
> > >>>>> - return ret;
> > >>>>> + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
> > >>>>> + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
> > >>>>> + v4l2_subdev_unlock_state(state);
> > >>>>>
> > >>>>> - if (source_fmt.format.width != csi_fmt->width) {
> > >>>>> + if (!format) {
> > >>>>> + dev_dbg(csi->dev,
> > >>>>> + "Skipping validation as no format present on \"%s\":%u:0\n",
> > >>>>> + link->source->entity->name, link->source->index);
> > >>>>> + return 0;
> > >>>>
> > >>>> Isn't this an error?
> > >>>
> > >>> Well, the j7 shim subdev introduced here has immutable and active links to
> > >>> all the video nodes, for each DMA channel (taken from DT), many of which
> > >>> may be unused for certain setups, and thus there might not be any valid
> > >>> format on the subdev source pad corresponding to an unused video node.
> > >>>
> > >>> Jacopo had a similar comment on v2, see this discussion (grep for Mali):
> > >>> https://lore.kernel.org/linux-media/4mnlnsj4co3agvln4qsasmgvgwiyoo7yu2h5wyh4rmzzafhm5u@avhnbw7iknms/
> > >>>
> > >>> I know other drivers use a different approach with mutable links, so it
> > >>> would be good if you/Laurent/Sakari can give your opinions on if only one
> > >>> of these two approaches should be taken for multi-stream pipelines.
> > >>
> > >> I see.
> > >>
> > >> Well, I don't have a definite answer. With some thinking both options
> > >> make certain sense. It makes sense to keep the links immutable and
> > >> always enabled, as there's no configuration that can be done. On the
> > >> other hand, it makes sense to require the unused links to be disabled,
> > >> as, well, they are not used.
> > >
> > > I'm not familiar with the implications this would have on this driver,
> > > but generally speaking, if a stream is added to the media pipeline by
> > > the pipeline build algorithm, then it is expected that applications
> > > would have configured it correctly. Streams that are not used are
> > > expected to be disabled if they would otherwise be added to the
> > > pipeline.
> >
> > I think the thing here is that the driver creates immutable
> > always-enabled media links between the videodevs and the first subdev.
> > Then, say, if only one stream is being used, only one of those links is
> > actually used, and for every other link the above check fails as there's
> > no stream, so no format.
> >
> > In TI CAL driver the links were mutable, and unused links had to be
> > disabled. There it made sense as the links had to be configurable (there
> > were two PHYs). Here, there's no configuration needed, so immutable
> > links make sense, but then they're enabled even when actually not used.
>
> If the routing table in the subdev does not contain any route that goes
> towards a video node, then that video node should not be added to the
> pipeline by the validation code, and no validation will be attempted. At
> least that's the theory.
Okay that sounds reasonable. I can take a look into the media pipeline
validation code next week. @Rishikesh, given you already have a working
setup, feel free to test if the link_validate callback is triggered on
video nodes that don't have any streams/routes pointing to them.
>
> I see that this driver implements .link_validate() as a
> media_entity_operations, not a subdev operation. I wonder if that could
> explain the issue.
>
Well earlier I was partially confused, now I'm fully confused :-)
How is v4l2_subdev_pad_ops.link_validate different from
media_entity_operations.link_validate?
I see mc-core.rst and v4l2-subdev.rst both talk about their own variant,
without making it clear which should be used for a subdev.
Anyway, I'll try to dig through the framework code to understand what's
going wrong.
> --
> Regards,
>
> Laurent Pinchart
Thanks,
Jai
On Thu, Jan 22, 2026 at 12:23:50PM +0530, Jai Luthra wrote:
> Quoting Laurent Pinchart (2026-01-21 16:22:32)
> > On Wed, Jan 21, 2026 at 09:38:29AM +0200, Tomi Valkeinen wrote:
> > > On 21/01/2026 01:25, Laurent Pinchart wrote:
> > > > On Thu, Jan 15, 2026 at 02:56:21PM +0200, Tomi Valkeinen wrote:
> > > >> On 15/01/2026 08:36, Jai Luthra wrote:
> > > >>> Quoting Tomi Valkeinen (2026-01-14 20:51:49)
> > > >>>> On 30/12/2025 10:32, Rishikesh Donadkar wrote:
> > > >>>>> From: Jai Luthra <j-luthra@ti.com>
> > > >>>>>
> > > >>>>> With single stream capture, it was simpler to use the video device as
> > > >>>>> the media entity representing the main TI CSI2RX device. Now with multi
> > > >>>>> stream capture coming into the picture, the model has shifted to each
> > > >>>>> video device having a link to the main device's subdev. The routing
> > > >>>>> would then be set on this subdev.
> > > >>>>>
> > > >>>>> Add this subdev, link each context to this subdev's entity and link the
> > > >>>>> subdev's entity to the source. Also add an array of media pads. It will
> > > >>>>> have one sink pad and source pads equal to the number of contexts.
> > > >>>>>
> > > >>>>> Support the new enable_stream()/disable_stream() APIs in the subdev
> > > >>>>> instead of s_stream() hook.
> > > >>>>>
> > > >>>>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> > > >>>>> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
> > > >>>>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > >>>>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> > > >>>>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> > > >>>>> ---
> > > >>>
> > > >>> [...]
> > > >>>
> > > >>>>> @@ -981,48 +1138,52 @@ static int ti_csi2rx_link_validate(struct media_link *link)
> > > >>>>> struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev);
> > > >>>>> struct ti_csi2rx_dev *csi = ctx->csi;
> > > >>>>> struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix;
> > > >>>>> - struct v4l2_subdev_format source_fmt = {
> > > >>>>> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > >>>>> - .pad = link->source->index,
> > > >>>>> - };
> > > >>>>> + struct v4l2_mbus_framefmt *format;
> > > >>>>> + struct v4l2_subdev_state *state;
> > > >>>>> const struct ti_csi2rx_fmt *ti_fmt;
> > > >>>>> - int ret;
> > > >>>>>
> > > >>>>> - ret = v4l2_subdev_call_state_active(csi->source, pad,
> > > >>>>> - get_fmt, &source_fmt);
> > > >>>>> - if (ret)
> > > >>>>> - return ret;
> > > >>>>> + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
> > > >>>>> + format = v4l2_subdev_state_get_format(state, link->source->index, 0);
> > > >>>>> + v4l2_subdev_unlock_state(state);
> > > >>>>>
> > > >>>>> - if (source_fmt.format.width != csi_fmt->width) {
> > > >>>>> + if (!format) {
> > > >>>>> + dev_dbg(csi->dev,
> > > >>>>> + "Skipping validation as no format present on \"%s\":%u:0\n",
> > > >>>>> + link->source->entity->name, link->source->index);
> > > >>>>> + return 0;
> > > >>>>
> > > >>>> Isn't this an error?
> > > >>>
> > > >>> Well, the j7 shim subdev introduced here has immutable and active links to
> > > >>> all the video nodes, for each DMA channel (taken from DT), many of which
> > > >>> may be unused for certain setups, and thus there might not be any valid
> > > >>> format on the subdev source pad corresponding to an unused video node.
> > > >>>
> > > >>> Jacopo had a similar comment on v2, see this discussion (grep for Mali):
> > > >>> https://lore.kernel.org/linux-media/4mnlnsj4co3agvln4qsasmgvgwiyoo7yu2h5wyh4rmzzafhm5u@avhnbw7iknms/
> > > >>>
> > > >>> I know other drivers use a different approach with mutable links, so it
> > > >>> would be good if you/Laurent/Sakari can give your opinions on if only one
> > > >>> of these two approaches should be taken for multi-stream pipelines.
> > > >>
> > > >> I see.
> > > >>
> > > >> Well, I don't have a definite answer. With some thinking both options
> > > >> make certain sense. It makes sense to keep the links immutable and
> > > >> always enabled, as there's no configuration that can be done. On the
> > > >> other hand, it makes sense to require the unused links to be disabled,
> > > >> as, well, they are not used.
> > > >
> > > > I'm not familiar with the implications this would have on this driver,
> > > > but generally speaking, if a stream is added to the media pipeline by
> > > > the pipeline build algorithm, then it is expected that applications
> > > > would have configured it correctly. Streams that are not used are
> > > > expected to be disabled if they would otherwise be added to the
> > > > pipeline.
> > >
> > > I think the thing here is that the driver creates immutable
> > > always-enabled media links between the videodevs and the first subdev.
> > > Then, say, if only one stream is being used, only one of those links is
> > > actually used, and for every other link the above check fails as there's
> > > no stream, so no format.
> > >
> > > In TI CAL driver the links were mutable, and unused links had to be
> > > disabled. There it made sense as the links had to be configurable (there
> > > were two PHYs). Here, there's no configuration needed, so immutable
> > > links make sense, but then they're enabled even when actually not used.
> >
> > If the routing table in the subdev does not contain any route that goes
> > towards a video node, then that video node should not be added to the
> > pipeline by the validation code, and no validation will be attempted. At
> > least that's the theory.
>
> Okay that sounds reasonable. I can take a look into the media pipeline
> validation code next week. @Rishikesh, given you already have a working
> setup, feel free to test if the link_validate callback is triggered on
> video nodes that don't have any streams/routes pointing to them.
>
> > I see that this driver implements .link_validate() as a
> > media_entity_operations, not a subdev operation. I wonder if that could
> > explain the issue.
>
> Well earlier I was partially confused, now I'm fully confused :-)
>
> How is v4l2_subdev_pad_ops.link_validate different from
> media_entity_operations.link_validate?
media_entity_operations.link_validate() is the entry point, called by
the media pipeline validation code that is agnostic to entity types. It
is called on the sink entity of each link.
When the sink is a subdev, the common practice is to implement
media_entity_operations.link_validate() using
v4l2_subdev_link_validate(), which iterates over streams and validate
them individually with v4l2_subdev_pad_ops.link_validate(). That
operation can be left out if the default implementation
v4l2_subdev_link_validate_default() is enough, or a custom
.link_validate() operation can be provided that calls
v4l2_subdev_link_validate_default() and performs additional checks (or
implements all checks manually without calling
v4l2_subdev_link_validate_default() for very uncommon cases).
In this case, though, the sink is a video device, so
v4l2_subdev_link_validate() can't be used. I overlooked that in my
previous reply, sorry about it. As a link to a video node can only carry
a single stream, it seems that the issue here is caused by the link
being included in the media pipeline in the first place.
drivers/media/mc/mc-entity.c contains detailed debug messages that
explain how a pipeline is constructed, I would start by enabling them
and investigating what happens.
> I see mc-core.rst and v4l2-subdev.rst both talk about their own variant,
> without making it clear which should be used for a subdev.
>
> Anyway, I'll try to dig through the framework code to understand what's
> going wrong.
--
Regards,
Laurent Pinchart
© 2016 - 2026 Red Hat, Inc.