Re: [PATCH v9 06/19] media: ti: j721e-csi2rx: add a subdev for the core device

[Jai Luthra]

Quoting Laurent Pinchart (2026-01-22 15:23:08)
> 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@xxxxxx>
> > > > >>>>>
> > > > >>>>> 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@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>
> > > > >>>>> Signed-off-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
> > > > >>>>> ---
> > > > >>>
> > > > >>> [...]
> > > > >>>
> > > > >>>>> @@ -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 finally got time to check this out, and yes, the subdev pads with no
route going to them are added in the pipeline currently, as the subdev is
missing the .has_pad_interdep operation. Using the framework helper, which
marks two pads as interdependent only if they have an active route, fixes it.

Rishikesh, can you please apply the following before you post v12 of your
series:

---------
diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index f126649ba36c..42da8bd6b53c 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -1340,10 +1340,10 @@ static int ti_csi2rx_link_validate(struct media_link *link)
        v4l2_subdev_unlock_state(state);

        if (!format) {
-               dev_dbg(csi->dev,
-                       "Skipping validation as no format present on \"%s\":%u:0\n",
+               dev_err(csi->dev,
+                       "No format present on \"%s\":%u:0\n",
                        link->source->entity->name, link->source->index);
-               return 0;
+               return -EPIPE;
        }

        if (format->width != csi_fmt->width) {
@@ -1389,6 +1389,7 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = {

 static const struct media_entity_operations ti_csi2rx_subdev_entity_ops = {
        .link_validate = v4l2_subdev_link_validate,
+       .has_pad_interdep = v4l2_subdev_has_pad_interdep,
 };

 static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx)
---------

And while above alone fixes this issue, I saw cdns-csi2rx pads that are unused
also get added to the pipeline, which seems wrong even if it doesn't break
anything. So please apply the below in the patch that adds multistream support
for cdns-csi2rx:

---------
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 7164a7d6f4a1..18737d00a7d7 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -803,6 +803,7 @@ static const struct v4l2_subdev_internal_ops csi2rx_internal_ops = {
 static const struct media_entity_operations csi2rx_media_ops = {
        .link_validate = v4l2_subdev_link_validate,
        .get_fwnode_pad = v4l2_subdev_get_fwnode_pad_1_to_1,
+       .has_pad_interdep = v4l2_subdev_has_pad_interdep,
 };

 static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
---------

Thanks,
Jai

> > 
> > > 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